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