Message-ID: <366e4392-bd00-4120-8585-a71b3952e365@linux.dev>
Date: Tue, 24 Sep 2024 18:37:06 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Alexis Lothoré <alexis.lothore@...tlin.com>,
 Jakub Kicinski <kuba@...nel.org>, Lorenzo Bianconi <lorenzo@...nel.org>
Cc: Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
 Shuah Khan <shuah@...nel.org>, "David S. Miller" <davem@...emloft.net>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>, ebpf@...uxfoundation.org,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
 linux-kselftest@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2] selftests/bpf: convert test_xdp_features.sh
 to test_progs

On 9/22/24 12:04 PM, Alexis Lothoré wrote:
> Hello all, sorry for the slow feedback, I have been off last week.
> 
> On 9/14/24 15:38, Jakub Kicinski wrote:
>> On Sat, 14 Sep 2024 11:25:47 +0200 Lorenzo Bianconi wrote:
>>> On Sep 13, Martin KaFai Lau wrote:
>>>> test a physical network device that supports a certain XDP features.
>>>>
>>>> iiuc, test_xdp_features.sh only uses the veth and veth will also be the only
>>>> device tested after moving to prog_tests/xdp_features.c? It is a reasonable
>>>> addition to test_progs for an end-to-end xdp test by using veth. However,
>>>> test_progs will not be able to test the physical network device.
>>>>
>>>> Lorenzo, is the xdp_features.c still used for device testing?
>>>
>>> correct, xdp_features.c is intended to test the real xdp features supported by
>>> the NIC under test (DUT), not just the advertised ones (iirc that was a
>>> requisite to add xdp kernel feature support). For this reason we need two
>>> separated processes running on the tester device and on the DUT (they are
>>> usually two different devices). test_xdp_features.sh was just a simple test
>>> script used to develop xdp_features.c.
>>> What about extending xdp_features.c to integrate it in the CI?
> 
> So IIUC Lorenzo's answer, we _need_ to keep the possibility to run this test on
> real hardware, and so we _need_ to still be able to run two processes, possibly
> on two different machines. If that's so, indeed my rework breaks this. I have
> then multiple questions/doubts before being able to rework this:
> - the main goal of this rework is to be able to automatically run this test in
> CI, and the resulting constraint is that it must integrate in a standalone,
> already-existing c program (test_progs). I guess I can preserve the standalone
> xdp_features program as it is, and make test_progs just start  it twice (on two
> different veths). It would involve the following changes:
>    - keep a dedicated build step for this small, standalone xdp_features.c, and
> add a "controller" part in test_progs (instead of fully migrating xdp_features
> program into test_progs, which  is what the current series revision does)
>    - simply make the controller part create the testing network in CI, fork/start
> the xdp_features program on both veths, and check return codes.
> My main concern is about the possible flakiness of this whole setup (multiple

The test could be simpler if it does not need to run in two separate machines.

Also, there are bpf_prog_test_run_opts() style tests that provide a device 
agnostic way to test the xdp helpers/features which should have covered most of 
the cases exercised in progs/xdp_features.c?

I am not sure which case in xdp_features.c does not have existing coverage in 
test_progs. From a quick look, it seems only BPF_MAP_TYPE_CPUMAP is missing 
(please check)? If that is the case, it may be more straight forward to add this 
one test case to the test_progs. Check if it can borrow a similar setup from 
prog_tests/test_xdp_veth.c, and then leave xdp_features.* as-is.

There are other .sh tests that could better use the test_progs migration. In 
particular the ones without existing test coverage. For non XDP related, 
test_tcp_check_syncookie.sh, test_flow_dissector.sh, and test_tc_edt.sh should 
be the good ones.

For XDP, test_xdp_meta.sh should be useful also. You may also want to check the 
test_xdp_redirect_*.sh.

> processes and tcp/udp channels involved), but if keeping the standalone version
> is really needed, I can give a try. Does it sound reasonable ?
> - one part of my overall goal is to clean up the tools/testing/selftests/bpf
> directory from anything that is not tested automatically. What should we do with
> the wrapping shell script (test_xdp_features.sh) ? Since test_progs will
> automate the test with veths, I guess it is still ok to just remove it ?
> 
>> No preference but just to raise awareness - drivers/net's NetDrvEpEnv
>> class provides the setup for running tests with an endpoint.
>> XDP tests intended for HW would fit there pretty well.
> 
> Thanks for the hint. If we want to keep some tooling for real hw xdp features
> testing, maybe we could add a small part in tools/testing/selftests/drivers/net
> and make it use this NetDrvEpEnv ? Or it is a bigger hint that the whole test
> about xdp features could be moved there (and then tested by net kselftests
> rather than by ebpf ci specifically) ? @Lorenzo and eBPF tests maintainers, any
> opinion ?
> 
> Thanks,
> 
> Alexis
> 

Powered by blists - more mailing lists