[dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices

[dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices

[Morten Brørup]

Thomas, Ferruh, Andrew (Ethernet API Maintainers),

A command line option was recently added to set which speed a vNIC reports when the link is up. This makes sense for Spanning Tree and other protocols which depend on link speed.

However, I suspect that this workaround rarely reflects the physical truth, and suggest that the application should handle it instead.

In other words... Instead of faking it in the virtual Ethernet drivers, I suggest that rte_ethdev.h defines a special speed value for vNICs which really don't have a physical link speed:

#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
+#define ETH_SPEED_NUM_UNKNOWN      1 /**< Unknown (virtual device) */
#define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */

Alternatively, we could expand the meaning of ETH_SPEED_NUM_NONE:

-#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
+#define ETH_SPEED_NUM_NONE         0 /**< Not defined or unknown (virtual device) */

The special value could also be used in cases like this:
http://inbox.dpdk.org/dev/AM0PR0502MB401907ADE7CEA27DC642DF35D2CB0@AM0PR0502MB4019.eurprd05.prod.outlook.com/T/#t


Med venlig hilsen / kind regards
- Morten Brørup

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices

[Thomas Monjalon]

01/04/2020 11:33, Morten Brørup:
> Thomas, Ferruh, Andrew (Ethernet API Maintainers),
> 
> A command line option was recently added to set which speed a vNIC reports when the link is up. This makes sense for Spanning Tree and other protocols which depend on link speed.

Please could you reference the patch?

> However, I suspect that this workaround rarely reflects the physical truth, and suggest that the application should handle it instead.

I don't understand why we need to define some speed for virtual devices.

> In other words... Instead of faking it in the virtual Ethernet drivers, I suggest that rte_ethdev.h defines a special speed value for vNICs which really don't have a physical link speed:
> 
> #define ETH_SPEED_NUM_NONE         0 /**< Not defined */

The only issue with this constant is the lack of RTE_ prefix :-)
Otherwise I think "0 - NONE - not defined" fits well with virtual device case.

> +#define ETH_SPEED_NUM_UNKNOWN      1 /**< Unknown (virtual device) */

1 means 1 Mbps

> #define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
> 
> Alternatively, we could expand the meaning of ETH_SPEED_NUM_NONE:
> 
> -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
> +#define ETH_SPEED_NUM_NONE         0 /**< Not defined or unknown (virtual device) */

Yes I agree with extending the comment for NONE.

> The special value could also be used in cases like this:
> http://inbox.dpdk.org/dev/AM0PR0502MB401907ADE7CEA27DC642DF35D2CB0@AM0PR0502MB4019.eurprd05.prod.outlook.com/T/#t

Yes, if speed is unknown, it should be reported as 0.

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices

[Benoit Ganne (bganne)]

Hi all,

>> Alternatively, we could expand the meaning of ETH_SPEED_NUM_NONE:
>> -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
>> +#define ETH_SPEED_NUM_NONE         0 /**< Not defined or unknown
>> (virtual device) */
> Yes I agree with extending the comment for NONE.
>> The special value could also be used in cases like this:
>>
>> http://inbox.dpdk.org/dev/AM0PR0502MB401907ADE7CEA27DC642DF35D2CB0@AM0PR0502MB4019.eurprd05.prod.outlook.com/T/#t
> Yes, if speed is unknown, it should be reported as 0.

Ok that should answer Matan concerns: it is perfectly fine for a PMD to return ETH_SPEED_NUM_NONE if it does not have better information.
From my app writer point of view it makes sense.

Best
ben

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Wednesday, April 1, 2020 11:54 AM
> 
> 01/04/2020 11:33, Morten Brørup:
> > Thomas, Ferruh, Andrew (Ethernet API Maintainers),
> >
> > A command line option was recently added to set which speed a vNIC
> reports when the link is up. This makes sense for Spanning Tree and
> other protocols which depend on link speed.
> 
> Please could you reference the patch?

It is a patch for the virtio driver:
http://inbox.dpdk.org/dev/20191212085012.9170-1-i.dyukov@samsung.com/T/#m052f90ea8c559406aeaefaea1fc24ed9bb573788

> 
> > However, I suspect that this workaround rarely reflects the physical
> truth, and suggest that the application should handle it instead.
> 
> I don't understand why we need to define some speed for virtual
> devices.
> 
> > In other words... Instead of faking it in the virtual Ethernet
> drivers, I suggest that rte_ethdev.h defines a special speed value for
> vNICs which really don't have a physical link speed:
> >
> > #define ETH_SPEED_NUM_NONE         0 /**< Not defined */
> 
> The only issue with this constant is the lack of RTE_ prefix :-)
> Otherwise I think "0 - NONE - not defined" fits well with virtual
> device case.
> 
> > +#define ETH_SPEED_NUM_UNKNOWN      1 /**< Unknown (virtual device)
> */
> 
> 1 means 1 Mbps
> 
> > #define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
> >
> > Alternatively, we could expand the meaning of ETH_SPEED_NUM_NONE:
> >
> > -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
> > +#define ETH_SPEED_NUM_NONE         0 /**< Not defined or unknown
> (virtual device) */
> 
> Yes I agree with extending the comment for NONE.
> 
> > The special value could also be used in cases like this:
> >
> http://inbox.dpdk.org/dev/AM0PR0502MB401907ADE7CEA27DC642DF35D2CB0@AM0P
> R0502MB4019.eurprd05.prod.outlook.com/T/#t
> 
> Yes, if speed is unknown, it should be reported as 0.

So the next related question is: Should a vNIC be allowed to report a fake speed if it does not know that the underlying hardware actually provides this speed?

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Ivan Dyukov]

