[v1,5/6] doc: update sample app with unknown speed
Checks
Commit Message
Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
---
doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Comments
On 4/27/20 12:57 PM, Ivan Dyukov wrote:
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
> doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst b/doc/guides/sample_app_ug/link_status_intr.rst
> index 5283be8b7..6ebc707b7 100644
> --- a/doc/guides/sample_app_ug/link_status_intr.rst
> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
> @@ -177,7 +177,8 @@ An example callback function that has been written as indicated below.
> printf("Failed to get port %d link status: %s\n\n",
> port_id, rte_strerror(-ret));
> } else if (link.link_status) {
> - printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id, (unsigned)link.link_speed,
> + printf("Port %d Link Up - speed %u%s - %s\n\n", port_id, (unsigned)link.link_speed,
> + (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") : (" Mbps"),
> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ? ("full-duplex") : ("half-duplex"));
> } else
> printf("Port %d Link Down\n\n", port_id);
>
I think that 0 looks nicer than UINT32_MAX when printed as integer
keeping in mind that it is unknown.
01.05.2020 16:28, Andrew Rybchenko пишет:
> On 4/27/20 12:57 PM, Ivan Dyukov wrote:
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>> ---
>> doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst
>> b/doc/guides/sample_app_ug/link_status_intr.rst
>> index 5283be8b7..6ebc707b7 100644
>> --- a/doc/guides/sample_app_ug/link_status_intr.rst
>> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
>> @@ -177,7 +177,8 @@ An example callback function that has been
>> written as indicated below.
>> printf("Failed to get port %d link status: %s\n\n",
>> port_id, rte_strerror(-ret));
>> } else if (link.link_status) {
>> - printf("Port %d Link Up - speed %u Mbps - %s\n\n",
>> port_id, (unsigned)link.link_speed,
>> + printf("Port %d Link Up - speed %u%s - %s\n\n", port_id,
>> (unsigned)link.link_speed,
>> + (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") :
>> (" Mbps"),
>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>> ("full-duplex") : ("half-duplex"));
>> } else
>> printf("Port %d Link Down\n\n", port_id);
>>
>
> I think that 0 looks nicer than UINT32_MAX when printed as integer
> keeping in mind that it is unknown.
>
zero will mislead developers about real value of the link_speed.
therefore we should print real value of the speed or print nothing. e.g.
if (link.link_speed == UINT32_MAX)
printf("Port %d Link Up - speed UNKNOWN - %s\n\n", port_id,
(link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
("full-duplex") : ("half-duplex"));
else
printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id,
link.link_speed,
(link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
("full-duplex") : ("half-duplex"));
On 5/2/20 10:35 PM, Ivan Dyukov wrote:
> 01.05.2020 16:28, Andrew Rybchenko пишет:
>> On 4/27/20 12:57 PM, Ivan Dyukov wrote:
>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>> ---
>>> doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst
>>> b/doc/guides/sample_app_ug/link_status_intr.rst
>>> index 5283be8b7..6ebc707b7 100644
>>> --- a/doc/guides/sample_app_ug/link_status_intr.rst
>>> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
>>> @@ -177,7 +177,8 @@ An example callback function that has been
>>> written as indicated below.
>>> printf("Failed to get port %d link status: %s\n\n",
>>> port_id, rte_strerror(-ret));
>>> } else if (link.link_status) {
>>> - printf("Port %d Link Up - speed %u Mbps - %s\n\n",
>>> port_id, (unsigned)link.link_speed,
>>> + printf("Port %d Link Up - speed %u%s - %s\n\n", port_id,
>>> (unsigned)link.link_speed,
>>> + (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") :
>>> (" Mbps"),
>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>> ("full-duplex") : ("half-duplex"));
>>> } else
>>> printf("Port %d Link Down\n\n", port_id);
>>>
>> I think that 0 looks nicer than UINT32_MAX when printed as integer
>> keeping in mind that it is unknown.
>>
> zero will mislead developers about real value of the link_speed.
> therefore we should print real value of the speed or print nothing. e.g.
>
> if (link.link_speed == UINT32_MAX)
>
> printf("Port %d Link Up - speed UNKNOWN - %s\n\n", port_id,
> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
> ("full-duplex") : ("half-duplex"));
> else
>
> printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id,
> link.link_speed,
>
> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
> ("full-duplex") : ("half-duplex"));
I'm not sure about 0 to be misleading, but it could be.
Above definitely will look better in stdout.
The only problem is code duplication in many-many places.
May be add simple function in ethdev to do it and use it in
all examples?
Please, wait more feedback before doing it.
Hi,
Sharing an observation especially with Fortville (X710), if the port is not started the advertised speed is not max speed (`10000` or `40000`). Hence would cross check on `link_state` be useful before display?
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> Sent: Sunday, May 3, 2020 7:27 PM
> To: Ivan Dyukov <i.dyukov@samsung.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org; v.kuramshin@samsung.com;
> david.marchand@redhat.com; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v1 5/6] doc: update sample app with unknown
> speed
>
> On 5/2/20 10:35 PM, Ivan Dyukov wrote:
> > 01.05.2020 16:28, Andrew Rybchenko пишет:
> >> On 4/27/20 12:57 PM, Ivan Dyukov wrote:
> >>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> >>> ---
> >>> doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst
> >>> b/doc/guides/sample_app_ug/link_status_intr.rst
> >>> index 5283be8b7..6ebc707b7 100644
> >>> --- a/doc/guides/sample_app_ug/link_status_intr.rst
> >>> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
> >>> @@ -177,7 +177,8 @@ An example callback function that has been
> >>> written as indicated below.
> >>> printf("Failed to get port %d link status: %s\n\n",
> >>> port_id, rte_strerror(-ret));
> >>> } else if (link.link_status) {
> >>> - printf("Port %d Link Up - speed %u Mbps - %s\n\n",
> >>> port_id, (unsigned)link.link_speed,
> >>> + printf("Port %d Link Up - speed %u%s - %s\n\n",
> >>> +port_id,
> >>> (unsigned)link.link_speed,
> >>> + (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") :
> >>> (" Mbps"),
> >>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
> >>> ("full-duplex") : ("half-duplex"));
> >>> } else
> >>> printf("Port %d Link Down\n\n", port_id);
> >>>
> >> I think that 0 looks nicer than UINT32_MAX when printed as integer
> >> keeping in mind that it is unknown.
> >>
> > zero will mislead developers about real value of the link_speed.
> > therefore we should print real value of the speed or print nothing. e.g.
> >
> > if (link.link_speed == UINT32_MAX)
> >
> > printf("Port %d Link Up - speed UNKNOWN - %s\n\n", port_id,
> > (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
> > ("full-duplex") : ("half-duplex"));
> > else
> >
> > printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id,
> > link.link_speed,
> >
> > (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
> > ("full-duplex") : ("half-duplex"));
>
> I'm not sure about 0 to be misleading, but it could be.
> Above definitely will look better in stdout.
> The only problem is code duplication in many-many places.
> May be add simple function in ethdev to do it and use it in all examples?
>
> Please, wait more feedback before doing it.
03.05.2020 16:57, Andrew Rybchenko пишет:
> On 5/2/20 10:35 PM, Ivan Dyukov wrote:
>> 01.05.2020 16:28, Andrew Rybchenko пишет:
>>> On 4/27/20 12:57 PM, Ivan Dyukov wrote:
>>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>>> ---
>>>> doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst
>>>> b/doc/guides/sample_app_ug/link_status_intr.rst
>>>> index 5283be8b7..6ebc707b7 100644
>>>> --- a/doc/guides/sample_app_ug/link_status_intr.rst
>>>> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
>>>> @@ -177,7 +177,8 @@ An example callback function that has been
>>>> written as indicated below.
>>>> printf("Failed to get port %d link status: %s\n\n",
>>>> port_id, rte_strerror(-ret));
>>>> } else if (link.link_status) {
>>>> - printf("Port %d Link Up - speed %u Mbps - %s\n\n",
>>>> port_id, (unsigned)link.link_speed,
>>>> + printf("Port %d Link Up - speed %u%s - %s\n\n", port_id,
>>>> (unsigned)link.link_speed,
>>>> + (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") :
>>>> (" Mbps"),
>>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>>> ("full-duplex") : ("half-duplex"));
>>>> } else
>>>> printf("Port %d Link Down\n\n", port_id);
>>>>
>>> I think that 0 looks nicer than UINT32_MAX when printed as integer
>>> keeping in mind that it is unknown.
>>>
>> zero will mislead developers about real value of the link_speed.
>> therefore we should print real value of the speed or print nothing. e.g.
>>
>> if (link.link_speed == UINT32_MAX)
>>
>> printf("Port %d Link Up - speed UNKNOWN - %s\n\n", port_id,
>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>> ("full-duplex") : ("half-duplex"));
>> else
>>
>> printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id,
>> link.link_speed,
>>
>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>> ("full-duplex") : ("half-duplex"));
> I'm not sure about 0 to be misleading, but it could be.
> Above definitely will look better in stdout.
> The only problem is code duplication in many-many places.
> May be add simple function in ethdev to do it and use it in
> all examples?
Also we can introduce new printf specifier using
register_printf_specifier function or even rewrite %u which will print
UNKNOWN in case of
UINT32_MAX.
>
> Please, wait more feedback before doing it.
>
>
On 5/4/20 6:46 PM, Ivan Dyukov wrote:
> 03.05.2020 16:57, Andrew Rybchenko пишет:
>> On 5/2/20 10:35 PM, Ivan Dyukov wrote:
>>> 01.05.2020 16:28, Andrew Rybchenko пишет:
>>>> On 4/27/20 12:57 PM, Ivan Dyukov wrote:
>>>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>>>> ---
>>>>> doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst
>>>>> b/doc/guides/sample_app_ug/link_status_intr.rst
>>>>> index 5283be8b7..6ebc707b7 100644
>>>>> --- a/doc/guides/sample_app_ug/link_status_intr.rst
>>>>> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
>>>>> @@ -177,7 +177,8 @@ An example callback function that has been
>>>>> written as indicated below.
>>>>> printf("Failed to get port %d link status: %s\n\n",
>>>>> port_id, rte_strerror(-ret));
>>>>> } else if (link.link_status) {
>>>>> - printf("Port %d Link Up - speed %u Mbps - %s\n\n",
>>>>> port_id, (unsigned)link.link_speed,
>>>>> + printf("Port %d Link Up - speed %u%s - %s\n\n", port_id,
>>>>> (unsigned)link.link_speed,
>>>>> + (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") :
>>>>> (" Mbps"),
>>>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>>>> ("full-duplex") : ("half-duplex"));
>>>>> } else
>>>>> printf("Port %d Link Down\n\n", port_id);
>>>>>
>>>> I think that 0 looks nicer than UINT32_MAX when printed as integer
>>>> keeping in mind that it is unknown.
>>>>
>>> zero will mislead developers about real value of the link_speed.
>>> therefore we should print real value of the speed or print nothing. e.g.
>>>
>>> if (link.link_speed == UINT32_MAX)
>>>
>>> printf("Port %d Link Up - speed UNKNOWN - %s\n\n", port_id,
>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>> ("full-duplex") : ("half-duplex"));
>>> else
>>>
>>> printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id,
>>> link.link_speed,
>>>
>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>> ("full-duplex") : ("half-duplex"));
>> I'm not sure about 0 to be misleading, but it could be.
>> Above definitely will look better in stdout.
>> The only problem is code duplication in many-many places.
>> May be add simple function in ethdev to do it and use it in
>> all examples?
> Also we can introduce new printf specifier using
> register_printf_specifier function or even rewrite %u which will print
> UNKNOWN in case of
> UINT32_MAX.
True, but it is too libc-specific as far as I know.
>> Please, wait more feedback before doing it.
>>
>>
04.05.2020 18:54, Andrew Rybchenko пишет:
> On 5/4/20 6:46 PM, Ivan Dyukov wrote:
>> 03.05.2020 16:57, Andrew Rybchenko пишет:
>>> On 5/2/20 10:35 PM, Ivan Dyukov wrote:
>>>> 01.05.2020 16:28, Andrew Rybchenko пишет:
>>>>> On 4/27/20 12:57 PM, Ivan Dyukov wrote:
>>>>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>>>>> ---
>>>>>> doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst
>>>>>> b/doc/guides/sample_app_ug/link_status_intr.rst
>>>>>> index 5283be8b7..6ebc707b7 100644
>>>>>> --- a/doc/guides/sample_app_ug/link_status_intr.rst
>>>>>> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
>>>>>> @@ -177,7 +177,8 @@ An example callback function that has been
>>>>>> written as indicated below.
>>>>>> printf("Failed to get port %d link status: %s\n\n",
>>>>>> port_id, rte_strerror(-ret));
>>>>>> } else if (link.link_status) {
>>>>>> - printf("Port %d Link Up - speed %u Mbps - %s\n\n",
>>>>>> port_id, (unsigned)link.link_speed,
>>>>>> + printf("Port %d Link Up - speed %u%s - %s\n\n", port_id,
>>>>>> (unsigned)link.link_speed,
>>>>>> + (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") :
>>>>>> (" Mbps"),
>>>>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>>>>> ("full-duplex") : ("half-duplex"));
>>>>>> } else
>>>>>> printf("Port %d Link Down\n\n", port_id);
>>>>>>
>>>>> I think that 0 looks nicer than UINT32_MAX when printed as integer
>>>>> keeping in mind that it is unknown.
>>>>>
>>>> zero will mislead developers about real value of the link_speed.
>>>> therefore we should print real value of the speed or print nothing. e.g.
>>>>
>>>> if (link.link_speed == UINT32_MAX)
>>>>
>>>> printf("Port %d Link Up - speed UNKNOWN - %s\n\n", port_id,
>>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>>> ("full-duplex") : ("half-duplex"));
>>>> else
>>>>
>>>> printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id,
>>>> link.link_speed,
>>>>
>>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>>> ("full-duplex") : ("half-duplex"));
>>> I'm not sure about 0 to be misleading, but it could be.
>>> Above definitely will look better in stdout.
>>> The only problem is code duplication in many-many places.
>>> May be add simple function in ethdev to do it and use it in
>>> all examples?
>> Also we can introduce new printf specifier using
>> register_printf_specifier function or even rewrite %u which will print
>> UNKNOWN in case of
>> UINT32_MAX.
> True, but it is too libc-specific as far as I know.
Yep, it has portability issues. It's GNU C extension. https://www.gnu.org/software/libc/manual/html_node/Customizing-Printf.html
>
>>> Please, wait more feedback before doing it.
>>>
>>>
>
On 5/3/2020 2:57 PM, Andrew Rybchenko wrote:
> On 5/2/20 10:35 PM, Ivan Dyukov wrote:
>> 01.05.2020 16:28, Andrew Rybchenko пишет:
>>> On 4/27/20 12:57 PM, Ivan Dyukov wrote:
>>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>>> ---
>>>> doc/guides/sample_app_ug/link_status_intr.rst | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst
>>>> b/doc/guides/sample_app_ug/link_status_intr.rst
>>>> index 5283be8b7..6ebc707b7 100644
>>>> --- a/doc/guides/sample_app_ug/link_status_intr.rst
>>>> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
>>>> @@ -177,7 +177,8 @@ An example callback function that has been
>>>> written as indicated below.
>>>> printf("Failed to get port %d link status: %s\n\n",
>>>> port_id, rte_strerror(-ret));
>>>> } else if (link.link_status) {
>>>> - printf("Port %d Link Up - speed %u Mbps - %s\n\n",
>>>> port_id, (unsigned)link.link_speed,
>>>> + printf("Port %d Link Up - speed %u%s - %s\n\n", port_id,
>>>> (unsigned)link.link_speed,
>>>> + (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") :
>>>> (" Mbps"),
>>>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>>> ("full-duplex") : ("half-duplex"));
>>>> } else
>>>> printf("Port %d Link Down\n\n", port_id);
>>>>
>>> I think that 0 looks nicer than UINT32_MAX when printed as integer
>>> keeping in mind that it is unknown.
>>>
>> zero will mislead developers about real value of the link_speed.
>> therefore we should print real value of the speed or print nothing. e.g.
>>
>> if (link.link_speed == UINT32_MAX)
>>
>> printf("Port %d Link Up - speed UNKNOWN - %s\n\n", port_id,
>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>> ("full-duplex") : ("half-duplex"));
>> else
>>
>> printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id,
>> link.link_speed,
>>
>> (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>> ("full-duplex") : ("half-duplex"));
>
> I'm not sure about 0 to be misleading, but it could be.
> Above definitely will look better in stdout.
+1
> The only problem is code duplication in many-many places.
+1
What about print signed value, so it will be "-1 UNKNOWN", not too bad as log
but concern is if it is confusing again?
> May be add simple function in ethdev to do it and use it in
> all examples?
>
> Please, wait more feedback before doing it.
>
@@ -177,7 +177,8 @@ An example callback function that has been written as indicated below.
printf("Failed to get port %d link status: %s\n\n",
port_id, rte_strerror(-ret));
} else if (link.link_status) {
- printf("Port %d Link Up - speed %u Mbps - %s\n\n", port_id, (unsigned)link.link_speed,
+ printf("Port %d Link Up - speed %u%s - %s\n\n", port_id, (unsigned)link.link_speed,
+ (link.link_speed == UINT32_MAX) ? ("(UNKNOWN)") : (" Mbps"),
(link.link_duplex == ETH_LINK_FULL_DUPLEX) ? ("full-duplex") : ("half-duplex"));
} else
printf("Port %d Link Down\n\n", port_id);