Re: [PATCH net-next 1/2] selftests: drv-net: gro: improve feature config

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jakub Kicinski wrote:
> We'll need to do a lot more feature handling to test HW-GRO and LRO.
> Clean up the feature handling for SW GRO a bit to let the next commit
> focus on the new test cases, only.
> 
> Make sure HW GRO-like features are not enabled for the SW tests.
> Be more careful about changing features as "nothing changed"
> situations may result in non-zero error code from ethtool.
> 
> Don't disable TSO on the local interface (receiver) when running over
> netdevsim, we just want GSO to break up the segments on the sender.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: linux-kselftest@vger.kernel.org
> ---
>  tools/testing/selftests/drivers/net/gro.py | 38 ++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/gro.py b/tools/testing/selftests/drivers/net/gro.py
> index ba83713bf7b5..6d633bdc7e67 100755
> --- a/tools/testing/selftests/drivers/net/gro.py
> +++ b/tools/testing/selftests/drivers/net/gro.py
> @@ -20,7 +20,7 @@ coalescing behavior.
>  import os
>  from lib.py import ksft_run, ksft_exit, ksft_pr
>  from lib.py import NetDrvEpEnv, KsftXfailEx
> -from lib.py import cmd, defer, bkg, ip
> +from lib.py import cmd, defer, bkg, ethtool, ip

Is there a pattern behind this order. Since inserted rather than
appended. Intended to be alphabetical?

>  from lib.py import ksft_variants
>  
>  
> @@ -70,6 +70,27 @@ from lib.py import ksft_variants
>          defer(ip, f"link set dev {dev['ifname']} mtu {dev['mtu']}", host=host)
>  
>  
> +def _set_ethtool_feat(dev, current, feats, host=None):
> +    s2n = {True: "on", False: "off"}
> +
> +    new = ["-K", dev]
> +    old = ["-K", dev]
> +    no_change = True
> +    for name, state in feats.items():
> +        new += [name, s2n[state]]
> +        old += [name, s2n[not state]]

Should the change set not only include items for which
current != state?

Now old assumes not state, but that is not necessarily true?
> +
> +        if current[name]["active"] != state:
> +            no_change = False
> +            if current[name]["fixed"]:
> +                raise KsftXfailEx(f"Device does not support {name}")
> +    if no_change:
> +        return
> +
> +    ethtool(" ".join(new), host=host)
> +    defer(ethtool, " ".join(old), host=host)
> +
> +
>  def _setup(cfg, test_name):
>      """ Setup hardware loopback mode for GRO testing. """
>  
> @@ -77,6 +98,11 @@ from lib.py import ksft_variants
>          cfg.bin_local = cfg.test_dir / "gro"
>          cfg.bin_remote = cfg.remote.deploy(cfg.bin_local)
>  
> +    if not hasattr(cfg, "feat"):
> +        cfg.feat = ethtool(f"-k {cfg.ifname}", json=True)[0]
> +        cfg.remote_feat = ethtool(f"-k {cfg.remote_ifname}",
> +                                  host=cfg.remote, json=True)[0]
> +
>      # "large" test needs at least 4k MTU
>      if test_name == "large":
>          _set_mtu_restore(cfg.dev, 4096, None)
> @@ -88,15 +114,21 @@ from lib.py import ksft_variants
>      _write_defer_restore(cfg, flush_path, "200000", defer_undo=True)
>      _write_defer_restore(cfg, irq_path, "10", defer_undo=True)
>  
> +    _set_ethtool_feat(cfg.ifname, cfg.feat,
> +                      {"generic-receive-offload": True,
> +                       "rx-gro-hw": False,
> +                       "large-receive-offload": False})
> +
>      try:
>          # Disable TSO for local tests
>          cfg.require_nsim()  # will raise KsftXfailEx if not running on nsim
>  
> -        cmd(f"ethtool -K {cfg.ifname} gro on tso off")
> -        cmd(f"ethtool -K {cfg.remote_ifname} gro on tso off", host=cfg.remote)
> +        _set_ethtool_feat(cfg.remote_ifname, cfg.remote_feat, {"tso": False},
> +                          host=cfg.remote)
>      except KsftXfailEx:
>          pass
>  
> +
>  def _gro_variants():
>      """Generator that yields all combinations of protocol and test types."""
>  
> -- 
> 2.51.1
>