01.04.2020 13:06, Morten Brørup пишет:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
>> Sent: Wednesday, April 1, 2020 11:54 AM
>>
>> 01/04/2020 11:33, Morten Brørup:
>>> Thomas, Ferruh, Andrew (Ethernet API Maintainers),
>>>
>>> A command line option was recently added to set which speed a vNIC
>> reports when the link is up. This makes sense for Spanning Tree and
>> other protocols which depend on link speed.
>>
>> Please could you reference the patch?
> It is a patch for the virtio driver:
> https://protect2.fireeye.com/url?k=e37beb37-beabe3df-e37a6078-000babff32e3-4aaaa0986ed7ec57&u=http://inbox.dpdk.org/dev/20191212085012.9170-1-i.dyukov@samsung.com/T/#m052f90ea8c559406aeaefaea1fc24ed9bb573788
This patch is related to similar work in qemu & kernel virtio driver. 
Please see 
https://lists.oasis-open.org/archives/virtio-comment/201911/msg00058.html. 
These changes have been implemented and released in kernel and qemu. 
speed is negotiated from qemu and then user may change the speed of 
virtio device using ethtool utility. I have added similiar patchset for 
pmd driver which do the same but for changing speed I used devargs 
instead of ethtool.
>
>>> However, I suspect that this workaround rarely reflects the physical
>> truth, and suggest that the application should handle it instead.
>>
>> I don't understand why we need to define some speed for virtual
>> devices.
>>
>>> In other words... Instead of faking it in the virtual Ethernet
>> drivers, I suggest that rte_ethdev.h defines a special speed value for
>> vNICs which really don't have a physical link speed:
>>> #define ETH_SPEED_NUM_NONE         0 /**< Not defined */
>> The only issue with this constant is the lack of RTE_ prefix :-)
>> Otherwise I think "0 - NONE - not defined" fits well with virtual
>> device case.
>>
>>> +#define ETH_SPEED_NUM_UNKNOWN      1 /**< Unknown (virtual device)
>> */
>>
>> 1 means 1 Mbps
>>
>>> #define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
>>>
>>> Alternatively, we could expand the meaning of ETH_SPEED_NUM_NONE:
>>>
>>> -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
>>> +#define ETH_SPEED_NUM_NONE         0 /**< Not defined or unknown
>> (virtual device) */
>>
>> Yes I agree with extending the comment for NONE.
>>
>>> The special value could also be used in cases like this:
>>>
>> https://protect2.fireeye.com/url?k=6154668d-3c846e65-6155edc2-000babff32e3-bf63d034253cac80&u=http://inbox.dpdk.org/dev/AM0PR0502MB401907ADE7CEA27DC642DF35D2CB0@AM0P
>> R0502MB4019.eurprd05.prod.outlook.com/T/#t
>>
>> Yes, if speed is unknown, it should be reported as 0.
> So the next related question is: Should a vNIC be allowed to report a fake speed if it does not know that the underlying hardware actually provides this speed?
>

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
> Sent: Thursday, April 2, 2020 2:54 PM
> 
> 01.04.2020 13:06, Morten Brørup пишет:
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> >> Sent: Wednesday, April 1, 2020 11:54 AM
> >>
> >> 01/04/2020 11:33, Morten Brørup:
> >>> Thomas, Ferruh, Andrew (Ethernet API Maintainers),
> >>>
> >>> A command line option was recently added to set which speed a vNIC
> >> reports when the link is up. This makes sense for Spanning Tree and
> >> other protocols which depend on link speed.
> >>
> >> Please could you reference the patch?
> > It is a patch for the virtio driver:
> > https://protect2.fireeye.com/url?k=e37beb37-beabe3df-e37a6078-
> 000babff32e3-
> 4aaaa0986ed7ec57&u=http://inbox.dpdk.org/dev/20191212085012.9170-1-
> i.dyukov@samsung.com/T/#m052f90ea8c559406aeaefaea1fc24ed9bb573788
> This patch is related to similar work in qemu & kernel virtio driver.
> Please see
> https://lists.oasis-open.org/archives/virtio-
> comment/201911/msg00058.html.
> These changes have been implemented and released in kernel and qemu.
> speed is negotiated from qemu and then user may change the speed of
> virtio device using ethtool utility. I have added similiar patchset for
> pmd driver which do the same but for changing speed I used devargs
> instead of ethtool.

Very interesting link, indeed!

It gives the virtio driver the possibility to expose a specific speed in Mbit/s, which I assume - when used correctly - should reflect the speed of the underlying hardware. So it could be 20 Gbit/s for a link aggregate (in IEEE 802.3 Ethernet terminology; "bond" in Linux terminology) of two 10G ports.

It also provides a special value for unknown speed.

> >
> >>> However, I suspect that this workaround rarely reflects the
> physical
> >> truth, and suggest that the application should handle it instead.
> >>
> >> I don't understand why we need to define some speed for virtual
> >> devices.
> >>
> >>> In other words... Instead of faking it in the virtual Ethernet
> >> drivers, I suggest that rte_ethdev.h defines a special speed value
> for
> >> vNICs which really don't have a physical link speed:
> >>> #define ETH_SPEED_NUM_NONE         0 /**< Not defined */
> >> The only issue with this constant is the lack of RTE_ prefix :-)
> >> Otherwise I think "0 - NONE - not defined" fits well with virtual
> >> device case.
> >>
> >>> +#define ETH_SPEED_NUM_UNKNOWN      1 /**< Unknown (virtual device)
> >> */
> >>
> >> 1 means 1 Mbps
> >>
> >>> #define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
> >>>
> >>> Alternatively, we could expand the meaning of ETH_SPEED_NUM_NONE:
> >>>
> >>> -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
> >>> +#define ETH_SPEED_NUM_NONE         0 /**< Not defined or unknown
> >> (virtual device) */
> >>
> >> Yes I agree with extending the comment for NONE.
> >>
> >>> The special value could also be used in cases like this:
> >>>
> >> https://protect2.fireeye.com/url?k=6154668d-3c846e65-6155edc2-
> 000babff32e3-
> bf63d034253cac80&u=http://inbox.dpdk.org/dev/AM0PR0502MB401907ADE7CEA27
> DC642DF35D2CB0@AM0P
> >> R0502MB4019.eurprd05.prod.outlook.com/T/#t
> >>
> >> Yes, if speed is unknown, it should be reported as 0.

