[PATCH net-next] selftests: drv-net: ping: make sure the ping test restores checksum offload

[PATCH net-next] selftests: drv-net: ping: make sure the ping test restores checksum offload

[Jakub Kicinski]

The ping test flips checksum offload on and off.
Make sure the original value is restored if test fails.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: shuah@kernel.org
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/drivers/net/ping.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/ping.py b/tools/testing/selftests/drivers/net/ping.py
index af8df2313a3b..e0f114612c1a 100755
--- a/tools/testing/selftests/drivers/net/ping.py
+++ b/tools/testing/selftests/drivers/net/ping.py
@@ -50,6 +50,16 @@ no_sleep=False
         cmd(f"echo {test_string} | socat -t 2 -u STDIN TCP:{cfg.remote_baddr}:{port}", shell=True)
     ksft_eq(nc.stdout.strip(), test_string)
 
+def _schedule_checksum_reset(cfg, netnl) -> None:
+    features = ethtool(f"-k {cfg.ifname}", json=True)
+    setting = ""
+    for side in ["tx", "rx"]:
+        f = features[0][side + "-checksumming"]
+        if not f["fixed"]:
+            setting += " " + side
+            setting += " " + ("on" if f["requested"] or f["active"] else "off")
+    defer(ethtool, f" -K {cfg.ifname} " + setting)
+
 def _set_offload_checksum(cfg, netnl, on) -> None:
     try:
         ethtool(f" -K {cfg.ifname} rx {on} tx {on} ")
@@ -139,6 +149,7 @@ no_sleep=False
 def test_default_v4(cfg, netnl) -> None:
     cfg.require_ipver("4")
 
+    _schedule_checksum_reset(cfg, netnl)
     _set_offload_checksum(cfg, netnl, "off")
     _test_v4(cfg)
     _test_tcp(cfg)
@@ -149,6 +160,7 @@ no_sleep=False
 def test_default_v6(cfg, netnl) -> None:
     cfg.require_ipver("6")
 
+    _schedule_checksum_reset(cfg, netnl)
     _set_offload_checksum(cfg, netnl, "off")
     _test_v6(cfg)
     _test_tcp(cfg)
@@ -157,6 +169,7 @@ no_sleep=False
     _test_tcp(cfg)
 
 def test_xdp_generic_sb(cfg, netnl) -> None:
+    _schedule_checksum_reset(cfg, netnl)
     _set_xdp_generic_sb_on(cfg)
     _set_offload_checksum(cfg, netnl, "off")
     _test_v4(cfg)
@@ -168,6 +181,7 @@ no_sleep=False
     _test_tcp(cfg)
 
 def test_xdp_generic_mb(cfg, netnl) -> None:
+    _schedule_checksum_reset(cfg, netnl)
     _set_xdp_generic_mb_on(cfg)
     _set_offload_checksum(cfg, netnl, "off")
     _test_v4(cfg)
@@ -179,6 +193,7 @@ no_sleep=False
     _test_tcp(cfg)
 
 def test_xdp_native_sb(cfg, netnl) -> None:
+    _schedule_checksum_reset(cfg, netnl)
     _set_xdp_native_sb_on(cfg)
     _set_offload_checksum(cfg, netnl, "off")
     _test_v4(cfg)
@@ -190,6 +205,7 @@ no_sleep=False
     _test_tcp(cfg)
 
 def test_xdp_native_mb(cfg, netnl) -> None:
+    _schedule_checksum_reset(cfg, netnl)
     _set_xdp_native_mb_on(cfg)
     _set_offload_checksum(cfg, netnl, "off")
     _test_v4(cfg)
-- 
2.49.0

Re: [PATCH net-next] selftests: drv-net: ping: make sure the ping test restores checksum offload

[David Wei]

On 5/8/25 14:40, Jakub Kicinski wrote:
> The ping test flips checksum offload on and off.
> Make sure the original value is restored if test fails.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: linux-kselftest@vger.kernel.org
> ---
>   tools/testing/selftests/drivers/net/ping.py | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/tools/testing/selftests/drivers/net/ping.py b/tools/testing/selftests/drivers/net/ping.py
> index af8df2313a3b..e0f114612c1a 100755
> --- a/tools/testing/selftests/drivers/net/ping.py
> +++ b/tools/testing/selftests/drivers/net/ping.py
> @@ -50,6 +50,16 @@ no_sleep=False
>           cmd(f"echo {test_string} | socat -t 2 -u STDIN TCP:{cfg.remote_baddr}:{port}", shell=True)
>       ksft_eq(nc.stdout.strip(), test_string)
>   
> +def _schedule_checksum_reset(cfg, netnl) -> None:
> +    features = ethtool(f"-k {cfg.ifname}", json=True)
> +    setting = ""
> +    for side in ["tx", "rx"]:
> +        f = features[0][side + "-checksumming"]
> +        if not f["fixed"]:

