diff mbox series

[net-next,1/5] selftests: drv-net: rss_ctx: fix cleanup in the basic test

Message ID 20240705015725.680275-2-kuba@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series selftests: drv-net: rss_ctx: more tests | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: linux-kselftest@vger.kernel.org shuah@kernel.org
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 26 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-07-05--06-00 (tests: 695)

Commit Message

Jakub Kicinski July 5, 2024, 1:57 a.m. UTC 
The basic test may fail without resetting the RSS indir table.
While at it reformat the doc a tiny bit.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/hw/rss_ctx.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Willem de Bruijn July 6, 2024, 1:53 p.m. UTC | #1 
Jakub Kicinski wrote:
> The basic test may fail without resetting the RSS indir table.
> While at it reformat the doc a tiny bit.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/hw/rss_ctx.py | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> index 475f2a63fcd5..de2a55c0f35c 100755
> --- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> +++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> @@ -64,9 +64,8 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
>  
>  
>  def test_rss_key_indir(cfg):
> -    """
> -    Test basics like updating the main RSS key and indirection table.
> -    """
> +    """Test basics like updating the main RSS key and indirection table."""
> +
>      if len(_get_rx_cnts(cfg)) < 2:
>          KsftSkipEx("Device has only one queue (or doesn't support queue stats)")
>  
> @@ -89,6 +88,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
>  
>      # Set the indirection table
>      ethtool(f"-X {cfg.ifname} equal 2")
> +    reset_indir = defer(ethtool, f"-X {cfg.ifname} default")
>      data = get_rss(cfg)
>      ksft_eq(0, min(data['rss-indirection-table']))
>      ksft_eq(1, max(data['rss-indirection-table']))
> @@ -104,7 +104,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
>      ksft_eq(sum(cnts[2:]), 0, "traffic on unused queues: " + str(cnts))
>  
>      # Restore, and check traffic gets spread again
> -    ethtool(f"-X {cfg.ifname} default")
> +    reset_indir.exec()

When is this explicit exec needed?
Jakub Kicinski July 8, 2024, 4:13 p.m. UTC | #2 
On Sat, 06 Jul 2024 09:53:41 -0400 Willem de Bruijn wrote:
> > @@ -89,6 +88,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
> >  
> >      # Set the indirection table
> >      ethtool(f"-X {cfg.ifname} equal 2")
> > +    reset_indir = defer(ethtool, f"-X {cfg.ifname} default")
> >      data = get_rss(cfg)
> >      ksft_eq(0, min(data['rss-indirection-table']))
> >      ksft_eq(1, max(data['rss-indirection-table']))
> > @@ -104,7 +104,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
> >      ksft_eq(sum(cnts[2:]), 0, "traffic on unused queues: " + str(cnts))
> >  
> >      # Restore, and check traffic gets spread again
> > -    ethtool(f"-X {cfg.ifname} default")
> > +    reset_indir.exec()  
> 
> When is this explicit exec needed?

When you want to run the cleanup _now_.

We construct the cleanup as soon as we allocate the resource,
it stays on the deferred list in case some exception makes us abort,
but the test may want to free the resource or reconfigure it further
as part of the test, in which case it can run .exec() (or cancel() to
discard the clean up without running it).

Here we do:
 1. constrain RSS
 2. run traffic (to check we're hitting expected queues)
 3. reset RSS
 4. run traffic (to check we're hitting all queues)

so step 3 runs the cleanup of step 1 explicitly.
Willem de Bruijn July 8, 2024, 4:53 p.m. UTC | #3 
Jakub Kicinski wrote:
> On Sat, 06 Jul 2024 09:53:41 -0400 Willem de Bruijn wrote:
> > > @@ -89,6 +88,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
> > >  
> > >      # Set the indirection table
> > >      ethtool(f"-X {cfg.ifname} equal 2")
> > > +    reset_indir = defer(ethtool, f"-X {cfg.ifname} default")
> > >      data = get_rss(cfg)
> > >      ksft_eq(0, min(data['rss-indirection-table']))
> > >      ksft_eq(1, max(data['rss-indirection-table']))
> > > @@ -104,7 +104,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
> > >      ksft_eq(sum(cnts[2:]), 0, "traffic on unused queues: " + str(cnts))
> > >  
> > >      # Restore, and check traffic gets spread again
> > > -    ethtool(f"-X {cfg.ifname} default")
> > > +    reset_indir.exec()  
> > 
> > When is this explicit exec needed?
> 
> When you want to run the cleanup _now_.
> 
> We construct the cleanup as soon as we allocate the resource,
> it stays on the deferred list in case some exception makes us abort,
> but the test may want to free the resource or reconfigure it further
> as part of the test, in which case it can run .exec() (or cancel() to
> discard the clean up without running it).
> 
> Here we do:
>  1. constrain RSS
>  2. run traffic (to check we're hitting expected queues)
>  3. reset RSS
>  4. run traffic (to check we're hitting all queues)
> 
> so step 3 runs the cleanup of step 1 explicitly.

Thanks. I was wondering why this test calls it explicitly, while
others do not. Had overlooked step 4, which requires the reset.
diff mbox series

Patch

diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
index 475f2a63fcd5..de2a55c0f35c 100755
--- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
+++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
@@ -64,9 +64,8 @@  from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
 
 
 def test_rss_key_indir(cfg):
-    """
-    Test basics like updating the main RSS key and indirection table.
-    """
+    """Test basics like updating the main RSS key and indirection table."""
+
     if len(_get_rx_cnts(cfg)) < 2:
         KsftSkipEx("Device has only one queue (or doesn't support queue stats)")
 
@@ -89,6 +88,7 @@  from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
 
     # Set the indirection table
     ethtool(f"-X {cfg.ifname} equal 2")
+    reset_indir = defer(ethtool, f"-X {cfg.ifname} default")
     data = get_rss(cfg)
     ksft_eq(0, min(data['rss-indirection-table']))
     ksft_eq(1, max(data['rss-indirection-table']))
@@ -104,7 +104,7 @@  from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
     ksft_eq(sum(cnts[2:]), 0, "traffic on unused queues: " + str(cnts))
 
     # Restore, and check traffic gets spread again
-    ethtool(f"-X {cfg.ifname} default")
+    reset_indir.exec()
 
     cnts = _get_rx_cnts(cfg)
     GenerateTraffic(cfg).wait_pkts_and_stop(20000)