Date:   Tue, 24 Nov 2020 22:28:19 +0000
From:   Weqaar Janjua <weqaar.janjua@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        Weqaar Janjua <weqaar.a.janjua@...el.com>, shuah@...nel.org,
        skhan@...uxfoundation.org, linux-kselftest@...r.kernel.org,
        Anders Roxell <anders.roxell@...aro.org>,
        jonathan.lemon@...il.com
Subject: Re: [PATCH bpf-next v2 5/5] selftests/bpf: xsk selftests -
 Bi-directional Sockets - SKB, DRV

On Tue, 24 Nov 2020 at 17:10, Yonghong Song <yhs@...com> wrote:
>
>
>
> On 11/24/20 7:11 AM, Weqaar Janjua wrote:
> > On Sat, 21 Nov 2020 at 20:14, Weqaar Janjua <weqaar.janjua@...il.com> wrote:
> >>
> >> On Fri, 20 Nov 2020 at 20:45, Yonghong Song <yhs@...com> wrote:
> >>>
> >>>
> >>>
> >>> On 11/20/20 5:00 AM, Weqaar Janjua wrote:
> >>>> Adds following tests:
> >>>>
> >>>> 1. AF_XDP SKB mode
> >>>>      d. Bi-directional Sockets
> >>>>         Configure sockets as bi-directional tx/rx sockets, sets up fill
> >>>>         and completion rings on each socket, tx/rx in both directions.
> >>>>         Only nopoll mode is used
> >>>>
> >>>> 2. AF_XDP DRV/Native mode
> >>>>      d. Bi-directional Sockets
> >>>>      * Only copy mode is supported because veth does not currently support
> >>>>        zero-copy mode
> >>>>
> >>>> Signed-off-by: Weqaar Janjua <weqaar.a.janjua@...el.com>
> >>>> ---
> >>>>    tools/testing/selftests/bpf/Makefile          |   4 +-
> >>>>    .../bpf/test_xsk_drv_bidirectional.sh         |  23 ++++
> >>>>    .../selftests/bpf/test_xsk_drv_teardown.sh    |   3 -
> >>>>    .../bpf/test_xsk_skb_bidirectional.sh         |  20 ++++
> >>>>    tools/testing/selftests/bpf/xdpxceiver.c      | 100 +++++++++++++-----
> >>>>    tools/testing/selftests/bpf/xdpxceiver.h      |   4 +
> >>>>    6 files changed, 126 insertions(+), 28 deletions(-)
> >>>>    create mode 100755 tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> >>>>    create mode 100755 tools/testing/selftests/bpf/test_xsk_skb_bidirectional.sh
> >>>>
> >>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> >>>> index 515b29d321d7..258bd72812e0 100644
> >>>> --- a/tools/testing/selftests/bpf/Makefile
> >>>> +++ b/tools/testing/selftests/bpf/Makefile
> >>>> @@ -78,7 +78,9 @@ TEST_PROGS := test_kmod.sh \
> >>>>        test_xsk_drv_nopoll.sh \
> >>>>        test_xsk_drv_poll.sh \
> >>>>        test_xsk_skb_teardown.sh \
> >>>> -     test_xsk_drv_teardown.sh
> >>>> +     test_xsk_drv_teardown.sh \
> >>>> +     test_xsk_skb_bidirectional.sh \
> >>>> +     test_xsk_drv_bidirectional.sh
> >>>>
> >>>>    TEST_PROGS_EXTENDED := with_addr.sh \
> >>>>        with_tunnels.sh \
> >>>> diff --git a/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh b/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> >>>> new file mode 100755
> >>>> index 000000000000..d3a7e2934d83
> >>>> --- /dev/null
> >>>> +++ b/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> >>>> @@ -0,0 +1,23 @@
> >>>> +#!/bin/bash
> >>>> +# SPDX-License-Identifier: GPL-2.0
> >>>> +# Copyright(c) 2020 Intel Corporation.
> >>>> +
> >>>> +# See test_xsk_prerequisites.sh for detailed information on tests
> >>>> +
> >>>> +. xsk_prereqs.sh
> >>>> +. xsk_env.sh
> >>>> +
> >>>> +TEST_NAME="DRV BIDIRECTIONAL SOCKETS"
> >>>> +
> >>>> +vethXDPnative ${VETH0} ${VETH1} ${NS1}
> >>>> +
> >>>> +params=("-N" "-B")
> >>>> +execxdpxceiver params
> >>>> +
> >>>> +retval=$?
> >>>> +test_status $retval "${TEST_NAME}"
> >>>> +
> >>>> +# Must be called in the last test to execute
> >>>> +cleanup_exit ${VETH0} ${VETH1} ${NS1}
> >>>
> >>> This also makes hard to run tests as users will not know this unless
> >>> they are familiar with the details of the tests.
> >>>
> >>> How about you have another scripts test_xsk.sh which includes all these
> >>> individual tests and pull the above cleanup_exit into test_xsk.sh?
> >>> User just need to run test_xsk.sh will be able to run all tests you
> >>> implemented here.
> >>>
> >> This works, test_xsk_* >> test_xsk.sh, will ship out as v3.
> >>
> > An issue with merging all tests in a single test_xsk.sh is reporting
> > number of test failures, with this approach a single test status is
> > printed by kselftest:
> >
> > # PREREQUISITES: [ PASS ]
> > # SKB NOPOLL: [ FAIL ]
> > # SKB POLL: [ PASS ]
> > ok 1 selftests: xsk-patch2: test_xsk.sh
> >
> > This is due to the fact Makefile has one TEST_PROGS = test_xsk.sh
> > (thus kselftest considers it one test?), where in the original
> > approach all tests have separate TEST_PROGS .sh which makes reporting
> > match each test and status. This can be a problem for automation.
> >
> > An alternative would be to exit each test with failure status but then
> > the tests will stop execution at the failed test without executing the
> > rest of xsk tests, which we probably wouldn't want.
> >
> > Suggestions please?
>
> I think it is okay to put everything xsk related to one test.
> If later on the test becomes more complex, you can have
> test_xsk_<1>.sh test_xsk_<2>.sh etc. But each .sh should be able to
> run independently without any particular order.
>
> You can have subtests inside the .sh file. See test_offload.py as
> an example. You do not need to exit after one subtest fails, you can
> continue to run the next one. currently test_offload.py
> may exit when some subtest failed, but I think you don't have to.
>
ACK, I will go ahead and merge all test_xsk_*.sh into test_xsk.sh.

Just to clarify that all current xsk tests are independent, there is
no subtest at present, and do not need to run in any order.

Thanks,
/Weqaar

> >
> >>>> +
> >>>> +test_exit $retval 0
> >>>> diff --git a/tools/testing/selftests/bpf/test_xsk_drv_teardown.sh b/tools/testing/selftests/bpf/test_xsk_drv_teardown.sh
> >>> [...]

Powered by blists - more mailing lists