Could the DPDK vNIC PMDs be updated accordingly? At least the virtio driver...

> > So the next related question is: Should a vNIC be allowed to report a
> fake speed if it does not know that the underlying hardware actually
> provides this speed?

Your link to the Kernel/QEMU patch answers this question: Yes, because we assume it reflects the underlying speed.

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Stephen Hemminger]

On Thu, 2 Apr 2020 15:50:00 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
> > Sent: Thursday, April 2, 2020 2:54 PM
> > 
> > 01.04.2020 13:06, Morten Brørup пишет:  
> > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > >> Sent: Wednesday, April 1, 2020 11:54 AM
> > >>
> > >> 01/04/2020 11:33, Morten Brørup:  
> > >>> Thomas, Ferruh, Andrew (Ethernet API Maintainers),
> > >>>
> > >>> A command line option was recently added to set which speed a vNIC  
> > >> reports when the link is up. This makes sense for Spanning Tree and
> > >> other protocols which depend on link speed.
> > >>
> > >> Please could you reference the patch?  
> > > It is a patch for the virtio driver:
> > > https://protect2.fireeye.com/url?k=e37beb37-beabe3df-e37a6078-  
> > 000babff32e3-
> > 4aaaa0986ed7ec57&u=http://inbox.dpdk.org/dev/20191212085012.9170-1-
> > i.dyukov@samsung.com/T/#m052f90ea8c559406aeaefaea1fc24ed9bb573788
> > This patch is related to similar work in qemu & kernel virtio driver.
> > Please see
> > https://lists.oasis-open.org/archives/virtio-
> > comment/201911/msg00058.html.
> > These changes have been implemented and released in kernel and qemu.
> > speed is negotiated from qemu and then user may change the speed of
> > virtio device using ethtool utility. I have added similiar patchset for
> > pmd driver which do the same but for changing speed I used devargs
> > instead of ethtool.  
> 
> Very interesting link, indeed!
> 
> It gives the virtio driver the possibility to expose a specific speed in Mbit/s, which I assume - when used correctly - should reflect the speed of the underlying hardware. So it could be 20 Gbit/s for a link aggregate (in IEEE 802.3 Ethernet terminology; "bond" in Linux terminology) of two 10G ports.
> 
> It also provides a special value for unknown speed.
> 
> > >  
> > >>> However, I suspect that this workaround rarely reflects the  
> > physical  
> > >> truth, and suggest that the application should handle it instead.
> > >>
> > >> I don't understand why we need to define some speed for virtual
> > >> devices.
> > >>  
> > >>> In other words... Instead of faking it in the virtual Ethernet  
> > >> drivers, I suggest that rte_ethdev.h defines a special speed value  
> > for  
> > >> vNICs which really don't have a physical link speed:  
> > >>> #define ETH_SPEED_NUM_NONE         0 /**< Not defined */  
> > >> The only issue with this constant is the lack of RTE_ prefix :-)
> > >> Otherwise I think "0 - NONE - not defined" fits well with virtual
> > >> device case.
> > >>  
> > >>> +#define ETH_SPEED_NUM_UNKNOWN      1 /**< Unknown (virtual device)  
> > >> */
> > >>
> > >> 1 means 1 Mbps
> > >>  
> > >>> #define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
> > >>>
> > >>> Alternatively, we could expand the meaning of ETH_SPEED_NUM_NONE:
> > >>>
> > >>> -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
> > >>> +#define ETH_SPEED_NUM_NONE         0 /**< Not defined or unknown  
> > >> (virtual device) */
> > >>
> > >> Yes I agree with extending the comment for NONE.
> > >>  
> > >>> The special value could also be used in cases like this:
> > >>>  
> > >> https://protect2.fireeye.com/url?k=6154668d-3c846e65-6155edc2-  
> > 000babff32e3-
> > bf63d034253cac80&u=http://inbox.dpdk.org/dev/AM0PR0502MB401907ADE7CEA27
> > DC642DF35D2CB0@AM0P  
> > >> R0502MB4019.eurprd05.prod.outlook.com/T/#t
> > >>
> > >> Yes, if speed is unknown, it should be reported as 0.  
> 
> Could the DPDK vNIC PMDs be updated accordingly? At least the virtio driver...
> 
> > > So the next related question is: Should a vNIC be allowed to report a  
> > fake speed if it does not know that the underlying hardware actually
> > provides this speed?  
> 
> Your link to the Kernel/QEMU patch answers this question: Yes, because we assume it reflects the underlying speed.
> 
> 

The problem is that if devices start returning this new value,
then it is an API breakage for existing applications that look at speed.

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Ivan Dyukov]

