| Message ID | 20240816-ups-bpf-next-selftests-use-khdr-v1-1-1e19f3d5b17a@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | selftests: bpf: avoid duplicated UAPI headers | expand |
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-details [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-details [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/mptcp/pm_nl_ctl.c [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-details [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-details [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.
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>
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-details [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(-)