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

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

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.