02.04.2020 16:50, Morten Brørup пишет:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
>> Sent: Thursday, April 2, 2020 2:54 PM
>>
>> 01.04.2020 13:06, Morten Brørup пишет:
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
>>>> Sent: Wednesday, April 1, 2020 11:54 AM
>>>>
>>>> 01/04/2020 11:33, Morten Brørup:
>>>>> Thomas, Ferruh, Andrew (Ethernet API Maintainers),
>>>>>
>>>>> A command line option was recently added to set which speed a vNIC
>>>> reports when the link is up. This makes sense for Spanning Tree and
>>>> other protocols which depend on link speed.
>>>>
>>>> Please could you reference the patch?
>>> It is a patch for the virtio driver:
>>> https://protect2.fireeye.com/url?k=e37beb37-beabe3df-e37a6078-
>> 000babff32e3-
>> 4aaaa0986ed7ec57&u=https://protect2.fireeye.com/url?k=b77dd9ba-ea1a9d9c-b77c52f5-0cc47a31384a-83e04670981c13ea&u=http://inbox.dpdk.org/dev/20191212085012.9170-1-
>> i.dyukov@samsung.com/T/#m052f90ea8c559406aeaefaea1fc24ed9bb573788
>> This patch is related to similar work in qemu & kernel virtio driver.
>> Please see
>> https://protect2.fireeye.com/url?k=1e499600-432ed226-1e481d4f-0cc47a31384a-5c4e61a49cf9986a&u=https://lists.oasis-open.org/archives/virtio-
>> comment/201911/msg00058.html.
>> These changes have been implemented and released in kernel and qemu.
>> speed is negotiated from qemu and then user may change the speed of
>> virtio device using ethtool utility. I have added similiar patchset for
>> pmd driver which do the same but for changing speed I used devargs
>> instead of ethtool.
> Very interesting link, indeed!
>
> It gives the virtio driver the possibility to expose a specific speed in Mbit/s, which I assume - when used correctly - should reflect the speed of the underlying hardware. So it could be 20 Gbit/s for a link aggregate (in IEEE 802.3 Ethernet terminology; "bond" in Linux terminology) of two 10G ports.
>
> It also provides a special value for unknown speed.
>
>>>>> However, I suspect that this workaround rarely reflects the
>> physical
>>>> truth, and suggest that the application should handle it instead.
>>>>
>>>> I don't understand why we need to define some speed for virtual
>>>> devices.
>>>>
>>>>> In other words... Instead of faking it in the virtual Ethernet
>>>> drivers, I suggest that rte_ethdev.h defines a special speed value
>> for
>>>> vNICs which really don't have a physical link speed:
>>>>> #define ETH_SPEED_NUM_NONE         0 /**< Not defined */
>>>> The only issue with this constant is the lack of RTE_ prefix :-)
>>>> Otherwise I think "0 - NONE - not defined" fits well with virtual
>>>> device case.
>>>>
>>>>> +#define ETH_SPEED_NUM_UNKNOWN      1 /**< Unknown (virtual device)
>>>> */
>>>>
>>>> 1 means 1 Mbps
>>>>
>>>>> #define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
>>>>>
>>>>> Alternatively, we could expand the meaning of ETH_SPEED_NUM_NONE:
>>>>>
>>>>> -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
>>>>> +#define ETH_SPEED_NUM_NONE         0 /**< Not defined or unknown
>>>> (virtual device) */
>>>>
>>>> Yes I agree with extending the comment for NONE.
>>>>
>>>>> The special value could also be used in cases like this:
>>>>>
>>>> https://protect2.fireeye.com/url?k=6154668d-3c846e65-6155edc2-
>> 000babff32e3-
>> bf63d034253cac80&u=https://protect2.fireeye.com/url?k=13709c46-4e17d860-13711709-0cc47a31384a-b18536864970b070&u=http://inbox.dpdk.org/dev/AM0PR0502MB401907ADE7CEA27
>> DC642DF35D2CB0@AM0P
>>>> R0502MB4019.eurprd05.prod.outlook.com/T/#t
>>>>
>>>> Yes, if speed is unknown, it should be reported as 0.
> Could the DPDK vNIC PMDs be updated accordingly? At least the virtio driver...
Current version of dpdk code on master always returns 10G speed for 
virtio device and many application rely on it. e.g. pktgen. If we'll 
change it, we break the apps.
>>> So the next related question is: Should a vNIC be allowed to report a
>> fake speed if it does not know that the underlying hardware actually
>> provides this speed?
> Your link to the Kernel/QEMU patch answers this question: Yes, because we assume it reflects the underlying speed.
>
>

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Thomas Monjalon]

02/04/2020 22:41, Ivan Dyukov:
> 02.04.2020 16:50, Morten Brørup пишет:
> >>>> Yes, if speed is unknown, it should be reported as 0.
> > 
> > Could the DPDK vNIC PMDs be updated accordingly? At least the virtio driver...
> 
> Current version of dpdk code on master always returns 10G speed for 
> virtio device and many application rely on it. e.g. pktgen. If we'll 
> change it, we break the apps.

I am OK with breaking such strange assumption.
I can understand the need for specifying the underlying hardware speed
through virtio driver. But hardcoded 10G... no!

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Ivan Dyukov]

02.04.2020 23:58, Thomas Monjalon пишет:
> 02/04/2020 22:41, Ivan Dyukov:
>> 02.04.2020 16:50, Morten Brørup пишет:
>>>>>> Yes, if speed is unknown, it should be reported as 0.
>>> Could the DPDK vNIC PMDs be updated accordingly? At least the virtio driver...
>> Current version of dpdk code on master always returns 10G speed for
>> virtio device and many application rely on it. e.g. pktgen. If we'll
>> change it, we break the apps.
> I am OK with breaking such strange assumption.
> I can understand the need for specifying the underlying hardware speed
> through virtio driver. But hardcoded 10G... no!
>
>
>
OK. I'll redefine it to 0xffffffff, like in kernel virtio.

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
> Sent: Friday, April 3, 2020 10:06 AM
> 
> 02.04.2020 23:58, Thomas Monjalon пишет:
> > 02/04/2020 22:41, Ivan Dyukov:
> >> 02.04.2020 16:50, Morten Brørup пишет:
> >>>>>> Yes, if speed is unknown, it should be reported as 0.
> >>> Could the DPDK vNIC PMDs be updated accordingly? At least the
> virtio driver...
> >> Current version of dpdk code on master always returns 10G speed for
> >> virtio device and many application rely on it. e.g. pktgen. If we'll
> >> change it, we break the apps.
> > I am OK with breaking such strange assumption.
> > I can understand the need for specifying the underlying hardware
> speed
> > through virtio driver. But hardcoded 10G... no!
> >
> >
> >
> OK. I'll redefine it to 0xffffffff, like in kernel virtio.
> 

