> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ivan Dyukov > Sent: Tuesday, September 8, 2020 8:16 AM > > Hi Ferruh, Morten, > > 07.09.2020 18:29, Morten Brørup пишет: > >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ferruh Yigit > >> Sent: Monday, September 7, 2020 3:25 PM > >> > >> On 8/11/2020 9:52 AM, Ivan Dyukov wrote: > >> > >> <...> > >> > >>> v9 changes: > >>> * remove rte_eth_link_printf function > >>> * add ETH_LINK_MAX_STR_LEN definition > >>> * add usage of ETH_LINK_MAX_STR_LEN in examples > >>> > >>> v1 changes: > >>> This is initial patchset which introduces UNKNOWN speed to dpdk > >>> applications. Also it contains changes related to printf formating. > >>> Patchset contains changes for app/ and doc/ folders. > >>> examples/ folder will be provided later. > >>> > >>> > >> Hi Ivan, > >> > >> Logging discussion is going on, this is preventing 'unknown' link > speed > >> merged > >> and used by drivers. This has potential conflict in more drivers. > >> > >> So I will get those patches (1,4,5,6/24) although logging link speed > >> won't be > >> correct, and logging discussions can continue separately. > >> > >> > >> > >> Related to the link speed logging, the problem we are trying to > solve > >> is > >> 'unknown' speed value representation (UINT_MAX) won't be correct and > >> can be > >> confusing, which requires additional check/parsing before log. > >> > >> The proposed solution is a link to string function. Because of the > >> logging > >> difference/needs in the applications the function provides fine > grade > >> logging > >> capability, which works fine but I think it is too complex for the > >> problem it is > >> solving. > >> I wonder if the logging link information differences in the > >> applications is an > >> actual need, or it happened by time since those samples/applications > >> developed > >> by different people and common logging function was missing. So > perhaps > >> we can > >> switch all to a generic logging. > > Good thinking, Ferruh. I agree that examples probably use different > formatting by accident, not by deliberate choice. > > >> What do you think following: > >> > >> For the immediate need for 'unknown' link speed parsing, can have a > >> 'rte_eth_link_speed_to_str()' function which returns only link speed > >> and > >> applications need custom message can use it for logging. > > That function is unlikely to serve application specific needs for > formatting speed. > > > > But it might serve the needs of DPDK libraries and examples. > > > >> And can have a simple/brief link to string function for generic > usage, > >> and if > >> application want custom message it can parse link struct itself. > >> The link string can be something like: "Link Up 10Gbps full-duplex > >> autoneg" > >> > > For DPDK libraries and examples, I agree that a generic one-format- > fits-all function should suffice. > > > > And as a DPDK application developer, parsing the link struct > ourselves suffices for our application. In this context, an strf-style > function is "nice to have", not "must have". > Almost all examples have same link status formating, except two cases: > testpmd has multiline link status format and another application has > extremely brief status info which is mixed with other text. so I would > prefer to keep such statuses without changes but we should give some > helpers for such cases.I would prefer MACROS: > #define RTE_ETH_LINK_STATUS_TO_STR(link_status) (link_status == > ETH_LINK_DOWN ? "Down" : "Up") > #define RTE_ETH_LINK_SPEED_TO_STR(link_speed) (link_speed == > ETH_SPEED_NUM_UNKNOWN ? "Unknown" : \ > link_speed == > ETH_SPEED_NUM_NONE ? "0" : \ > link_speed == > ETH_SPEED_NUM_10M ? "10 Mbps" : \ > link_speed == > ETH_SPEED_NUM_100M ? "100 Mbps" : \ > link_speed == > ETH_SPEED_NUM_1G ? "1 Gbps" : \ > link_speed > ==ETH_SPEED_NUM_2_5G ? "2.5 Gbps" : \ > link_speed > ==ETH_SPEED_NUM_5G ? "5 Gbps" : \ > link_speed > ==ETH_SPEED_NUM_10G ? "10 Gbps" : \ > link_speed == > ETH_SPEED_NUM_20G ? "20 Gbps" : \ > link_speed == > ETH_SPEED_NUM_25G ? "25 Gbps" : \ > link_speed == > ETH_SPEED_NUM_40G ? "40 Gbps" : \ > link_speed == > ETH_SPEED_NUM_50G ? "50 Gbps" : \ > link_speed == > ETH_SPEED_NUM_56G ? "56 Gbps" : \ > link_speed == ETH_SPEED_NUM_100G ? "100 Gbps" : \ > link_speed == ETH_SPEED_NUM_200G ? "200 Gbps" : \ > "Invalid speed") > #define RTE_ETH_LINK_DUPLEX_TO_STR(link_duplex) (link_duplex == > ETH_LINK_FULL_DUPLEX? "FDX" : "HDX") > #define RTE_ETH_LINK_AUTONEG_TO_STR(link_autoneg) (link_autoneg== > ETH_LINK_FULL_DUPLEX? "Autoneg" : "Fixed") > > and one function which construct a default status string: > > int rte_eth_link_to_str(char *str, size_t len, const struct > rte_eth_link > *eth_link) { > > static const char link_down_str[] = "Link down"; > > static const char link_up_str[] = "Link up at"; > > if (eth_link->link_status == ETH_LINK_DOWN) > > return snprintf(str, len, "%s", link_down_str); > > else > > return snprintf(str, len, "%s %s %s %s", link_up_str, > > RTE_ETH_LINK_SPEED_TO_STR(eth_link->link_speed), > > RTE_ETH_LINK_DUPLEX_TO_STR(eth_link->link_duplex), > > RTE_ETH_LINK_AUTONEG_TO_STR(eth_link->link_autoneg)); > > }
I think that the DPDK developer community has a preference for functions over macros. This is not in the fast path, so there is no need for macros. Make the macros functions instead, and consider it... Acked-by: Morten Brørup <m...@smartsharesystems.com>
