[v9,00/24] ethdev: allow unknown link speed

  MAINTAINERS                                              |   1 +
 app/proc-info/main.c                                     |   9 +-
 app/test-pipeline/init.c                                 |  11 ++-
 app/test-pmd/config.c                                    |  20 +++--
 app/test-pmd/testpmd.c                                   |   9 +-
 app/test/Makefile                                        |   3 +
 app/test/meson.build                                     |   2 +
 app/test/test_ethdev_link.c                              | 299 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 app/test/test_pmd_perf.c                                 |  17 ++--
 doc/guides/sample_app_ug/link_status_intr.rst            |  10 +--
 drivers/net/i40e/i40e_ethdev.c                           |   5 +-
 drivers/net/i40e/i40e_ethdev_vf.c                        |  10 +--
 drivers/net/ice/ice_ethdev.c                             |   5 +-
 drivers/net/ixgbe/ixgbe_ethdev.c                         |   6 +-
 examples/bbdev_app/main.c                                |   8 +-
 examples/ioat/ioatfwd.c                                  |  13 ++-
 examples/ip_fragmentation/main.c                         |  13 ++-
 examples/ip_pipeline/cli.c                               |  12 +--
 examples/ip_reassembly/main.c                            |  12 +--
 examples/ipsec-secgw/ipsec-secgw.c                       |  12 +--
 examples/ipv4_multicast/main.c                           |  12 +--
 examples/kni/main.c                                      |  26 ++----
 examples/l2fwd-crypto/main.c                             |  12 +--
 examples/l2fwd-event/main.c                              |  12 +--
 examples/l2fwd-jobstats/main.c                           |  12 +--
 examples/l2fwd-keepalive/main.c                          |  12 +--
 examples/l2fwd/main.c                                    |  12 +--
 examples/l3fwd-acl/main.c                                |  12 +--
 examples/l3fwd-graph/main.c                              |  14 +--
 examples/l3fwd-power/main.c                              |  13 +--
 examples/l3fwd/main.c                                    |  12 +--
 examples/link_status_interrupt/main.c                    |  30 +++----
 examples/multi_process/client_server_mp/mp_server/init.c |  14 ++-
 examples/multi_process/symmetric_mp/main.c               |  12 +--
 examples/ntb/ntb_fwd.c                                   |  10 +--
 examples/performance-thread/l3fwd-thread/main.c          |  12 +--
 examples/qos_sched/init.c                                |  10 +--
 examples/server_node_efd/server/init.c                   |  15 ++--
 examples/vm_power_manager/main.c                         |  14 ++-
 lib/librte_ethdev/rte_ethdev.c                           | 174 ++++++++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h                           |  76 ++++++++++++++---
 lib/librte_ethdev/rte_ethdev_version.map                 |   4 +
 42 files changed, 715 insertions(+), 282 deletions(-)


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

v8 changes:
* rename rte_eth_link_strf to rte_eth_link_to_str
* refactor rte_eth_link_to_str according to review comments
* fix codestyle
* fix commit message in 02 patch
* fix compile error in ntb application
* merge "app" and "doc" commits

v7 changes:
* fix meson build
* change _strf function. now it does not fails in case of unknown specifiers like %d. it just copy it to target string.
* remove invalid_fmt unit test.
* add unknown specifier test.
* fix codestyle

v6 changes:
* fix spelling in comments according to checkpatch warning

v5 changes:
* rename rte_eth_link_format to rte_eth_link_strf
* add '\n' to default strings
* update remaining examples. patch with subj 'examples: new link status print format' contains examples which have no maintainers.
TBD:
update remaining nic drivers with 'unknown' speed.  It should be provided in separate patchset.

v4 changes:
* refactor rte_eth_link_format using strlcat func instead of snprintf
* added new checks to unit tests
* few minor fixes according review comments
TBD:
update examples in 'example' folder with new status printing mechanism
update remaining nic drivers with 'unknown' speed

v3 changes:
* remove rte_eth_link_prepare_text function
* add rte_eth_link_format and rte_eth_link_printf functions
* added unit tests for rte_eth_link_format function
TBD:
update examples in 'example' folder with new status printing mechanism
update remaining nic drivers with 'unknown' speed

v2 changes:
* add function which format link status to textual representation
* update drivers for Intel nics with 'unknown' speed
TBD:
update examples in 'example' folder with new status printing mechanism
update remaining nic drivers with 'unknown' speed

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.
  

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
> 
> v8 changes:
> * rename rte_eth_link_strf to rte_eth_link_to_str
> * refactor rte_eth_link_to_str according to review comments
> * fix codestyle
> * fix commit message in 02 patch
> * fix compile error in ntb application
> * merge "app" and "doc" commits
> 
> v7 changes:
> * fix meson build
> * change _strf function. now it does not fails in case of unknown specifiers like %d. it just copy it to target string.
> * remove invalid_fmt unit test.
> * add unknown specifier test.
> * fix codestyle
> 
> v6 changes:
> * fix spelling in comments according to checkpatch warning
> 
> v5 changes:
> * rename rte_eth_link_format to rte_eth_link_strf
> * add '\n' to default strings
> * update remaining examples. patch with subj 'examples: new link status print format' contains examples which have no maintainers.
> TBD:
> update remaining nic drivers with 'unknown' speed.  It should be provided in separate patchset.
> 
> v4 changes:
> * refactor rte_eth_link_format using strlcat func instead of snprintf
> * added new checks to unit tests
> * few minor fixes according review comments
> TBD:
> update examples in 'example' folder with new status printing mechanism
> update remaining nic drivers with 'unknown' speed
> 
> v3 changes:
> * remove rte_eth_link_prepare_text function
> * add rte_eth_link_format and rte_eth_link_printf functions
> * added unit tests for rte_eth_link_format function
> TBD:
> update examples in 'example' folder with new status printing mechanism
> update remaining nic drivers with 'unknown' speed
> 
> v2 changes:
> * add function which format link status to textual representation
> * update drivers for Intel nics with 'unknown' speed
> TBD:
> update examples in 'example' folder with new status printing mechanism
> update remaining nic drivers with 'unknown' speed
> 
> 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.

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.

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"
  

> From: dev [mailto:dev-bounces@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".

Med venlig hilsen / kind regards
- Morten Brørup
  

Hi Ferruh, Morten,

07.09.2020 18:29, Morten Brørup пишет:
>> From: dev [mailto:dev-bounces@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
>
>
>
  

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