| Message ID | 20240703-upstream-bpf-next-20240506-mptcp-subflow-test-v3-2-ebdc2d494049@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/bpf: new MPTCP subflow subtest | expand |
Hello, @BPF people: this new tool doesn't compile on the BPF CI [1]. Can I have a hand to find the best way to fix this please? (see below) [1] https://github.com/kernel-patches/bpf/pull/7296 On 03/07/2024 20:57, Matthieu Baerts (NGI0) wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > This patch adds a symlink to MPTCP's pm_nl_ctl tool into bpf selftests, > and updates Makefile to compile it. > > This is useful to run MPTCP BPF selftests on systems with an old version > of IPRoute2. This tool can be used as an alternative to 'ip mptcp'. (...) > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index e0b3887b3d2d..204269d0b5b8 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \ > flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \ > test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \ > xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \ > - xdp_features bpf_test_no_cfi.ko > + xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl On the BPF CI, we have such errors: mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file not found 20 | #include "linux/mptcp.h" | ^~~~~~~~~~~~~~~ On my side, I don't have any issue, because the compiler uses the mptcp.h file from the system: /usr/include/linux/mptcp.h I suppose that's not OK on the BPF CI, as it looks like it doesn't have this file there, probably because it still uses Ubuntu 20.04 as base, which doesn't include this file in the linux-libc-dev package. When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the other programs from that list -- is compiled (V=1), I see that the following "-I" options are given: -I${PWD}/tools/testing/selftests/bpf -I${BUILD}//tools/include -I${BUILD}/include/generated -I${PWD}/tools/lib -I${PWD}/tools/include -I${PWD}/tools/include/uapi -I${BUILD}/ It will then not look at -I${PWD}/usr/include or the directory generated with: make headers_install INSTALL_HDR_PATH=(...) I guess that's why people have duplicated files in 'tools/include/uapi', but I also understood from Jakub that it is not a good idea to continue to do so. What would be the best solution to avoid a copy? A symlink still looks like a workaround. In the other selftests, KHDR_INCLUDES is used to be able to include the path containing the UAPI headers. So if someone built the headers in a seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES can be overridden to look there, instead of ${KERNEL_SRC}/usr/include. Would it be OK to do that? Would it work for the CI without extra changes? Or do you still prefer a copy/symlink to 'tools/include/uapi' instead? Cheers, Matt
On 7/4/24 3:48 AM, Matthieu Baerts wrote: >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile >> index e0b3887b3d2d..204269d0b5b8 100644 >> --- a/tools/testing/selftests/bpf/Makefile >> +++ b/tools/testing/selftests/bpf/Makefile >> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \ >> flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \ >> test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \ >> xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \ >> - xdp_features bpf_test_no_cfi.ko >> + xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl > On the BPF CI, we have such errors: > > mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file not found > 20 | #include "linux/mptcp.h" > | ^~~~~~~~~~~~~~~ > > On my side, I don't have any issue, because the compiler uses the > mptcp.h file from the system: /usr/include/linux/mptcp.h > > I suppose that's not OK on the BPF CI, as it looks like it doesn't have > this file there, probably because it still uses Ubuntu 20.04 as base, > which doesn't include this file in the linux-libc-dev package. > > When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the other > programs from that list -- is compiled (V=1), I see that the following > "-I" options are given: > > -I${PWD}/tools/testing/selftests/bpf > -I${BUILD}//tools/include > -I${BUILD}/include/generated > -I${PWD}/tools/lib > -I${PWD}/tools/include > -I${PWD}/tools/include/uapi > -I${BUILD}/ > > It will then not look at -I${PWD}/usr/include or the directory generated > with: > > make headers_install INSTALL_HDR_PATH=(...) It sounds like the tools/testing/selftests/net/mptcp/Makefile is looking at this include path, so it works? iiu the bpf/Makefile correctly, it has the bpftool "make" compiled and installed at tools/testing/selftests/bpf/tools/sbin/. May be directly compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"? > > I guess that's why people have duplicated files in 'tools/include/uapi', > but I also understood from Jakub that it is not a good idea to continue > to do so. > > What would be the best solution to avoid a copy? A symlink still looks > like a workaround. > > In the other selftests, KHDR_INCLUDES is used to be able to include the > path containing the UAPI headers. So if someone built the headers in a Meaning KHDR_INCLUDES should be used and -I${PWD}/tools/include/uapi can be retired? I haven't looked into the details. I quickly tried but it fails in my environment. > seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES can be > overridden to look there, instead of ${KERNEL_SRC}/usr/include. Would it > be OK to do that? Would it work for the CI without extra changes? Or do > you still prefer a copy/symlink to 'tools/include/uapi' instead? > > Cheers, > Matt
Hi Martin, Thank you for your reply! On 06/07/2024 01:10, Martin KaFai Lau wrote: > On 7/4/24 3:48 AM, Matthieu Baerts wrote: >>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/ >>> selftests/bpf/Makefile >>> index e0b3887b3d2d..204269d0b5b8 100644 >>> --- a/tools/testing/selftests/bpf/Makefile >>> +++ b/tools/testing/selftests/bpf/Makefile >>> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \ >>> flow_dissector_load test_flow_dissector >>> test_tcp_check_syncookie_user \ >>> test_lirc_mode2_user xdping test_cpp runqslower bench >>> bpf_testmod.ko \ >>> xskxceiver xdp_redirect_multi xdp_synproxy veristat >>> xdp_hw_metadata \ >>> - xdp_features bpf_test_no_cfi.ko >>> + xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl >> On the BPF CI, we have such errors: >> >> mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file not found >> 20 | #include "linux/mptcp.h" >> | ^~~~~~~~~~~~~~~ >> >> On my side, I don't have any issue, because the compiler uses the >> mptcp.h file from the system: /usr/include/linux/mptcp.h >> >> I suppose that's not OK on the BPF CI, as it looks like it doesn't have >> this file there, probably because it still uses Ubuntu 20.04 as base, >> which doesn't include this file in the linux-libc-dev package. >> >> When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the other >> programs from that list -- is compiled (V=1), I see that the following >> "-I" options are given: >> >> -I${PWD}/tools/testing/selftests/bpf >> -I${BUILD}//tools/include >> -I${BUILD}/include/generated >> -I${PWD}/tools/lib >> -I${PWD}/tools/include >> -I${PWD}/tools/include/uapi >> -I${BUILD}/ >> >> It will then not look at -I${PWD}/usr/include or the directory generated >> with: >> >> make headers_install INSTALL_HDR_PATH=(...) > > It sounds like the tools/testing/selftests/net/mptcp/Makefile is looking > at this include path, so it works? Yes it does work. > iiu the bpf/Makefile correctly, it has the bpftool "make" compiled and > installed at tools/testing/selftests/bpf/tools/sbin/. May be directly > compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"? That could be an alternative, I didn't know it would be OK to add such dependence, good idea. >> I guess that's why people have duplicated files in 'tools/include/uapi', >> but I also understood from Jakub that it is not a good idea to continue >> to do so. >> >> What would be the best solution to avoid a copy? A symlink still looks >> like a workaround. >> >> In the other selftests, KHDR_INCLUDES is used to be able to include the >> path containing the UAPI headers. So if someone built the headers in a > > Meaning KHDR_INCLUDES should be used and -I${PWD}/tools/include/uapi can > be retired? That's the idea, yes, for "userspace programs". I mean: for BPF programs requiring vmlinux.h (BPF_CFLAGS), I guess you will still need the bpf.h file from tools/include/uapi, no? > I haven't looked into the details. I quickly tried but it > fails in my environment. Do you not have issues because some files have something like: #include <uapi/linux/(...).h> On my side, I had a working version using this patch: > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index 7c5827d20c2e..112f14d40852 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 > But only after having removed these extra 'uapi/': $ git grep -l '<uapi/' -- tools/testing/selftests/bpf | \ xargs sed -i 's|#include <uapi/|#include <|g' Is it not OK for you like that? Note that I built the selftests using KHDR_INCLUDES=-I$INSTALL_HDR_PATH. >> seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES can be >> overridden to look there, instead of ${KERNEL_SRC}/usr/include. Would it >> be OK to do that? Would it work for the CI without extra changes? Or do >> you still prefer a copy/symlink to 'tools/include/uapi' instead? Cheers, Matt
Hi Matt, On Sat, 2024-07-06 at 02:25 +0200, Matthieu Baerts wrote: > Hi Martin, > > Thank you for your reply! > > On 06/07/2024 01:10, Martin KaFai Lau wrote: > > On 7/4/24 3:48 AM, Matthieu Baerts wrote: > > > > diff --git a/tools/testing/selftests/bpf/Makefile > > > > b/tools/testing/ > > > > selftests/bpf/Makefile > > > > index e0b3887b3d2d..204269d0b5b8 100644 > > > > --- a/tools/testing/selftests/bpf/Makefile > > > > +++ b/tools/testing/selftests/bpf/Makefile > > > > @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = > > > > test_skb_cgroup_id_user \ > > > > flow_dissector_load test_flow_dissector > > > > test_tcp_check_syncookie_user \ > > > > test_lirc_mode2_user xdping test_cpp runqslower bench > > > > bpf_testmod.ko \ > > > > xskxceiver xdp_redirect_multi xdp_synproxy veristat > > > > xdp_hw_metadata \ > > > > - xdp_features bpf_test_no_cfi.ko > > > > + xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl > > > On the BPF CI, we have such errors: > > > > > > mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file > > > not found > > > 20 | #include "linux/mptcp.h" > > > | ^~~~~~~~~~~~~~~ > > > > > > On my side, I don't have any issue, because the compiler uses the > > > mptcp.h file from the system: /usr/include/linux/mptcp.h > > > > > > I suppose that's not OK on the BPF CI, as it looks like it > > > doesn't have > > > this file there, probably because it still uses Ubuntu 20.04 as > > > base, > > > which doesn't include this file in the linux-libc-dev package. > > > > > > When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the > > > other > > > programs from that list -- is compiled (V=1), I see that the > > > following > > > "-I" options are given: > > > > > > -I${PWD}/tools/testing/selftests/bpf > > > -I${BUILD}//tools/include > > > -I${BUILD}/include/generated > > > -I${PWD}/tools/lib > > > -I${PWD}/tools/include > > > -I${PWD}/tools/include/uapi > > > -I${BUILD}/ > > > > > > It will then not look at -I${PWD}/usr/include or the directory > > > generated > > > with: > > > > > > make headers_install INSTALL_HDR_PATH=(...) > > > > It sounds like the tools/testing/selftests/net/mptcp/Makefile is > > looking > > at this include path, so it works? > > Yes it does work. > > > iiu the bpf/Makefile correctly, it has the bpftool "make" compiled > > and > > installed at tools/testing/selftests/bpf/tools/sbin/. May be > > directly > > compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"? > > That could be an alternative, I didn't know it would be OK to add > such > dependence, good idea. > > > > I guess that's why people have duplicated files in > > > 'tools/include/uapi', > > > but I also understood from Jakub that it is not a good idea to > > > continue > > > to do so. > > > > > > What would be the best solution to avoid a copy? A symlink still > > > looks > > > like a workaround. > > > > > > In the other selftests, KHDR_INCLUDES is used to be able to > > > include the > > > path containing the UAPI headers. So if someone built the headers > > > in a > > > > Meaning KHDR_INCLUDES should be used and - > > I${PWD}/tools/include/uapi can > > be retired? > > That's the idea, yes, for "userspace programs". I mean: for BPF > programs > requiring vmlinux.h (BPF_CFLAGS), I guess you will still need the > bpf.h > file from tools/include/uapi, no? > > > I haven't looked into the details. I quickly tried but it > > fails in my environment. > > Do you not have issues because some files have something like: > > #include <uapi/linux/(...).h> > > On my side, I had a working version using this patch: > > > diff --git a/tools/testing/selftests/bpf/Makefile > > b/tools/testing/selftests/bpf/Makefile > > index 7c5827d20c2e..112f14d40852 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 > > > > But only after having removed these extra 'uapi/': > > $ git grep -l '<uapi/' -- tools/testing/selftests/bpf | \ > xargs sed -i 's|#include <uapi/|#include <|g' > > Is it not OK for you like that? > > Note that I built the selftests using KHDR_INCLUDES=- > I$INSTALL_HDR_PATH. Do you need me to do anything here? This patchset seems to have been waiting for several months. Another option is to roll back to v2, not add this mptcp_pm_nl_ctl tool, and continue to use "ip mptcp". I remember mentioning in the comments of v2 that BPF CI systems will also be upgraded to new Ubuntu system and iproute2 in the future. At this time now we can make a check for "ip mptcp" and only run this test on systems that support "ip mptcp", and skip the test with test__skip() for systems that do not support it, so that CI can also pass. WDYT? Thanks, -Geliang > > > > seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES > > > can be > > > overridden to look there, instead of ${KERNEL_SRC}/usr/include. > > > Would it > > > be OK to do that? Would it work for the CI without extra changes? > > > Or do > > > you still prefer a copy/symlink to 'tools/include/uapi' instead? > > Cheers, > Matt
Hi Geliang, Thank you for your reply! On 24/07/2024 09:42, Geliang Tang wrote: > Hi Matt, > > On Sat, 2024-07-06 at 02:25 +0200, Matthieu Baerts wrote: >> Hi Martin, >> >> Thank you for your reply! >> >> On 06/07/2024 01:10, Martin KaFai Lau wrote: >>> On 7/4/24 3:48 AM, Matthieu Baerts wrote: >>>>> diff --git a/tools/testing/selftests/bpf/Makefile >>>>> b/tools/testing/ >>>>> selftests/bpf/Makefile >>>>> index e0b3887b3d2d..204269d0b5b8 100644 >>>>> --- a/tools/testing/selftests/bpf/Makefile >>>>> +++ b/tools/testing/selftests/bpf/Makefile >>>>> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = >>>>> test_skb_cgroup_id_user \ >>>>> flow_dissector_load test_flow_dissector >>>>> test_tcp_check_syncookie_user \ >>>>> test_lirc_mode2_user xdping test_cpp runqslower bench >>>>> bpf_testmod.ko \ >>>>> xskxceiver xdp_redirect_multi xdp_synproxy veristat >>>>> xdp_hw_metadata \ >>>>> - xdp_features bpf_test_no_cfi.ko >>>>> + xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl >>>> On the BPF CI, we have such errors: >>>> >>>> mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file >>>> not found >>>> 20 | #include "linux/mptcp.h" >>>> | ^~~~~~~~~~~~~~~ >>>> >>>> On my side, I don't have any issue, because the compiler uses the >>>> mptcp.h file from the system: /usr/include/linux/mptcp.h >>>> >>>> I suppose that's not OK on the BPF CI, as it looks like it >>>> doesn't have >>>> this file there, probably because it still uses Ubuntu 20.04 as >>>> base, >>>> which doesn't include this file in the linux-libc-dev package. >>>> >>>> When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the >>>> other >>>> programs from that list -- is compiled (V=1), I see that the >>>> following >>>> "-I" options are given: >>>> >>>> -I${PWD}/tools/testing/selftests/bpf >>>> -I${BUILD}//tools/include >>>> -I${BUILD}/include/generated >>>> -I${PWD}/tools/lib >>>> -I${PWD}/tools/include >>>> -I${PWD}/tools/include/uapi >>>> -I${BUILD}/ >>>> >>>> It will then not look at -I${PWD}/usr/include or the directory >>>> generated >>>> with: >>>> >>>> make headers_install INSTALL_HDR_PATH=(...) >>> >>> It sounds like the tools/testing/selftests/net/mptcp/Makefile is >>> looking >>> at this include path, so it works? >> >> Yes it does work. >> >>> iiu the bpf/Makefile correctly, it has the bpftool "make" compiled >>> and >>> installed at tools/testing/selftests/bpf/tools/sbin/. May be >>> directly >>> compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"? >> >> That could be an alternative, I didn't know it would be OK to add >> such >> dependence, good idea. >> >>>> I guess that's why people have duplicated files in >>>> 'tools/include/uapi', >>>> but I also understood from Jakub that it is not a good idea to >>>> continue >>>> to do so. >>>> >>>> What would be the best solution to avoid a copy? A symlink still >>>> looks >>>> like a workaround. >>>> >>>> In the other selftests, KHDR_INCLUDES is used to be able to >>>> include the >>>> path containing the UAPI headers. So if someone built the headers >>>> in a >>> >>> Meaning KHDR_INCLUDES should be used and - >>> I${PWD}/tools/include/uapi can >>> be retired? >> >> That's the idea, yes, for "userspace programs". I mean: for BPF >> programs >> requiring vmlinux.h (BPF_CFLAGS), I guess you will still need the >> bpf.h >> file from tools/include/uapi, no? >> >>> I haven't looked into the details. I quickly tried but it >>> fails in my environment. >> >> Do you not have issues because some files have something like: >> >> #include <uapi/linux/(...).h> >> >> On my side, I had a working version using this patch: >> >>> diff --git a/tools/testing/selftests/bpf/Makefile >>> b/tools/testing/selftests/bpf/Makefile >>> index 7c5827d20c2e..112f14d40852 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 >>> >> >> But only after having removed these extra 'uapi/': >> >> $ git grep -l '<uapi/' -- tools/testing/selftests/bpf | \ >> xargs sed -i 's|#include <uapi/|#include <|g' >> >> Is it not OK for you like that? >> >> Note that I built the selftests using KHDR_INCLUDES=- >> I$INSTALL_HDR_PATH. > > Do you need me to do anything here? This patchset seems to have been > waiting for several months. Sorry, I was not sure my suggestion here above was OK for Martin and other BPF maintainers. Maybe sending a proper patch implementing these modifications would be easier? > Another option is to roll back to v2, not add this mptcp_pm_nl_ctl > tool, and continue to use "ip mptcp". I remember mentioning in the > comments of v2 that BPF CI systems will also be upgraded to new Ubuntu > system and iproute2 in the future. At this time now we can make a check > for "ip mptcp" and only run this test on systems that support "ip > mptcp", and skip the test with test__skip() for systems that do not > support it, so that CI can also pass. I think it would be good to get rid of some duplicated header files, but well, that's not directly linked to the new test we want to add, it is more something to ease the maintenance of these duplicated files. Then yes, it might be good to skip the test if "ip mptcp" is not supported. Cheers, Matt
On 7/24/24 1:24 AM, Matthieu Baerts wrote: > Hi Geliang, > > Thank you for your reply! > > On 24/07/2024 09:42, Geliang Tang wrote: >> Hi Matt, >> >> On Sat, 2024-07-06 at 02:25 +0200, Matthieu Baerts wrote: >>> Hi Martin, >>> >>> Thank you for your reply! >>> >>> On 06/07/2024 01:10, Martin KaFai Lau wrote: >>>> On 7/4/24 3:48 AM, Matthieu Baerts wrote: >>>>>> diff --git a/tools/testing/selftests/bpf/Makefile >>>>>> b/tools/testing/ >>>>>> selftests/bpf/Makefile >>>>>> index e0b3887b3d2d..204269d0b5b8 100644 >>>>>> --- a/tools/testing/selftests/bpf/Makefile >>>>>> +++ b/tools/testing/selftests/bpf/Makefile >>>>>> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = >>>>>> test_skb_cgroup_id_user \ >>>>>> flow_dissector_load test_flow_dissector >>>>>> test_tcp_check_syncookie_user \ >>>>>> test_lirc_mode2_user xdping test_cpp runqslower bench >>>>>> bpf_testmod.ko \ >>>>>> xskxceiver xdp_redirect_multi xdp_synproxy veristat >>>>>> xdp_hw_metadata \ >>>>>> - xdp_features bpf_test_no_cfi.ko >>>>>> + xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl >>>>> On the BPF CI, we have such errors: >>>>> >>>>> mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file >>>>> not found >>>>> 20 | #include "linux/mptcp.h" >>>>> | ^~~~~~~~~~~~~~~ >>>>> >>>>> On my side, I don't have any issue, because the compiler uses the >>>>> mptcp.h file from the system: /usr/include/linux/mptcp.h >>>>> >>>>> I suppose that's not OK on the BPF CI, as it looks like it >>>>> doesn't have >>>>> this file there, probably because it still uses Ubuntu 20.04 as >>>>> base, >>>>> which doesn't include this file in the linux-libc-dev package. >>>>> >>>>> When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the >>>>> other >>>>> programs from that list -- is compiled (V=1), I see that the >>>>> following >>>>> "-I" options are given: >>>>> >>>>> -I${PWD}/tools/testing/selftests/bpf >>>>> -I${BUILD}//tools/include >>>>> -I${BUILD}/include/generated >>>>> -I${PWD}/tools/lib >>>>> -I${PWD}/tools/include >>>>> -I${PWD}/tools/include/uapi >>>>> -I${BUILD}/ >>>>> >>>>> It will then not look at -I${PWD}/usr/include or the directory >>>>> generated >>>>> with: >>>>> >>>>> make headers_install INSTALL_HDR_PATH=(...) >>>> >>>> It sounds like the tools/testing/selftests/net/mptcp/Makefile is >>>> looking >>>> at this include path, so it works? >>> >>> Yes it does work. >>> >>>> iiu the bpf/Makefile correctly, it has the bpftool "make" compiled >>>> and >>>> installed at tools/testing/selftests/bpf/tools/sbin/. May be >>>> directly >>>> compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"? >>> >>> That could be an alternative, I didn't know it would be OK to add >>> such >>> dependence, good idea. >>> >>>>> I guess that's why people have duplicated files in >>>>> 'tools/include/uapi', >>>>> but I also understood from Jakub that it is not a good idea to >>>>> continue >>>>> to do so. >>>>> >>>>> What would be the best solution to avoid a copy? A symlink still >>>>> looks >>>>> like a workaround. >>>>> >>>>> In the other selftests, KHDR_INCLUDES is used to be able to >>>>> include the >>>>> path containing the UAPI headers. So if someone built the headers >>>>> in a >>>> >>>> Meaning KHDR_INCLUDES should be used and - >>>> I${PWD}/tools/include/uapi can >>>> be retired? >>> >>> That's the idea, yes, for "userspace programs". I mean: for BPF >>> programs >>> requiring vmlinux.h (BPF_CFLAGS), I guess you will still need the >>> bpf.h >>> file from tools/include/uapi, no? >>> >>>> I haven't looked into the details. I quickly tried but it >>>> fails in my environment. >>> >>> Do you not have issues because some files have something like: >>> >>> #include <uapi/linux/(...).h> >>> >>> On my side, I had a working version using this patch: >>> >>>> diff --git a/tools/testing/selftests/bpf/Makefile >>>> b/tools/testing/selftests/bpf/Makefile >>>> index 7c5827d20c2e..112f14d40852 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 >>>> >>> >>> But only after having removed these extra 'uapi/': >>> >>> $ git grep -l '<uapi/' -- tools/testing/selftests/bpf | \ >>> xargs sed -i 's|#include <uapi/|#include <|g' >>> >>> Is it not OK for you like that? I tried and it works for me with the above changes. The other $(APIDIR) usages in the Makefile can be replaced also? Matt, do you want to post a patch and see how does it go with the bpf CI? [ Sorry for the late reply. ] >>> >>> Note that I built the selftests using KHDR_INCLUDES=- >>> I$INSTALL_HDR_PATH.
diff --git a/MAINTAINERS b/MAINTAINERS index cd3277a98cfe..4ea5db496698 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15756,6 +15756,7 @@ F: include/trace/events/mptcp.h F: include/uapi/linux/mptcp*.h F: net/mptcp/ F: tools/testing/selftests/bpf/*/*mptcp*.c +F: tools/testing/selftests/bpf/*mptcp*.c F: tools/testing/selftests/net/mptcp/ NETWORKING [TCP] diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index e0b3887b3d2d..204269d0b5b8 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \ flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \ test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \ xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \ - xdp_features bpf_test_no_cfi.ko + xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi @@ -645,6 +645,7 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \ $(OUTPUT)/xdp_synproxy \ $(OUTPUT)/sign-file \ $(OUTPUT)/uprobe_multi \ + $(OUTPUT)/mptcp_pm_nl_ctl \ ima_setup.sh \ verify_sig_setup.sh \ $(wildcard progs/btf_dump_test_case_*.c) \ diff --git a/tools/testing/selftests/bpf/mptcp_pm_nl_ctl.c b/tools/testing/selftests/bpf/mptcp_pm_nl_ctl.c new file mode 120000 index 000000000000..5a08c255b278 --- /dev/null +++ b/tools/testing/selftests/bpf/mptcp_pm_nl_ctl.c @@ -0,0 +1 @@ +../net/mptcp/pm_nl_ctl.c \ No newline at end of file