Thomas, you were opposed to using 1 as the special value for "unknown", as it is likely interpreted as 1 Mbps instead, and I agree with your reasoning on that.

Since using an extremely large value is as close to infinity we can get, Ivan's suggested value makes sense to me instead:

+#define ETH_SPEED_NUM_UNKNOWN      0xffffffff /**< Unknown */

In theory, it also addresses Stephen's concern about breaking the ABI for existing applications that look at speed. They will get a non-zero speed, which is not a randomly chosen fake 10G speed; so I consider it an improvement.
Although in reality, such applications may still break if they are unable to handle the extreme speed of 4.3 Pbps.

I considered "Unlimited" instead of "Unknown", but Unlimited is not really correct, so I settled with Unknown.

Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices

[Thomas Monjalon]

03/04/2020 11:45, Morten Brørup:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
> > Sent: Friday, April 3, 2020 10:06 AM
> > 
> > 02.04.2020 23:58, Thomas Monjalon пишет:
> > > 02/04/2020 22:41, Ivan Dyukov:
> > >> 02.04.2020 16:50, Morten Brørup пишет:
> > >>>>>> Yes, if speed is unknown, it should be reported as 0.
> > >>> Could the DPDK vNIC PMDs be updated accordingly? At least the
> > virtio driver...
> > >> Current version of dpdk code on master always returns 10G speed for
> > >> virtio device and many application rely on it. e.g. pktgen. If we'll
> > >> change it, we break the apps.
> > > I am OK with breaking such strange assumption.
> > > I can understand the need for specifying the underlying hardware
> > speed
> > > through virtio driver. But hardcoded 10G... no!
> > >
> > >
> > >
> > OK. I'll redefine it to 0xffffffff, like in kernel virtio.
> > 
> 
> Thomas, you were opposed to using 1 as the special value for "unknown", as it is likely interpreted as 1 Mbps instead, and I agree with your reasoning on that.
> 
> Since using an extremely large value is as close to infinity we can get, Ivan's suggested value makes sense to me instead:
> 
> +#define ETH_SPEED_NUM_UNKNOWN      0xffffffff /**< Unknown */
> 
> In theory, it also addresses Stephen's concern about breaking the ABI for existing applications that look at speed. They will get a non-zero speed, which is not a randomly chosen fake 10G speed; so I consider it an improvement.
> Although in reality, such applications may still break if they are unable to handle the extreme speed of 4.3 Pbps.
> 
> I considered "Unlimited" instead of "Unknown", but Unlimited is not really correct, so I settled with Unknown.

Yes it makes sense.
The only drawback is that the information "speed is accurate" will
be checked against two constants:
	ETH_SPEED_NUM_NONE or ETH_SPEED_NUM_UNKNOWN
It can be mitigated with a function though.

[dpdk-dev] [PATCH 0/2] ethdev link speed

[Thomas Monjalon]

Following 2 discussions about link speed,
here is an API update and a small clean-up.

Morten's proposal:
http://inbox.dpdk.org/dev/4957996.Zugxlq0yvN@xps/
Recent Ivan's virtio patch assuming the same API update:
http://inbox.dpdk.org/dev/20200406085855.25773-6-i.dyukov@samsung.com/
Benoit's proposal to return mlx5 link status even without speed info:
http://inbox.dpdk.org/dev/20200327172449.6514-1-bganne@cisco.com/

Thomas Monjalon (2):
  ethdev: deduplicate functions to get link infos
  ethdev: allow unknown link speed

 lib/librte_ethdev/rte_ethdev.c | 52 ++++++++++++++--------------------
 lib/librte_ethdev/rte_ethdev.h | 20 ++++++-------
 2 files changed, 32 insertions(+), 40 deletions(-)

-- 
2.26.0

[dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos

[Thomas Monjalon]

There are two function to retrieve link informations.
The only small difference is a boolean timeout parameter.
Adding a new static function, with an additional parameter,
removes the code redundancy.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c | 52 ++++++++++++++--------------------
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 0854ef8832..0df39dff97 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -2332,44 +2332,36 @@ rte_eth_allmulticast_get(uint16_t port_id)
 	return dev->data->all_multicast;
 }
 
+static int
+get_link_infos(uint16_t port_id, struct rte_eth_link *eth_link, int wait)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (dev->data->dev_conf.intr_conf.lsc &&
+	    dev->data->dev_started)
+		rte_eth_linkstatus_get(dev, eth_link);
+	else {
+		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
+		(*dev->dev_ops->link_update)(dev, wait);
+		*eth_link = dev->data->dev_link;
+	}
+
+	return 0;
+}
+
 int
 rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
 {
-	struct rte_eth_dev *dev;
-
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
-	dev = &rte_eth_devices[port_id];
-
-	if (dev->data->dev_conf.intr_conf.lsc &&
-	    dev->data->dev_started)
-		rte_eth_linkstatus_get(dev, eth_link);
-	else {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
-		(*dev->dev_ops->link_update)(dev, 1);
-		*eth_link = dev->data->dev_link;
-	}
-
-	return 0;
+	return get_link_infos(port_id, eth_link, 1);
 }
 
 int
 rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
 {
-	struct rte_eth_dev *dev;
-
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
-	dev = &rte_eth_devices[port_id];
-
-	if (dev->data->dev_conf.intr_conf.lsc &&
-	    dev->data->dev_started)
-		rte_eth_linkstatus_get(dev, eth_link);
-	else {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
-		(*dev->dev_ops->link_update)(dev, 0);
-		*eth_link = dev->data->dev_link;
-	}
-
-	return 0;
+	return get_link_infos(port_id, eth_link, 0);
 }
 
 int
