| Message ID | 20211001110856.14730-7-quentin@isovalent.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | install libbpf headers when using the library | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: kpsingh@kernel.org kafai@fb.com john.fastabend@gmail.com yhs@fb.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 33 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next | success | VM_Test |
On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote: > > API headers from libbpf should not be accessed directly from the > library's source directory. Instead, they should be exported with "make > install_headers". Let's make sure that bpf/preload/iterators/Makefile > installs the headers properly when building. > > Signed-off-by: Quentin Monnet <quentin@isovalent.com> > --- > kernel/bpf/preload/iterators/Makefile | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/kernel/bpf/preload/iterators/Makefile b/kernel/bpf/preload/iterators/Makefile > index 28fa8c1440f4..cf549dab3e20 100644 > --- a/kernel/bpf/preload/iterators/Makefile > +++ b/kernel/bpf/preload/iterators/Makefile > @@ -6,9 +6,11 @@ LLVM_STRIP ?= llvm-strip > DEFAULT_BPFTOOL := $(OUTPUT)/sbin/bpftool > BPFTOOL ?= $(DEFAULT_BPFTOOL) > LIBBPF_SRC := $(abspath ../../../../tools/lib/bpf) > -BPFOBJ := $(OUTPUT)/libbpf.a > -BPF_INCLUDE := $(OUTPUT) > -INCLUDES := -I$(OUTPUT) -I$(BPF_INCLUDE) -I$(abspath ../../../../tools/lib) \ > +LIBBPF_OUTPUT := $(abspath $(OUTPUT))/libbpf > +LIBBPF_DESTDIR := $(LIBBPF_OUTPUT) > +LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)/include > +BPFOBJ := $(LIBBPF_OUTPUT)/libbpf.a > +INCLUDES := -I$(OUTPUT) -I$(LIBBPF_INCLUDE) \ > -I$(abspath ../../../../tools/include/uapi) > CFLAGS := -g -Wall > > @@ -44,13 +46,15 @@ $(OUTPUT)/iterators.bpf.o: iterators.bpf.c $(BPFOBJ) | $(OUTPUT) > -c $(filter %.c,$^) -o $@ && \ > $(LLVM_STRIP) -g $@ > > -$(OUTPUT): > +$(OUTPUT) $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE): > $(call msg,MKDIR,$@) > - $(Q)mkdir -p $(OUTPUT) > + $(Q)mkdir -p $@ > > -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) > +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ > + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) Would it make sense for libbpf's Makefile to create include and output directories on its own? We wouldn't need to have these order-only dependencies everywhere, right? > $(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) \ > - OUTPUT=$(abspath $(dir $@))/ $(abspath $@) > + OUTPUT=$(abspath $(dir $@))/ prefix= \ > + DESTDIR=$(LIBBPF_DESTDIR) $(abspath $@) install_headers > > $(DEFAULT_BPFTOOL): > $(Q)$(MAKE) $(submake_extras) -C ../../../../tools/bpf/bpftool \ > -- > 2.30.2 >
On Sat, 2 Oct 2021 at 00:20, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > API headers from libbpf should not be accessed directly from the > > library's source directory. Instead, they should be exported with "make > > install_headers". Let's make sure that bpf/preload/iterators/Makefile > > installs the headers properly when building. > > > > -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) > > +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ > > + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) > > Would it make sense for libbpf's Makefile to create include and output > directories on its own? We wouldn't need to have these order-only > dependencies everywhere, right? Good point, I'll have a look at it. Quentin
On Sat, 2 Oct 2021 at 21:27, Quentin Monnet <quentin@isovalent.com> wrote: > > On Sat, 2 Oct 2021 at 00:20, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > API headers from libbpf should not be accessed directly from the > > > library's source directory. Instead, they should be exported with "make > > > install_headers". Let's make sure that bpf/preload/iterators/Makefile > > > installs the headers properly when building. > > > > > > > -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) > > > +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ > > > + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) > > > > Would it make sense for libbpf's Makefile to create include and output > > directories on its own? We wouldn't need to have these order-only > > dependencies everywhere, right? > > Good point, I'll have a look at it. > Quentin So libbpf already creates the include (and parent $(DESTDIR)) directory, so I can get rid of the related dependencies. But I don't see an easy solution for the output directory for the object files. The issue is that libbpf's Makefile includes tools/scripts/Makefile.include, which checks $(OUTPUT) and errors out if the directory does not exist. This prevents us from creating the directory as part of the regular targets. We could create it unconditionally before running any target, but it's ugly; and I don't see any simple workaround. So I'll remove the deps on $(LIBBPF_INCLUDE) and keep the ones on $(LIBBPF_OUTPUT).
On Sat, Oct 2, 2021 at 3:12 PM Quentin Monnet <quentin@isovalent.com> wrote: > > On Sat, 2 Oct 2021 at 21:27, Quentin Monnet <quentin@isovalent.com> wrote: > > > > On Sat, 2 Oct 2021 at 00:20, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > > > API headers from libbpf should not be accessed directly from the > > > > library's source directory. Instead, they should be exported with "make > > > > install_headers". Let's make sure that bpf/preload/iterators/Makefile > > > > installs the headers properly when building. > > > > > > > > > > -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) > > > > +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ > > > > + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) > > > > > > Would it make sense for libbpf's Makefile to create include and output > > > directories on its own? We wouldn't need to have these order-only > > > dependencies everywhere, right? > > > > Good point, I'll have a look at it. > > Quentin > > So libbpf already creates the include (and parent $(DESTDIR)) > directory, so I can get rid of the related dependencies. But I don't > see an easy solution for the output directory for the object files. > The issue is that libbpf's Makefile includes > tools/scripts/Makefile.include, which checks $(OUTPUT) and errors out Did you check what benefits the use of tools/scripts/Makefile.include brings? Last time I had to deal with some non-trivial Makefile problem, this extra dance with tools/scripts/Makefile.include and some related complexities didn't seem very justified. So unless there are some very big benefits to having tool's Makefile.include included, I'd rather simplify libbpf's in-kernel Makefile and make it more straightforward. We have a completely independent separate Makefile for libbpf in Github, and I think it's more straightforward. Doesn't have to be done in this change, of course, but I was curious to hear your thoughts given you seem to have spent tons of time on this already. > if the directory does not exist. This prevents us from creating the > directory as part of the regular targets. We could create it > unconditionally before running any target, but it's ugly; and I don't > see any simple workaround. > > So I'll remove the deps on $(LIBBPF_INCLUDE) and keep the ones on > $(LIBBPF_OUTPUT).
On Mon, 4 Oct 2021 at 20:11, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sat, Oct 2, 2021 at 3:12 PM Quentin Monnet <quentin@isovalent.com> wrote: > > > > On Sat, 2 Oct 2021 at 21:27, Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > On Sat, 2 Oct 2021 at 00:20, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > > > > > API headers from libbpf should not be accessed directly from the > > > > > library's source directory. Instead, they should be exported with "make > > > > > install_headers". Let's make sure that bpf/preload/iterators/Makefile > > > > > installs the headers properly when building. > > > > > > > > > > > > > -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) > > > > > +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ > > > > > + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) > > > > > > > > Would it make sense for libbpf's Makefile to create include and output > > > > directories on its own? We wouldn't need to have these order-only > > > > dependencies everywhere, right? > > > > > > Good point, I'll have a look at it. > > > Quentin > > > > So libbpf already creates the include (and parent $(DESTDIR)) > > directory, so I can get rid of the related dependencies. But I don't > > see an easy solution for the output directory for the object files. > > The issue is that libbpf's Makefile includes > > tools/scripts/Makefile.include, which checks $(OUTPUT) and errors out > > Did you check what benefits the use of tools/scripts/Makefile.include > brings? Last time I had to deal with some non-trivial Makefile > problem, this extra dance with tools/scripts/Makefile.include and some > related complexities didn't seem very justified. So unless there are > some very big benefits to having tool's Makefile.include included, I'd > rather simplify libbpf's in-kernel Makefile and make it more > straightforward. We have a completely independent separate Makefile > for libbpf in Github, and I think it's more straightforward. Doesn't > have to be done in this change, of course, but I was curious to hear > your thoughts given you seem to have spent tons of time on this > already. No, I haven't checked in details so far. I remember that several elements defined in the Makefile.include are used in libbpf's Makefile, and I stopped at that, because I thought that a refactoring of the latter would be beyond the current set. But yes, I can have a look at it and see if it's worth changing in a follow-up.
On Mon, 4 Oct 2021 at 22:30, Quentin Monnet <quentin@isovalent.com> wrote: > > On Mon, 4 Oct 2021 at 20:11, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Sat, Oct 2, 2021 at 3:12 PM Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > On Sat, 2 Oct 2021 at 21:27, Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > > > On Sat, 2 Oct 2021 at 00:20, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > > > > > > > API headers from libbpf should not be accessed directly from the > > > > > > library's source directory. Instead, they should be exported with "make > > > > > > install_headers". Let's make sure that bpf/preload/iterators/Makefile > > > > > > installs the headers properly when building. > > > > > > > > > > > > > > > > -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) > > > > > > +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ > > > > > > + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) > > > > > > > > > > Would it make sense for libbpf's Makefile to create include and output > > > > > directories on its own? We wouldn't need to have these order-only > > > > > dependencies everywhere, right? > > > > > > > > Good point, I'll have a look at it. > > > > Quentin > > > > > > So libbpf already creates the include (and parent $(DESTDIR)) > > > directory, so I can get rid of the related dependencies. But I don't > > > see an easy solution for the output directory for the object files. > > > The issue is that libbpf's Makefile includes > > > tools/scripts/Makefile.include, which checks $(OUTPUT) and errors out > > > > Did you check what benefits the use of tools/scripts/Makefile.include > > brings? Last time I had to deal with some non-trivial Makefile > > problem, this extra dance with tools/scripts/Makefile.include and some > > related complexities didn't seem very justified. So unless there are > > some very big benefits to having tool's Makefile.include included, I'd > > rather simplify libbpf's in-kernel Makefile and make it more > > straightforward. We have a completely independent separate Makefile > > for libbpf in Github, and I think it's more straightforward. Doesn't > > have to be done in this change, of course, but I was curious to hear > > your thoughts given you seem to have spent tons of time on this > > already. > > No, I haven't checked in details so far. I remember that several > elements defined in the Makefile.include are used in libbpf's > Makefile, and I stopped at that, because I thought that a refactoring > of the latter would be beyond the current set. But yes, I can have a > look at it and see if it's worth changing in a follow-up. Looking more at tools/scripts/Makefile.include: It's 160-line long and does not include any other Makefile, so there's nothing in it that we couldn't re-implement in libbpf's Makefile if necessary. This being said, it has a number of items that, I think, are good to keep there and share with the other tools. For example: - The $(EXTRA_WARNINGS) definitions - QUIET_GEN, QUIET_LINK, QUIET_CLEAN, which are not mandatory to have but integrate nicely with the way other tools (or kernel components) are built - Some overwrites for the toolchain, if $(LLVM) or $(CROSS_COMPILE) are set Thinking more about this, if we want to create the $(OUTPUT) directory in libbpf itself, we could maybe just enclose the check on its pre-existence in tools/scripts/Makefile.include with a dedicated variable ("ifneq ($(_skip_output_check),) ...") and set the latter in Makefile.include. This way we wouldn't have to change the current Makefile infra too much, and can keep the include. Quentin
On Tue, Oct 5, 2021 at 1:03 PM Quentin Monnet <quentin@isovalent.com> wrote: > > On Mon, 4 Oct 2021 at 22:30, Quentin Monnet <quentin@isovalent.com> wrote: > > > > On Mon, 4 Oct 2021 at 20:11, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > On Sat, Oct 2, 2021 at 3:12 PM Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > > > On Sat, 2 Oct 2021 at 21:27, Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > > > > > On Sat, 2 Oct 2021 at 00:20, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > > > > > > > > > API headers from libbpf should not be accessed directly from the > > > > > > > library's source directory. Instead, they should be exported with "make > > > > > > > install_headers". Let's make sure that bpf/preload/iterators/Makefile > > > > > > > installs the headers properly when building. > > > > > > > > > > > > > > > > > > > -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) > > > > > > > +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ > > > > > > > + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) > > > > > > > > > > > > Would it make sense for libbpf's Makefile to create include and output > > > > > > directories on its own? We wouldn't need to have these order-only > > > > > > dependencies everywhere, right? > > > > > > > > > > Good point, I'll have a look at it. > > > > > Quentin > > > > > > > > So libbpf already creates the include (and parent $(DESTDIR)) > > > > directory, so I can get rid of the related dependencies. But I don't > > > > see an easy solution for the output directory for the object files. > > > > The issue is that libbpf's Makefile includes > > > > tools/scripts/Makefile.include, which checks $(OUTPUT) and errors out > > > > > > Did you check what benefits the use of tools/scripts/Makefile.include > > > brings? Last time I had to deal with some non-trivial Makefile > > > problem, this extra dance with tools/scripts/Makefile.include and some > > > related complexities didn't seem very justified. So unless there are > > > some very big benefits to having tool's Makefile.include included, I'd > > > rather simplify libbpf's in-kernel Makefile and make it more > > > straightforward. We have a completely independent separate Makefile > > > for libbpf in Github, and I think it's more straightforward. Doesn't > > > have to be done in this change, of course, but I was curious to hear > > > your thoughts given you seem to have spent tons of time on this > > > already. > > > > No, I haven't checked in details so far. I remember that several > > elements defined in the Makefile.include are used in libbpf's > > Makefile, and I stopped at that, because I thought that a refactoring > > of the latter would be beyond the current set. But yes, I can have a > > look at it and see if it's worth changing in a follow-up. > > Looking more at tools/scripts/Makefile.include: It's 160-line long and > does not include any other Makefile, so there's nothing in it that we > couldn't re-implement in libbpf's Makefile if necessary. This being > said, it has a number of items that, I think, are good to keep there > and share with the other tools. For example: > > - The $(EXTRA_WARNINGS) definitions > - QUIET_GEN, QUIET_LINK, QUIET_CLEAN, which are not mandatory to have > but integrate nicely with the way other tools (or kernel components) > are built > - Some overwrites for the toolchain, if $(LLVM) or $(CROSS_COMPILE) are set > I looked at Makefile again, I had bigger reservations about tools/build/Makefile.include actually, as it causes some round-about ways to do the actual build, e.g.: $(Q)$(MAKE) $(build)=libbpf OUTPUT=$(SHARED_OBJDIR) CFLAGS="$(CFLAGS) $(SHLIB_FLAGS)" Like, what's going on here? What's $(build)? Everything can be deciphered, but a simple operation of compiling one file at a time becomes some maze of indirect make invocations... But that's a problem for another day. So never mind. > Thinking more about this, if we want to create the $(OUTPUT) directory > in libbpf itself, we could maybe just enclose the check on its > pre-existence in tools/scripts/Makefile.include with a dedicated > variable ("ifneq ($(_skip_output_check),) ...") and set the latter in > Makefile.include. This way we wouldn't have to change the current > Makefile infra too much, and can keep the include. _skip_output_check to bifurcate the behavior? I'd rather not. > > Quentin
diff --git a/kernel/bpf/preload/iterators/Makefile b/kernel/bpf/preload/iterators/Makefile index 28fa8c1440f4..cf549dab3e20 100644 --- a/kernel/bpf/preload/iterators/Makefile +++ b/kernel/bpf/preload/iterators/Makefile @@ -6,9 +6,11 @@ LLVM_STRIP ?= llvm-strip DEFAULT_BPFTOOL := $(OUTPUT)/sbin/bpftool BPFTOOL ?= $(DEFAULT_BPFTOOL) LIBBPF_SRC := $(abspath ../../../../tools/lib/bpf) -BPFOBJ := $(OUTPUT)/libbpf.a -BPF_INCLUDE := $(OUTPUT) -INCLUDES := -I$(OUTPUT) -I$(BPF_INCLUDE) -I$(abspath ../../../../tools/lib) \ +LIBBPF_OUTPUT := $(abspath $(OUTPUT))/libbpf +LIBBPF_DESTDIR := $(LIBBPF_OUTPUT) +LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)/include +BPFOBJ := $(LIBBPF_OUTPUT)/libbpf.a +INCLUDES := -I$(OUTPUT) -I$(LIBBPF_INCLUDE) \ -I$(abspath ../../../../tools/include/uapi) CFLAGS := -g -Wall @@ -44,13 +46,15 @@ $(OUTPUT)/iterators.bpf.o: iterators.bpf.c $(BPFOBJ) | $(OUTPUT) -c $(filter %.c,$^) -o $@ && \ $(LLVM_STRIP) -g $@ -$(OUTPUT): +$(OUTPUT) $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE): $(call msg,MKDIR,$@) - $(Q)mkdir -p $(OUTPUT) + $(Q)mkdir -p $@ -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) $(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) \ - OUTPUT=$(abspath $(dir $@))/ $(abspath $@) + OUTPUT=$(abspath $(dir $@))/ prefix= \ + DESTDIR=$(LIBBPF_DESTDIR) $(abspath $@) install_headers $(DEFAULT_BPFTOOL): $(Q)$(MAKE) $(submake_extras) -C ../../../../tools/bpf/bpftool \
API headers from libbpf should not be accessed directly from the library's source directory. Instead, they should be exported with "make install_headers". Let's make sure that bpf/preload/iterators/Makefile installs the headers properly when building. Signed-off-by: Quentin Monnet <quentin@isovalent.com> --- kernel/bpf/preload/iterators/Makefile | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)