| Message ID | 978e1192c07e970b8944c2a729ae42bf97667a53.1731107871.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool-next,v2] rxclass: Make output for RSS context action explicit | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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>
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); >
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).
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.
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);
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(-)