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

[Ivan Dyukov]

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));

}

> Med venlig hilsen / kind regards
> - Morten Brørup
>
>
>