Message-ID: <CAADnVQKnEuQ4XZROLWH47mEZNwnJ9TuyTrauOnuobcLgVMrimw@mail.gmail.com>
Date: Fri, 23 Aug 2024 15:28:40 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Matthieu Baerts <matttbe@...nel.org>
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

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/.
We're doing it for bpf.h because new selftests typically need
something from bpf.h that was just added in the previous patch.

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

Also realize that bpf selftests are not really kselftests.
We use a few common .mk for convenience. That's about it.

Powered by blists - more mailing lists