Re: virtio_net: ethtool supported link modes

[Radu Rendec]

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

Re: virtio_net: ethtool supported link modes

[Radu Rendec]

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

Re: virtio_net: ethtool supported link modes

[Michael S. Tsirkin]

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

Re: virtio_net: ethtool supported link modes

[Michael S. Tsirkin]

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

Re: virtio_net: ethtool supported link modes

[Radu Rendec]

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

Re: virtio_net: ethtool supported link modes

[Radu Rendec]

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

Re: virtio_net: ethtool supported link modes

[Michael S. Tsirkin]

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

Re: virtio_net: ethtool supported link modes

[Michael S. Tsirkin]

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

Re: virtio_net: ethtool supported link modes

[Radu Rendec]

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

Re: virtio_net: ethtool supported link modes

[Radu Rendec]

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

Re: virtio_net: ethtool supported link modes

[Jason Wang]

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

Re: virtio_net: ethtool supported link modes

[Jason Wang]

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

virtio_net: ethtool supported link modes

[Radu Rendec]

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

virtio_net: ethtool supported link modes

[Radu Rendec]

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