| Message ID | 890cd515345f7c1ed6fba4bf0e43c53b34ccefaa.1731094323.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool-next] rxclass: Make output for RSS context action explicit | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Nov 8, 2024, at 11:32 AM, Daniel Xu wrote: > Currently, if the action for an ntuple rule is to redirect to an RSS > context, the RSS context is printed as an attribute. At the same time, > a wrong action is printed. For example: > > # ethtool -X eth0 hfunc toeplitz context new start 24 equal 8 > New RSS context is 1 > > # ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1 > Added rule with ID 0 > > # ethtool -n eth0 rule 0 > Filter: 0 > Rule Type: Raw IPv6 > Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > Dest IP addr: <redacted> mask: :: > Traffic Class: 0x0 mask: 0xff > Protocol: 0 mask: 0xff > L4 bytes: 0x0 mask: 0xffffffff > RSS Context ID: 1 > Action: Direct to queue 0 > > This is wrong and misleading. Fix by treating RSS context as a explicit > action. The new output looks like this: > > # ./ethtool -n eth0 rule 0 > Filter: 0 > Rule Type: Raw IPv6 > Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > Dest IP addr: <redacted> mask: :: > Traffic Class: 0x0 mask: 0xff > Protocol: 0 mask: 0xff > L4 bytes: 0x0 mask: 0xffffffff > Action: Direct to RSS context id 1 > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > --- > rxclass.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/rxclass.c b/rxclass.c > index f17e3a5..80d6419 100644 > --- a/rxclass.c > +++ b/rxclass.c > @@ -248,13 +248,12 @@ static void rxclass_print_nfc_rule(struct > ethtool_rx_flow_spec *fsp, > > rxclass_print_nfc_spec_ext(fsp); > > - if (fsp->flow_type & FLOW_RSS) > - fprintf(stdout, "\tRSS Context ID: %u\n", rss_context); > - > if (fsp->ring_cookie == RX_CLS_FLOW_DISC) { > fprintf(stdout, "\tAction: Drop\n"); > } else if (fsp->ring_cookie == RX_CLS_FLOW_WAKE) { > fprintf(stdout, "\tAction: Wake-on-LAN\n"); > + } else if (fsp->flow_type & FLOW_RSS) > + fprintf(stdout, "\tRSS context id %u\n", rss_context); Gah, missed { at end of line while transcribing patches between different machines... I can resend if y'all want. > } else { > u64 vf = ethtool_get_flow_spec_ring_vf(fsp->ring_cookie); > u64 queue = ethtool_get_flow_spec_ring(fsp->ring_cookie); > -- > 2.46.0
On 08/11/2024 19:32, Daniel Xu wrote: > Currently, if the action for an ntuple rule is to redirect to an RSS > context, the RSS context is printed as an attribute. At the same time, > a wrong action is printed. For example: > > # ethtool -X eth0 hfunc toeplitz context new start 24 equal 8 > New RSS context is 1 > > # ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1 > Added rule with ID 0 > > # ethtool -n eth0 rule 0 > Filter: 0 > Rule Type: Raw IPv6 > Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > Dest IP addr: <redacted> mask: :: > Traffic Class: 0x0 mask: 0xff > Protocol: 0 mask: 0xff > L4 bytes: 0x0 mask: 0xffffffff > RSS Context ID: 1 > Action: Direct to queue 0 > > This is wrong and misleading. Fix by treating RSS context as a explicit > action. The new output looks like this: > > # ./ethtool -n eth0 rule 0 > Filter: 0 > Rule Type: Raw IPv6 > Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > Dest IP addr: <redacted> mask: :: > Traffic Class: 0x0 mask: 0xff > Protocol: 0 mask: 0xff > L4 bytes: 0x0 mask: 0xffffffff > Action: Direct to RSS context id 1 > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> I believe this patch is incorrect. My understanding is that on packet reception, the integer returned from the RSS indirection table is *added* to the queue number from the ntuple rule, so that for instance the same indirection table can be used for one rule distributing packets over queues 0-3 and for another rule distributing a different subset of packets over queues 4-7. I'm not sure if this behaviour is documented anywhere, and different NICs may have different interpretations, but this is how sfc ef10 behaves. -Ed
On Fri, Nov 08, 2024 at 07:56:41PM +0000, Edward Cree wrote: > On 08/11/2024 19:32, Daniel Xu wrote: > > Currently, if the action for an ntuple rule is to redirect to an RSS > > context, the RSS context is printed as an attribute. At the same time, > > a wrong action is printed. For example: > > > > # ethtool -X eth0 hfunc toeplitz context new start 24 equal 8 > > New RSS context is 1 > > > > # ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1 > > Added rule with ID 0 > > > > # ethtool -n eth0 rule 0 > > Filter: 0 > > Rule Type: Raw IPv6 > > Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > > Dest IP addr: <redacted> mask: :: > > Traffic Class: 0x0 mask: 0xff > > Protocol: 0 mask: 0xff > > L4 bytes: 0x0 mask: 0xffffffff > > RSS Context ID: 1 > > Action: Direct to queue 0 > > > > This is wrong and misleading. Fix by treating RSS context as a explicit > > action. The new output looks like this: > > > > # ./ethtool -n eth0 rule 0 > > Filter: 0 > > Rule Type: Raw IPv6 > > Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > > Dest IP addr: <redacted> mask: :: > > Traffic Class: 0x0 mask: 0xff > > Protocol: 0 mask: 0xff > > L4 bytes: 0x0 mask: 0xffffffff > > Action: Direct to RSS context id 1 > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > > I believe this patch is incorrect. My understanding is that on > packet reception, the integer returned from the RSS indirection > table is *added* to the queue number from the ntuple rule, so > that for instance the same indirection table can be used for one > rule distributing packets over queues 0-3 and for another rule > distributing a different subset of packets over queues 4-7. > I'm not sure if this behaviour is documented anywhere, and > different NICs may have different interpretations, but this is > how sfc ef10 behaves. I just wanted to chime in and say that my understanding has always been more aligned with Daniel's and I had also found the ethtool output confusing when directing flows that match a rule to a custom context. If Daniel's patch is wrong (I don't know enough to say if it is or not), would it be possible to have some alternate ethtool output that's less confusing? Or for this specific output to be outlined in the documentation somewhere?
On Fri, Nov 08, 2024 at 12:34:49PM -0800, Joe Damato wrote: > On Fri, Nov 08, 2024 at 07:56:41PM +0000, Edward Cree wrote: > > On 08/11/2024 19:32, Daniel Xu wrote: > > > Currently, if the action for an ntuple rule is to redirect to an RSS > > > context, the RSS context is printed as an attribute. At the same time, > > > a wrong action is printed. For example: > > > > > > # ethtool -X eth0 hfunc toeplitz context new start 24 equal 8 > > > New RSS context is 1 > > > > > > # ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1 > > > Added rule with ID 0 > > > > > > # ethtool -n eth0 rule 0 > > > Filter: 0 > > > Rule Type: Raw IPv6 > > > Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > > > Dest IP addr: <redacted> mask: :: > > > Traffic Class: 0x0 mask: 0xff > > > Protocol: 0 mask: 0xff > > > L4 bytes: 0x0 mask: 0xffffffff > > > RSS Context ID: 1 > > > Action: Direct to queue 0 > > > > > > This is wrong and misleading. Fix by treating RSS context as a explicit > > > action. The new output looks like this: > > > > > > # ./ethtool -n eth0 rule 0 > > > Filter: 0 > > > Rule Type: Raw IPv6 > > > Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff > > > Dest IP addr: <redacted> mask: :: > > > Traffic Class: 0x0 mask: 0xff > > > Protocol: 0 mask: 0xff > > > L4 bytes: 0x0 mask: 0xffffffff > > > Action: Direct to RSS context id 1 > > > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > > > > I believe this patch is incorrect. My understanding is that on > > packet reception, the integer returned from the RSS indirection > > table is *added* to the queue number from the ntuple rule, so > > that for instance the same indirection table can be used for one > > rule distributing packets over queues 0-3 and for another rule > > distributing a different subset of packets over queues 4-7. > > I'm not sure if this behaviour is documented anywhere, and > > different NICs may have different interpretations, but this is > > how sfc ef10 behaves. > > I just wanted to chime in and say that my understanding has always > been more aligned with Daniel's and I had also found the ethtool > output confusing when directing flows that match a rule to a custom > context. > > If Daniel's patch is wrong (I don't know enough to say if it is or > not), would it be possible to have some alternate ethtool output > that's less confusing? Or for this specific output to be outlined in > the documentation somewhere? Sorry for the quick follow up, but I just tested this and I do think there is an issue with ethtool's output. Here's an example from my system where I create 18 queues and a custom RSS context to send flows to queues 16 and 17 only: $ ethtool --version ethtool version 6.7 $ sudo ethtool -L eth2 combined 18 $ sudo ethtool -X eth2 weight 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 1 context new New RSS context is 1 $ sudo ethtool -U eth2 flow-type tcp4 dst-port 11211 context 1 Added rule with ID 1023 $ sudo ethtool -n eth2 rule 1023 Filter: 1023 Rule Type: TCP over IPv4 Src IP addr: 0.0.0.0 mask: 255.255.255.255 Dest IP addr: 0.0.0.0 mask: 255.255.255.255 TOS: 0x0 mask: 0xff Src port: 0 mask: 0xffff Dest port: 11211 mask: 0x0 RSS Context ID: 1 Action: Direct to queue 0 I don't understand why this would say "Direct to queue 0" when the weights specifically disallow this for context 1.
On 08/11/2024 20:34, Joe Damato wrote: > On Fri, Nov 08, 2024 at 07:56:41PM +0000, Edward Cree wrote: >> I believe this patch is incorrect. My understanding is that on >> packet reception, the integer returned from the RSS indirection >> table is *added* to the queue number from the ntuple rule, so >> that for instance the same indirection table can be used for one >> rule distributing packets over queues 0-3 and for another rule >> distributing a different subset of packets over queues 4-7. >> I'm not sure if this behaviour is documented anywhere, and >> different NICs may have different interpretations, but this is >> how sfc ef10 behaves. I've looked up the history, and my original commit[1] adding RSS + ntuple specified this addition behaviour in both the patch description and the ethtool uapi header comments. The kerneldoc comment for struct ethtool_rxnfc still has this language: * For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update. * @fs.@location either specifies the location to use or is a special * location value with %RX_CLS_LOC_SPECIAL flag set. On return, * @fs.@location is the actual rule location. If @fs.@flow_type * includes the %FLOW_RSS flag, @rss_context is the RSS context ID to * use for flow spreading traffic which matches this rule. The value * from the rxfh indirection table will be added to @fs.@ring_cookie * to choose which ring to deliver to. The ethtool man page, however, does not document this. > I just wanted to chime in and say that my understanding has always > been more aligned with Daniel's and I had also found the ethtool > output confusing when directing flows that match a rule to a custom > context. > > If Daniel's patch is wrong (I don't know enough to say if it is or > not), would it be possible to have some alternate ethtool output > that's less confusing? Or for this specific output to be outlined in > the documentation somewhere? I think sensible output would be to keep Daniel's "Action: Direct to RSS context id %u", but also print something like "Queue base offset: %u" with the ring index that was previously printed as the Action. If the base offset is zero its output could possibly be suppressed. And we should update the ethtool man page to describe the adding behaviour, and audit device drivers to ensure that any that don't support it reject RSS filters with nonzero ring_cookie, as specified in [1]. Does this sound reasonable? -ed [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=84a1d9c4820080bebcbd413a845076dcb62f45fa
Hi Ed, Joe, Thanks for looking at this so quickly. On Fri, Nov 08, 2024 at 09:13:50PM GMT, Edward Cree wrote: > On 08/11/2024 20:34, Joe Damato wrote: > > On Fri, Nov 08, 2024 at 07:56:41PM +0000, Edward Cree wrote: > >> I believe this patch is incorrect. My understanding is that on > >> packet reception, the integer returned from the RSS indirection > >> table is *added* to the queue number from the ntuple rule, so > >> that for instance the same indirection table can be used for one > >> rule distributing packets over queues 0-3 and for another rule > >> distributing a different subset of packets over queues 4-7. > >> I'm not sure if this behaviour is documented anywhere, and > >> different NICs may have different interpretations, but this is > >> how sfc ef10 behaves. > > I've looked up the history, and my original commit[1] adding RSS + > ntuple specified this addition behaviour in both the patch > description and the ethtool uapi header comments. The kerneldoc > comment for struct ethtool_rxnfc still has this language: > * For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update. > * @fs.@location either specifies the location to use or is a special > * location value with %RX_CLS_LOC_SPECIAL flag set. On return, > * @fs.@location is the actual rule location. If @fs.@flow_type > * includes the %FLOW_RSS flag, @rss_context is the RSS context ID to > * use for flow spreading traffic which matches this rule. The value > * from the rxfh indirection table will be added to @fs.@ring_cookie > * to choose which ring to deliver to. > The ethtool man page, however, does not document this. FWIW, bnxt on 6.9-ish is probably non-compliant (assuming this is the correct usage of the API): # ethtool -N eth0 flow-type ip6 dst-ip ::1 context 1 queue 5 Added rule with ID 0 # ethtool -n eth0 32 RX rings available Total 1 rules Filter: 0 Rule Type: Raw IPv6 Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff Dest IP addr: ::1 mask: :: Traffic Class: 0x0 mask: 0xff Protocol: 0 mask: 0xff L4 bytes: 0x0 mask: 0xffffffff RSS Context ID: 1 Action: Direct to queue 0 Note the "Direct to queue 0" even though I specified queue 5. I'm running a new batch of tests on 6.11 next week so I'll update here if it magically got fixed. > > > I just wanted to chime in and say that my understanding has always > > been more aligned with Daniel's and I had also found the ethtool > > output confusing when directing flows that match a rule to a custom > > context. > > > > If Daniel's patch is wrong (I don't know enough to say if it is or > > not), would it be possible to have some alternate ethtool output > > that's less confusing? Or for this specific output to be outlined in > > the documentation somewhere? > > I think sensible output would be to keep Daniel's "Action: Direct to > RSS context id %u", but also print something like "Queue base offset: > %u" with the ring index that was previously printed as the Action. > If the base offset is zero its output could possibly be suppressed. > And we should update the ethtool man page to describe the adding > behaviour, and audit device drivers to ensure that any that don't > support it reject RSS filters with nonzero ring_cookie, as specified > in [1]. > Does this sound reasonable? That sounds good to me. I'll send out a v2 for the ethtool changes. I'm probably not qualified enough to do an audit. But since I've been poking around the bxnt driver the past week I'll give it a look and see if I can convince myself of anything. Thanks, Daniel
diff --git a/rxclass.c b/rxclass.c index f17e3a5..80d6419 100644 --- a/rxclass.c +++ b/rxclass.c @@ -248,13 +248,12 @@ static void rxclass_print_nfc_rule(struct ethtool_rx_flow_spec *fsp, rxclass_print_nfc_spec_ext(fsp); - if (fsp->flow_type & FLOW_RSS) - fprintf(stdout, "\tRSS Context ID: %u\n", rss_context); - if (fsp->ring_cookie == RX_CLS_FLOW_DISC) { fprintf(stdout, "\tAction: Drop\n"); } else if (fsp->ring_cookie == RX_CLS_FLOW_WAKE) { fprintf(stdout, "\tAction: Wake-on-LAN\n"); + } else if (fsp->flow_type & FLOW_RSS) + fprintf(stdout, "\tRSS context id %u\n", rss_context); } else { u64 vf = ethtool_get_flow_spec_ring_vf(fsp->ring_cookie); u64 queue = ethtool_get_flow_spec_ring(fsp->ring_cookie);
Currently, if the action for an ntuple rule is to redirect to an RSS context, the RSS context is printed as an attribute. At the same time, a wrong action is printed. For example: # ethtool -X eth0 hfunc toeplitz context new start 24 equal 8 New RSS context is 1 # ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1 Added rule with ID 0 # ethtool -n eth0 rule 0 Filter: 0 Rule Type: Raw IPv6 Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff Dest IP addr: <redacted> mask: :: Traffic Class: 0x0 mask: 0xff Protocol: 0 mask: 0xff L4 bytes: 0x0 mask: 0xffffffff RSS Context ID: 1 Action: Direct to queue 0 This is wrong and misleading. Fix by treating RSS context as a explicit action. The new output looks like this: # ./ethtool -n eth0 rule 0 Filter: 0 Rule Type: Raw IPv6 Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff Dest IP addr: <redacted> mask: :: Traffic Class: 0x0 mask: 0xff Protocol: 0 mask: 0xff L4 bytes: 0x0 mask: 0xffffffff Action: Direct to RSS context id 1 Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> --- rxclass.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)