* virtio_net: ethtool supported link modes @ 2017-08-31 17:04 Radu Rendec 2017-09-01 3:36 ` Jason Wang 2017-09-01 15:43 ` Michael S. Tsirkin 0 siblings, 2 replies; 7+ messages in thread From: Radu Rendec @ 2017-08-31 17:04 UTC (permalink / raw) To: virtualization, netdev, linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang Hello, Looking at the code in virtnet_set_link_ksettings, it seems the speed and duplex can be set to any valid value. The driver will "remember" them and report them back in virtnet_get_link_ksettings. However, the supported link modes (link_modes.supported in struct ethtool_link_ksettings) is always 0, indicating that no speed/duplex setting is supported. Does it make more sense to set (at least a few of) the supported link modes, such as 10baseT_Half ... 10000baseT_Full? I would expect to see consistency between what is reported in link_modes.supported and what can actually be set. Could you please share your opinion on this? Thank you, Radu Rendec ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: virtio_net: ethtool supported link modes 2017-08-31 17:04 virtio_net: ethtool supported link modes Radu Rendec @ 2017-09-01 3:36 ` Jason Wang 2017-09-01 12:01 ` Radu Rendec 2017-09-01 15:43 ` Michael S. Tsirkin 1 sibling, 1 reply; 7+ messages in thread From: Jason Wang @ 2017-09-01 3:36 UTC (permalink / raw) To: Radu Rendec, virtualization, netdev, linux-kernel; +Cc: Michael S. Tsirkin On 2017年09月01日 01:04, Radu Rendec wrote: > Hello, > > Looking at the code in virtnet_set_link_ksettings, it seems the speed > and duplex can be set to any valid value. The driver will "remember" > them and report them back in virtnet_get_link_ksettings. > > However, the supported link modes (link_modes.supported in struct > ethtool_link_ksettings) is always 0, indicating that no speed/duplex > setting is supported. > > Does it make more sense to set (at least a few of) the supported link > modes, such as 10baseT_Half ... 10000baseT_Full? > > I would expect to see consistency between what is reported in > link_modes.supported and what can actually be set. Could you please > share your opinion on this? I think the may make sense only if there's a hardware implementation for virtio. And we probably need to extend virtio spec for adding new commands. Thanks > > Thank you, > Radu Rendec > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: virtio_net: ethtool supported link modes 2017-09-01 3:36 ` Jason Wang @ 2017-09-01 12:01 ` Radu Rendec 0 siblings, 0 replies; 7+ messages in thread From: Radu Rendec @ 2017-09-01 12:01 UTC (permalink / raw) To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization, netdev, linux-kernel On Fri, 2017-09-01 at 11:36 +0800, Jason Wang wrote: > > On 2017年09月01日 01:04, Radu Rendec wrote: > > Hello, > > > > Looking at the code in virtnet_set_link_ksettings, it seems the speed > > and duplex can be set to any valid value. The driver will "remember" > > them and report them back in virtnet_get_link_ksettings. > > > > However, the supported link modes (link_modes.supported in struct > > ethtool_link_ksettings) is always 0, indicating that no speed/duplex > > setting is supported. > > > > Does it make more sense to set (at least a few of) the supported link > > modes, such as 10baseT_Half ... 10000baseT_Full? > > > > I would expect to see consistency between what is reported in > > link_modes.supported and what can actually be set. Could you please > > share your opinion on this? > > I think the may make sense only if there's a hardware implementation for > virtio. And we probably need to extend virtio spec for adding new commands. So you're saying that the "hardware" should provide the supported link modes (e.g. by using feature bits at the virtio layer) and the virtio_net driver should just translate them and expose them as link_modes.supported? Then for consistency, I assume setting speed/duplex via ethtool should also go into the virtio layer (currently virtio_net seems to just store them for future retrieval via ethtool). Thanks, Radu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: virtio_net: ethtool supported link modes 2017-08-31 17:04 virtio_net: ethtool supported link modes Radu Rendec 2017-09-01 3:36 ` Jason Wang @ 2017-09-01 15:43 ` Michael S. Tsirkin 2017-09-01 16:19 ` Radu Rendec 1 sibling, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2017-09-01 15:43 UTC (permalink / raw) To: Radu Rendec; +Cc: virtualization, netdev, linux-kernel, Jason Wang, virtio-dev On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote: > Hello, > > Looking at the code in virtnet_set_link_ksettings, it seems the speed > and duplex can be set to any valid value. The driver will "remember" > them and report them back in virtnet_get_link_ksettings. > > However, the supported link modes (link_modes.supported in struct > ethtool_link_ksettings) is always 0, indicating that no speed/duplex > setting is supported. > > Does it make more sense to set (at least a few of) the supported link > modes, such as 10baseT_Half ... 10000baseT_Full? > > I would expect to see consistency between what is reported in > link_modes.supported and what can actually be set. Could you please > share your opinion on this? > > Thank you, > Radu Rendec I would like to know more about why this is desirable. We used not to support the modes at all, but it turned out some tools are confused by this: e.g. people would try to bond virtio with a hardware device, tools would see a mismatch in speed and features between bonded devices and get confused. See commit 16032be56c1f66770da15cb94f0eb366c37aff6e Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Date: Wed Feb 3 04:04:37 2016 +0100 virtio_net: add ethtool support for set and get of settings as well as the discussion around it https://www.spinics.net/lists/netdev/msg362111.html If you think we need to add more hacks like this, a stronger motivation than "to see consistency" would be needed. -- MST ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: virtio_net: ethtool supported link modes 2017-09-01 15:43 ` Michael S. Tsirkin @ 2017-09-01 16:19 ` Radu Rendec 2017-09-01 17:45 ` Michael S. Tsirkin 0 siblings, 1 reply; 7+ messages in thread From: Radu Rendec @ 2017-09-01 16:19 UTC (permalink / raw) To: Michael S. Tsirkin Cc: virtualization, netdev, linux-kernel, Jason Wang, virtio-dev On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote: > On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote: > > Looking at the code in virtnet_set_link_ksettings, it seems the speed > > and duplex can be set to any valid value. The driver will "remember" > > them and report them back in virtnet_get_link_ksettings. > > > > However, the supported link modes (link_modes.supported in struct > > ethtool_link_ksettings) is always 0, indicating that no speed/duplex > > setting is supported. > > > > Does it make more sense to set (at least a few of) the supported link > > modes, such as 10baseT_Half ... 10000baseT_Full? > > > > I would expect to see consistency between what is reported in > > link_modes.supported and what can actually be set. Could you please > > share your opinion on this? > > I would like to know more about why this is desirable. > > We used not to support the modes at all, but it turned out > some tools are confused by this: e.g. people would try to > bond virtio with a hardware device, tools would see > a mismatch in speed and features between bonded devices > and get confused. > > See > > commit 16032be56c1f66770da15cb94f0eb366c37aff6e > Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > Date: Wed Feb 3 04:04:37 2016 +0100 > > virtio_net: add ethtool support for set and get of settings > > > as well as the discussion around it > https://www.spinics.net/lists/netdev/msg362111.html Thanks for pointing these out. It is much more clear now why modes support is implemented the way it is and what the expectations are. > If you think we need to add more hacks like this, a stronger > motivation than "to see consistency" would be needed. The use case behind my original question is very simple: * Net device is queried via ethtool for supported modes. * Supported modes are presented to user. * User can configure any of the supported modes. This is done transparently to the net device type (driver), so it actually makes sense for physical NICs. This alone of course is not a good enough motivation to modify the driver. And it can be easily addressed in user-space at the application level by testing for the driver. I was merely trying to avoid driver-specific workarounds (i.e. keep the application driver agnostic) and wondered if "advertising" supported modes through ethtool made any sense and/or would be a desirable change from the driver perspective. I believe I have my answers now. Thanks, Radu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: virtio_net: ethtool supported link modes 2017-09-01 16:19 ` Radu Rendec @ 2017-09-01 17:45 ` Michael S. Tsirkin 2017-09-04 14:59 ` Radu Rendec 0 siblings, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2017-09-01 17:45 UTC (permalink / raw) To: Radu Rendec; +Cc: virtualization, netdev, linux-kernel, Jason Wang, virtio-dev On Fri, Sep 01, 2017 at 05:19:53PM +0100, Radu Rendec wrote: > On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote: > > On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote: > > > Looking at the code in virtnet_set_link_ksettings, it seems the speed > > > and duplex can be set to any valid value. The driver will "remember" > > > them and report them back in virtnet_get_link_ksettings. > > > > > > However, the supported link modes (link_modes.supported in struct > > > ethtool_link_ksettings) is always 0, indicating that no speed/duplex > > > setting is supported. > > > > > > Does it make more sense to set (at least a few of) the supported link > > > modes, such as 10baseT_Half ... 10000baseT_Full? > > > > > > I would expect to see consistency between what is reported in > > > link_modes.supported and what can actually be set. Could you please > > > share your opinion on this? > > > > I would like to know more about why this is desirable. > > > > We used not to support the modes at all, but it turned out > > some tools are confused by this: e.g. people would try to > > bond virtio with a hardware device, tools would see > > a mismatch in speed and features between bonded devices > > and get confused. > > > > See > > > > commit 16032be56c1f66770da15cb94f0eb366c37aff6e > > Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > > Date: Wed Feb 3 04:04:37 2016 +0100 > > > > virtio_net: add ethtool support for set and get of settings > > > > > > as well as the discussion around it > > https://www.spinics.net/lists/netdev/msg362111.html > > Thanks for pointing these out. It is much more clear now why modes > support is implemented the way it is and what the expectations are. > > > If you think we need to add more hacks like this, a stronger > > motivation than "to see consistency" would be needed. > > The use case behind my original question is very simple: > * Net device is queried via ethtool for supported modes. > * Supported modes are presented to user. > * User can configure any of the supported modes. Since this has no effect on virtio, isn't presenting "no supported modes" to user the right thing to do? > This is done transparently to the net device type (driver), so it > actually makes sense for physical NICs. > > This alone of course is not a good enough motivation to modify the > driver. And it can be easily addressed in user-space at the application > level by testing for the driver. I think you might want to special-case no supported modes. Special-casing virtio is probably best avoided. > I was merely trying to avoid driver-specific workarounds (i.e. keep the > application driver agnostic) I think that's the right approach. So if driver does not present any supported modes this probably means it is not necessary to display or program any. > and wondered if "advertising" supported > modes through ethtool made any sense and/or would be a desirable change > from the driver perspective. I believe I have my answers now. > > Thanks, > Radu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: virtio_net: ethtool supported link modes 2017-09-01 17:45 ` Michael S. Tsirkin @ 2017-09-04 14:59 ` Radu Rendec 0 siblings, 0 replies; 7+ messages in thread From: Radu Rendec @ 2017-09-04 14:59 UTC (permalink / raw) To: Michael S. Tsirkin Cc: virtualization, netdev, linux-kernel, Jason Wang, virtio-dev On Fri, 2017-09-01 at 20:45 +0300, Michael S. Tsirkin wrote: > On Fri, Sep 01, 2017 at 05:19:53PM +0100, Radu Rendec wrote: > > On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote: > > > On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote: > > > > Looking at the code in virtnet_set_link_ksettings, it seems the speed > > > > and duplex can be set to any valid value. The driver will "remember" > > > > them and report them back in virtnet_get_link_ksettings. > > > > > > > > However, the supported link modes (link_modes.supported in struct > > > > ethtool_link_ksettings) is always 0, indicating that no speed/duplex > > > > setting is supported. > > > > > > > > Does it make more sense to set (at least a few of) the supported link > > > > modes, such as 10baseT_Half ... 10000baseT_Full? > > > > > > > > I would expect to see consistency between what is reported in > > > > link_modes.supported and what can actually be set. Could you please > > > > share your opinion on this? > > > > The use case behind my original question is very simple: > > * Net device is queried via ethtool for supported modes. > > * Supported modes are presented to user. > > * User can configure any of the supported modes. > > Since this has no effect on virtio, isn't presenting > "no supported modes" to user the right thing to do? Yes, that makes sense. > > This is done transparently to the net device type (driver), so it > > actually makes sense for physical NICs. > > > > This alone of course is not a good enough motivation to modify the > > driver. And it can be easily addressed in user-space at the application > > level by testing for the driver. > > I think you might want to special-case no supported modes. > Special-casing virtio is probably best avoided. > > > I was merely trying to avoid driver-specific workarounds (i.e. keep the > > application driver agnostic) > > I think that's the right approach. So if driver does not present > any supported modes this probably means it is not necessary > to display or program any. Yes, apparently it boils down to special-casing no supported modes. This avoids both modifying virtio and special-casing virtio, and keeps the application driver-agnostic at the same time. Thanks for all the feedback. It was very helpful in figuring out the right approach. I really appreciate it. Radu ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-09-04 14:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-31 17:04 virtio_net: ethtool supported link modes Radu Rendec 2017-09-01 3:36 ` Jason Wang 2017-09-01 12:01 ` Radu Rendec 2017-09-01 15:43 ` Michael S. Tsirkin 2017-09-01 16:19 ` Radu Rendec 2017-09-01 17:45 ` Michael S. Tsirkin 2017-09-04 14:59 ` Radu Rendec
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).