DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices
@ 2020-04-01  9:33 Morten Brørup
  2020-04-01  9:53 ` Thomas Monjalon
  0 siblings, 1 reply; 23+ messages in thread
From: Morten Brørup @ 2020-04-01  9:33 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev
  Cc: Matan Azrad, Benoit Ganne (bganne)

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices
  2020-04-01  9:33 [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices Morten Brørup
@ 2020-04-01  9:53 ` Thomas Monjalon
  2020-04-01 10:03   ` Benoit Ganne (bganne)
  2020-04-01 10:06   ` [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices Morten Brørup
  0 siblings, 2 replies; 23+ messages in thread
From: Thomas Monjalon @ 2020-04-01  9:53 UTC (permalink / raw)
  To: Ferruh Yigit, Andrew Rybchenko, Morten Brørup
  Cc: dev, Matan Azrad, Benoit Ganne (bganne)

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.




^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices
  2020-04-01  9:53 ` Thomas Monjalon
@ 2020-04-01 10:03   ` Benoit Ganne (bganne)
  2020-04-01 10:06   ` [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices Morten Brørup
  1 sibling, 0 replies; 23+ messages in thread
From: Benoit Ganne (bganne) @ 2020-04-01 10:03 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Morten Brørup
  Cc: dev, Matan Azrad

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-01  9:53 ` Thomas Monjalon
  2020-04-01 10:03   ` Benoit Ganne (bganne)
@ 2020-04-01 10:06   ` Morten Brørup
  2020-04-02 12:54     ` Ivan Dyukov
  1 sibling, 1 reply; 23+ messages in thread
From: Morten Brørup @ 2020-04-01 10:06 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, Matan Azrad, Benoit Ganne (bganne),
	Ivan Dyukov, maxime.coquelin, tiwei.bie, amorenoz, zhihong.wang,
	xiaolong.ye

> 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?


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-01 10:06   ` [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices Morten Brørup
@ 2020-04-02 12:54     ` Ivan Dyukov
  2020-04-02 13:50       ` Morten Brørup
  0 siblings, 1 reply; 23+ messages in thread
From: Ivan Dyukov @ 2020-04-02 12:54 UTC (permalink / raw)
  To: Morten Brørup, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, Matan Azrad, Benoit Ganne (bganne),
	maxime.coquelin, tiwei.bie, amorenoz, zhihong.wang, xiaolong.ye

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?
>


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-02 12:54     ` Ivan Dyukov
@ 2020-04-02 13:50       ` Morten Brørup
  2020-04-02 15:29         ` Stephen Hemminger
  2020-04-02 20:41         ` Ivan Dyukov
  0 siblings, 2 replies; 23+ messages in thread
From: Morten Brørup @ 2020-04-02 13:50 UTC (permalink / raw)
  To: Ivan Dyukov, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, Matan Azrad, Benoit Ganne (bganne),
	maxime.coquelin, tiwei.bie, amorenoz, zhihong.wang, xiaolong.ye

> 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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-02 13:50       ` Morten Brørup
@ 2020-04-02 15:29         ` Stephen Hemminger
  2020-04-02 20:41         ` Ivan Dyukov
  1 sibling, 0 replies; 23+ messages in thread
From: Stephen Hemminger @ 2020-04-02 15:29 UTC (permalink / raw)
  To: Morten Brørup
  Cc: Ivan Dyukov, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko,
	dev, Matan Azrad, Benoit Ganne (bganne),
	maxime.coquelin, tiwei.bie, amorenoz, zhihong.wang, xiaolong.ye

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.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-02 13:50       ` Morten Brørup
  2020-04-02 15:29         ` Stephen Hemminger
@ 2020-04-02 20:41         ` Ivan Dyukov
  2020-04-02 20:58           ` Thomas Monjalon
  1 sibling, 1 reply; 23+ messages in thread
From: Ivan Dyukov @ 2020-04-02 20:41 UTC (permalink / raw)
  To: Morten Brørup, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, Matan Azrad, Benoit Ganne (bganne),
	maxime.coquelin, 'Vladimir Kuramshin',
	amorenoz, zhihong.wang, xiaolong.ye

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.
>
>


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-02 20:41         ` Ivan Dyukov
@ 2020-04-02 20:58           ` Thomas Monjalon
  2020-04-03  8:05             ` Ivan Dyukov
  0 siblings, 1 reply; 23+ messages in thread
From: Thomas Monjalon @ 2020-04-02 20:58 UTC (permalink / raw)
  To: Morten Brørup, Ferruh Yigit, Andrew Rybchenko, Ivan Dyukov
  Cc: dev, Matan Azrad, Benoit Ganne (bganne),
	maxime.coquelin, 'Vladimir Kuramshin',
	amorenoz, zhihong.wang, xiaolong.ye

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!



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-02 20:58           ` Thomas Monjalon
@ 2020-04-03  8:05             ` Ivan Dyukov
  2020-04-03  9:45               ` Morten Brørup
  0 siblings, 1 reply; 23+ messages in thread
From: Ivan Dyukov @ 2020-04-03  8:05 UTC (permalink / raw)
  To: Thomas Monjalon, Morten Brørup, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, Matan Azrad, Benoit Ganne (bganne),
	maxime.coquelin, 'Vladimir Kuramshin',
	amorenoz, zhihong.wang, xiaolong.ye

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.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-03  8:05             ` Ivan Dyukov
@ 2020-04-03  9:45               ` Morten Brørup
  2020-04-03 11:01                 ` Thomas Monjalon
  2020-04-07 22:26                 ` [dpdk-dev] [PATCH 0/2] ethdev link speed Thomas Monjalon
  0 siblings, 2 replies; 23+ messages in thread
From: Morten Brørup @ 2020-04-03  9:45 UTC (permalink / raw)
  To: Ivan Dyukov, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, Matan Azrad, Benoit Ganne (bganne),
	maxime.coquelin, Vladimir Kuramshin, amorenoz, zhihong.wang,
	xiaolong.ye, Stephen Hemminger

> 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.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices
  2020-04-03  9:45               ` Morten Brørup
@ 2020-04-03 11:01                 ` Thomas Monjalon
  2020-04-07 22:26                 ` [dpdk-dev] [PATCH 0/2] ethdev link speed Thomas Monjalon
  1 sibling, 0 replies; 23+ messages in thread
From: Thomas Monjalon @ 2020-04-03 11:01 UTC (permalink / raw)
  To: Ivan Dyukov, Morten Brørup
  Cc: Ferruh Yigit, Andrew Rybchenko, dev, Matan Azrad,
	Benoit Ganne (bganne),
	maxime.coquelin, Vladimir Kuramshin, amorenoz, zhihong.wang,
	xiaolong.ye, Stephen Hemminger

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [dpdk-dev] [PATCH 0/2] ethdev link speed
  2020-04-03  9:45               ` Morten Brørup
  2020-04-03 11:01                 ` Thomas Monjalon
@ 2020-04-07 22:26                 ` Thomas Monjalon
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos Thomas Monjalon
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed Thomas Monjalon
  1 sibling, 2 replies; 23+ messages in thread
From: Thomas Monjalon @ 2020-04-07 22:26 UTC (permalink / raw)
  To: dev

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos
  2020-04-07 22:26                 ` [dpdk-dev] [PATCH 0/2] ethdev link speed Thomas Monjalon
@ 2020-04-07 22:26                   ` Thomas Monjalon
  2020-04-08  5:21                     ` Asaf Penso
  2020-04-13 14:14                     ` Andrew Rybchenko
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed Thomas Monjalon
  1 sibling, 2 replies; 23+ messages in thread
From: Thomas Monjalon @ 2020-04-07 22:26 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Andrew Rybchenko

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed
  2020-04-07 22:26                 ` [dpdk-dev] [PATCH 0/2] ethdev link speed Thomas Monjalon
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos Thomas Monjalon
@ 2020-04-07 22:26                   ` Thomas Monjalon
  2020-04-08  5:39                     ` Jerin Jacob
                                       ` (3 more replies)
  1 sibling, 4 replies; 23+ messages in thread
From: Thomas Monjalon @ 2020-04-07 22:26 UTC (permalink / raw)
  To: dev; +Cc: Morten Brørup, Benoit Ganne, Ferruh Yigit, Andrew Rybchenko

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos Thomas Monjalon
@ 2020-04-08  5:21                     ` Asaf Penso
  2020-04-08 12:49                       ` Thomas Monjalon
  2020-04-13 14:14                     ` Andrew Rybchenko
  1 sibling, 1 reply; 23+ messages in thread
From: Asaf Penso @ 2020-04-08  5:21 UTC (permalink / raw)
  To: Thomas Monjalon, dev; +Cc: Ferruh Yigit, Andrew Rybchenko

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed Thomas Monjalon
@ 2020-04-08  5:39                     ` Jerin Jacob
  2020-04-10 10:53                     ` Morten Brørup
                                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 23+ messages in thread
From: Jerin Jacob @ 2020-04-08  5:39 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dpdk-dev, Morten Brørup, Benoit Ganne, Ferruh Yigit,
	Andrew Rybchenko

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos
  2020-04-08  5:21                     ` Asaf Penso
@ 2020-04-08 12:49                       ` Thomas Monjalon
  0 siblings, 0 replies; 23+ messages in thread
From: Thomas Monjalon @ 2020-04-08 12:49 UTC (permalink / raw)
  To: Asaf Penso; +Cc: dev, Ferruh Yigit, Andrew Rybchenko

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.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed Thomas Monjalon
  2020-04-08  5:39                     ` Jerin Jacob
@ 2020-04-10 10:53                     ` Morten Brørup
  2020-04-13 14:26                     ` Andrew Rybchenko
  2020-04-26 11:44                     ` Matan Azrad
  3 siblings, 0 replies; 23+ messages in thread
From: Morten Brørup @ 2020-04-10 10:53 UTC (permalink / raw)
  To: Thomas Monjalon, dev; +Cc: Benoit Ganne, Ferruh Yigit, Andrew Rybchenko

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos Thomas Monjalon
  2020-04-08  5:21                     ` Asaf Penso
@ 2020-04-13 14:14                     ` Andrew Rybchenko
  1 sibling, 0 replies; 23+ messages in thread
From: Andrew Rybchenko @ 2020-04-13 14:14 UTC (permalink / raw)
  To: Thomas Monjalon, dev; +Cc: Ferruh Yigit

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.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed Thomas Monjalon
  2020-04-08  5:39                     ` Jerin Jacob
  2020-04-10 10:53                     ` Morten Brørup
@ 2020-04-13 14:26                     ` Andrew Rybchenko
  2020-04-16 13:42                       ` Ferruh Yigit
  2020-04-26 11:44                     ` Matan Azrad
  3 siblings, 1 reply; 23+ messages in thread
From: Andrew Rybchenko @ 2020-04-13 14:26 UTC (permalink / raw)
  To: Thomas Monjalon, dev; +Cc: Morten Brørup, Benoit Ganne, Ferruh Yigit

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.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed
  2020-04-13 14:26                     ` Andrew Rybchenko
@ 2020-04-16 13:42                       ` Ferruh Yigit
  0 siblings, 0 replies; 23+ messages in thread
From: Ferruh Yigit @ 2020-04-16 13:42 UTC (permalink / raw)
  To: Andrew Rybchenko, Thomas Monjalon, dev; +Cc: Morten Brørup, Benoit Ganne

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?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed
  2020-04-07 22:26                   ` [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed Thomas Monjalon
                                       ` (2 preceding siblings ...)
  2020-04-13 14:26                     ` Andrew Rybchenko
@ 2020-04-26 11:44                     ` Matan Azrad
  3 siblings, 0 replies; 23+ messages in thread
From: Matan Azrad @ 2020-04-26 11:44 UTC (permalink / raw)
  To: Thomas Monjalon, dev
  Cc: Morten Brørup, Benoit Ganne, Ferruh Yigit, Andrew Rybchenko

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-04-26 11:44 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01  9:33 [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernet devices Morten Brørup
2020-04-01  9:53 ` Thomas Monjalon
2020-04-01 10:03   ` Benoit Ganne (bganne)
2020-04-01 10:06   ` [dpdk-dev] [RFC] ethdev: use special speed for virtual Ethernetdevices Morten Brørup
2020-04-02 12:54     ` Ivan Dyukov
2020-04-02 13:50       ` Morten Brørup
2020-04-02 15:29         ` Stephen Hemminger
2020-04-02 20:41         ` Ivan Dyukov
2020-04-02 20:58           ` Thomas Monjalon
2020-04-03  8:05             ` Ivan Dyukov
2020-04-03  9:45               ` Morten Brørup
2020-04-03 11:01                 ` Thomas Monjalon
2020-04-07 22:26                 ` [dpdk-dev] [PATCH 0/2] ethdev link speed Thomas Monjalon
2020-04-07 22:26                   ` [dpdk-dev] [PATCH 1/2] ethdev: deduplicate functions to get link infos Thomas Monjalon
2020-04-08  5:21                     ` Asaf Penso
2020-04-08 12:49                       ` Thomas Monjalon
2020-04-13 14:14                     ` Andrew Rybchenko
2020-04-07 22:26                   ` [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed Thomas Monjalon
2020-04-08  5:39                     ` Jerin Jacob
2020-04-10 10:53                     ` Morten Brørup
2020-04-13 14:26                     ` Andrew Rybchenko
2020-04-16 13:42                       ` Ferruh Yigit
2020-04-26 11:44                     ` Matan Azrad

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).