netdev - Re: [PATCH bpf-next v2 5/5] selftests/bpf: xsk selftests - Bi-directional Sockets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ebfb17fa-39e0-5810-f1a6-20c6804172c8@fb.com>
Date:   Tue, 24 Nov 2020 09:09:58 -0800
From:   Yonghong Song <yhs@...com>
To:     Weqaar Janjua <weqaar.janjua@...il.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 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.

> 
>>>> +
>>>> +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
>>> [...]