[net,0/2] rxfh with custom RSS fixes

  Greetings:

While attempting to get the RX flow hash key for a custom RSS context on
my mlx5 NIC, I got an error:

$ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
Cannot get RX network flow hashing options: Invalid argument

I dug into this a bit and noticed two things:

1. ETHTOOL_GRXFH supports custom RSS contexts, but ETHTOOL_SRXFH does
not. I moved the copy logic out of ETHTOOL_GRXFH and into a helper so
that both ETHTOOL_{G,S}RXFH now call it, which fixes ETHTOOL_SRXFH. This
is patch 1/2.

2. mlx5 defaulted to RSS context 0 for both ETHTOOL_{G,S}RXFH paths. I
have modified the driver to support custom contexts for both paths. It
is now possible to get and set the flow hash key for custom RSS contexts
with mlx5. This is patch 2/2.

See commit messages for more details.

The patches include the relevant fixes tags, as I think both commits are
fixing previous code, but if this change is preferred for net-next I can
resend.

Thanks.

Joe Damato (2):
  net: ethtool: Unify ETHTOOL_{G,S}RXFH rxnfc copy
  net/mlx5: Fix flowhash key set/get for custom RSS

 .../ethernet/mellanox/mlx5/core/en/rx_res.c   | 23 +++++-
 .../ethernet/mellanox/mlx5/core/en/rx_res.h   |  5 +-
 .../mellanox/mlx5/core/en_fs_ethtool.c        | 33 +++++---
 net/ethtool/ioctl.c                           | 75 ++++++++++---------
 4 files changed, 84 insertions(+), 52 deletions(-)
  

On 23/07/2023 16:06, Joe Damato wrote:
> Greetings:
> 
> While attempting to get the RX flow hash key for a custom RSS context on
> my mlx5 NIC, I got an error:
> 
> $ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> Cannot get RX network flow hashing options: Invalid argument
> 
> I dug into this a bit and noticed two things:
> 
> 1. ETHTOOL_GRXFH supports custom RSS contexts, but ETHTOOL_SRXFH does
> not. I moved the copy logic out of ETHTOOL_GRXFH and into a helper so
> that both ETHTOOL_{G,S}RXFH now call it, which fixes ETHTOOL_SRXFH. This
> is patch 1/2.

As I see it, this is a new feature, not a fix, so belongs on net-next.
(No existing driver accepts FLOW_RSS in ETHTOOL_SRXFH's cmd->flow_type,
 which is just as well as if they did this would be a uABI break.)

Going forward, ETHTOOL_SRXFH will hopefully be integrated into the new
 RSS context kAPI I'm working on[1], so that we can have a new netlink
 uAPI for RSS configuration that's all in one place instead of the
 piecemeal-grown ethtool API with its backwards-compatible hacks.
