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