I checked and found that "fixed" is a ternary:

         "rx-checksumming": {
             "active": true,
             "fixed": false,
             "requested": true
         },
         "tx-checksumming": {
             "active": true,
             "fixed": null,
             "requested": null
         },

Python loads this JSON as False and None types respectively, and `not
f["fixed"]` is true for both False and None. Maybe this doesn't matter
but flagging it.

> +            setting += " " + side
> +            setting += " " + ("on" if f["requested"] or f["active"] else "off")
> +    defer(ethtool, f" -K {cfg.ifname} " + setting)

This does rx/tx-gro too even if not explicitly requested. I assume that
is okay?

Re: [PATCH net-next] selftests: drv-net: ping: make sure the ping test restores checksum offload

[Jakub Kicinski]

On Thu, 8 May 2025 14:59:12 -0700 David Wei wrote:
> > +def _schedule_checksum_reset(cfg, netnl) -> None:
> > +    features = ethtool(f"-k {cfg.ifname}", json=True)
> > +    setting = ""
> > +    for side in ["tx", "rx"]:
> > +        f = features[0][side + "-checksumming"]
> > +        if not f["fixed"]:  
> 
> I checked and found that "fixed" is a ternary:
> 
>          "rx-checksumming": {
>              "active": true,
>              "fixed": false,
>              "requested": true
>          },
>          "tx-checksumming": {
>              "active": true,
>              "fixed": null,
>              "requested": null
>          },
> 
> Python loads this JSON as False and None types respectively, and `not
> f["fixed"]` is true for both False and None. Maybe this doesn't matter
> but flagging it.

I think so, yes.

> > +            setting += " " + side
> > +            setting += " " + ("on" if f["requested"] or f["active"] else "off")
> > +    defer(ethtool, f" -K {cfg.ifname} " + setting)  
> 
> This does rx/tx-gro too even if not explicitly requested. I assume that
> is okay?

You mean because those are automatically updated when we change
checksumming? If so then yes.

Re: [PATCH net-next] selftests: drv-net: ping: make sure the ping test restores checksum offload

[David Wei]

On 5/8/25 18:37, Jakub Kicinski wrote:
> On Thu, 8 May 2025 14:59:12 -0700 David Wei wrote:
>>> +def _schedule_checksum_reset(cfg, netnl) -> None:
>>> +    features = ethtool(f"-k {cfg.ifname}", json=True)
>>> +    setting = ""
>>> +    for side in ["tx", "rx"]:
>>> +        f = features[0][side + "-checksumming"]
>>> +        if not f["fixed"]:
>>
>> I checked and found that "fixed" is a ternary:
>>
>>           "rx-checksumming": {
>>               "active": true,
>>               "fixed": false,
>>               "requested": true
>>           },
>>           "tx-checksumming": {
>>               "active": true,
>>               "fixed": null,
>>               "requested": null
>>           },
>>
>> Python loads this JSON as False and None types respectively, and `not
>> f["fixed"]` is true for both False and None. Maybe this doesn't matter
>> but flagging it.
> 
> I think so, yes.
> 
>>> +            setting += " " + side
>>> +            setting += " " + ("on" if f["requested"] or f["active"] else "off")
>>> +    defer(ethtool, f" -K {cfg.ifname} " + setting)
>>
>> This does rx/tx-gro too even if not explicitly requested. I assume that
>> is okay?
> 
> You mean because those are automatically updated when we change
> checksumming? If so then yes.

Thanks for responding.

Reviewed-by: David Wei <dw@davidwei.uk>

Re: [PATCH net-next] selftests: drv-net: ping: make sure the ping test restores checksum offload

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  8 May 2025 14:40:05 -0700 you wrote:
> The ping test flips checksum offload on and off.
> Make sure the original value is restored if test fails.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: linux-kselftest@vger.kernel.org
> 
> [...]

Here is the summary with links:
  - [net-next] selftests: drv-net: ping: make sure the ping test restores checksum offload
    https://git.kernel.org/netdev/net-next/c/ef5224ed25e0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html