[PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Daniel Xu]

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);
 	} 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

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Daniel Xu]

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

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Edward Cree]

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

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Joe Damato]

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?

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Joe Damato]

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.

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Edward Cree]

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

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Daniel Xu]

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

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Jakub Kicinski]

On Fri, 8 Nov 2024 21:13:50 +0000 Edward Cree wrote:
> 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?

I'd suggest we merge Daniel's patch (almost) as is, and you can
(re)establish the behavior sfc wants but you owe us:
 - fixes for helpers used in "is the queue in use" checks like
   ethtool_get_max_rss_ctx_channel()
 - "opt in" flag for drivers which actually support this rather 
   than silently ignoring ring_cookie if rss ctx is set
 - selftest

:(

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Edward Cree]

On 09/11/2024 17:42, Jakub Kicinski wrote:
> I'd suggest we merge Daniel's patch (almost) as is, and you can
> (re)establish the behavior sfc wants but you owe us:
>  - fixes for helpers used in "is the queue in use" checks like
>    ethtool_get_max_rss_ctx_channel()
>  - "opt in" flag for drivers which actually support this rather 
>    than silently ignoring ring_cookie if rss ctx is set
>  - selftest

Sure, I'll get to work on those.
But I don't think Daniel's patch should be merged; the old output
 is confusing or misleading, but the new output is incorrect (when
 run against a current kernel and sfc, or a future fixed kernel and
 any driver that opts in to allow nonzero ring_cookie).
If ring_cookie is nonzero then it *must* be printed, unless ethtool
 has some way to *know* it's ignored.  Regressions are worse than
 existing bugs, after all.

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Jakub Kicinski]

On Mon, 11 Nov 2024 10:47:47 +0000 Edward Cree wrote:
> On 09/11/2024 17:42, Jakub Kicinski wrote:
> > I'd suggest we merge Daniel's patch (almost) as is, and you can
> > (re)establish the behavior sfc wants but you owe us:
> >  - fixes for helpers used in "is the queue in use" checks like
> >    ethtool_get_max_rss_ctx_channel()
> >  - "opt in" flag for drivers which actually support this rather 
> >    than silently ignoring ring_cookie if rss ctx is set
> >  - selftest  
> 
> Sure, I'll get to work on those.
> But I don't think Daniel's patch should be merged; the old output
>  is confusing or misleading, but the new output is incorrect (when
>  run against a current kernel and sfc, or a future fixed kernel and
>  any driver that opts in to allow nonzero ring_cookie).
> If ring_cookie is nonzero then it *must* be printed, unless ethtool
>  has some way to *know* it's ignored.  Regressions are worse than
>  existing bugs, after all.

If you promise sending patches for the kernel side validation soon -
printing when non-zero makes sense :)

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Edward Cree]

On 09/11/2024 17:42, Jakub Kicinski wrote:
>  - fixes for helpers used in "is the queue in use" checks like
>    ethtool_get_max_rss_ctx_channel()

If there's an RSS context that names a queue, but no rxnfc filters
 currently target that context, should the queue be considered "in
 use" or not?  (Currently it is.)
I'm trying to figure out how much of ethtool_get_max_rss_ctx_channel
 can be subsumed by the logic I'll need to add to
 ethtool_get_max_rxnfc_channel; if we don't count unused contexts as
 'using' their queues then ethtool_get_max_rss_ctx_channel() can
 almost entirely disappear.

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Jakub Kicinski]

On Tue, 12 Nov 2024 09:24:07 +0000 Edward Cree wrote:
> On 09/11/2024 17:42, Jakub Kicinski wrote:
> >  - fixes for helpers used in "is the queue in use" checks like
> >    ethtool_get_max_rss_ctx_channel()  
> 
> If there's an RSS context that names a queue, but no rxnfc filters
>  currently target that context, should the queue be considered "in
>  use" or not?  (Currently it is.)
> I'm trying to figure out how much of ethtool_get_max_rss_ctx_channel
>  can be subsumed by the logic I'll need to add to
>  ethtool_get_max_rxnfc_channel; if we don't count unused contexts as
>  'using' their queues then ethtool_get_max_rss_ctx_channel() can
>  almost entirely disappear.

