Hi Alexei,
Thank you for your reply!
On 24/08/2024 00:28, Alexei Starovoitov wrote:
> On Sat, Aug 17, 2024 at 7:51 AM Matthieu Baerts <matttbe@xxxxxxxxxx> wrote:
>>
>> Hi Alexei,
>>
>> Thank you for the review.
>>
>> On 17/08/2024 09:22, Alexei Starovoitov wrote:
>>> On Fri, Aug 16, 2024 at 7:56 PM Matthieu Baerts (NGI0)
>>> <matttbe@xxxxxxxxxx> wrote:
>>>>
>>>> Instead of duplicating UAPI header files in 'tools/include/uapi', the
>>>> BPF selftests can also look at the header files inside the kernel
>>>> source.
>>>>
>>>> To do that, the kernel selftests infrastructure provides the
>>>> 'KHDR_INCLUDES' variable. This is what is being used in most selftests,
>>>> because it is what is recommended in the documentation [1]. If the
>>>> selftests are not executed from the kernel sources, it is possible to
>>>> override the variable, e.g.
>>>>
>>>> make KHDR_INCLUDES="-I${HDR_DIR}/include" -C "${KSFT_DIR}"
>>>>
>>>> ... where ${HDR_DIR} has been generated by this command:
>>>>
>>>> make headers_install INSTALL_HDR_PATH="${HDR_DIR}"
>>>>
>>>> Thanks to 'KHDR_INCLUDES', it is no longer needed to duplicate header
>>>> files for userspace test programs, and these programs can include UAPI
>>>> header files without the 'uapi' prefix.
>>>>
>>>> Note that it is still required to use 'tools/include/uapi' -- APIDIR,
>>>> which corresponds to TOOLS_INCLUDES from lib.mk -- for the BPF programs,
>>>> not to conflict with what is already defined in vmlinux.h.
>>>>
>>>> Link: https://docs.kernel.org/dev-tools/kselftest.html#contributing-new-tests-details [1]
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@xxxxxxxxxx>
>>>> ---
>>>> tools/testing/selftests/bpf/Makefile | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/assign_reuse.c | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/tc_links.c | 4 ++--
>>>> tools/testing/selftests/bpf/prog_tests/tc_netkit.c | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/tc_opts.c | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/user_ringbuf.c | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/xdp_bonding.c | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c | 2 +-
>>>> tools/testing/selftests/bpf/prog_tests/xdp_link.c | 2 +-
>>>> tools/testing/selftests/bpf/xdp_features.c | 4 ++--
>>>> 12 files changed, 14 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>>>> index 4eceb491a8ae..6a7aeae7e206 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
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/assign_reuse.c b/tools/testing/selftests/bpf/prog_tests/assign_reuse.c
>>>> index 989ee4d9785b..3d06bf5a1ba4 100644
>>>> --- a/tools/testing/selftests/bpf/prog_tests/assign_reuse.c
>>>> +++ b/tools/testing/selftests/bpf/prog_tests/assign_reuse.c
>>>> @@ -1,6 +1,6 @@
>>>> // SPDX-License-Identifier: GPL-2.0
>>>> /* Copyright (c) 2023 Isovalent */
>>>> -#include <uapi/linux/if_link.h>
>>>> +#include <linux/if_link.h>
>>>
>>> No. This is not an option.
>>> User space shouldn't include kernel headers like this.
>>> Long ago tools/include directory was specifically
>>> created to break such dependency.
>>> Back then it was done for perf.
>>
>> I'm sorry, but I think we are not talking about the same thing here:
>> here, I'm only modifying the "normal" userspace programs, not the ones
>> used to generate the BPF objects. Perf is a special case I suppose, it
>> needs to know the kernel internals. It is the same with BPF programs
>> requiring vmlinux.h. But I think "normal" userspace programs in the
>> sefltests can use the UAPI headers, no?
>
> Not really. perf is a normal user space that doesn't look into
> kernel internals.
> It's used to rely on a few .h from kernel src tree for convenience,
> since they're not present in what's installed after 'make headers'.
> Hence the tools/include dir was created.
>
> Using KHDR_INCLUDES is fine, but it's not ok to search replace
> s/uapi\/linux/linux/ everywhere.
> Like the example I quoted above.
> tools/.../if_link.h is much older than include/uapi/linux/if_link.h
> and it's ok.
> We're not planning to update it.
> It's like building selftests on the system with older glibc.
> There is no requirement to have every .h in the tools/ dir
> up-to-date with the latest in include/.
OK, sorry, I didn't know it was fine not to have them up-to-date.
KSelftests doc indicates that it is important to use the headers from
'make headers' to be able to find regressions in these header files. But
thanks to your comment below, I now understand bpf selftests are not
really kselftests.
> We're doing it for bpf.h because new selftests typically need
> something from bpf.h that was just added in the previous patch.
OK. Then, if I understand correctly, this doesn't apply to if_xdp.h, and
we can remove the warning mentioning this file is out-of-date, but not
remove the duplicated header file. This file in tools/include/uapi is a
snapshot of an old version on purpose, no need to warn people it is not
up-to-date then.
>> I understand that I could indeed fix my initial problem by duplicating
>> mptcp.h in tools/include/uapi/linux/, but this doesn't look to be
>> allowed any more by the Netdev maintainers, e.g. recently, 'ethtool.h'
>> has been duplicated there in commit 7effe3fdc049 ("tools: Add ethtool.h
>> header to tooling infra"), but removed quickly after in commit
>> bbe91a9f6889 ("tools: remove redundant ethtool.h from tooling infra").
>> In this case, it was fine to simply drop it, because the linked test
>> doesn't require a recent version. Jakub mentioned [4] that these
>> duplicated headers should be avoided, and the ones generated by 'make
>> headers' should be used instead: what is being suggested here.
>
> This is a different issue. There are very few .h in tools/ that
> needs a sync.
> bpf.h is one of them. ethtool.h is certainly not.
>
> you need something for mpctp.h. Let's talk about it,
> but switching everything to KHDR_INCLUDES is not ok,
> since there are a bunch of things in play.
> Sometimes selftests are built standalone and with non-glibc-s.
I didn't know there would be issues to use the latest version of the
UAPI headers. It was fine on my side, but indeed, I didn't check all the
possible combinations. Without [1], I cannot ask the BPF CI to check.
Fine not to switch everything to KHDR_INCLUDES then.
Now that the CI runners have been updated to use Ubuntu 24.04 [1], we
can use mptcp.h from the system headers, or do some actions via
IPRoute2. So not having KHDR_INCLUDES is no longer blocking us for the
moment. I think it might still be useful to add it for future use, and
also to use the latest version of the UAPI headers that are not in
'tools/include/uapi', but I don't want to insist if you prefer not to
use the latest version.
[1] https://github.com/libbpf/ci/pull/131
[2]
https://lore.kernel.org/bpf/364C4C5B-27A0-4210-84E2-8CA9867E4127@xxxxxxxx/
> Also realize that bpf selftests are not really kselftests.
> We use a few common .mk for convenience. That's about it.
OK, sorry, I didn't know about that! That explains why they cannot be
used like the other ones.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
