Hi Alexei,
Thank you for your reply!
On 24/08/2024 00:28, Alexei Starovoitov wrote:
On Sat, Aug 17, 2024 at 7:51 AM Matthieu Baerts matttbe@kernel.org wrote:
Hi Alexei,
Thank you for the review.
On 17/08/2024 09:22, Alexei Starovoitov wrote:
On Fri, Aug 16, 2024 at 7:56 PM Matthieu Baerts (NGI0) matttbe@kernel.org wrote:
Instead of duplicating UAPI header files in 'tools/include/uapi', the BPF selftests can also look at the header files inside the kernel source.
To do that, the kernel selftests infrastructure provides the 'KHDR_INCLUDES' variable. This is what is being used in most selftests, because it is what is recommended in the documentation [1]. If the selftests are not executed from the kernel sources, it is possible to override the variable, e.g.
make KHDR_INCLUDES="-I${HDR_DIR}/include" -C "${KSFT_DIR}"
... where ${HDR_DIR} has been generated by this command:
make headers_install INSTALL_HDR_PATH="${HDR_DIR}"
Thanks to 'KHDR_INCLUDES', it is no longer needed to duplicate header files for userspace test programs, and these programs can include UAPI header files without the 'uapi' prefix.
Note that it is still required to use 'tools/include/uapi' -- APIDIR, which corresponds to TOOLS_INCLUDES from lib.mk -- for the BPF programs, not to conflict with what is already defined in vmlinux.h.
Link: https://docs.kernel.org/dev-tools/kselftest.html#contributing-new-tests-deta... [1] Signed-off-by: Matthieu Baerts (NGI0) matttbe@kernel.org
tools/testing/selftests/bpf/Makefile | 2 +- tools/testing/selftests/bpf/prog_tests/assign_reuse.c | 2 +- tools/testing/selftests/bpf/prog_tests/tc_links.c | 4 ++-- tools/testing/selftests/bpf/prog_tests/tc_netkit.c | 2 +- tools/testing/selftests/bpf/prog_tests/tc_opts.c | 2 +- tools/testing/selftests/bpf/prog_tests/user_ringbuf.c | 2 +- tools/testing/selftests/bpf/prog_tests/xdp_bonding.c | 2 +- tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c | 2 +- tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c | 2 +- tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c | 2 +- tools/testing/selftests/bpf/prog_tests/xdp_link.c | 2 +- tools/testing/selftests/bpf/xdp_features.c | 4 ++-- 12 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 4eceb491a8ae..6a7aeae7e206 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -37,7 +37,7 @@ CFLAGS += -g $(OPT_FLAGS) -rdynamic \ -Wall -Werror -fno-omit-frame-pointer \ $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS) \ -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR) \
-I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)
-I$(TOOLSINCDIR) $(KHDR_INCLUDES) -I$(OUTPUT)LDFLAGS += $(SAN_LDFLAGS) LDLIBS += $(LIBELF_LIBS) -lz -lrt -lpthread
diff --git a/tools/testing/selftests/bpf/prog_tests/assign_reuse.c b/tools/testing/selftests/bpf/prog_tests/assign_reuse.c index 989ee4d9785b..3d06bf5a1ba4 100644 --- a/tools/testing/selftests/bpf/prog_tests/assign_reuse.c +++ b/tools/testing/selftests/bpf/prog_tests/assign_reuse.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2023 Isovalent */ -#include <uapi/linux/if_link.h> +#include <linux/if_link.h>
No. This is not an option. User space shouldn't include kernel headers like this. Long ago tools/include directory was specifically created to break such dependency. Back then it was done for perf.
I'm sorry, but I think we are not talking about the same thing here: here, I'm only modifying the "normal" userspace programs, not the ones used to generate the BPF objects. Perf is a special case I suppose, it needs to know the kernel internals. It is the same with BPF programs requiring vmlinux.h. But I think "normal" userspace programs in the sefltests can use the UAPI headers, no?
Not really. perf is a normal user space that doesn't look into kernel internals. It's used to rely on a few .h from kernel src tree for convenience, since they're not present in what's installed after 'make headers'. Hence the tools/include dir was created.
Using KHDR_INCLUDES is fine, but it's not ok to search replace s/uapi/linux/linux/ everywhere. Like the example I quoted above. tools/.../if_link.h is much older than include/uapi/linux/if_link.h and it's ok. We're not planning to update it. It's like building selftests on the system with older glibc. There is no requirement to have every .h in the tools/ dir up-to-date with the latest in include/.
OK, sorry, I didn't know it was fine not to have them up-to-date.
KSelftests doc indicates that it is important to use the headers from 'make headers' to be able to find regressions in these header files. But thanks to your comment below, I now understand bpf selftests are not really kselftests.
We're doing it for bpf.h because new selftests typically need something from bpf.h that was just added in the previous patch.
OK. Then, if I understand correctly, this doesn't apply to if_xdp.h, and we can remove the warning mentioning this file is out-of-date, but not remove the duplicated header file. This file in tools/include/uapi is a snapshot of an old version on purpose, no need to warn people it is not up-to-date then.
I understand that I could indeed fix my initial problem by duplicating mptcp.h in tools/include/uapi/linux/, but this doesn't look to be allowed any more by the Netdev maintainers, e.g. recently, 'ethtool.h' has been duplicated there in commit 7effe3fdc049 ("tools: Add ethtool.h header to tooling infra"), but removed quickly after in commit bbe91a9f6889 ("tools: remove redundant ethtool.h from tooling infra"). In this case, it was fine to simply drop it, because the linked test doesn't require a recent version. Jakub mentioned [4] that these duplicated headers should be avoided, and the ones generated by 'make headers' should be used instead: what is being suggested here.
This is a different issue. There are very few .h in tools/ that needs a sync. bpf.h is one of them. ethtool.h is certainly not.
you need something for mpctp.h. Let's talk about it, but switching everything to KHDR_INCLUDES is not ok, since there are a bunch of things in play. Sometimes selftests are built standalone and with non-glibc-s.
I didn't know there would be issues to use the latest version of the UAPI headers. It was fine on my side, but indeed, I didn't check all the possible combinations. Without [1], I cannot ask the BPF CI to check. Fine not to switch everything to KHDR_INCLUDES then.
Now that the CI runners have been updated to use Ubuntu 24.04 [1], we can use mptcp.h from the system headers, or do some actions via IPRoute2. So not having KHDR_INCLUDES is no longer blocking us for the moment. I think it might still be useful to add it for future use, and also to use the latest version of the UAPI headers that are not in 'tools/include/uapi', but I don't want to insist if you prefer not to use the latest version.
[1] https://github.com/libbpf/ci/pull/131 [2] https://lore.kernel.org/bpf/364C4C5B-27A0-4210-84E2-8CA9867E4127@meta.com/
Also realize that bpf selftests are not really kselftests. We use a few common .mk for convenience. That's about it.
OK, sorry, I didn't know about that! That explains why they cannot be used like the other ones.
Cheers, Matt