Message-ID: <CAMy_GT8fdX2uUAM1j9Lzje+K5BnLH82dmx3CE=mGV9UqSRBWAg@mail.gmail.com>
Date:   Wed, 8 Mar 2023 18:02:26 +0800
From:   Po-Hsu Lin <po-hsu.lin@...onical.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        netdev@...r.kernel.org, idosch@...lanox.com,
        danieller@...lanox.com, petrm@...lanox.com, shuah@...nel.org,
        pabeni@...hat.com, edumazet@...gle.com, davem@...emloft.net
Subject: Re: [PATCHv2] selftests: net: devlink_port_split.py: skip test if no
 suitable device available

On Wed, Mar 8, 2023 at 9:02 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue,  7 Mar 2023 23:00:30 +0800 Po-Hsu Lin wrote:
> > The `devlink -j port show` command output may not contain the "flavour"
> > key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic),
> > iproute2-5.15.0:
> >   {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"},
> >            "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"},
> >            "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"},
> >            "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}}
> >
> > This will cause a KeyError exception.
>
> I looked closer and I don't understand why the key is not there.
> Both 5.19 kernel should always put this argument out, and 5.15
> iproute2 should always interpret it.
>
> Am I looking wrong? Do you see how we can get a dump with no flavor?
>
> I worry that this is some endianness problem, and we just misreport
> stuff on big-endian.
>
> > Create a validate_devlink_output() to check for this "flavour" from
> > devlink command output to avoid this KeyError exception. Also let
> > it handle the check for `devlink -j dev show` output in main().
> >
> > Apart from this, if the test was not started because of any reason
> > (e.g. "lanes" does not exist, max lanes is 0 or the flavour of the
> > designated device is not "physical" and etc.) The script will still
> > return 0 and thus causing a false-negative test result.
> >
> > Use a test_ran flag to determine if these tests were skipped and
> > return KSFT_SKIP to make it more clear.
> >
> > V2: factor out the skip logic from main(), update commit message and
> >     skip reasons accordingly.
> > Link: https://bugs.launchpad.net/bugs/1937133
> > Fixes: f3348a82e727 ("selftests: net: Add port split test")
> > Signed-off-by: Po-Hsu Lin <po-hsu.lin@...onical.com>
> > ---
> >  tools/testing/selftests/net/devlink_port_split.py | 36 +++++++++++++++++++----
> >  1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/devlink_port_split.py b/tools/testing/selftests/net/devlink_port_split.py
> > index 2b5d6ff..749606c 100755
> > --- a/tools/testing/selftests/net/devlink_port_split.py
> > +++ b/tools/testing/selftests/net/devlink_port_split.py
> > @@ -59,6 +59,8 @@ class devlink_ports(object):
> >          assert stderr == ""
> >          ports = json.loads(stdout)['port']
> >
> > +        validate_devlink_output(ports, 'flavour')
>
> If it's just a matter of kernel/iproute2 version we shouldn't need to
> check here again?
>
> >          for port in ports:
> >              if dev in port:
> >                  if ports[port]['flavour'] == 'physical':
> > @@ -220,6 +222,27 @@ def split_splittable_port(port, k, lanes, dev):
> >      unsplit(port.bus_info)
> >
> >
> > +def validate_devlink_output(devlink_data, target_property=None):
> > +    """
> > +    Determine if test should be skipped by checking:
> > +      1. devlink_data contains values
> > +      2. The target_property exist in devlink_data
> > +    """
> > +    skip_reason = None
> > +    if any(devlink_data.values()):
> > +        if target_property:
> > +            skip_reason = "{} not found in devlink output, test skipped".format(target_property)
> > +            for key in devlink_data:
> > +                if target_property in devlink_data[key]:
> > +                    skip_reason = None
> > +    else:
> > +        skip_reason = 'devlink output is empty, test skipped'
> > +
> > +    if skip_reason:
> > +        print(skip_reason)
> > +        sys.exit(KSFT_SKIP)
>
> Looks good, so..
>
> >  def make_parser():
> >      parser = argparse.ArgumentParser(description='A test for port splitting.')
> >      parser.add_argument('--dev',
> > @@ -231,6 +254,7 @@ def make_parser():
> >
> >
> >  def main(cmdline=None):
> > +    test_ran = False
>
> I don't think we need the test_ran tracking any more?
We still need this here to check if the test was actually started.

Take the following output for example (ARM64 server with Mellanox
Ethernet controller, running Ubuntu 22.10 5.19.0-35):
$ devlink port show
pci/0000:01:00.0/65535: type eth netdev enp1s0f0 flavour physical port
0 splittable false
pci/0000:01:00.1/131071: type eth netdev enp1s0f1 flavour physical
port 1 splittable false
There is no "lanes" attribute here, thus the max_lanes in main() will
be 0. The test won't be started at all but returns 0.


>
> >      parser = make_parser()
> >      args = parser.parse_args(cmdline)
> >
> > @@ -240,12 +264,9 @@ def main(cmdline=None):
> >          stdout, stderr = run_command(cmd)
> >          assert stderr == ""
> >
> > +        validate_devlink_output(json.loads(stdout))
> >          devs = json.loads(stdout)['dev']
> > -        if devs:
> > -            dev = list(devs.keys())[0]
> > -        else:
> > -            print("no devlink device was found, test skipped")
> > -            sys.exit(KSFT_SKIP)
> > +        dev = list(devs.keys())[0]
> >
> >      cmd = "devlink dev show %s" % dev
> >      stdout, stderr = run_command(cmd)
> > @@ -277,6 +298,11 @@ def main(cmdline=None):
> >                  split_splittable_port(port, lane, max_lanes, dev)
> >
> >                  lane //= 2
> > +        test_ran = True
> > +
> > +    if not test_ran:
> > +        print("Test not started, no suitable device for the test")
> > +        sys.exit(KSFT_SKIP)
> >
> >
> >  if __name__ == "__main__":
>

Powered by blists - more mailing lists