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

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

[Daniel Xu]

Currently `ethtool -n` prints out misleading output if the action for an
ntuple rule is to redirect to an RSS context. 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

The above output suggests that the HW will direct to queue 0 where in
reality queue 0 is just the base offset from which the redirection table
lookup in the RSS context is added to.

Fix by making output more clear. Also suppress base offset queue for the
common case of 0. Example of new output:

    # ./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>
---
Changes from v1:
* Reword to support queue base offset API
* Fix compile error
* Improve wording (also a transcription error)

 rxclass.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/rxclass.c b/rxclass.c
index f17e3a5..ac9b529 100644
--- a/rxclass.c
+++ b/rxclass.c
@@ -248,13 +248,17 @@ 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) {
+		u64 queue = ethtool_get_flow_spec_ring(fsp->ring_cookie);
+
+		fprintf(stdout, "\tAction: Direct to RSS context id %u", rss_context);
+		if (queue)
+			fprintf(stdout, " (queue base offset: %llu)", queue);
+		fprintf(stdout, "\n");
 	} 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 v2] rxclass: Make output for RSS context action explicit

[Joe Damato]

On Mon, Nov 11, 2024 at 12:05:38PM -0700, Daniel Xu wrote:
> Currently `ethtool -n` prints out misleading output if the action for an
> ntuple rule is to redirect to an RSS context. 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
> 
> The above output suggests that the HW will direct to queue 0 where in
> reality queue 0 is just the base offset from which the redirection table
> lookup in the RSS context is added to.
> 
> Fix by making output more clear. Also suppress base offset queue for the
> common case of 0. Example of new output:
> 
>     # ./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>
> ---
> Changes from v1:
> * Reword to support queue base offset API
> * Fix compile error
> * Improve wording (also a transcription error)
> 
>  rxclass.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

I tested this on a machine with an mlx5 NIC.

FWIW: I only learned about ring_cookie from this thread, so I don't
feel qualified to give an official "Reviewed-by", but:

Before the patch:

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
sudo ethtool -U eth2 flow-type tcp4 dst-port 11211 context 1

$ 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

After the patch:

$ ./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
	Action: Direct to RSS context id 1

The results after the patch make more sense to me, personally. I
have NOT audited mlx5 at all, so no idea how (or if) it deals with
ring_cookie.

It'd be really great to add the bit about "queue base offset" to the
man page for ethtool.

All that said, since I tested it and it works and makes sense to me:

Tested-by: Joe Damato <jdamato@fastly.com>

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

[Edward Cree]

On 11/11/2024 19:05, Daniel Xu wrote:
> Currently `ethtool -n` prints out misleading output if the action for an
> ntuple rule is to redirect to an RSS context. 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
> 
> The above output suggests that the HW will direct to queue 0 where in
> reality queue 0 is just the base offset from which the redirection table
> lookup in the RSS context is added to.
> 
> Fix by making output more clear. Also suppress base offset queue for the
> common case of 0. Example of new output:
> 
>     # ./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>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

but someone who understands ethtool_get_flow_spec_ring_vf() should
 look at this as well to check whether that information's needed too.

> ---
> Changes from v1:
> * Reword to support queue base offset API
> * Fix compile error
> * Improve wording (also a transcription error)
> 
>  rxclass.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/rxclass.c b/rxclass.c
> index f17e3a5..ac9b529 100644
> --- a/rxclass.c
> +++ b/rxclass.c
> @@ -248,13 +248,17 @@ 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) {
> +		u64 queue = ethtool_get_flow_spec_ring(fsp->ring_cookie);
> +
> +		fprintf(stdout, "\tAction: Direct to RSS context id %u", rss_context);
> +		if (queue)
> +			fprintf(stdout, " (queue base offset: %llu)", queue);
> +		fprintf(stdout, "\n");
>  	} else {
>  		u64 vf = ethtool_get_flow_spec_ring_vf(fsp->ring_cookie);
>  		u64 queue = ethtool_get_flow_spec_ring(fsp->ring_cookie);
> 

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

[Jakub Kicinski]

On Mon, 11 Nov 2024 12:05:38 -0700 Daniel Xu wrote:
> -	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) {
> +		u64 queue = ethtool_get_flow_spec_ring(fsp->ring_cookie);
> +
> +		fprintf(stdout, "\tAction: Direct to RSS context id %u", rss_context);

Do you have strong feelings about the change in formatting?
Looking at Ed's comment on the new test made me wonder if the change 
in capitalization is for the better.

Action: Direct to RSS context id 1 (queue base offset: 2)

vs

Action: Direct to RSS Context ID: 1 (queue base offset: 2)

Given "id" is a word (: I like the ID format, the extra colon is
annoying but OTOH if we retain it your regexp in the other patch
would match before and after..

Actually the best formatting IMHO would be to skip the ID altogether:

Action: Direct to RSS Context 1 (queue base offset: 2)

So please respin this, either set the ID to upper case or skip it.
And depending on the decision here respin the test (feel free to
repost before 24h passes, if you do).

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

[Daniel Xu]

Hi Jakub,

On Tue, Nov 12, 2024, at 7:40 AM, Jakub Kicinski wrote:
> On Mon, 11 Nov 2024 12:05:38 -0700 Daniel Xu wrote:
>> -	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) {
>> +		u64 queue = ethtool_get_flow_spec_ring(fsp->ring_cookie);
>> +
>> +		fprintf(stdout, "\tAction: Direct to RSS context id %u", rss_context);
>
> Do you have strong feelings about the change in formatting?
> Looking at Ed's comment on the new test made me wonder if the change 
> in capitalization is for the better.
>
> Action: Direct to RSS context id 1 (queue base offset: 2)
>
> vs
>
> Action: Direct to RSS Context ID: 1 (queue base offset: 2)
>
> Given "id" is a word (: I like the ID format, the extra colon is
> annoying but OTOH if we retain it your regexp in the other patch
> would match before and after..
>
> Actually the best formatting IMHO would be to skip the ID altogether:
>
> Action: Direct to RSS Context 1 (queue base offset: 2)

No strong opinions other than I agree second colon should be skipped.
Let's go with this one.