Repository navigation
Conversation
|
Hmm, this PR breaks the build on my machine: make
lib
libxdp
CC staticobjs/libxdp.o
CC staticobjs/xsk.o
CLANG xdp-dispatcher.o
In file included from xdp-dispatcher.c:3:
../../headers/linux/bpf.h:11:10: fatal error: 'linux/types.h' file not found
11 | #include <linux/types.h>
| ^~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [Makefile:149: xdp-dispatcher.o] Error 1
make[1]: *** [Makefile:20: libxdp] Error 2
make: *** [Makefile:31: lib] Error 2Also, your |
|
As for the toolchain bug workaround, yeah that's a bit ugly, so I'd prefer to |
Thanks for the review! |
4075db1 to
264c14c
Compare
Thanks |
|
I'm guessing the selftests are not deterministic and have a small error rate? Here is a table recap and the failures look pretty random:
Error is always "ERROR: IPv6 packet not received" during test_capt_pcapng which ends up failing. |
|
Yeah, they're a bit flaky sometimes - let me re-run the failed ones... |
|
What do you think about this new iteration? I was thinking about sending V2 for the Buildroot series, but I'd prefer doing that with xdp-tools patches that the maintainer agrees with. |
tohojo
left a comment
There was a problem hiding this comment.
OK with the overall approach, with some nits:
| PING_WAIT_SECS="0.1" | ||
| else | ||
| PING_WAIT_SECS="1" | ||
| fi |
There was a problem hiding this comment.
We should wrap this in a function (with caching) so it's only executed when needed.
There was a problem hiding this comment.
I was going to send this, but instead used a simpler approach: busybox timeout supports 0.1 so we use that everywhere. Most ping -W 0.1 calls are doing it ping -W0.1 -c1 so transition is smooth. See commit message for "tests: avoid fractional ping -W timeout" for the one edge case.
| fi | ||
|
|
||
| STATEDIR="$(mktemp -d --tmpdir=${TMPDIR:-/tmp} --suffix=.xdptest)" | ||
| STATEDIR="$(mktemp -d -p "${TMPDIR:-/tmp}" xdptest.XXXXXX)" |
There was a problem hiding this comment.
Let's just turn this into the same invocation as in test-libxdp.sh (-d -t and drop the -p) instead of having subtly different invocations that achieve the same thing...
| *) BPF_FREESTANDING=1 ;; | ||
| esac | ||
| fi | ||
|
|
There was a problem hiding this comment.
Let's drop the heuristic and make this explicit opt-in only. That seems less likely to break things...
| : ${BPFTOOL=bpftool} | ||
| : ${READELF=readelf} | ||
| : ${ARCH_INCLUDES=} | ||
| : ${BPF_FREESTANDING:=} |
There was a problem hiding this comment.
Naming nit - "freestanding" is not obvious. Maybe BPF_SKIP_STD_INCLUDE?
264c14c to
667602f
Compare
667602f to
5431d35
Compare
BPF objects target the arch-independent BPF ISA but get access to the
arch-specific target sysroot in BPF_CFLAGS. This causes trouble.
Issues are:
- Arch-specific kernel UAPI headers provide <asm/byteorder.h> which can
be broken. MIPS version uses __MIPSEB__/__MIPSEL__ for endianess
detection and #error out if both are undefined. ARM64
does #ifdef __AARCH64EB__ #else ..., meaning we fallback silently to
little-endian. Luckily everyone does LE on ARM64.
- Arch-specific kernel UAPI headers provide <asm/bitsperlong.h> which
can be broken in similar ways. MIPS defines __BITS_PER_LONG using
_MIPS_SZLONG, which is undefined. Again, a known working architecture
has a silent fallback: x86 picks 32 or 64 based on if __x86_64__ is
defined.
- `-nostdlibinc` is needed to ensure our host clang isn't falling back
to host headers, which might work by accident.
We get close to the same setup as the kernel BPF selftests. They however
do not introduce generic asm/*.h, they prefer a list of -D macros.
Required when targeting a MIPS64 platform. Without this patch the
package build fails compiling the vendored BPF objects:
xdp_flowtable.bpf.c:39:26: error: incomplete definition of type 'struct iphdr'
xdp_flowtable.bpf.c:41:18: error: call to undeclared function 'bpf_htons'
Also validated aarch64 build still works fine (and ran through QEMU).
We still need to support host headers for native builds. That stays the
default (x86 is kind) meaning any cross-compilation setup must
configure BPF_SKIP_STD_INCLUDE to 1.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
The test suite uses GNU-only mktemp options that BusyBox mktemp does not
support:
mktemp: unrecognized option '--suffix=.xdptest'
Fix both mktemp callers (test_runner.sh & test-libxdp.sh) by avoiding
GNU-isms and using only busybox supported flags. This allows running
the test suite using busybox (somewhat, because it's not enough).
Validated on an aarch64 QEMU system built with Buildroot (glibc
toolchain, BusyBox userland). Without this patch every test run dies at
startup, before any test executes:
$ ./test_runner.sh tests/xdp-monitor/test-xdp-monitor.sh
mktemp: unrecognized option '--suffix=.xdptest'
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
BusyBox ps does not support the --ppid and -p options:
ps: unrecognized option '--ppid'
Because of the '|| return 0', the error meant kill_process_group()
returned immediately: the wait loop was skipped and the SIGTERM
escalation never happened. This could make stop_background() read the
output file before the process was done writing it.
Fix by probing the process group with 'kill -0', which is a shell
builtin and therefore always available.
Validated on an aarch64 QEMU system built with Buildroot (glibc
toolchain, BusyBox userland). Run the test_ports_allow xdp-filter test
in verbose mode (V=1) so that test stderr is visible.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
BusyBox sort does not support the -C (silent check) flag, while it does
support -c and -V. On failure, version_greater_or_equal() always
returned false, silently skipping the tshark attribute checks on
versions >= 3.6.7.
Use -c instead; the exit status is the same. Unlike -C, -c prints a
diagnostic on stderr when the input is out of order, so also redirect
stderr to keep the original silence.
Validated on an aarch64 QEMU system built with Buildroot (glibc
toolchain, BusyBox userland):
$ printf '3.6.7\n4.4.17\n' | sort -V -c; echo $?
0
$ printf '3.6.7\n4.4.17\n' | sort -V -C; echo $?
sort: invalid option -- 'C'
2
With -C, version_greater_or_equal() always fails and the tshark >= 3.6.7
attribute checks in test_capt_pcapng are silently skipped, making the
test pass trivially.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
BusyBox ping only accepts integer values for the -W option:
ping: invalid number '0.1'
All but one calls that are affected are doing `ping -W 0.1 -c1` to check
for successful packet ping-pong. Those are easy to fix, by using
`timeout 0.1 ping -c1`. Technically this doesn't time the same thing,
in practice it is fine.
One call is different however:
# test_multi_pkt()
timeout 40 $PING6 -q -W 0.1 -s "$PKT_SIZE" -c 20000 -f "$INSIDE_IP6" || return 1
For this one we replace the `-W 0.1` by the more basic `-W 1`. To make
sure the basic failure case (zero packet pass) still triggers quickly,
we do a pre-ping with a single packet.
Validated on an aarch64 QEMU system built with Buildroot (glibc
toolchain, BusyBox userland). Without this patch:
$ ./test_runner.sh tests/xdp-filter/test-xdp-filter.sh test_ipv6_allow
Checking command 'ping6 -W 0.1 -c 1 fc42:dead:cafe:2::1' filter 'dst fc42:dead:cafe:2::1'
ping6: invalid number '0.1'
[test_ipv6_allow] FAIL
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Two incompatibilities with BusyBox ping/ping6:
- BusyBox numbers ICMP echo sequences starting at 0, while iputils
starts at 1. Relax the capture regexes to any sequence number.
- BusyBox has no flood mode (-f). Use adaptive mode (-A) instead,
which pings as soon as a reply is received. On the fast veth test
interfaces this is an equivalent traffic generator.
Validated on an aarch64 QEMU system built with Buildroot (glibc
toolchain, BusyBox userland). Without this patch:
[test_capt_pcap] FAIL
ERROR: IPv6 packet not received (busybox seq starts at 0)
[test_perf_wakeup] FAIL
ping6: invalid option -- 'f' (no flood mode)
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
5431d35 to
66e503e
Compare
tohojo
left a comment
There was a problem hiding this comment.
Hmm, so the switch to using timeout 0.1 seems to make the CI flakier than before. AFAICT, the Github runner is simply so slow that timeout elapses before a reply comes back (for the tests that are supposed to succeed).
I guess we can try with a slightly higher timeout? Say 0.2 or 0.3...
Patch 1: BPF objects target the arch-independent BPF ISA but get access to the arch-specific target sysroot in BPF_CFLAGS. This causes trouble: on MIPS it's a build error. Fix by patching two arch headers with dummies that are arch-agnostic.
Patches 2 to 6 is about running the test suite on a BusyBox system.
This has been tested using Buildroot and two targets: a Mobileye EyeQ5 and a QEMU arm64 run. The second one is mostly because BPF support under MIPS is not 100%, meaning many tests get skipped.
See here for the Buildroot series. V1 was before the QEMU testing which explains why there are less BusyBox fixes there. I'll post V2 just afterwards. In particular I want to point out this patch that is required if you want to build xdp-tools for MIPS64 platforms. As it is a toolchain bug workaround, I'm not sure it deserves to be merged in xdp-tools? Opinions?
Thanks!