-- 
2.26.0

Re: [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos

[Asaf Penso]

Thank you, Thomas, for taking care of this.
PSB.

Regards,
Asaf Penso

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> Sent: Wednesday, April 8, 2020 1:27 AM
> To: dev@dpdk.org
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link
> infos
> 
> There are two function to retrieve link informations.
> The only small difference is a boolean timeout parameter.
> Adding a new static function, with an additional parameter,
> removes the code redundancy.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 52 ++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 0854ef8832..0df39dff97 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -2332,44 +2332,36 @@ rte_eth_allmulticast_get(uint16_t port_id)
>  	return dev->data->all_multicast;
>  }
> 
> +static int
> +get_link_infos(uint16_t port_id, struct rte_eth_link *eth_link, int wait)

I would recommend renaming to link_get_infos, to have the same naming convention as rte_eth_*link_get* and rte_eth_*link_get*_nowait

> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (dev->data->dev_conf.intr_conf.lsc &&
> +	    dev->data->dev_started)
> +		rte_eth_linkstatus_get(dev, eth_link);
> +	else {
> +		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >link_update, -ENOTSUP);
> +		(*dev->dev_ops->link_update)(dev, wait);
> +		*eth_link = dev->data->dev_link;
> +	}
> +
> +	return 0;

Since it's a static function, I think it can return void, and the calling functions can decide what to return, but it's a matter of taste.
Do we want to check that the return value for eth_link is not NULL and return -1 in case it is?

> +}
> +
>  int
>  rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>  {
> -	struct rte_eth_dev *dev;
> -
> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> -	dev = &rte_eth_devices[port_id];
> -
> -	if (dev->data->dev_conf.intr_conf.lsc &&
> -	    dev->data->dev_started)
> -		rte_eth_linkstatus_get(dev, eth_link);
> -	else {
> -		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >link_update, -ENOTSUP);
> -		(*dev->dev_ops->link_update)(dev, 1);
> -		*eth_link = dev->data->dev_link;
> -	}
> -
> -	return 0;
> +	return get_link_infos(port_id, eth_link, 1);
>  }
> 
>  int
>  rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
>  {
> -	struct rte_eth_dev *dev;
> -
> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> -	dev = &rte_eth_devices[port_id];
> -
> -	if (dev->data->dev_conf.intr_conf.lsc &&
> -	    dev->data->dev_started)
> -		rte_eth_linkstatus_get(dev, eth_link);
> -	else {
> -		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >link_update, -ENOTSUP);
> -		(*dev->dev_ops->link_update)(dev, 0);
> -		*eth_link = dev->data->dev_link;
> -	}
> -
> -	return 0;
> +	return get_link_infos(port_id, eth_link, 0);
>  }
> 
>  int
> --
> 2.26.0

Re: [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos

[Thomas Monjalon]

08/04/2020 07:21, Asaf Penso:
> From: Thomas Monjalon
> > +static int
> > +get_link_infos(uint16_t port_id, struct rte_eth_link *eth_link, int wait)
> 
> I would recommend renaming to link_get_infos, to have the same naming
> convention as rte_eth_*link_get* and rte_eth_*link_get*_nowait

No strong opinion.
get_link_infos looks more natural english.
If others prefer to have a sort of consistency, fine.

> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +	dev = &rte_eth_devices[port_id];
> > +
> > +	if (dev->data->dev_conf.intr_conf.lsc &&
> > +	    dev->data->dev_started)
> > +		rte_eth_linkstatus_get(dev, eth_link);
> > +	else {
> > +		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> > >link_update, -ENOTSUP);
> > +		(*dev->dev_ops->link_update)(dev, wait);
> > +		*eth_link = dev->data->dev_link;
> > +	}
> > +
> > +	return 0;
> 
> Since it's a static function, I think it can return void,

No, it cannot return void because some errors may be returned
with the macros RTE_ETH_VALID_PORTID_OR_ERR_RET and
RTE_FUNC_PTR_OR_ERR_RET.

> and the calling functions can decide what to return,
> but it's a matter of taste.
> 
> Do we want to check that the return value for eth_link
> is not NULL and return -1 in case it is?

eth_link must not be NULL. It is allocated by the caller.

