[dpdk-dev] [PATCH v3 2/7] ethdev: add a link status textrepresentation

Morten Brørup mb at smartsharesystems.com
Mon Jun 22 09:43:37 CEST 2020


> From: Ferruh Yigit [mailto:ferruh.yigit at intel.com]
> Sent: Monday, June 22, 2020 9:05 AM
> 
> On 6/18/2020 1:32 PM, Morten Brørup wrote:
> >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ferruh Yigit
> >> Sent: Thursday, June 18, 2020 2:03 PM
> >>
> >> On 6/18/2020 11:08 AM, Ivan Dyukov wrote:
> >>> Hi Ferruh,
> >>>
> >>> Thank you for the comments.
> >>>
> >>> My answers are inlined.
> >>>
> >>> Best regards,
> >>> Ivan
> >>> 17.06.2020 19:45, Ferruh Yigit пишет:
> >>>> On 6/15/2020 10:01 AM, Ivan Dyukov wrote:
> >>>>> This commit add function which treat link status structure
> >>>>> and format it to text representation.
> >>>> If I am following correctly, the initial need was to escape from
> >> speed checks
> >>>> everytime loging link information caused by this new 'unknown'
> >> speed.
> >>>>
> >>>> And later suggestion was to have a pre-formatted text for link
> >> logging.
> >>> Correct.
> >>>>
> >>>> This patch brings additional link status printing/formatting
> >> capability with
> >>>> custom format string support and with new format specifiers for
> link
> >> (like, '%D'
> >>>> link duplex state),
> >>>> although this is nice work and thanks for it, I am not sure this
> >> complexity and
> >>>> two new APIs are really needed.
> >>> Yep.
> >>>> For me only 'rte_eth_link_format()' without custom format support
> >> looks good
> >>>> enough but I won't object if the concensus is to have them.
> >>>> I am aware there are multiple applications you are updating
> logging
> >> slightly
> >>>> different which requires this flexibility but what happens if they
> >> use same
> >>>> pre-formatted text, is that difference really required or happened
> >> by time based
> >>>> on developers taste?
> >>> I have changed only few applications but I have plan to change all
> >> dpdk
> >>> examples. Even in those few apps, we have various link status
> logging
> >>> text. e.g  app/test-pmd/config.c
> >>> @@ -600,10 +600,9 @@ port_infos_display(portid_t port_id)
> >>>          } else
> >>>                  printf("\nmemory allocation on the socket:
> >>> %u",port->socket_id);
> >>>
> >>> -       printf("\nLink status: %s\n", (link.link_status) ? ("up") :
> >>> ("down"));
> >>> -       printf("Link speed: %u Mbps\n", (unsigned)
> link.link_speed);
> >>> -       printf("Link duplex: %s\n", (link.link_duplex ==
> >>> ETH_LINK_FULL_DUPLEX) ?
> >>> -              ("full-duplex") : ("half-duplex"));
> >>> +       rte_eth_link_printf("\nLink status: %S\n"
> >>> +                           "Link speed: %M Mbps\n"
> >>> +                           "Link duplex: %D\n", &link);
> >>> the status is logged in 3 lines. this is special text layoting and
> I
> >>> don't know how it will look in one line. Myabe it will require
> >> reformat
> >>> all screen. I don't want to change screen layoting in this
> patchset.
> >>
> >> cc'ed Morten & Stephen.
> >>
> >> To keep existing layout in the applications yes you need more
> >> flexibility, a
> >> pre-formatted text won't cut it, but I guess I prefer your approach
> in
> >> v2 for
> >> simplicity.
> >>
> >> And I would prefer extending the one in v2 with a larger string for
> the
> >> pre-formatted text, so both flexibility and the standard output can
> be
> >> provided,
> >> Morten didn't like it but if I understand correctly his comment was
> to
> >> keep more
> >> simple solution in ethdev and applications can do it themselves if
> they
> >> want
> >> more custom log formatting, but this patch adds more complex and
> >> flexible
> >> logging support to the ethdev.
> >>
> >
> > If you are going to add a flexible link status formatting function,
> like strftime(), it should not print to stdout, but to a string buffer,
> like strftime(). It will also allow for new format specifiers in the
> future, e.g. %1D for "FDX"/"HDX", %2D for "Full"/"Half" or %3D for
> "full-duplex"/"half-duplex". This would provide more flexible support
> for a larger number of application specific formats.
> 
> These will make it even more complex, do we really need this?

It only needs to support the format specifiers required for the preferred DPDK text format, initially. It can be extended at a later time with more format specifiers.

And the implementation could be somewhat simplified by specifying that the length of the provided string buffer should be large enough to hold the result, or only a partial (or possibly empty) string will be returned.

I prefer a flexible formatting function over the initially suggested function that takes a struct of string buffers with only one way of formatting the strings. Or, we can leave it up to the application to do the text formatting.

> 
> >
> > Then the function for printing to stdout in a DPDK preferred format
> (if you add such a function to the library) can use the above
> eth_link_strf() function.


More information about the dev mailing list