But that will take a while, so I think this should go in even though
 it's technically an extension to legacy ethtool; it was part of the
 documented uAPI and userland implements it, it just never got
 implemented on the kernel side (because the initial driver with
 context support, sfc, didn't support SRXFH).

> 2. mlx5 defaulted to RSS context 0 for both ETHTOOL_{G,S}RXFH paths. I
> have modified the driver to support custom contexts for both paths. It
> is now possible to get and set the flow hash key for custom RSS contexts
> with mlx5. This is patch 2/2.

My feeling would be that this isn't a Fix either, but not my place to say.

-ed

[1]: https://lore.kernel.org/netdev/ecaae93a-d41d-4c3d-8e52-2800baa7080d@lunn.ch/T/
  

On Mon, Jul 24, 2023 at 08:27:43PM +0100, Edward Cree wrote:
> On 23/07/2023 16:06, Joe Damato wrote:
> > Greetings:
> > 
> > While attempting to get the RX flow hash key for a custom RSS context on
> > my mlx5 NIC, I got an error:
> > 
> > $ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> > Cannot get RX network flow hashing options: Invalid argument
> > 
> > I dug into this a bit and noticed two things:
> > 
> > 1. ETHTOOL_GRXFH supports custom RSS contexts, but ETHTOOL_SRXFH does
> > not. I moved the copy logic out of ETHTOOL_GRXFH and into a helper so
> > that both ETHTOOL_{G,S}RXFH now call it, which fixes ETHTOOL_SRXFH. This
> > is patch 1/2.
> 
> As I see it, this is a new feature, not a fix, so belongs on net-next.
> (No existing driver accepts FLOW_RSS in ETHTOOL_SRXFH's cmd->flow_type,
>  which is just as well as if they did this would be a uABI break.)
> 
> Going forward, ETHTOOL_SRXFH will hopefully be integrated into the new
>  RSS context kAPI I'm working on[1], so that we can have a new netlink
>  uAPI for RSS configuration that's all in one place instead of the
>  piecemeal-grown ethtool API with its backwards-compatible hacks.
> But that will take a while, so I think this should go in even though
>  it's technically an extension to legacy ethtool; it was part of the
>  documented uAPI and userland implements it, it just never got
>  implemented on the kernel side (because the initial driver with
>  context support, sfc, didn't support SRXFH).
> 
> > 2. mlx5 defaulted to RSS context 0 for both ETHTOOL_{G,S}RXFH paths. I
> > have modified the driver to support custom contexts for both paths. It
> > is now possible to get and set the flow hash key for custom RSS contexts
> > with mlx5. This is patch 2/2.
> 
> My feeling would be that this isn't a Fix either, but not my place to say.

Thanks for the context above; I'll let the Mellanox folks weigh in on what
they think about the code in the second patch before I proceed.

I suspect that you are probably right and that net-next might be a more
appropriate place for this. If the code is ack'd by Mellanox (and they
agree re: net-next), I can re-send this series to net-next with the Fixes
removed and the Ack's added.
  

On Mon, 24 Jul 2023 20:27:43 +0100 Edward Cree wrote:
> On 23/07/2023 16:06, Joe Damato wrote:
> > Greetings:
> > 
> > While attempting to get the RX flow hash key for a custom RSS context on
> > my mlx5 NIC, I got an error:
> > 
> > $ sudo ethtool -u eth1 rx-flow-hash tcp4 context 1
> > Cannot get RX network flow hashing options: Invalid argument
> > 
> > I dug into this a bit and noticed two things:
> > 
> > 1. ETHTOOL_GRXFH supports custom RSS contexts, but ETHTOOL_SRXFH does
> > not. I moved the copy logic out of ETHTOOL_GRXFH and into a helper so
> > that both ETHTOOL_{G,S}RXFH now call it, which fixes ETHTOOL_SRXFH. This
> > is patch 1/2.  
> 
> As I see it, this is a new feature, not a fix, so belongs on net-next.
> (No existing driver accepts FLOW_RSS in ETHTOOL_SRXFH's cmd->flow_type,
>  which is just as well as if they did this would be a uABI break.)
> 
> Going forward, ETHTOOL_SRXFH will hopefully be integrated into the new
>  RSS context kAPI I'm working on[1], so that we can have a new netlink
>  uAPI for RSS configuration that's all in one place instead of the
>  piecemeal-grown ethtool API with its backwards-compatible hacks.
> But that will take a while, so I think this should go in even though
>  it's technically an extension to legacy ethtool; it was part of the
>  documented uAPI and userland implements it, it just never got
>  implemented on the kernel side (because the initial driver with
>  context support, sfc, didn't support SRXFH).

What's the status on your work? Are you planning to split the RSS
config from ethtool or am I reading too much into what you said?

It'd be great to push the uAPI extensions back and make them
netlink-only, but we can't make Joe wait if it takes a long time
to finish up the basic conversion :(
  

On 24/07/2023 23:08, Jakub Kicinski wrote:
> What's the status on your work? Are you planning to split the RSS
> config from ethtool or am I reading too much into what you said?

I was just thinking that when netlink dumps get added it would make
 sense to also extend the netlink version of SRSSH (which is what
 calls the rxfh_context ethtool_ops, via the misleadingly named
 ethtool_set_rxfh()) to include the hash fields setting that's
 currently done through ETHTOOL_SRXFH.  In which case I should add
 that data to struct ethtool_rxfh_context, and include it in the
 get/create/modify_rss_context ethtool_ops API.
Since it only occurred to me to consider that setting when I saw
 Joe's patches, I haven't really figured out yet how to go about
 the implementation of that.

More generally the status of my RSS work is that I've been umming
 and ahhing about that mutex you didn't like (I still think it's
 the Right Thing) so I've not made much progress with it.
And I should place on record that probably once I've gotten the
 kernel-driver API done I'll leave the netlink/uAPI stuff for
 someone else to do as I really don't have the relevant expertise.

> It'd be great to push the uAPI extensions back and make them
> netlink-only, but we can't make Joe wait if it takes a long time
> to finish up the basic conversion :(

Yeah as I said upthread I don't think we should make Joe wait, if
 he's got a use case that actually needs it (have you, Joe?  Or
 is it only GRXFH you need and the investigation just led you to
 notice SRXFH was broken?)

-ed
  

On Tue, Jul 25, 2023 at 09:40:24AM +0100, Edward Cree wrote:
> On 24/07/2023 23:08, Jakub Kicinski wrote:
> > It'd be great to push the uAPI extensions back and make them
> > netlink-only, but we can't make Joe wait if it takes a long time
> > to finish up the basic conversion :(
> 
> Yeah as I said upthread I don't think we should make Joe wait, if
>  he's got a use case that actually needs it (have you, Joe?  Or
>  is it only GRXFH you need and the investigation just led you to
>  notice SRXFH was broken?)

In short, yes: I'd like to be able to get and set the flow hash keys for
custom RSS contexts on mlx5 which is why I included the patch to mlx5 in
this series... but to be fair I am just one user :) I think it's really
up to you all on the direction you want to go.

Longer story: I am working on building a system which relies on custom RSS
contexts, flow rules to associate flows with RSS contexts (and thus
specific sets of queues), and epoll based busy poll. It's a long story ;)

I had considered changing the flow hash key to see if I could alter the
behavior of the system I am working on, but I ran into both issues
immediately (GRXFH and FLOW_RSS was not supported by mlx5, and SRXFH
was broken) which led me down the path of attempting to fix both so that I
could get and set the flow hash keys. I thought the code might be useful,
so I submit it upstream -- I was not aware of the netlink work (but it
looks really useful!).

It seems that the Mellanox folks are OK with the proposed driver change,
so I am going to send a v2 rebased on net-next with their requested changes.
  

On Tue, 25 Jul 2023 09:40:24 +0100 Edward Cree wrote:
> More generally the status of my RSS work is that I've been umming
>  and ahhing about that mutex you didn't like (I still think it's
>  the Right Thing) so I've not made much progress with it.

I had a look at the code again, and I don't think the mutex is a deal
breaker. More of an aesthetic thing, so to speak. If you strongly
prefer to keep the mutex that's fine.