Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link

[Martin KaFai Lau <martin.lau@linux.dev>]

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