* [PATCH bpf-next 1/3] bpftool: fix install for libbpf's internal header(s)
2021-10-09 21:03 [PATCH bpf-next 0/3] fixes for bpftool's Makefile Quentin Monnet
@ 2021-10-09 21:03 ` Quentin Monnet
2021-10-19 23:44 ` Andrii Nakryiko
2021-10-09 21:03 ` [PATCH bpf-next 2/3] bpftool: do not FORCE-build libbpf Quentin Monnet
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Quentin Monnet @ 2021-10-09 21:03 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, Quentin Monnet
We recently updated bpftool's Makefile to make it install the headers
from libbpf, instead of pulling them directly from libbpf's directory.
There is also an additional header, internal to libbpf, that needs be
installed. The way that bpftool's Makefile installs that particular
header is currently correct, but would break if we were to modify
$(LIBBPF_INTERNAL_HDRS) to make it point to more than one header.
Use a static pattern rule instead, so that the Makefile can withstand
the addition of other headers to install.
The objective is simply to make the Makefile more robust. It should
_not_ be read as an invitation to import more internal headers from
libbpf into bpftool.
Fixes: f012ade10b34 ("bpftool: Install libbpf headers instead of including the dir")
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
tools/bpf/bpftool/Makefile | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 9c2d13c513f0..2c510293f32b 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -14,7 +14,7 @@ else
Q = @
endif
-BPF_DIR = $(srctree)/tools/lib/bpf/
+BPF_DIR = $(srctree)/tools/lib/bpf
ifneq ($(OUTPUT),)
_OUTPUT := $(OUTPUT)
@@ -25,6 +25,7 @@ BOOTSTRAP_OUTPUT := $(_OUTPUT)/bootstrap/
LIBBPF_OUTPUT := $(_OUTPUT)/libbpf/
LIBBPF_DESTDIR := $(LIBBPF_OUTPUT)
LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)/include
+LIBBPF_HDRS_DIR := $(LIBBPF_INCLUDE)/bpf
LIBBPF = $(LIBBPF_OUTPUT)libbpf.a
LIBBPF_BOOTSTRAP_OUTPUT = $(BOOTSTRAP_OUTPUT)libbpf/
@@ -32,7 +33,8 @@ LIBBPF_BOOTSTRAP = $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
# We need to copy nlattr.h which is not otherwise exported by libbpf, but still
# required by bpftool.
-LIBBPF_INTERNAL_HDRS := nlattr.h
+_LIBBPF_INTERNAL_HDRS := nlattr.h
+LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,$(_LIBBPF_INTERNAL_HDRS))
ifeq ($(BPFTOOL_VERSION),)
BPFTOOL_VERSION := $(shell make -rR --no-print-directory -sC ../../.. kernelversion)
@@ -45,10 +47,9 @@ $(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) \
DESTDIR=$(LIBBPF_DESTDIR) prefix= $(LIBBPF) install_headers
-$(LIBBPF_INCLUDE)/bpf/$(LIBBPF_INTERNAL_HDRS): \
- $(addprefix $(BPF_DIR),$(LIBBPF_INTERNAL_HDRS)) $(LIBBPF)
- $(call QUIET_INSTALL, bpf/$(notdir $@))
- $(Q)install -m 644 -t $(LIBBPF_INCLUDE)/bpf/ $(BPF_DIR)$(notdir $@)
+$(LIBBPF_INTERNAL_HDRS): $(LIBBPF_HDRS_DIR)/%.h: $(BPF_DIR)/%.h $(LIBBPF)
+ $(call QUIET_INSTALL, $@)
+ $(Q)install -m 644 -t $(LIBBPF_HDRS_DIR) $<
$(LIBBPF_BOOTSTRAP): FORCE | $(LIBBPF_BOOTSTRAP_OUTPUT)
$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_BOOTSTRAP_OUTPUT) \
@@ -150,7 +151,7 @@ BOOTSTRAP_OBJS = $(addprefix $(BOOTSTRAP_OUTPUT),main.o common.o json_writer.o g
$(BOOTSTRAP_OBJS): $(LIBBPF_BOOTSTRAP)
OBJS = $(patsubst %.c,$(OUTPUT)%.o,$(SRCS)) $(OUTPUT)disasm.o
-$(OBJS): $(LIBBPF) $(LIBBPF_INCLUDE)/bpf/$(LIBBPF_INTERNAL_HDRS)
+$(OBJS): $(LIBBPF) $(LIBBPF_INTERNAL_HDRS)
VMLINUX_BTF_PATHS ?= $(if $(O),$(O)/vmlinux) \
$(if $(KBUILD_OUTPUT),$(KBUILD_OUTPUT)/vmlinux) \
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next 1/3] bpftool: fix install for libbpf's internal header(s)
2021-10-09 21:03 ` [PATCH bpf-next 1/3] bpftool: fix install for libbpf's internal header(s) Quentin Monnet
@ 2021-10-19 23:44 ` Andrii Nakryiko
0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2021-10-19 23:44 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf
On Sat, Oct 9, 2021 at 2:03 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> We recently updated bpftool's Makefile to make it install the headers
> from libbpf, instead of pulling them directly from libbpf's directory.
> There is also an additional header, internal to libbpf, that needs be
> installed. The way that bpftool's Makefile installs that particular
> header is currently correct, but would break if we were to modify
> $(LIBBPF_INTERNAL_HDRS) to make it point to more than one header.
>
> Use a static pattern rule instead, so that the Makefile can withstand
> the addition of other headers to install.
>
> The objective is simply to make the Makefile more robust. It should
> _not_ be read as an invitation to import more internal headers from
> libbpf into bpftool.
>
> Fixes: f012ade10b34 ("bpftool: Install libbpf headers instead of including the dir")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
> tools/bpf/bpftool/Makefile | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 9c2d13c513f0..2c510293f32b 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -14,7 +14,7 @@ else
> Q = @
> endif
>
> -BPF_DIR = $(srctree)/tools/lib/bpf/
> +BPF_DIR = $(srctree)/tools/lib/bpf
>
> ifneq ($(OUTPUT),)
> _OUTPUT := $(OUTPUT)
> @@ -25,6 +25,7 @@ BOOTSTRAP_OUTPUT := $(_OUTPUT)/bootstrap/
> LIBBPF_OUTPUT := $(_OUTPUT)/libbpf/
> LIBBPF_DESTDIR := $(LIBBPF_OUTPUT)
> LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)/include
> +LIBBPF_HDRS_DIR := $(LIBBPF_INCLUDE)/bpf
>
> LIBBPF = $(LIBBPF_OUTPUT)libbpf.a
> LIBBPF_BOOTSTRAP_OUTPUT = $(BOOTSTRAP_OUTPUT)libbpf/
> @@ -32,7 +33,8 @@ LIBBPF_BOOTSTRAP = $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
>
> # We need to copy nlattr.h which is not otherwise exported by libbpf, but still
> # required by bpftool.
> -LIBBPF_INTERNAL_HDRS := nlattr.h
> +_LIBBPF_INTERNAL_HDRS := nlattr.h
Felt weird and ugly to have _LIBBPF_INTERNAL_HDRS, so I just inlined it.
> +LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,$(_LIBBPF_INTERNAL_HDRS))
>
> ifeq ($(BPFTOOL_VERSION),)
> BPFTOOL_VERSION := $(shell make -rR --no-print-directory -sC ../../.. kernelversion)
> @@ -45,10 +47,9 @@ $(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
> $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) \
> DESTDIR=$(LIBBPF_DESTDIR) prefix= $(LIBBPF) install_headers
>
> -$(LIBBPF_INCLUDE)/bpf/$(LIBBPF_INTERNAL_HDRS): \
> - $(addprefix $(BPF_DIR),$(LIBBPF_INTERNAL_HDRS)) $(LIBBPF)
> - $(call QUIET_INSTALL, bpf/$(notdir $@))
> - $(Q)install -m 644 -t $(LIBBPF_INCLUDE)/bpf/ $(BPF_DIR)$(notdir $@)
> +$(LIBBPF_INTERNAL_HDRS): $(LIBBPF_HDRS_DIR)/%.h: $(BPF_DIR)/%.h $(LIBBPF)
> + $(call QUIET_INSTALL, $@)
> + $(Q)install -m 644 -t $(LIBBPF_HDRS_DIR) $<
>
> $(LIBBPF_BOOTSTRAP): FORCE | $(LIBBPF_BOOTSTRAP_OUTPUT)
> $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_BOOTSTRAP_OUTPUT) \
> @@ -150,7 +151,7 @@ BOOTSTRAP_OBJS = $(addprefix $(BOOTSTRAP_OUTPUT),main.o common.o json_writer.o g
> $(BOOTSTRAP_OBJS): $(LIBBPF_BOOTSTRAP)
>
> OBJS = $(patsubst %.c,$(OUTPUT)%.o,$(SRCS)) $(OUTPUT)disasm.o
> -$(OBJS): $(LIBBPF) $(LIBBPF_INCLUDE)/bpf/$(LIBBPF_INTERNAL_HDRS)
> +$(OBJS): $(LIBBPF) $(LIBBPF_INTERNAL_HDRS)
>
> VMLINUX_BTF_PATHS ?= $(if $(O),$(O)/vmlinux) \
> $(if $(KBUILD_OUTPUT),$(KBUILD_OUTPUT)/vmlinux) \
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next 2/3] bpftool: do not FORCE-build libbpf
2021-10-09 21:03 [PATCH bpf-next 0/3] fixes for bpftool's Makefile Quentin Monnet
2021-10-09 21:03 ` [PATCH bpf-next 1/3] bpftool: fix install for libbpf's internal header(s) Quentin Monnet
@ 2021-10-09 21:03 ` Quentin Monnet
2021-10-09 21:03 ` [PATCH bpf-next 3/3] bpftool: turn check on zlib from a phony target into a conditional error Quentin Monnet
2021-10-18 14:05 ` [PATCH bpf-next 0/3] fixes for bpftool's Makefile John Fastabend
3 siblings, 0 replies; 7+ messages in thread
From: Quentin Monnet @ 2021-10-09 21:03 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, Quentin Monnet
In bpftool's Makefile, libbpf has a FORCE dependency, to make sure we
rebuild it in case its source files changed. Let's instead make the
rebuild depend on the source files directly, through a call to the
"$(wildcard ...)" function. This avoids descending into libbpf's
directory if there is nothing to update.
Do the same for the bootstrap libbpf version.
This results in a slightly faster operation and less verbose output when
running make a second time in bpftool's directory.
Before:
Auto-detecting system features:
... libbfd: [ on ]
... disassembler-four-args: [ on ]
... zlib: [ on ]
... libcap: [ on ]
... clang-bpf-co-re: [ on ]
make[1]: Entering directory '/root/dev/linux/tools/lib/bpf'
make[1]: Entering directory '/root/dev/linux/tools/lib/bpf'
make[1]: Nothing to be done for 'install_headers'.
make[1]: Leaving directory '/root/dev/linux/tools/lib/bpf'
make[1]: Leaving directory '/root/dev/linux/tools/lib/bpf'
After:
Auto-detecting system features:
... libbfd: [ on ]
... disassembler-four-args: [ on ]
... zlib: [ on ]
... libcap: [ on ]
... clang-bpf-co-re: [ on ]
Other ways to clean up the output could be to pass the "-s" option, or
to redirect the output to >/dev/null, when calling make recursively to
descend into libbpf's directory. However, this would suppress some
useful output if something goes wrong during the build. A better
alternative would be to pass "--no-print-directory" to the recursive
make, but that would still leave us with some noise for
"install_headers". Skipping the descent into libbpf's directory if no
source file has changed works best, and seems the most logical option
overall.
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
tools/bpf/bpftool/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 2c510293f32b..4acec74f459b 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -43,7 +43,7 @@ endif
$(LIBBPF_OUTPUT) $(BOOTSTRAP_OUTPUT) $(LIBBPF_BOOTSTRAP_OUTPUT):
$(QUIET_MKDIR)mkdir -p $@
-$(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
+$(LIBBPF): $(wildcard $(BPF_DIR)/*.[ch] $(BPF_DIR)/Makefile) | $(LIBBPF_OUTPUT)
$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) \
DESTDIR=$(LIBBPF_DESTDIR) prefix= $(LIBBPF) install_headers
@@ -51,7 +51,7 @@ $(LIBBPF_INTERNAL_HDRS): $(LIBBPF_HDRS_DIR)/%.h: $(BPF_DIR)/%.h $(LIBBPF)
$(call QUIET_INSTALL, $@)
$(Q)install -m 644 -t $(LIBBPF_HDRS_DIR) $<
-$(LIBBPF_BOOTSTRAP): FORCE | $(LIBBPF_BOOTSTRAP_OUTPUT)
+$(LIBBPF_BOOTSTRAP): $(wildcard $(BPF_DIR)/*.[ch] $(BPF_DIR)/Makefile) | $(LIBBPF_BOOTSTRAP_OUTPUT)
$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_BOOTSTRAP_OUTPUT) \
ARCH= CC=$(HOSTCC) LD=$(HOSTLD) $@
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH bpf-next 3/3] bpftool: turn check on zlib from a phony target into a conditional error
2021-10-09 21:03 [PATCH bpf-next 0/3] fixes for bpftool's Makefile Quentin Monnet
2021-10-09 21:03 ` [PATCH bpf-next 1/3] bpftool: fix install for libbpf's internal header(s) Quentin Monnet
2021-10-09 21:03 ` [PATCH bpf-next 2/3] bpftool: do not FORCE-build libbpf Quentin Monnet
@ 2021-10-09 21:03 ` Quentin Monnet
2021-10-18 14:05 ` [PATCH bpf-next 0/3] fixes for bpftool's Makefile John Fastabend
3 siblings, 0 replies; 7+ messages in thread
From: Quentin Monnet @ 2021-10-09 21:03 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, Quentin Monnet
One of bpftool's object files depends on zlib. To make sure we do not
attempt to build that object when the library is not available, commit
d66fa3c70e59 ("tools: bpftool: add feature check for zlib") introduced a
feature check to detect whether zlib is present.
This check comes as a rule for which the target ("zdep") is a
nonexistent file (phony target), which means that the Makefile always
attempts to rebuild it. It is mostly harmless. However, one side effect
is that, on running again once bpftool is already built, make considers
that "something" (the recipe for zdep) was executed, and does not print
the usual message "make: Nothing to be done for 'all'", which is a
user-friendly indicator that the build went fine.
Before, with some level of debugging information:
$ make --debug=m
[...]
Reading makefiles...
Auto-detecting system features:
... libbfd: [ on ]
... disassembler-four-args: [ on ]
... zlib: [ on ]
... libcap: [ on ]
... clang-bpf-co-re: [ on ]
Updating makefiles....
Updating goal targets....
File 'all' does not exist.
File 'zdep' does not exist.
Must remake target 'zdep'.
File 'all' does not exist.
Must remake target 'all'.
Successfully remade target file 'all'.
After the patch:
$ make --debug=m
[...]
Auto-detecting system features:
... libbfd: [ on ]
... disassembler-four-args: [ on ]
... zlib: [ on ]
... libcap: [ on ]
... clang-bpf-co-re: [ on ]
Updating makefiles....
Updating goal targets....
File 'all' does not exist.
Must remake target 'all'.
Successfully remade target file 'all'.
make: Nothing to be done for 'all'.
(Note the last line, which is not part of make's debug information.)
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
tools/bpf/bpftool/Makefile | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 4acec74f459b..2174e21aa57b 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -199,7 +199,10 @@ $(BOOTSTRAP_OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
$(OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
$(QUIET_CC)$(CC) $(CFLAGS) -c -MMD -o $@ $<
-$(OUTPUT)feature.o: | zdep
+$(OUTPUT)feature.o:
+ifneq ($(feature-zlib), 1)
+ $(error "No zlib found")
+endif
$(BPFTOOL_BOOTSTRAP): $(BOOTSTRAP_OBJS) $(LIBBPF_BOOTSTRAP)
$(QUIET_LINK)$(HOSTCC) $(CFLAGS) $(LDFLAGS) -o $@ $(BOOTSTRAP_OBJS) \
@@ -255,10 +258,7 @@ doc-uninstall:
FORCE:
-zdep:
- @if [ "$(feature-zlib)" != "1" ]; then echo "No zlib found"; exit 1 ; fi
-
.SECONDARY:
-.PHONY: all FORCE clean install-bin install uninstall zdep
+.PHONY: all FORCE clean install-bin install uninstall
.PHONY: doc doc-clean doc-install doc-uninstall
.DEFAULT_GOAL := all
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* RE: [PATCH bpf-next 0/3] fixes for bpftool's Makefile
2021-10-09 21:03 [PATCH bpf-next 0/3] fixes for bpftool's Makefile Quentin Monnet
` (2 preceding siblings ...)
2021-10-09 21:03 ` [PATCH bpf-next 3/3] bpftool: turn check on zlib from a phony target into a conditional error Quentin Monnet
@ 2021-10-18 14:05 ` John Fastabend
2021-10-19 23:44 ` Andrii Nakryiko
3 siblings, 1 reply; 7+ messages in thread
From: John Fastabend @ 2021-10-18 14:05 UTC (permalink / raw)
To: Quentin Monnet, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, Quentin Monnet
Quentin Monnet wrote:
> This set contains one fix for bpftool's Makefile, to make sure that the
> headers internal to libbpf are installed properly even if we add more
> headers to the relevant Makefile variable in the future (although we'd like
> to avoid that if possible).
>
> The other patches aim at cleaning up the output from the Makefile, in
> particular when running the command "make" another time after bpftool is
> built.
>
> Quentin Monnet (3):
> bpftool: fix install for libbpf's internal header(s)
> bpftool: do not FORCE-build libbpf
> bpftool: turn check on zlib from a phony target into a conditional
> error
>
> tools/bpf/bpftool/Makefile | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> --
> 2.30.2
>
I'm not a Makefile expert, but from my side these look good. Thanks.
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next 0/3] fixes for bpftool's Makefile
2021-10-18 14:05 ` [PATCH bpf-next 0/3] fixes for bpftool's Makefile John Fastabend
@ 2021-10-19 23:44 ` Andrii Nakryiko
0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2021-10-19 23:44 UTC (permalink / raw)
To: John Fastabend
Cc: Quentin Monnet, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Networking, bpf
On Mon, Oct 18, 2021 at 7:05 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Quentin Monnet wrote:
> > This set contains one fix for bpftool's Makefile, to make sure that the
> > headers internal to libbpf are installed properly even if we add more
> > headers to the relevant Makefile variable in the future (although we'd like
> > to avoid that if possible).
> >
> > The other patches aim at cleaning up the output from the Makefile, in
> > particular when running the command "make" another time after bpftool is
> > built.
> >
> > Quentin Monnet (3):
> > bpftool: fix install for libbpf's internal header(s)
> > bpftool: do not FORCE-build libbpf
> > bpftool: turn check on zlib from a phony target into a conditional
> > error
> >
> > tools/bpf/bpftool/Makefile | 29 +++++++++++++++--------------
> > 1 file changed, 15 insertions(+), 14 deletions(-)
> >
> > --
> > 2.30.2
> >
>
> I'm not a Makefile expert, but from my side these look good. Thanks.
>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
Applied to bpf-next, thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread