Netlink handler for ethtool --show-rxfh breaks driver compatibility

Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Vladimir Oltean]

Hi,

Commit ffab99c1f382 ("netlink: add netlink handler for get rss (-x)") in
the ethtool user space binary breaks compatibility with device drivers.

Namely, before the change, ethtool --show-rxfh did not emit a
ETHTOOL_MSG_CHANNELS_GET netlink message or even the ETHTOOL_GCHANNELS
ioctl variant. Now it does, and this effectively forces a new
requirement for drivers to implement ethtool_ops :: get_channels() in
the kernel.

The following drivers implement ethtool_ops :: get_rxfh() but not
ethtool_ops :: get_channels():
- drivers/net/ethernet/microchip/lan743x_ethtool.c
- drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
- drivers/net/ethernet/marvell/mvneta.c
- drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
- drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
- drivers/net/ethernet/sfc/ef100_ethtool.c
- drivers/net/ethernet/sfc/falcon/ethtool.c
- drivers/net/ethernet/sfc/siena/ethtool.c
- drivers/net/ethernet/sfc/ethtool.c
- drivers/net/ethernet/intel/ixgbevf/ethtool.c

Thus, for them, this is a breaking ABI change which must be addressed.

A demo for the enetc driver.

Before:
  $ ethtool --show-rxfh eno0
  RX flow hash indirection table for eno0 with 2 RX ring(s):
      0:      0     1     0     1     0     1     0     1
      8:      0     1     0     1     0     1     0     1
     16:      0     1     0     1     0     1     0     1
     24:      0     1     0     1     0     1     0     1
     32:      0     1     0     1     0     1     0     1
     40:      0     1     0     1     0     1     0     1
     48:      0     1     0     1     0     1     0     1
     56:      0     1     0     1     0     1     0     1
  RSS hash key:
  0d:1f:cb:76:88:82:dd:ea:70:c9:ef:53:3e:f3:bf:60:5c:79:60:09:32:ff:88:fa:aa:39:63:31:ef:ad:31:e4:ac:57:ec:d2:09:4d:9a:01
  RSS hash function:
      toeplitz: on
      xor: off
      crc32: off

After:
  $ ethtool --show-rxfh eno0
  netlink error: Operation not supported

Sadly, I do not have the time to investigate a possible fix for this
issue, but I am more than happy to test out proposals.

Thanks,
Vladimir

RE: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Mogilappagari, Sudheer]

Hi Vladimir, 

Here is related discussion wrt this topic https://lore.kernel.org/netdev/IA1PR11MB626656578C50634B3C90E0C4E40E9@IA1PR11MB6266.namprd11.prod.outlook.com/T/#mdfcc6e25edb5b7719356db4759dc13e2a9020487

While introducing netlink support for ethtool --show-rxfh
the tradeoff was whether to modify the command output or to
use ETHTOOL_GCHANNELS to get the queue count. We went with
not modifying command output. Didn't realize about drivers
with no get_channels() support. Currently I have no ideas
on how to resolve this other than drivers implementing
get_channels() support. Any other ideas are welcome.

Though not a solution, one workaround is to compile ethtool
with no netlink support. 

Thanks
Sudheer



> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Sent: Thursday, July 11, 2024 4:46 AM
> To: Michal Kubecek <mkubecek@suse.cz>; Mogilappagari, Sudheer
> <sudheer.mogilappagari@intel.com>; netdev@vger.kernel.org
> Cc: Wei Fang <wei.fang@nxp.com>
> Subject: Netlink handler for ethtool --show-rxfh breaks driver
> compatibility
> 
> Hi,
> 
> Commit ffab99c1f382 ("netlink: add netlink handler for get rss (-x)")
> in the ethtool user space binary breaks compatibility with device
> drivers.
> 
> Namely, before the change, ethtool --show-rxfh did not emit a
> ETHTOOL_MSG_CHANNELS_GET netlink message or even the ETHTOOL_GCHANNELS
> ioctl variant. Now it does, and this effectively forces a new
> requirement for drivers to implement ethtool_ops :: get_channels() in
> the kernel.
> 
> The following drivers implement ethtool_ops :: get_rxfh() but not
> ethtool_ops :: get_channels():
> - drivers/net/ethernet/microchip/lan743x_ethtool.c
> - drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
> - drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> - drivers/net/ethernet/marvell/mvneta.c
> - drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> - drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> - drivers/net/ethernet/sfc/ef100_ethtool.c
> - drivers/net/ethernet/sfc/falcon/ethtool.c
> - drivers/net/ethernet/sfc/siena/ethtool.c
> - drivers/net/ethernet/sfc/ethtool.c
> - drivers/net/ethernet/intel/ixgbevf/ethtool.c
> 
> Thus, for them, this is a breaking ABI change which must be addressed.
> 
> A demo for the enetc driver.
> 
> Before:
>   $ ethtool --show-rxfh eno0
>   RX flow hash indirection table for eno0 with 2 RX ring(s):
>       0:      0     1     0     1     0     1     0     1
>       8:      0     1     0     1     0     1     0     1
>      16:      0     1     0     1     0     1     0     1
>      24:      0     1     0     1     0     1     0     1
>      32:      0     1     0     1     0     1     0     1
>      40:      0     1     0     1     0     1     0     1
>      48:      0     1     0     1     0     1     0     1
>      56:      0     1     0     1     0     1     0     1
>   RSS hash key:
> 
> 0d:1f:cb:76:88:82:dd:ea:70:c9:ef:53:3e:f3:bf:60:5c:79:60:09:32:ff:88:fa
> :aa:39:63:31:ef:ad:31:e4:ac:57:ec:d2:09:4d:9a:01
>   RSS hash function:
>       toeplitz: on
>       xor: off
>       crc32: off
> 
> After:
>   $ ethtool --show-rxfh eno0
>   netlink error: Operation not supported
> 
> Sadly, I do not have the time to investigate a possible fix for this
> issue, but I am more than happy to test out proposals.
> 
> Thanks,
> Vladimir

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Vladimir Oltean]

Hi Sudheer,

On Mon, Jul 15, 2024 at 07:37:45AM +0000, Mogilappagari, Sudheer wrote:
> Hi Vladimir, 
> 
> Here is related discussion wrt this topic https://lore.kernel.org/netdev/IA1PR11MB626656578C50634B3C90E0C4E40E9@IA1PR11MB6266.namprd11.prod.outlook.com/T/#mdfcc6e25edb5b7719356db4759dc13e2a9020487
> 
> While introducing netlink support for ethtool --show-rxfh
> the tradeoff was whether to modify the command output or to
> use ETHTOOL_GCHANNELS to get the queue count. We went with
> not modifying command output. Didn't realize about drivers
> with no get_channels() support. Currently I have no ideas
> on how to resolve this other than drivers implementing
> get_channels() support. Any other ideas are welcome.
> 
> Though not a solution, one workaround is to compile ethtool
> with no netlink support. 
> 
> Thanks
> Sudheer

I guess the bottom line is that ETHTOOL_GRXRINGS != ETHTOOL_GCHANNELS.
There is a fair amount of drivers which implements ethtool_ops ::
get_rxnfc() :: struct ethtool_rxnfc->cmd == ETHTOOL_GRXRINGS differently
than ethtool_ops :: get_channels().

Looking at Documentation/networking/ethtool-netlink.rst, I see
ETHTOOL_GRXRINGS has no netlink equivalent. So print_indir_table()
should still be called with the result of the ETHTOOL_GRXRINGS ioctl
even in the netlink case?

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Jakub Kicinski]

On Mon, 15 Jul 2024 14:58:07 +0300 Vladimir Oltean wrote:
> Looking at Documentation/networking/ethtool-netlink.rst, I see
> ETHTOOL_GRXRINGS has no netlink equivalent. So print_indir_table()
> should still be called with the result of the ETHTOOL_GRXRINGS ioctl
> even in the netlink case?

How about we fall back to the old method if netlink returns EOPNOTSUPP
for CHANNELS_GET? The other API is a bit of a historic coincidence.

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Vladimir Oltean]

