Re: [PATCH net-next v2 12/12] selftests: ncdevmem: Add automated test

[Stanislav Fomichev <stfomichev@gmail.com>]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Only RX side for now and small message to test the setup.
> > In the future, we can extend it to TX side and to testing
> > both sides with a couple of megs of data.
> >
> 
> This is really awesome. Thank you.
> 
> >   make \
> >         -C tools/testing/selftests \
> >         TARGETS="drivers/hw/net" \
> >         install INSTALL_PATH=~/tmp/ksft
> >
> >   scp ~/tmp/ksft ${HOST}:
> >   scp ~/tmp/ksft ${PEER}:
> >
> >   cfg+="NETIF=${DEV}\n"
> >   cfg+="LOCAL_V6=${HOST_IP}\n"
> >   cfg+="REMOTE_V6=${PEER_IP}\n"
> 
> Not a review comment but noob question: does NIPA not support ipv4? Or
> is ipv6 preferred here?

Yes, absolutely, you can pass it LOCAL/REMOTE_V4 but you'll have to make
some changes to the selftest itself to use the v4 addresses. Things like
'-s {cfg.v6}' will have to be changed to '-s ::ffff:{cfg.v4}'.

I wonder whether it might be a good idea to have some new config method that
falls back to v4-mapped-v6 (::ffff:{cfg.v4}) to support both v4 and v6
transparently for the selftests that don't care about particular transport?

I'll leave it for you to explore...

> >   cfg+="REMOTE_TYPE=ssh\n"
> >   cfg+="REMOTE_ARGS=root@${PEER}\n"
> >
> >   echo -e "$cfg" | ssh root@${HOST} "cat > ksft/drivers/net/net.config"
> >   ssh root@${HOST} "cd ksft && ./run_kselftest.sh -t drivers/net:devmem.py"
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  .../testing/selftests/drivers/net/hw/Makefile |  1 +
> >  .../selftests/drivers/net/hw/devmem.py        | 46 +++++++++++++++++++
> >  2 files changed, 47 insertions(+)
> >  create mode 100755 tools/testing/selftests/drivers/net/hw/devmem.py
> >
> > diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> > index 7bce46817953..a582b1bb3ae1 100644
> > --- a/tools/testing/selftests/drivers/net/hw/Makefile
> > +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> > @@ -3,6 +3,7 @@
> >  TEST_PROGS = \
> >         csum.py \
> >         devlink_port_split.py \
> > +       devmem.py \
> >         ethtool.sh \
> >         ethtool_extended_state.sh \
> >         ethtool_mm.sh \
> > diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> > new file mode 100755
> > index 000000000000..29085591616b
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> > @@ -0,0 +1,46 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +import errno
> > +from lib.py import ksft_run, ksft_exit
> > +from lib.py import ksft_eq, KsftSkipEx
> > +from lib.py import NetDrvEpEnv
> > +from lib.py import bkg, cmd, rand_port, wait_port_listen
> > +from lib.py import ksft_disruptive
> > +
> > +
> > +def require_devmem(cfg):
> > +    if not hasattr(cfg, "_devmem_probed"):
> > +        port = rand_port()
> > +        probe_command = f"./ncdevmem -f {cfg.ifname}"
> > +        cfg._devmem_supported = cmd(probe_command, fail=False, shell=True).ret == 0
> > +        cfg._devmem_probed = True
> > +
> > +    if not cfg._devmem_supported:
> > +        raise KsftSkipEx("Test requires devmem support")
> > +
> > +
> > +@ksft_disruptive
> > +def check_rx(cfg) -> None:
> > +    cfg.require_v6()
> > +    require_devmem(cfg)
> > +
> > +    port = rand_port()
> > +    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port}"
> 
> So AFAICT adding validation to this test is simple. What you would do
> is change the above line to:
> 
> listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port} -v 7"
> 
> then, below...
> 
> > +
> > +    with bkg(listen_cmd) as nc:
> > +        wait_port_listen(port)
> > +        cmd(f"echo -e \"hello\\nworld\"| nc {cfg.v6} {port}", host=cfg.remote, shell=True)
> > +
> 
> ...change this to the equivalent of 'yes $(echo -e
> \\x01\\x02\\x03\\x04\\x05\\x06) | tr \\n \\0 | head -c 1G"
> 
> > +    ksft_eq(nc.stdout.strip(), "hello\nworld")
> > +
> 
> ...then remove this ksft_eq().
> 
> But this is just a suggestion, I think you were leaving this to future
> work for me, which is fine.
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>

Let me try. Worst case I leave it as is and you'll follow up with
the conversion once you get the TX side going..