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
--
Sponsored by the NGI0 Core fund.
