If a BPF selftest program requires (recent) UAPI headers [1], it is currently needed to duplicate these header files into tools/include/uapi. That's not a good solution, because it is a duplication that needs to be kept up-to-date, while the required files are only a few directories away.
A solution to avoid these duplicated files is to use the KHDR_INCLUDES from the kselftest infrastructure. That is what is being done in the first patch.
The second patch removes 'if_xdp.h', which is no longer needed, and was causing a warning when building the libbpf required by the BPF selftests. There could be more duplicated UAPI header files that could be removed, but I didn't spend too much time checking which ones are not used by anything else from the 'tools' directory.
Hopefully, these modifications should not cause any issues on the different CIs, because it is using the recommended method for the kernel selftests. If this causes issues on the CIs side, it should be easy to fix by overriding the KHDR_INCLUDES variable, and it might be better to do that, because it likely means the CI is not following the recommended way to execute the kernel selftests. See patch 1/2 for more details about that.
Link: https://lore.kernel.org/all/08f925cd-e267-4a6b-84b1-792515c4e199@kernel.org/... [1] Signed-off-by: Matthieu Baerts (NGI0) matttbe@kernel.org --- Matthieu Baerts (NGI0) (2): selftests: bpf: use KHDR_INCLUDES for the UAPI headers selftests: bpf: remove duplicated UAPI if_xdp headers
tools/include/uapi/linux/if_xdp.h | 173 --------------------- tools/lib/bpf/Makefile | 3 - tools/testing/selftests/bpf/Makefile | 2 +- .../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 +- .../selftests/bpf/prog_tests/user_ringbuf.c | 2 +- .../testing/selftests/bpf/prog_tests/xdp_bonding.c | 2 +- .../selftests/bpf/prog_tests/xdp_cpumap_attach.c | 2 +- .../selftests/bpf/prog_tests/xdp_devmap_attach.c | 2 +- .../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 +- 14 files changed, 14 insertions(+), 190 deletions(-) --- base-commit: fdf1c728fac541891ef1aa773bfd42728626769c change-id: 20240816-ups-bpf-next-selftests-use-khdr-28f935c8848a
Best regards,
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> #include <test_progs.h>
#include <netinet/tcp.h> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_links.c b/tools/testing/selftests/bpf/prog_tests/tc_links.c index 1af9ec1149aa..532e162185c3 100644 --- a/tools/testing/selftests/bpf/prog_tests/tc_links.c +++ b/tools/testing/selftests/bpf/prog_tests/tc_links.c @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2023 Isovalent */ -#include <uapi/linux/if_link.h> -#include <uapi/linux/pkt_sched.h> +#include <linux/if_link.h> +#include <linux/pkt_sched.h> #include <net/if.h> #include <test_progs.h>
diff --git a/tools/testing/selftests/bpf/prog_tests/tc_netkit.c b/tools/testing/selftests/bpf/prog_tests/tc_netkit.c index b9135720024c..1c8b8f03e873 100644 --- a/tools/testing/selftests/bpf/prog_tests/tc_netkit.c +++ b/tools/testing/selftests/bpf/prog_tests/tc_netkit.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> #include <net/if.h> #include <test_progs.h>
diff --git a/tools/testing/selftests/bpf/prog_tests/tc_opts.c b/tools/testing/selftests/bpf/prog_tests/tc_opts.c index 196abf223465..14aee536c519 100644 --- a/tools/testing/selftests/bpf/prog_tests/tc_opts.c +++ b/tools/testing/selftests/bpf/prog_tests/tc_opts.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> #include <net/if.h> #include <test_progs.h>
diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c index dfff6feac12c..a37dda1b3cd2 100644 --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c @@ -12,7 +12,7 @@ #include <sys/syscall.h> #include <sys/sysinfo.h> #include <test_progs.h> -#include <uapi/linux/bpf.h> +#include <linux/bpf.h> #include <unistd.h>
#include "user_ringbuf_fail.skel.h" diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c index 6d8b54124cb3..ee9bd8606d14 100644 --- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c @@ -18,7 +18,7 @@ #include <linux/if_bonding.h> #include <linux/limits.h> #include <linux/udp.h> -#include <uapi/linux/netdev.h> +#include <linux/netdev.h>
#include "xdp_dummy.skel.h" #include "xdp_redirect_multi_kern.skel.h" diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c index 481626a875d1..e63a193e29e8 100644 --- a/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c +++ b/tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 -#include <uapi/linux/bpf.h> +#include <linux/bpf.h> #include <linux/if_link.h> #include <test_progs.h>
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c index ce6812558287..b916fa945b15 100644 --- a/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c +++ b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 -#include <uapi/linux/bpf.h> +#include <linux/bpf.h> #include <linux/if_link.h> #include <test_progs.h>
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c index bad0ea167be7..c40a25837233 100644 --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c @@ -9,7 +9,7 @@ #include <linux/in6.h> #include <linux/udp.h> #include <bpf/bpf_endian.h> -#include <uapi/linux/netdev.h> +#include <linux/netdev.h> #include "test_xdp_do_redirect.skel.h"
struct udp_packet { diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_link.c b/tools/testing/selftests/bpf/prog_tests/xdp_link.c index e7e9f3c22edf..ab92c395a7c7 100644 --- a/tools/testing/selftests/bpf/prog_tests/xdp_link.c +++ b/tools/testing/selftests/bpf/prog_tests/xdp_link.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2020 Facebook */ -#include <uapi/linux/if_link.h> +#include <linux/if_link.h> #include <test_progs.h> #include "test_xdp_link.skel.h"
diff --git a/tools/testing/selftests/bpf/xdp_features.c b/tools/testing/selftests/bpf/xdp_features.c index 595c79141cf3..99b54674d94e 100644 --- a/tools/testing/selftests/bpf/xdp_features.c +++ b/tools/testing/selftests/bpf/xdp_features.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 -#include <uapi/linux/bpf.h> -#include <uapi/linux/netdev.h> +#include <linux/bpf.h> +#include <linux/netdev.h> #include <linux/if_link.h> #include <signal.h> #include <argp.h>
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.
pw-bot: cr
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?
Here, the BPF userspace programs will take the kernel headers generated by 'make headers', not directly from 'include/uapi' in the source. That's what is done in other kernel selftests, and what is documented in the kselftests doc [1]. With this modification, I'm able to compile the BPF selftests.
I see that the BPF CI didn't manage to build them, but I think that's because it sets 'KBUILD_OUTPUT', and build the BPF selftests directly, not via the 'tools/testing/selftests/Makefile', which is the recommended way, which supports 'KBUILD_OUTPUT'. Please see [2] for a fix for the BPF CI.
A bit of context: my goal here is to be able to add new BPF selftests to cover cases where BPF is used to modify MPTCP sockets and subflows. To setup these tests, we need to communicate with the kernel, either by using IPRoute2 -- but the BPF CI is still using IPRoute2 5.5 from pre-COVID time -- or with a dedicated userspace program. This program [2] has nothing special, it needs to include <linux/mptcp.h> to get the value of some defines and enums to be able to communicate with the kernel via Netlink. If I want to compile this program on my system, outside the kselftest, the compiler will use the file from '/usr/include/linux/mptcp.h', coming from the kernel UAPI. When validating new features in the selftests, it will use the version from the kernel source, the result of a 'make headers'. That works fine, we don't have to duplicate the MPTCP UAPI header file when building the MPTCP selftests. Same with the other selftests. Without this patch, I would need to duplicate the 'mptcp.h' header if I want to use the same program in the BPF selftests, because KHDR_INCLUDES is not used in the CFLAGS when compiling the userspace programs like everywhere else in the selftests.
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.
Do you think it would not work? If not, any idea what I can do? Duplicating the defines and enums I need in the .c file doesn't seem like a nice solution either.
[1] https://docs.kernel.org/dev-tools/kselftest.html [2] https://github.com/libbpf/ci/pull/131 [3] https://elixir.bootlin.com/linux/v6.10.5/source/tools/testing/selftests/net/... [4] https://lore.kernel.org/netdev/20240429132207.58ecf430@kernel.org/T/#u
Cheers, Matt
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/. We're doing it for bpf.h because new selftests typically need something from bpf.h that was just added in the previous patch.
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.
Also realize that bpf selftests are not really kselftests. We use a few common .mk for convenience. That's about it.
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
On Sat, Aug 24, 2024 at 3:37 AM Matthieu Baerts matttbe@kernel.org wrote:
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.
Let's not fix what is not broken. We'll add KHDR_INCLUDES when it's actually necessary.
Thanks to the previous commit, this file is no longer needed, because the BPF selftests will take the UAPI headers from the kernel source.
Note that this file was an old copy of the UAPI one, which was causing a warning at compilation time:
Warning: Kernel ABI header at 'tools/include/uapi/linux/if_xdp.h' differs from latest version at 'include/uapi/linux/if_xdp.h'
Signed-off-by: Matthieu Baerts (NGI0) matttbe@kernel.org --- tools/include/uapi/linux/if_xdp.h | 173 -------------------------------------- tools/lib/bpf/Makefile | 3 - 2 files changed, 176 deletions(-)
diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h deleted file mode 100644 index 2f082b01ff22..000000000000 --- a/tools/include/uapi/linux/if_xdp.h +++ /dev/null @@ -1,173 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ -/* - * if_xdp: XDP socket user-space interface - * Copyright(c) 2018 Intel Corporation. - * - * Author(s): Björn Töpel bjorn.topel@intel.com - * Magnus Karlsson magnus.karlsson@intel.com - */ - -#ifndef _LINUX_IF_XDP_H -#define _LINUX_IF_XDP_H - -#include <linux/types.h> - -/* Options for the sxdp_flags field */ -#define XDP_SHARED_UMEM (1 << 0) -#define XDP_COPY (1 << 1) /* Force copy-mode */ -#define XDP_ZEROCOPY (1 << 2) /* Force zero-copy mode */ -/* If this option is set, the driver might go sleep and in that case - * the XDP_RING_NEED_WAKEUP flag in the fill and/or Tx rings will be - * set. If it is set, the application need to explicitly wake up the - * driver with a poll() (Rx and Tx) or sendto() (Tx only). If you are - * running the driver and the application on the same core, you should - * use this option so that the kernel will yield to the user space - * application. - */ -#define XDP_USE_NEED_WAKEUP (1 << 3) -/* By setting this option, userspace application indicates that it can - * handle multiple descriptors per packet thus enabling AF_XDP to split - * multi-buffer XDP frames into multiple Rx descriptors. Without this set - * such frames will be dropped. - */ -#define XDP_USE_SG (1 << 4) - -/* Flags for xsk_umem_config flags */ -#define XDP_UMEM_UNALIGNED_CHUNK_FLAG (1 << 0) - -/* Force checksum calculation in software. Can be used for testing or - * working around potential HW issues. This option causes performance - * degradation and only works in XDP_COPY mode. - */ -#define XDP_UMEM_TX_SW_CSUM (1 << 1) - -/* Request to reserve tx_metadata_len bytes of per-chunk metadata. - */ -#define XDP_UMEM_TX_METADATA_LEN (1 << 2) - -struct sockaddr_xdp { - __u16 sxdp_family; - __u16 sxdp_flags; - __u32 sxdp_ifindex; - __u32 sxdp_queue_id; - __u32 sxdp_shared_umem_fd; -}; - -/* XDP_RING flags */ -#define XDP_RING_NEED_WAKEUP (1 << 0) - -struct xdp_ring_offset { - __u64 producer; - __u64 consumer; - __u64 desc; - __u64 flags; -}; - -struct xdp_mmap_offsets { - struct xdp_ring_offset rx; - struct xdp_ring_offset tx; - struct xdp_ring_offset fr; /* Fill */ - struct xdp_ring_offset cr; /* Completion */ -}; - -/* XDP socket options */ -#define XDP_MMAP_OFFSETS 1 -#define XDP_RX_RING 2 -#define XDP_TX_RING 3 -#define XDP_UMEM_REG 4 -#define XDP_UMEM_FILL_RING 5 -#define XDP_UMEM_COMPLETION_RING 6 -#define XDP_STATISTICS 7 -#define XDP_OPTIONS 8 - -struct xdp_umem_reg { - __u64 addr; /* Start of packet data area */ - __u64 len; /* Length of packet data area */ - __u32 chunk_size; - __u32 headroom; - __u32 flags; - __u32 tx_metadata_len; -}; - -struct xdp_statistics { - __u64 rx_dropped; /* Dropped for other reasons */ - __u64 rx_invalid_descs; /* Dropped due to invalid descriptor */ - __u64 tx_invalid_descs; /* Dropped due to invalid descriptor */ - __u64 rx_ring_full; /* Dropped due to rx ring being full */ - __u64 rx_fill_ring_empty_descs; /* Failed to retrieve item from fill ring */ - __u64 tx_ring_empty_descs; /* Failed to retrieve item from tx ring */ -}; - -struct xdp_options { - __u32 flags; -}; - -/* Flags for the flags field of struct xdp_options */ -#define XDP_OPTIONS_ZEROCOPY (1 << 0) - -/* Pgoff for mmaping the rings */ -#define XDP_PGOFF_RX_RING 0 -#define XDP_PGOFF_TX_RING 0x80000000 -#define XDP_UMEM_PGOFF_FILL_RING 0x100000000ULL -#define XDP_UMEM_PGOFF_COMPLETION_RING 0x180000000ULL - -/* Masks for unaligned chunks mode */ -#define XSK_UNALIGNED_BUF_OFFSET_SHIFT 48 -#define XSK_UNALIGNED_BUF_ADDR_MASK \ - ((1ULL << XSK_UNALIGNED_BUF_OFFSET_SHIFT) - 1) - -/* Request transmit timestamp. Upon completion, put it into tx_timestamp - * field of union xsk_tx_metadata. - */ -#define XDP_TXMD_FLAGS_TIMESTAMP (1 << 0) - -/* Request transmit checksum offload. Checksum start position and offset - * are communicated via csum_start and csum_offset fields of union - * xsk_tx_metadata. - */ -#define XDP_TXMD_FLAGS_CHECKSUM (1 << 1) - -/* AF_XDP offloads request. 'request' union member is consumed by the driver - * when the packet is being transmitted. 'completion' union member is - * filled by the driver when the transmit completion arrives. - */ -struct xsk_tx_metadata { - __u64 flags; - - union { - struct { - /* XDP_TXMD_FLAGS_CHECKSUM */ - - /* Offset from desc->addr where checksumming should start. */ - __u16 csum_start; - /* Offset from csum_start where checksum should be stored. */ - __u16 csum_offset; - } request; - - struct { - /* XDP_TXMD_FLAGS_TIMESTAMP */ - __u64 tx_timestamp; - } completion; - }; -}; - -/* Rx/Tx descriptor */ -struct xdp_desc { - __u64 addr; - __u32 len; - __u32 options; -}; - -/* UMEM descriptor is __u64 */ - -/* Flag indicating that the packet continues with the buffer pointed out by the - * next frame in the ring. The end of the packet is signalled by setting this - * bit to zero. For single buffer packets, every descriptor has 'options' set - * to 0 and this maintains backward compatibility. - */ -#define XDP_PKT_CONTD (1 << 0) - -/* TX packet carries valid metadata. */ -#define XDP_TX_METADATA (1 << 1) - -#endif /* _LINUX_IF_XDP_H */ diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile index 2cf892774346..977dba26ba87 100644 --- a/tools/lib/bpf/Makefile +++ b/tools/lib/bpf/Makefile @@ -148,9 +148,6 @@ $(BPF_IN_SHARED): force $(BPF_GENERATED) @(test -f ../../include/uapi/linux/bpf_common.h -a -f ../../../include/uapi/linux/bpf_common.h && ( \ (diff -B ../../include/uapi/linux/bpf_common.h ../../../include/uapi/linux/bpf_common.h >/dev/null) || \ echo "Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf_common.h' differs from latest version at 'include/uapi/linux/bpf_common.h'" >&2 )) || true - @(test -f ../../include/uapi/linux/if_xdp.h -a -f ../../../include/uapi/linux/if_xdp.h && ( \ - (diff -B ../../include/uapi/linux/if_xdp.h ../../../include/uapi/linux/if_xdp.h >/dev/null) || \ - echo "Warning: Kernel ABI header at 'tools/include/uapi/linux/if_xdp.h' differs from latest version at 'include/uapi/linux/if_xdp.h'" >&2 )) || true $(Q)$(MAKE) $(build)=libbpf OUTPUT=$(SHARED_OBJDIR) CFLAGS="$(CFLAGS) $(SHLIB_FLAGS)"
$(BPF_IN_STATIC): force $(BPF_GENERATED)
linux-kselftest-mirror@lists.linaro.org
-
Alexei Starovoitov -
Matthieu Baerts -
Matthieu Baerts (NGI0)