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

[Matthieu Baerts <matttbe@kernel.org>]

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.