Message-ID: <41ef2c53-600a-47d6-a35f-674e1e7860f8@kernel.org>
Date: Sat, 24 Aug 2024 12:37:17 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: MPTCP Upstream <mptcp@...ts.linux.dev>,
 Andrii Nakryiko <andrii@...nel.org>, Eduard Zingerman <eddyz87@...il.com>,
 Mykola Lysenko <mykolal@...com>, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Shuah Khan <shuah@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Jesper Dangaard Brouer <hawk@...nel.org>, bpf <bpf@...r.kernel.org>,
 "open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>,
 LKML <linux-kernel@...r.kernel.org>,
 Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 1/2] selftests: bpf: use KHDR_INCLUDES for the
 UAPI headers

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@...nel.org> 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@...nel.org> 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@...nel.org>
>>>> ---
>>>>  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@meta.com/

> 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.

Powered by blists - more mailing lists