[dpdk-dev] [PATCH v9 00/24] ethdev: allow unknown link speed

[Morten Brørup]

> From: dev [mailto:dev-bounces at 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-bounces at 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 <mb at smartsharesystems.com>