| Message ID | 20230723150658.241597-1-jdamato@fastly.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rxfh with custom RSS fixes | expand |
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.
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(-)