Hm, interesting idea...
Practically speaking I think it introduces complexity and I'm not sure
anyone will actually benefit (IOW why would anyone want to keep /
create context for inactive queues?).

Drivers may not expect to have contexts pointing to disabled queues.

My gut feeling is that we should just leave a comment for posterity
somewhere in the code but continue to validate both based on rules 
and based on "direct" context membership.

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Edward Cree]

On 12/11/2024 15:24, Jakub Kicinski wrote:
> Hm, interesting idea...
> Practically speaking I think it introduces complexity and I'm not sure
> anyone will actually benefit (IOW why would anyone want to keep /
> create context for inactive queues?).

Conceivably to save re-configuring them next time they increase the
 queues again?  But I suppose anyone doing that kind of complicated
 demand-flexible tuning will be using some kind of userland software
 that can automate that.
Anyway I don't have a dog in this fight as sfc doesn't support ethtool
 set-channels.  (Which will make it difficult for me to test this; had
 I better extend netdevsim to support RSS & rxnfc?)

> My gut feeling is that we should just leave a comment for posterity
> somewhere in the code but continue to validate both based on rules 
> and based on "direct" context membership.

Will do.

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Jakub Kicinski]

On Wed, 13 Nov 2024 03:30:05 +0000 Edward Cree wrote:
> On 12/11/2024 15:24, Jakub Kicinski wrote:
> > Hm, interesting idea...
> > Practically speaking I think it introduces complexity and I'm not sure
> > anyone will actually benefit (IOW why would anyone want to keep /
> > create context for inactive queues?).  
> 
> Conceivably to save re-configuring them next time they increase the
>  queues again?  But I suppose anyone doing that kind of complicated
>  demand-flexible tuning will be using some kind of userland software
>  that can automate that.
> Anyway I don't have a dog in this fight as sfc doesn't support ethtool
>  set-channels.  (Which will make it difficult for me to test this; had
>  I better extend netdevsim to support RSS & rxnfc?)

Good question on the netdevsim. Adding the callbacks seems fine.
But making it actually do RSS and nfc on packets to make the HW tests
pass would be more of a lift. So I think you'd have to add a separate
test under drivers/net/netdevsim for this. Is that your thinking?

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Jakub Kicinski]

On Wed, 13 Nov 2024 16:46:35 -0800 Jakub Kicinski wrote:
> On Wed, 13 Nov 2024 03:30:05 +0000 Edward Cree wrote:
> > On 12/11/2024 15:24, Jakub Kicinski wrote:  
> > > Hm, interesting idea...
> > > Practically speaking I think it introduces complexity and I'm not sure
> > > anyone will actually benefit (IOW why would anyone want to keep /
> > > create context for inactive queues?).    
> > 
> > Conceivably to save re-configuring them next time they increase the
> >  queues again?  But I suppose anyone doing that kind of complicated
> >  demand-flexible tuning will be using some kind of userland software
> >  that can automate that.
> > Anyway I don't have a dog in this fight as sfc doesn't support ethtool
> >  set-channels.  (Which will make it difficult for me to test this; had
> >  I better extend netdevsim to support RSS & rxnfc?)  
> 
> Good question on the netdevsim. Adding the callbacks seems fine.
> But making it actually do RSS and nfc on packets to make the HW tests
> pass would be more of a lift. So I think you'd have to add a separate
> test under drivers/net/netdevsim for this. Is that your thinking?

s/your/you're/

Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

[Edward Cree]

On 14/11/2024 00:46, Jakub Kicinski wrote:
> Good question on the netdevsim. Adding the callbacks seems fine.
> But making it actually do RSS and nfc on packets to make the HW tests
> pass would be more of a lift. So I think you'd have to add a separate
> test under drivers/net/netdevsim for this. Is that your thinking?

Actually I was just planning to test it manually since the HW tests
 running on other devices should cover regression testing going
 forward, but I guess I could give the relevant tests from rss_ctx.py
 an option to skip _send_traffic_check(), and call them from the new
 netdevsim test.  test_rss_queue_reconfigure() covers this bit of
 code; some of the others like test_rss_context_dump() might also be
 worth running on netdevsim.