On Mon, Jul 15, 2024 at 06:11:37AM -0700, Jakub Kicinski wrote:
> On Mon, 15 Jul 2024 14:58:07 +0300 Vladimir Oltean wrote:
> > Looking at Documentation/networking/ethtool-netlink.rst, I see
> > ETHTOOL_GRXRINGS has no netlink equivalent. So print_indir_table()
> > should still be called with the result of the ETHTOOL_GRXRINGS ioctl
> > even in the netlink case?
> 
> How about we fall back to the old method if netlink returns EOPNOTSUPP
> for CHANNELS_GET? The other API is a bit of a historic coincidence.

Explain "historic coincidence" like I'm 5?

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Jakub Kicinski]

On Mon, 15 Jul 2024 16:22:53 +0300 Vladimir Oltean wrote:
> On Mon, Jul 15, 2024 at 06:11:37AM -0700, Jakub Kicinski wrote:
> > On Mon, 15 Jul 2024 14:58:07 +0300 Vladimir Oltean wrote:  
> > > Looking at Documentation/networking/ethtool-netlink.rst, I see
> > > ETHTOOL_GRXRINGS has no netlink equivalent. So print_indir_table()
> > > should still be called with the result of the ETHTOOL_GRXRINGS ioctl
> > > even in the netlink case?  
> > 
> > How about we fall back to the old method if netlink returns EOPNOTSUPP
> > for CHANNELS_GET? The other API is a bit of a historic coincidence.  
> 
> Explain "historic coincidence" like I'm 5?

The definition I have in mind is that the design can't be well
understood without taking into account the history, i.e. the order
in which things were developed and the information we were working
with at the time.

In this case, simply put, GRXRINGS was added well before GCHANNELS
and to assign any semantic distinction between GRXRINGS and GCHANNELS 
is revisionist, for lack of a better word.

I could be wrong, but that's what I meant by "historic coincidence".

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Vladimir Oltean]

On Mon, Jul 15, 2024 at 06:39:31AM -0700, Jakub Kicinski wrote:
> The definition I have in mind is that the design can't be well
> understood without taking into account the history, i.e. the order
> in which things were developed and the information we were working
> with at the time.
> 
> In this case, simply put, GRXRINGS was added well before GCHANNELS
> and to assign any semantic distinction between GRXRINGS and GCHANNELS 
> is revisionist, for lack of a better word.

Are you saying a channel is a ring?

Semantical differences / lack thereof aside - it is factually not the
same thing to report a number retrieved through a different UAPI
interface in the netlink handler variant for the same command.
You have the chance of either reporting a different number on the same
NIC, or GCHANNELS not being implemented by its driver.

revisionist
noun
someone who examines and tries to change existing beliefs about how
events happened or what their importance or meaning is

> I could be wrong, but that's what I meant by "historic coincidence".

And the fact that ethtool --show-rxfh uses GCHANNELS when the kernel is
compiled with CONFIG_ETHTOOL_NETLINK support, but GRXRINGS when it isn't,
helps de-blur the lines how?

I can't avoid the feeling that introducing GCHANNELS into the mix is
what is revisionist :( I hope I'm not missing something.

I'm just a simple user, I came here because the command stopped working,
not because I want to split hairs.

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Jakub Kicinski]

On Mon, 15 Jul 2024 18:05:43 +0300 Vladimir Oltean wrote:
> On Mon, Jul 15, 2024 at 06:39:31AM -0700, Jakub Kicinski wrote:
> > The definition I have in mind is that the design can't be well
> > understood without taking into account the history, i.e. the order
> > in which things were developed and the information we were working
> > with at the time.
> > 
> > In this case, simply put, GRXRINGS was added well before GCHANNELS
> > and to assign any semantic distinction between GRXRINGS and GCHANNELS 
> > is revisionist, for lack of a better word.  
> 
> Are you saying a channel is a ring?

The information about rings can be computed based on channels as
currently used by drivers.

> Semantical differences / lack thereof aside - it is factually not the
> same thing to report a number retrieved through a different UAPI
> interface in the netlink handler variant for the same command.
> You have the chance of either reporting a different number on the same
> NIC

They can provide a different number? Which number is the user
supposed to trust? Out of the 4 APIs we have? Or the NIC has
a different ring count depending on the API?

> or GCHANNELS not being implemented by its driver.
> 
> revisionist
> noun
> someone who examines and tries to change existing beliefs about how
> events happened or what their importance or meaning is