Re: [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos

[Andrew Rybchenko]

On 4/8/20 1:26 AM, Thomas Monjalon wrote:
> There are two function to retrieve link informations.
> The only small difference is a boolean timeout parameter.
> Adding a new static function, with an additional parameter,
> removes the code redundancy.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

and I'm fine with get_link_infos() function name since
there a number of ^get_*() static functions in the file.

[dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed

[Thomas Monjalon]

When querying the link informations, the link status is
a mandatory major information.
Other boolean values are supposed to be accurate:
	- duplex mode (half/full)
	- negotiation (auto/fixed)

This API update is making explicit that the link speed information
is optional.
The value ETH_SPEED_NUM_NONE (0) was already part of the API.
The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover
two different cases:
	- speed is not known by the driver
	- device is virtual

Suggested-by: Morten Brørup <mb@smartsharesystems.com>
Suggested-by: Benoit Ganne <bganne@cisco.com>
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593ad11..2d51fd3444 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -300,6 +300,7 @@ struct rte_eth_stats {
 #define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
 #define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
 #define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
+#define ETH_SPEED_NUM_UNKNOWN UINT32_MAX /**< Unknown */
 
 /**
  * A structure used to retrieve link-level information of an Ethernet port.
@@ -2245,15 +2246,16 @@ int rte_eth_allmulticast_disable(uint16_t port_id);
 int rte_eth_allmulticast_get(uint16_t port_id);
 
 /**
- * Retrieve the status (ON/OFF), the speed (in Mbps) and the mode (HALF-DUPLEX
- * or FULL-DUPLEX) of the physical link of an Ethernet device. It might need
- * to wait up to 9 seconds in it.
+ * Retrieve the link status (up/down), the duplex mode (half/full),
+ * the negotiation (auto/fixed), and if available, the speed (Mbps).
+ *
+ * It might need to wait up to 9 seconds.
+ * @see rte_eth_link_get_nowait.
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param link
- *   A pointer to an *rte_eth_link* structure to be filled with
- *   the status, the speed and the mode of the Ethernet device link.
+ *   Link informations written back.
  * @return
  *   - (0) if successful.
  *   - (-ENOTSUP) if the function is not supported in PMD driver.
@@ -2262,15 +2264,13 @@ int rte_eth_allmulticast_get(uint16_t port_id);
 int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
 
 /**
- * Retrieve the status (ON/OFF), the speed (in Mbps) and the mode (HALF-DUPLEX
- * or FULL-DUPLEX) of the physical link of an Ethernet device. It is a no-wait
- * version of rte_eth_link_get().
+ * Retrieve the link status (up/down), the duplex mode (half/full),
+ * the negotiation (auto/fixed), and if available, the speed (Mbps).
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param link
- *   A pointer to an *rte_eth_link* structure to be filled with
- *   the status, the speed and the mode of the Ethernet device link.
+ *   Link informations written back.
  * @return
  *   - (0) if successful.
  *   - (-ENOTSUP) if the function is not supported in PMD driver.
-- 
2.26.0

Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed

[Jerin Jacob]

On Wed, Apr 8, 2020 at 3:57 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> When querying the link informations, the link status is
> a mandatory major information.
> Other boolean values are supposed to be accurate:
>         - duplex mode (half/full)
>         - negotiation (auto/fixed)
>
> This API update is making explicit that the link speed information
> is optional.
> The value ETH_SPEED_NUM_NONE (0) was already part of the API.
> The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover
> two different cases:
>         - speed is not known by the driver
>         - device is virtual

LGTM.

>
> Suggested-by: Morten Brørup <mb@smartsharesystems.com>
> Suggested-by: Benoit Ganne <bganne@cisco.com>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad11..2d51fd3444 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -300,6 +300,7 @@ struct rte_eth_stats {
>  #define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
>  #define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
>  #define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
> +#define ETH_SPEED_NUM_UNKNOWN UINT32_MAX /**< Unknown */

IMO, It is better to add the following information in the same or
other words under @note in Doxygen comment
for the release documentation perspective.

The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover
two different cases:
        - speed is not known by the driver
        - device is virtual

Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, April 8, 2020 12:27 AM
> 
> When querying the link informations, the link status is
> a mandatory major information.
> Other boolean values are supposed to be accurate:
> 	- duplex mode (half/full)
> 	- negotiation (auto/fixed)
> 
> This API update is making explicit that the link speed information
> is optional.
> The value ETH_SPEED_NUM_NONE (0) was already part of the API.
> The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover
> two different cases:
> 	- speed is not known by the driver
> 	- device is virtual
> 
> Suggested-by: Morten Brørup <mb@smartsharesystems.com>
> Suggested-by: Benoit Ganne <bganne@cisco.com>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h
> b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad11..2d51fd3444 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -300,6 +300,7 @@ struct rte_eth_stats {
>  #define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
>  #define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
>  #define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
> +#define ETH_SPEED_NUM_UNKNOWN UINT32_MAX /**< Unknown */
> 
>  /**
>   * A structure used to retrieve link-level information of an Ethernet
> port.
> @@ -2245,15 +2246,16 @@ int rte_eth_allmulticast_disable(uint16_t
> port_id);
>  int rte_eth_allmulticast_get(uint16_t port_id);
> 
>  /**
> - * Retrieve the status (ON/OFF), the speed (in Mbps) and the mode
> (HALF-DUPLEX
> - * or FULL-DUPLEX) of the physical link of an Ethernet device. It
> might need
> - * to wait up to 9 seconds in it.
> + * Retrieve the link status (up/down), the duplex mode (half/full),
> + * the negotiation (auto/fixed), and if available, the speed (Mbps).
> + *
> + * It might need to wait up to 9 seconds.
> + * @see rte_eth_link_get_nowait.
>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param link
> - *   A pointer to an *rte_eth_link* structure to be filled with
> - *   the status, the speed and the mode of the Ethernet device link.
> + *   Link informations written back.

informations -> information

>   * @return
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
> @@ -2262,15 +2264,13 @@ int rte_eth_allmulticast_get(uint16_t port_id);
>  int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
> 
>  /**
> - * Retrieve the status (ON/OFF), the speed (in Mbps) and the mode
> (HALF-DUPLEX
> - * or FULL-DUPLEX) of the physical link of an Ethernet device. It is a
> no-wait
> - * version of rte_eth_link_get().
> + * Retrieve the link status (up/down), the duplex mode (half/full),
> + * the negotiation (auto/fixed), and if available, the speed (Mbps).
>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param link
> - *   A pointer to an *rte_eth_link* structure to be filled with
> - *   the status, the speed and the mode of the Ethernet device link.
> + *   Link informations written back.

informations -> information

>   * @return
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
> --
> 2.26.0
> 

Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed

[Andrew Rybchenko]

On 4/8/20 1:26 AM, Thomas Monjalon wrote:
> When querying the link informations, the link status is
> a mandatory major information.
> Other boolean values are supposed to be accurate:
> 	- duplex mode (half/full)
> 	- negotiation (auto/fixed)
> 
> This API update is making explicit that the link speed information
> is optional.
> The value ETH_SPEED_NUM_NONE (0) was already part of the API.
> The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover
> two different cases:
> 	- speed is not known by the driver
> 	- device is virtual
> 
> Suggested-by: Morten Brørup <mb@smartsharesystems.com>
> Suggested-by: Benoit Ganne <bganne@cisco.com>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

I'm afraid it requires more efforts to keep things consistent,
e.g.:
 - printing of link speed in proc-info
 - comparison of rate vs link_speed in testpmd
 - printing of link speed in testpmd as %u Mbps
 - division by 1000 in test-pipeline
 - link speed printing in doc/guides/sample_app_ug/link_status_intr.rst
 - link speed printing in many-many examples

IMHO, it is not nice to print UING32_MAX and let use
guess what it means.

Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed

[Ferruh Yigit]

On 4/13/2020 3:26 PM, Andrew Rybchenko wrote:
> On 4/8/20 1:26 AM, Thomas Monjalon wrote:
>> When querying the link informations, the link status is
>> a mandatory major information.
>> Other boolean values are supposed to be accurate:
>> 	- duplex mode (half/full)
>> 	- negotiation (auto/fixed)
>>
>> This API update is making explicit that the link speed information
>> is optional.
>> The value ETH_SPEED_NUM_NONE (0) was already part of the API.
>> The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover
>> two different cases:
>> 	- speed is not known by the driver
>> 	- device is virtual
>>
>> Suggested-by: Morten Brørup <mb@smartsharesystems.com>
>> Suggested-by: Benoit Ganne <bganne@cisco.com>
>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> I'm afraid it requires more efforts to keep things consistent,
> e.g.:
>  - printing of link speed in proc-info
>  - comparison of rate vs link_speed in testpmd
>  - printing of link speed in testpmd as %u Mbps
>  - division by 1000 in test-pipeline
>  - link speed printing in doc/guides/sample_app_ug/link_status_intr.rst
>  - link speed printing in many-many examples
> 
> IMHO, it is not nice to print UING32_MAX and let use
> guess what it means.
> 

+1

Hi Thomas,

The two patches in the set seems unrelated, and first one looks OK, do you want
to separate them so that this doesn't block that one?

Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed

[Matan Azrad]

Hi

From: Thomas Monjalon
> When querying the link informations, the link status is a mandatory major
> information.
> Other boolean values are supposed to be accurate:
> 	- duplex mode (half/full)
> 	- negotiation (auto/fixed)
> 
> This API update is making explicit that the link speed information is optional.
> The value ETH_SPEED_NUM_NONE (0) was already part of the API.
> The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover two
> different cases:
> 	- speed is not known by the driver
> 	- device is virtual
> 
> Suggested-by: Morten Brørup <mb@smartsharesystems.com>
> Suggested-by: Benoit Ganne <bganne@cisco.com>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad11..2d51fd3444 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -300,6 +300,7 @@ struct rte_eth_stats {
>  #define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
>  #define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
>  #define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
> +#define ETH_SPEED_NUM_UNKNOWN UINT32_MAX /**< Unknown */
> 
>  /**
>   * A structure used to retrieve link-level information of an Ethernet port.
> @@ -2245,15 +2246,16 @@ int rte_eth_allmulticast_disable(uint16_t
> port_id);  int rte_eth_allmulticast_get(uint16_t port_id);
> 
>  /**
> - * Retrieve the status (ON/OFF), the speed (in Mbps) and the mode (HALF-
> DUPLEX
> - * or FULL-DUPLEX) of the physical link of an Ethernet device. It might need
> - * to wait up to 9 seconds in it.
> + * Retrieve the link status (up/down), the duplex mode (half/full),
> + * the negotiation (auto/fixed), and if available, the speed (Mbps).
> + *
> + * It might need to wait up to 9 seconds.
> + * @see rte_eth_link_get_nowait.
>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param link
> - *   A pointer to an *rte_eth_link* structure to be filled with
> - *   the status, the speed and the mode of the Ethernet device link.
> + *   Link informations written back.
>   * @return
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
> @@ -2262,15 +2264,13 @@ int rte_eth_allmulticast_get(uint16_t port_id);
> int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
> 
>  /**
> - * Retrieve the status (ON/OFF), the speed (in Mbps) and the mode (HALF-
> DUPLEX
> - * or FULL-DUPLEX) of the physical link of an Ethernet device. It is a no-wait
> - * version of rte_eth_link_get().
> + * Retrieve the link status (up/down), the duplex mode (half/full),
> + * the negotiation (auto/fixed), and if available, the speed (Mbps).

The current API doesn’t say that link speed is optional, so no link speed information means error in API.
When making link speed optional, it may break existing app that expects to get error in API when link speed is not available in order to call the API again.
So I think it breaks API.
I agree for the change but think that this is better to integrate it in 20.11 after sending prior deprecation notice.

Matan


>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param link
> - *   A pointer to an *rte_eth_link* structure to be filled with
> - *   the status, the speed and the mode of the Ethernet device link.
> + *   Link informations written back.
>   * @return
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
> --
> 2.26.0