Why not also look up "for lack of a better word" :|

> > I could be wrong, but that's what I meant by "historic coincidence".  
> 
> And the fact that ethtool --show-rxfh uses GCHANNELS when the kernel is
> compiled with CONFIG_ETHTOOL_NETLINK support, but GRXRINGS when it isn't,
> helps de-blur the lines how?

IDK what you mean, given the slice of my message you're responding to.

> I can't avoid the feeling that introducing GCHANNELS into the mix is
> what is revisionist :( I hope I'm not missing something.

You are missing the fact that other parts of the stack use different
APIs. Why does RXFH need its own way of reading queue count if we have
channels and rx queue count in rtnl?

> I'm just a simple user, I came here because the command stopped working,
> not because I want to split hairs.

Plainly :|

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

On Mon, Jul 15, 2024 at 08:26:00AM -0700, Jakub Kicinski wrote:
> On Mon, 15 Jul 2024 18:05:43 +0300 Vladimir Oltean wrote:
> 
> > Semantical differences / lack thereof aside - it is factually not the
> > same thing to report a number retrieved through a different UAPI
> > interface in the netlink handler variant for the same command.
> > You have the chance of either reporting a different number on the same
> > NIC
> 
> They can provide a different number? Which number is the user
> supposed to trust? Out of the 4 APIs we have? Or the NIC has
> a different ring count depending on the API?

This is IMHO the most important question. My understanding was that
those two APIs provide just two different ways to query the same value
and the existence of both is rather result of evolution than intent.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Vladimir Oltean]

On Mon, Jul 15, 2024 at 08:26:00AM -0700, Jakub Kicinski wrote:
> The information about rings can be computed based on channels as
> currently used by drivers.
(...)
> 
> They can provide a different number? Which number is the user
> supposed to trust? Out of the 4 APIs we have? Or the NIC has
> a different ring count depending on the API?

To stay on point on GRXRINGS vs GCHANNELS, "man ethtool" does say
"A channel is an IRQ and the set of queues that can trigger that IRQ."
Doesn't sound either (a) identical or (b) that you can recover the # of
rings from the # of channels, unless you make an assumption about how
they are distributed to IRQs...

> > > I could be wrong, but that's what I meant by "historic coincidence".  
> > 
> > And the fact that ethtool --show-rxfh uses GCHANNELS when the kernel is
> > compiled with CONFIG_ETHTOOL_NETLINK support, but GRXRINGS when it isn't,
> > helps de-blur the lines how?
> 
> IDK what you mean, given the slice of my message you're responding to.

Not connected to that particular sentence, it's just a comment about the
lack of consistency implied by your proposal, placed at the end of your
quoted email.

> > I can't avoid the feeling that introducing GCHANNELS into the mix is
> > what is revisionist :( I hope I'm not missing something.
> 
> You are missing the fact that other parts of the stack use different
> APIs. Why does RXFH need its own way of reading queue count if we have
> channels and rx queue count in rtnl?

Maybe if introduced today, it wouldn't have to, but the more relevant
question for my situation here is "why change it?"

I only expressed my dislike of a netlink/no netlink inconsistency
because your question was seemingly addressed to me. Luckily I am not
the one who needs to make that decision and I will gladly test out any
patch that fixes the regression.

Re: Netlink handler for ethtool --show-rxfh breaks driver compatibility

[Jakub Kicinski]

On Mon, 15 Jul 2024 18:46:53 +0300 Vladimir Oltean wrote:
> > They can provide a different number? Which number is the user
> > supposed to trust? Out of the 4 APIs we have? Or the NIC has
> > a different ring count depending on the API?  
> 
> To stay on point on GRXRINGS vs GCHANNELS, "man ethtool" does say
> "A channel is an IRQ and the set of queues that can trigger that IRQ."
> Doesn't sound either (a) identical or (b) that you can recover the # of
> rings from the # of channels, unless you make an assumption about how
> they are distributed to IRQs...

A bit short on time to write a proper reply, but thinking some more
about it, I think you're right. The choice of channels is not great.
It works today, but if we start stacking multiple Rx queues on a single
IRQ one day - things will get complicated. So let's go back to GRXRINGS
for now.