bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/9] install libbpf headers when using the library
@ 2021-10-01 11:08 Quentin Monnet
  2021-10-01 11:08 ` [PATCH bpf-next v2 1/9] tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h> Quentin Monnet
                   ` (9 more replies)
  0 siblings, 10 replies; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

Libbpf is used at several locations in the repository. Most of the time,
the tools relying on it build the library in its own directory, and include
the headers from there. This works, but this is not the cleanest approach.
It generates objects outside of the directory of the tool which is being
built, and it also increases the risk that developers include a header file
internal to libbpf, which is not supposed to be exposed to user
applications.

This set adjusts all involved Makefiles to make sure that libbpf is built
locally (with respect to the tool's directory or provided build directory),
and by ensuring that "make install_headers" is run from libbpf's Makefile
to export user headers properly.

This comes at a cost: given that the libbpf was so far mostly compiled in
its own directory by the different components using it, compiling it once
would be enough for all those components. With the new approach, each
component compiles its own version. To mitigate this cost, efforts were
made to reuse the compiled library when possible:

- Make the bpftool version in samples/bpf reuse the library previously
  compiled for the selftests.
- Make the bpftool version in BPF selftests reuse the library previously
  compiled for the selftests.
- Similarly, make resolve_btfids in BPF selftests reuse the same compiled
  library.
- Similarly, make runqslower in BPF selftests reuse the same compiled
  library; and make it rely on the bpftool version also compiled from the
  selftests (instead of compiling its own version).
- runqslower, when compiled independently, needs its own version of
  bpftool: make them share the same compiled libbpf.

As a result:

- Compiling the samples/bpf should compile libbpf just once.
- Compiling the BPF selftests should compile libbpf just once.
- Compiling the kernel (with BTF support) should now lead to compiling
  libbpf twice: one for resolve_btfids, one for kernel/bpf/preload.
- Compiling runqslower individually should compile libbpf just once. Same
  thing for bpftool, resolve_btfids, and kernel/bpf/preload/iterators.

(Not accounting for the boostrap version of libbpf required by bpftool,
which was already placed under a dedicated .../boostrap/libbpf/ directory,
and for which the count remains unchanged.)

A few commits in the series also contain drive-by clean-up changes for
bpftool includes, samples/bpf/.gitignore, or test_bpftool_build.sh. Please
refer to individual commit logs for details.

v2: Declare an additional dependency on libbpf's headers for
    iterators/iterators.o in kernel/preload/Makefile to make sure that
    these headers are exported before we compile the object file (and not
    just before we link it).

Quentin Monnet (9):
  tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h>
  tools: bpftool: install libbpf headers instead of including the dir
  tools: resolve_btfids: install libbpf headers when building
  tools: runqslower: install libbpf headers when building
  bpf: preload: install libbpf headers when building
  bpf: iterators: install libbpf headers when building
  samples/bpf: install libbpf headers when building
  samples/bpf: update .gitignore
  selftests/bpf: better clean up for runqslower in test_bpftool_build.sh

 kernel/bpf/preload/Makefile                   | 25 ++++++++++---
 kernel/bpf/preload/iterators/Makefile         | 18 ++++++----
 samples/bpf/.gitignore                        |  3 ++
 samples/bpf/Makefile                          | 36 +++++++++++++------
 tools/bpf/bpftool/Makefile                    | 27 ++++++++------
 tools/bpf/bpftool/gen.c                       |  1 -
 tools/bpf/bpftool/prog.c                      |  1 -
 tools/bpf/resolve_btfids/Makefile             | 17 ++++++---
 tools/bpf/resolve_btfids/main.c               |  4 +--
 tools/bpf/runqslower/Makefile                 | 12 ++++---
 tools/testing/selftests/bpf/Makefile          | 22 ++++++++----
 .../selftests/bpf/test_bpftool_build.sh       |  4 +++
 12 files changed, 116 insertions(+), 54 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 1/9] tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h>
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 11:08 ` [PATCH bpf-next v2 2/9] tools: bpftool: install libbpf headers instead of including the dir Quentin Monnet
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

It seems that the header file was never necessary to compile bpftool,
and it is not part of the headers exported from libbpf. Let's remove the
includes from prog.c and gen.c.

Fixes: d510296d331a ("bpftool: Use syscall/loader program in "prog load" and "gen skeleton" command.")
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/gen.c  | 1 -
 tools/bpf/bpftool/prog.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index cc835859465b..b2ffc18eafc1 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -18,7 +18,6 @@
 #include <sys/stat.h>
 #include <sys/mman.h>
 #include <bpf/btf.h>
-#include <bpf/bpf_gen_internal.h>
 
 #include "json_writer.h"
 #include "main.h"
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 9c3e343b7d87..7323dd490873 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -25,7 +25,6 @@
 #include <bpf/bpf.h>
 #include <bpf/btf.h>
 #include <bpf/libbpf.h>
-#include <bpf/bpf_gen_internal.h>
 #include <bpf/skel_internal.h>
 
 #include "cfg.h"
-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 2/9] tools: bpftool: install libbpf headers instead of including the dir
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
  2021-10-01 11:08 ` [PATCH bpf-next v2 1/9] tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h> Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 22:55   ` Andrii Nakryiko
  2021-10-01 11:08 ` [PATCH bpf-next v2 3/9] tools: resolve_btfids: install libbpf headers when building Quentin Monnet
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

Bpftool relies on libbpf, therefore it relies on a number of headers
from the library and must be linked against the library. The Makefile
for bpftool exposes these objects by adding tools/lib as an include
directory ("-I$(srctree)/tools/lib"). This is a working solution, but
this is not the cleanest one. The risk is to involuntarily include
objects that are not intended to be exposed by the libbpf.

The headers needed to compile bpftool should in fact be "installed" from
libbpf, with its "install_headers" Makefile target. In addition, there
is one header which is internal to the library and not supposed to be
used by external applications, but that bpftool uses anyway.

Adjust the Makefile in order to install the header files properly before
compiling bpftool. Also copy the additional internal header file
(nlattr.h), but call it out explicitly. Build (and install headers) in a
subdirectory under bpftool/ instead of tools/lib/bpf/. When descending
from a parent Makefile, this is configurable by setting the OUTPUT,
LIBBPF_OUTPUT and LIBBPF_DESTDIR variables.

Also adjust the Makefile for BPF selftests, so as to reuse the (host)
libbpf compiled earlier and to avoid compiling a separate version of the
library just for bpftool.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/Makefile           | 27 ++++++++++++++++-----------
 tools/testing/selftests/bpf/Makefile |  2 ++
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 1fcf5b01a193..78e42963535a 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -17,16 +17,16 @@ endif
 BPF_DIR = $(srctree)/tools/lib/bpf/
 
 ifneq ($(OUTPUT),)
-  LIBBPF_OUTPUT = $(OUTPUT)/libbpf/
-  LIBBPF_PATH = $(LIBBPF_OUTPUT)
-  BOOTSTRAP_OUTPUT = $(OUTPUT)/bootstrap/
+  _OUTPUT := $(OUTPUT)
 else
-  LIBBPF_OUTPUT =
-  LIBBPF_PATH = $(BPF_DIR)
-  BOOTSTRAP_OUTPUT = $(CURDIR)/bootstrap/
+  _OUTPUT := $(CURDIR)
 endif
+BOOTSTRAP_OUTPUT := $(_OUTPUT)/bootstrap/
+LIBBPF_OUTPUT := $(_OUTPUT)/libbpf/
+LIBBPF_DESTDIR := $(LIBBPF_OUTPUT)
+LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)/include
 
-LIBBPF = $(LIBBPF_PATH)libbpf.a
+LIBBPF = $(LIBBPF_OUTPUT)libbpf.a
 LIBBPF_BOOTSTRAP_OUTPUT = $(BOOTSTRAP_OUTPUT)libbpf/
 LIBBPF_BOOTSTRAP = $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
 
@@ -37,8 +37,14 @@ endif
 $(LIBBPF_OUTPUT) $(BOOTSTRAP_OUTPUT) $(LIBBPF_BOOTSTRAP_OUTPUT):
 	$(QUIET_MKDIR)mkdir -p $@
 
+# We need to copy nlattr.h which is not otherwise exported by libbpf, but still
+# required by bpftool.
 $(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
-	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) $(LIBBPF_OUTPUT)libbpf.a
+	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) \
+		DESTDIR=$(LIBBPF_DESTDIR) prefix= \
+		$(LIBBPF_OUTPUT)libbpf.a install_headers
+	$(call QUIET_INSTALL, bpf/nlattr.h)
+	$(Q)install -m 644 -t $(LIBBPF_INCLUDE)/bpf/ $(BPF_DIR)nlattr.h
 
 $(LIBBPF_BOOTSTRAP): FORCE | $(LIBBPF_BOOTSTRAP_OUTPUT)
 	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_BOOTSTRAP_OUTPUT) \
@@ -60,10 +66,10 @@ CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
 CFLAGS += $(filter-out -Wswitch-enum -Wnested-externs,$(EXTRA_WARNINGS))
 CFLAGS += -DPACKAGE='"bpftool"' -D__EXPORTED_HEADERS__ \
 	-I$(if $(OUTPUT),$(OUTPUT),.) \
+	-I$(LIBBPF_INCLUDE) \
 	-I$(srctree)/kernel/bpf/ \
 	-I$(srctree)/tools/include \
 	-I$(srctree)/tools/include/uapi \
-	-I$(srctree)/tools/lib \
 	-I$(srctree)/tools/perf
 CFLAGS += -DBPFTOOL_VERSION='"$(BPFTOOL_VERSION)"'
 ifneq ($(EXTRA_CFLAGS),)
@@ -167,8 +173,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF)
 	$(QUIET_CLANG)$(CLANG) \
 		-I$(if $(OUTPUT),$(OUTPUT),.) \
 		-I$(srctree)/tools/include/uapi/ \
-		-I$(LIBBPF_PATH) \
-		-I$(srctree)/tools/lib \
+		-I$(LIBBPF_INCLUDE) \
 		-g -O2 -Wall -target bpf -c $< -o $@ && $(LLVM_STRIP) -g $@
 
 $(OUTPUT)%.skel.h: $(OUTPUT)%.bpf.o $(BPFTOOL_BOOTSTRAP)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 326ea75ce99e..5432bfc99740 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -209,6 +209,8 @@ $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)    \
 		    CC=$(HOSTCC) LD=$(HOSTLD)				       \
 		    EXTRA_CFLAGS='-g -O0'				       \
 		    OUTPUT=$(HOST_BUILD_DIR)/bpftool/			       \
+		    LIBBPF_OUTPUT=$(HOST_BUILD_DIR)/libbpf/		       \
+		    LIBBPF_DESTDIR=$(HOST_SCRATCH_DIR)/			       \
 		    prefix= DESTDIR=$(HOST_SCRATCH_DIR)/ install
 
 all: docs
-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 3/9] tools: resolve_btfids: install libbpf headers when building
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
  2021-10-01 11:08 ` [PATCH bpf-next v2 1/9] tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h> Quentin Monnet
  2021-10-01 11:08 ` [PATCH bpf-next v2 2/9] tools: bpftool: install libbpf headers instead of including the dir Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 23:01   ` Andrii Nakryiko
  2021-10-01 11:08 ` [PATCH bpf-next v2 4/9] tools: runqslower: " Quentin Monnet
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

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 resolve_btfids installs the
headers properly when building.

When descending from a parent Makefile, the specific output directories
for building the library and exporting the headers are configurable with
LIBBPF_OUT and LIBBPF_DESTDIR, respectively. This is in addition to
OUTPUT, on top of which those variables are constructed by default.

Also adjust the Makefile for the BPF selftests in order to point to the
(target) libbpf shared with other tools, instead of building a version
specific to resolve_btfids.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/resolve_btfids/Makefile    | 17 ++++++++++++-----
 tools/bpf/resolve_btfids/main.c      |  4 ++--
 tools/testing/selftests/bpf/Makefile |  7 +++++--
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile
index 08b75e314ae7..89a46d4d0768 100644
--- a/tools/bpf/resolve_btfids/Makefile
+++ b/tools/bpf/resolve_btfids/Makefile
@@ -29,25 +29,31 @@ BPFOBJ     := $(OUTPUT)/libbpf/libbpf.a
 LIBBPF_OUT := $(abspath $(dir $(BPFOBJ)))/
 SUBCMDOBJ  := $(OUTPUT)/libsubcmd/libsubcmd.a
 
+LIBBPF_DESTDIR := $(LIBBPF_OUT)
+LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)include
+
 BINARY     := $(OUTPUT)/resolve_btfids
 BINARY_IN  := $(BINARY)-in.o
 
 all: $(BINARY)
 
-$(OUTPUT) $(OUTPUT)/libbpf $(OUTPUT)/libsubcmd:
+$(OUTPUT) $(OUTPUT)/libsubcmd $(LIBBPF_OUT) $(LIBBPF_INCLUDE):
 	$(call msg,MKDIR,,$@)
 	$(Q)mkdir -p $(@)
 
 $(SUBCMDOBJ): fixdep FORCE | $(OUTPUT)/libsubcmd
 	$(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(abspath $(dir $@))/ $(abspath $@)
 
-$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT)/libbpf
-	$(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC)  OUTPUT=$(LIBBPF_OUT) $(abspath $@)
+$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile)	       \
+	   | $(LIBBPF_OUT) $(LIBBPF_INCLUDE)
+	$(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) OUTPUT=$(LIBBPF_OUT)    \
+		    DESTDIR=$(LIBBPF_DESTDIR) prefix=			       \
+		    $(abspath $@) install_headers
 
 CFLAGS := -g \
           -I$(srctree)/tools/include \
           -I$(srctree)/tools/include/uapi \
-          -I$(LIBBPF_SRC) \
+          -I$(LIBBPF_INCLUDE) \
           -I$(SUBCMD_SRC)
 
 LIBS = -lelf -lz
@@ -65,7 +71,8 @@ $(BINARY): $(BPFOBJ) $(SUBCMDOBJ) $(BINARY_IN)
 clean_objects := $(wildcard $(OUTPUT)/*.o                \
                             $(OUTPUT)/.*.o.cmd           \
                             $(OUTPUT)/.*.o.d             \
-                            $(OUTPUT)/libbpf             \
+                            $(LIBBPF_OUT)                \
+                            $(LIBBPF_DESTDIR)            \
                             $(OUTPUT)/libsubcmd          \
                             $(OUTPUT)/resolve_btfids)
 
diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index de6365b53c9c..91af785e6de5 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -60,8 +60,8 @@
 #include <linux/rbtree.h>
 #include <linux/zalloc.h>
 #include <linux/err.h>
-#include <btf.h>
-#include <libbpf.h>
+#include <bpf/btf.h>
+#include <bpf/libbpf.h>
 #include <parse-options.h>
 
 #define BTF_IDS_SECTION	".BTF_ids"
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 5432bfc99740..0167514ccaa2 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -122,9 +122,11 @@ BPFOBJ := $(BUILD_DIR)/libbpf/libbpf.a
 ifneq ($(CROSS_COMPILE),)
 HOST_BUILD_DIR		:= $(BUILD_DIR)/host
 HOST_SCRATCH_DIR	:= $(OUTPUT)/host-tools
+HOST_INCLUDE_DIR	:= $(HOST_SCRATCH_DIR)/include
 else
 HOST_BUILD_DIR		:= $(BUILD_DIR)
 HOST_SCRATCH_DIR	:= $(SCRATCH_DIR)
+HOST_INCLUDE_DIR	:= $(INCLUDE_DIR)
 endif
 HOST_BPFOBJ := $(HOST_BUILD_DIR)/libbpf/libbpf.a
 RESOLVE_BTFIDS := $(HOST_BUILD_DIR)/resolve_btfids/resolve_btfids
@@ -152,7 +154,7 @@ $(notdir $(TEST_GEN_PROGS)						\
 # sort removes libbpf duplicates when not cross-building
 MAKE_DIRS := $(sort $(BUILD_DIR)/libbpf $(HOST_BUILD_DIR)/libbpf	       \
 	       $(HOST_BUILD_DIR)/bpftool $(HOST_BUILD_DIR)/resolve_btfids      \
-	       $(INCLUDE_DIR))
+	       $(INCLUDE_DIR) $(HOST_INCLUDE_DIR))
 $(MAKE_DIRS):
 	$(call msg,MKDIR,,$@)
 	$(Q)mkdir -p $@
@@ -235,7 +237,7 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)		       \
 ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
 $(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)                \
 	   ../../../include/uapi/linux/bpf.h                                   \
-	   | $(INCLUDE_DIR) $(HOST_BUILD_DIR)/libbpf
+	   | $(HOST_INCLUDE_DIR) $(HOST_BUILD_DIR)/libbpf
 	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR)                             \
 		    EXTRA_CFLAGS='-g -O0'				       \
 		    OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
@@ -260,6 +262,7 @@ $(RESOLVE_BTFIDS): $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/resolve_btfids	\
 		       $(TOOLSDIR)/lib/str_error_r.c
 	$(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/resolve_btfids	\
 		CC=$(HOSTCC) LD=$(HOSTLD) AR=$(HOSTAR) \
+		LIBBPF_INCLUDE=$(HOST_INCLUDE_DIR) \
 		OUTPUT=$(HOST_BUILD_DIR)/resolve_btfids/ BPFOBJ=$(HOST_BPFOBJ)
 
 # Get Clang's default includes on this system, as opposed to those seen by
-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 4/9] tools: runqslower: install libbpf headers when building
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
                   ` (2 preceding siblings ...)
  2021-10-01 11:08 ` [PATCH bpf-next v2 3/9] tools: resolve_btfids: install libbpf headers when building Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 11:08 ` [PATCH bpf-next v2 5/9] bpf: preload: " Quentin Monnet
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

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 runqslower installs the
headers properly when building.

When descending from a parent Makefile, the specific output directories
for building the library and exporting the headers are configurable with
BPFOBJ_OUTPUT and BPF_DESTDIR, respectively. This is in addition to
OUTPUT, on top of which those variables are constructed by default.

Also adjust the Makefile for the BPF selftests. We pass a number of
variables to the "make" invocation, because we want to point runqslower
to the (target) libbpf shared with other tools, instead of building its
own version. In addition, runqslower relies on (target) bpftool, and we
also want to pass the proper variables to its Makefile so that bpftool
itself reuses the same libbpf.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/runqslower/Makefile        | 12 +++++++-----
 tools/testing/selftests/bpf/Makefile | 15 +++++++++------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
index 3818ec511fd2..73ae8569878d 100644
--- a/tools/bpf/runqslower/Makefile
+++ b/tools/bpf/runqslower/Makefile
@@ -9,9 +9,9 @@ BPFTOOL ?= $(DEFAULT_BPFTOOL)
 LIBBPF_SRC := $(abspath ../../lib/bpf)
 BPFOBJ_OUTPUT := $(OUTPUT)libbpf/
 BPFOBJ := $(BPFOBJ_OUTPUT)libbpf.a
-BPF_INCLUDE := $(BPFOBJ_OUTPUT)
-INCLUDES := -I$(OUTPUT) -I$(BPF_INCLUDE) -I$(abspath ../../lib)        \
-       -I$(abspath ../../include/uapi)
+BPF_DESTDIR := $(BPFOBJ_OUTPUT)
+BPF_INCLUDE := $(BPF_DESTDIR)/include
+INCLUDES := -I$(OUTPUT) -I$(BPF_INCLUDE) -I$(abspath ../../include/uapi)
 CFLAGS := -g -Wall
 
 # Try to detect best kernel BTF source
@@ -81,8 +81,10 @@ else
 endif
 
 $(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(BPFOBJ_OUTPUT)
-	$(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) OUTPUT=$(BPFOBJ_OUTPUT) $@
+	$(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) OUTPUT=$(BPFOBJ_OUTPUT) \
+		    DESTDIR=$(BPFOBJ_OUTPUT) prefix= $(abspath $@) install_headers
 
 $(DEFAULT_BPFTOOL): | $(BPFTOOL_OUTPUT)
 	$(Q)$(MAKE) $(submake_extras) -C ../bpftool OUTPUT=$(BPFTOOL_OUTPUT)   \
-		    CC=$(HOSTCC) LD=$(HOSTLD)
+		    LIBBPF_OUTPUT=$(BPFOBJ_OUTPUT)			       \
+		    LIBBPF_DESTDIR=$(BPF_DESTDIR) CC=$(HOSTCC) LD=$(HOSTLD)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 0167514ccaa2..6e7be0a0d79a 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -130,6 +130,7 @@ HOST_INCLUDE_DIR	:= $(INCLUDE_DIR)
 endif
 HOST_BPFOBJ := $(HOST_BUILD_DIR)/libbpf/libbpf.a
 RESOLVE_BTFIDS := $(HOST_BUILD_DIR)/resolve_btfids/resolve_btfids
+RUNQSLOWER_OUTPUT := $(BUILD_DIR)/runqslower/
 
 VMLINUX_BTF_PATHS ?= $(if $(O),$(O)/vmlinux)				\
 		     $(if $(KBUILD_OUTPUT),$(KBUILD_OUTPUT)/vmlinux)	\
@@ -154,7 +155,7 @@ $(notdir $(TEST_GEN_PROGS)						\
 # sort removes libbpf duplicates when not cross-building
 MAKE_DIRS := $(sort $(BUILD_DIR)/libbpf $(HOST_BUILD_DIR)/libbpf	       \
 	       $(HOST_BUILD_DIR)/bpftool $(HOST_BUILD_DIR)/resolve_btfids      \
-	       $(INCLUDE_DIR) $(HOST_INCLUDE_DIR))
+	       $(RUNQSLOWER_OUTPUT) $(INCLUDE_DIR) $(HOST_INCLUDE_DIR))
 $(MAKE_DIRS):
 	$(call msg,MKDIR,,$@)
 	$(Q)mkdir -p $@
@@ -183,11 +184,13 @@ $(OUTPUT)/test_stub.o: test_stub.c $(BPFOBJ)
 
 DEFAULT_BPFTOOL := $(HOST_SCRATCH_DIR)/sbin/bpftool
 
-$(OUTPUT)/runqslower: $(BPFOBJ) | $(DEFAULT_BPFTOOL)
-	$(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/runqslower	\
-		    OUTPUT=$(SCRATCH_DIR)/ VMLINUX_BTF=$(VMLINUX_BTF)   \
-		    BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR) &&	\
-		    cp $(SCRATCH_DIR)/runqslower $@
+$(OUTPUT)/runqslower: $(BPFOBJ) | $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT)
+	$(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/runqslower	       \
+		    OUTPUT=$(RUNQSLOWER_OUTPUT) VMLINUX_BTF=$(VMLINUX_BTF)     \
+		    BPFTOOL_OUTPUT=$(BUILD_DIR)/bpftool/		       \
+		    BPFOBJ_OUTPUT=$(BUILD_DIR)/libbpf			       \
+		    BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR) &&	       \
+		    cp $(RUNQSLOWER_OUTPUT)runqslower $@
 
 TEST_GEN_PROGS_EXTENDED += $(DEFAULT_BPFTOOL)
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 5/9] bpf: preload: install libbpf headers when building
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
                   ` (3 preceding siblings ...)
  2021-10-01 11:08 ` [PATCH bpf-next v2 4/9] tools: runqslower: " Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 11:08 ` [PATCH bpf-next v2 6/9] bpf: iterators: " Quentin Monnet
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

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/Makefile installs the
headers properly when building.

Note that we declare an additional dependency for iterators/iterators.o:
having $(LIBBPF_A) as a dependency to "$(obj)/bpf_preload_umd" is not
sufficient, as it makes it required only at the linking step. But we
need libbpf to be compiled, and in particular its headers to be
exported, before we attempt to compile iterators.o. The issue would not
occur before this commit, because libbpf's headers were not exported and
were always available under tools/lib/bpf.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 kernel/bpf/preload/Makefile | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/preload/Makefile b/kernel/bpf/preload/Makefile
index 1951332dd15f..e6a94278b935 100644
--- a/kernel/bpf/preload/Makefile
+++ b/kernel/bpf/preload/Makefile
@@ -1,21 +1,36 @@
 # SPDX-License-Identifier: GPL-2.0
 
 LIBBPF_SRCS = $(srctree)/tools/lib/bpf/
-LIBBPF_A = $(obj)/libbpf.a
-LIBBPF_OUT = $(abspath $(obj))
+LIBBPF_OUT = $(abspath $(obj))/libbpf
+LIBBPF_A = $(LIBBPF_OUT)/libbpf.a
+LIBBPF_DESTDIR = $(LIBBPF_OUT)
+LIBBPF_INCLUDE = $(LIBBPF_DESTDIR)/include
 
 # Although not in use by libbpf's Makefile, set $(O) so that the "dummy" test
 # in tools/scripts/Makefile.include always succeeds when building the kernel
 # with $(O) pointing to a relative path, as in "make O=build bindeb-pkg".
-$(LIBBPF_A):
-	$(Q)$(MAKE) -C $(LIBBPF_SRCS) O=$(LIBBPF_OUT)/ OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a
+$(LIBBPF_A): | $(LIBBPF_OUT) $(LIBBPF_INCLUDE)
+	$(Q)$(MAKE) -C $(LIBBPF_SRCS) O=$(LIBBPF_OUT)/ OUTPUT=$(LIBBPF_OUT)/   \
+		DESTDIR=$(LIBBPF_DESTDIR) prefix=			       \
+		$(LIBBPF_OUT)/libbpf.a install_headers
+
+libbpf-hdrs: $(LIBBPF_A)
+
+.PHONY: libbpf-hdrs
+
+$(LIBBPF_OUT) $(LIBBPF_INCLUDE):
+	$(call msg,MKDIR,$@)
+	$(Q)mkdir -p $@
 
 userccflags += -I $(srctree)/tools/include/ -I $(srctree)/tools/include/uapi \
-	-I $(srctree)/tools/lib/ -Wno-unused-result
+	-I $(LIBBPF_INCLUDE) -Wno-unused-result
 
 userprogs := bpf_preload_umd
 
 clean-files := $(userprogs) bpf_helper_defs.h FEATURE-DUMP.libbpf staticobjs/ feature/
+clean-files += $(LIBBPF_OUT) $(LIBBPF_DESTDIR)
+
+$(obj)/iterators/iterators.o: libbpf-hdrs
 
 bpf_preload_umd-objs := iterators/iterators.o
 bpf_preload_umd-userldlibs := $(LIBBPF_A) -lelf -lz
-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
                   ` (4 preceding siblings ...)
  2021-10-01 11:08 ` [PATCH bpf-next v2 5/9] bpf: preload: " Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 23:19   ` Andrii Nakryiko
  2021-10-01 11:08 ` [PATCH bpf-next v2 7/9] samples/bpf: " Quentin Monnet
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

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)
 	$(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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 7/9] samples/bpf: install libbpf headers when building
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
                   ` (5 preceding siblings ...)
  2021-10-01 11:08 ` [PATCH bpf-next v2 6/9] bpf: iterators: " Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 23:22   ` Andrii Nakryiko
  2021-10-01 11:08 ` [PATCH bpf-next v2 8/9] samples/bpf: update .gitignore Quentin Monnet
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

API headers from libbpf should not be accessed directly from the source
directory. Instead, they should be exported with "make install_headers".
Make sure that samples/bpf/Makefile installs the headers properly when
building.

The object compiled from and exported by libbpf are now placed into a
subdirectory of sample/bpf/ instead of remaining in tools/lib/bpf/. We
attempt to remove this directory on "make clean". However, the "clean"
target re-enters the samples/bpf/ directory from the root of the
repository ("$(MAKE) -C ../../ M=$(CURDIR) clean"), in such a way that
$(srctree) and $(src) are not defined, making it impossible to use
$(LIBBPF_OUTPUT) and $(LIBBPF_DESTDIR) in the recipe. So we only attempt
to clean $(CURDIR)/libbpf, which is the default value.

We also change the output directory for bpftool, to place the generated
objects under samples/bpf/bpftool/ instead of building in bpftool's
directory directly. Doing so, we make sure bpftool reuses the libbpf
library previously compiled and installed.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 samples/bpf/Makefile | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 4dc20be5fb96..7de602c2c705 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -59,7 +59,11 @@ tprogs-y += xdp_redirect
 tprogs-y += xdp_monitor
 
 # Libbpf dependencies
-LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
+LIBBPF_SRC = $(TOOLS_PATH)/lib/bpf
+LIBBPF_OUTPUT = $(abspath $(BPF_SAMPLES_PATH))/libbpf
+LIBBPF_DESTDIR = $(LIBBPF_OUTPUT)
+LIBBPF_INCLUDE = $(LIBBPF_DESTDIR)/include
+LIBBPF = $(LIBBPF_OUTPUT)/libbpf.a
 
 CGROUP_HELPERS := ../../tools/testing/selftests/bpf/cgroup_helpers.o
 TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
@@ -198,7 +202,7 @@ TPROGS_CFLAGS += -Wstrict-prototypes
 
 TPROGS_CFLAGS += -I$(objtree)/usr/include
 TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
-TPROGS_CFLAGS += -I$(srctree)/tools/lib/
+TPROGS_CFLAGS += -I$(LIBBPF_INCLUDE)
 TPROGS_CFLAGS += -I$(srctree)/tools/include
 TPROGS_CFLAGS += -I$(srctree)/tools/perf
 TPROGS_CFLAGS += -DHAVE_ATTR_TEST=0
@@ -268,16 +272,28 @@ all:
 clean:
 	$(MAKE) -C ../../ M=$(CURDIR) clean
 	@find $(CURDIR) -type f -name '*~' -delete
+	@/bin/rm -rf $(CURDIR)/libbpf $(CURDIR)/bpftool
 
-$(LIBBPF): FORCE
+$(LIBBPF): FORCE | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE)
 # Fix up variables inherited from Kbuild that tools/ build system won't like
-	$(MAKE) -C $(dir $@) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)" \
-		LDFLAGS=$(TPROGS_LDFLAGS) srctree=$(BPF_SAMPLES_PATH)/../../ O=
+	$(MAKE) -C $(LIBBPF_SRC) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)" \
+		LDFLAGS=$(TPROGS_LDFLAGS) srctree=$(BPF_SAMPLES_PATH)/../../ \
+		O= OUTPUT=$(LIBBPF_OUTPUT)/ DESTDIR=$(LIBBPF_DESTDIR) prefix= \
+		$@ install_headers
 
 BPFTOOLDIR := $(TOOLS_PATH)/bpf/bpftool
-BPFTOOL := $(BPFTOOLDIR)/bpftool
-$(BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)
-	    $(MAKE) -C $(BPFTOOLDIR) srctree=$(BPF_SAMPLES_PATH)/../../
+BPFTOOL_OUTPUT := $(abspath $(BPF_SAMPLES_PATH))/bpftool
+BPFTOOL := $(BPFTOOL_OUTPUT)/bpftool
+$(BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \
+	    | $(BPFTOOL_OUTPUT)
+	    $(MAKE) -C $(BPFTOOLDIR) srctree=$(BPF_SAMPLES_PATH)/../../ \
+		OUTPUT=$(BPFTOOL_OUTPUT)/ \
+		LIBBPF_OUTPUT=$(LIBBPF_OUTPUT) \
+		LIBBPF_DESTDIR=$(LIBBPF_DESTDIR)
+
+$(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) $(BPFTOOL_OUTPUT):
+	$(call msg,MKDIR,$@)
+	$(Q)mkdir -p $@
 
 $(obj)/syscall_nrs.h:	$(obj)/syscall_nrs.s FORCE
 	$(call filechk,offsets,__SYSCALL_NRS_H__)
@@ -367,7 +383,7 @@ $(obj)/%.bpf.o: $(src)/%.bpf.c $(obj)/vmlinux.h $(src)/xdp_sample.bpf.h $(src)/x
 	$(Q)$(CLANG) -g -O2 -target bpf -D__TARGET_ARCH_$(SRCARCH) \
 		-Wno-compare-distinct-pointer-types -I$(srctree)/include \
 		-I$(srctree)/samples/bpf -I$(srctree)/tools/include \
-		-I$(srctree)/tools/lib $(CLANG_SYS_INCLUDES) \
+		-I$(LIBBPF_INCLUDE) $(CLANG_SYS_INCLUDES) \
 		-c $(filter %.bpf.c,$^) -o $@
 
 LINKED_SKELS := xdp_redirect_cpu.skel.h xdp_redirect_map_multi.skel.h \
@@ -404,7 +420,7 @@ $(obj)/%.o: $(src)/%.c
 	@echo "  CLANG-bpf " $@
 	$(Q)$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(BPF_EXTRA_CFLAGS) \
 		-I$(obj) -I$(srctree)/tools/testing/selftests/bpf/ \
-		-I$(srctree)/tools/lib/ \
+		-I$(LIBBPF_INCLUDE) \
 		-D__KERNEL__ -D__BPF_TRACING__ -Wno-unused-value -Wno-pointer-sign \
 		-D__TARGET_ARCH_$(SRCARCH) -Wno-compare-distinct-pointer-types \
 		-Wno-gnu-variable-sized-type-not-at-end \
-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 8/9] samples/bpf: update .gitignore
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
                   ` (6 preceding siblings ...)
  2021-10-01 11:08 ` [PATCH bpf-next v2 7/9] samples/bpf: " Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 11:08 ` [PATCH bpf-next v2 9/9] selftests/bpf: better clean up for runqslower in test_bpftool_build.sh Quentin Monnet
  2021-10-01 23:05 ` [PATCH bpf-next v2 0/9] install libbpf headers when using the library Andrii Nakryiko
  9 siblings, 0 replies; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

Update samples/bpf/.gitignore to ignore files generated when building
the samples. Add:

  - vmlinux.h
  - the generated skeleton files (*.skel.h)
  - the samples/bpf/libbpf/ directory, recently introduced as an output
    directory for building libbpf and installing its headers.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 samples/bpf/.gitignore | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
index fcba217f0ae2..01f94ce79df8 100644
--- a/samples/bpf/.gitignore
+++ b/samples/bpf/.gitignore
@@ -57,3 +57,6 @@ testfile.img
 hbm_out.log
 iperf.*
 *.out
+*.skel.h
+vmlinux.h
+libbpf/
-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH bpf-next v2 9/9] selftests/bpf: better clean up for runqslower in test_bpftool_build.sh
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
                   ` (7 preceding siblings ...)
  2021-10-01 11:08 ` [PATCH bpf-next v2 8/9] samples/bpf: update .gitignore Quentin Monnet
@ 2021-10-01 11:08 ` Quentin Monnet
  2021-10-01 23:05 ` [PATCH bpf-next v2 0/9] install libbpf headers when using the library Andrii Nakryiko
  9 siblings, 0 replies; 25+ messages in thread
From: Quentin Monnet @ 2021-10-01 11:08 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Quentin Monnet

The script test_bpftool_build.sh attempts to build bpftool in the
various supported ways, to make sure nothing breaks.

One of those ways is to run "make tools/bpf" from the root of the kernel
repository. This command builds bpftool, along with the other tools
under tools/bpf, and runqslower in particular. After running the
command and upon a successful bpftool build, the script attempts to
cleanup the generated objects. However, after building with this target
and in the case of runqslower, the files are not cleaned up as expected.

This is because the "tools/bpf" target sets $(OUTPUT) to
.../tools/bpf/runqslower/ when building the tool, causing the object
files to be placed directly under the runqslower directory. But when
running "cd tools/bpf; make clean", the value for $(OUTPUT) is set to
".output" (relative to the runqslower directory) by runqslower's
Makefile, and this is where the Makefile looks for files to clean up.

We cannot easily fix in the root Makefile (where "tools/bpf" is defined)
or in tools/scripts/Makefile.include (setting $(OUTPUT)), where changing
the way the output variables are passed would likely have consequences
elsewhere. We could change runqslower's Makefile to build in the
repository instead of in a dedicated ".output/", but doing so just to
accommodate a test script doesn't sound great. Instead, let's just make
sure that we clean up runqslower properly by adding the correct command
to the script.

This will attempt to clean runqslower twice: the first try with command
"cd tools/bpf; make clean" will search for tools/bpf/runqslower/.output
and fail to clean it (but will still clean the other tools, in
particular bpftool), the second one (added in this commit) sets the
$(OUTPUT) variable like for building with the "tool/bpf" target and
should succeed.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/testing/selftests/bpf/test_bpftool_build.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_bpftool_build.sh b/tools/testing/selftests/bpf/test_bpftool_build.sh
index b03a87571592..1453a53ed547 100755
--- a/tools/testing/selftests/bpf/test_bpftool_build.sh
+++ b/tools/testing/selftests/bpf/test_bpftool_build.sh
@@ -90,6 +90,10 @@ echo -e "... through kbuild\n"
 
 if [ -f ".config" ] ; then
 	make_and_clean tools/bpf
+	## "make tools/bpf" sets $(OUTPUT) to ...tools/bpf/runqslower for
+	## runqslower, but the default (used for the "clean" target) is .output.
+	## Let's make sure we clean runqslower's directory properly.
+	make -C tools/bpf/runqslower OUTPUT=${KDIR_ROOT_DIR}/tools/bpf/runqslower/ clean
 
 	## $OUTPUT is overwritten in kbuild Makefile, and thus cannot be passed
 	## down from toplevel Makefile to bpftool's Makefile.
-- 
2.30.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 2/9] tools: bpftool: install libbpf headers instead of including the dir
  2021-10-01 11:08 ` [PATCH bpf-next v2 2/9] tools: bpftool: install libbpf headers instead of including the dir Quentin Monnet
@ 2021-10-01 22:55   ` Andrii Nakryiko
  0 siblings, 0 replies; 25+ messages in thread
From: Andrii Nakryiko @ 2021-10-01 22:55 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Bpftool relies on libbpf, therefore it relies on a number of headers
> from the library and must be linked against the library. The Makefile
> for bpftool exposes these objects by adding tools/lib as an include
> directory ("-I$(srctree)/tools/lib"). This is a working solution, but
> this is not the cleanest one. The risk is to involuntarily include
> objects that are not intended to be exposed by the libbpf.
>
> The headers needed to compile bpftool should in fact be "installed" from
> libbpf, with its "install_headers" Makefile target. In addition, there
> is one header which is internal to the library and not supposed to be
> used by external applications, but that bpftool uses anyway.
>
> Adjust the Makefile in order to install the header files properly before
> compiling bpftool. Also copy the additional internal header file
> (nlattr.h), but call it out explicitly. Build (and install headers) in a
> subdirectory under bpftool/ instead of tools/lib/bpf/. When descending
> from a parent Makefile, this is configurable by setting the OUTPUT,
> LIBBPF_OUTPUT and LIBBPF_DESTDIR variables.
>
> Also adjust the Makefile for BPF selftests, so as to reuse the (host)
> libbpf compiled earlier and to avoid compiling a separate version of the
> library just for bpftool.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/Makefile           | 27 ++++++++++++++++-----------
>  tools/testing/selftests/bpf/Makefile |  2 ++
>  2 files changed, 18 insertions(+), 11 deletions(-)
>

Looks good, but with Makefile no one can ever be sure :) Let's see how
this works in practice...

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 1fcf5b01a193..78e42963535a 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -17,16 +17,16 @@ endif
>  BPF_DIR = $(srctree)/tools/lib/bpf/

[...]

> +# We need to copy nlattr.h which is not otherwise exported by libbpf, but still
> +# required by bpftool.
>  $(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
> -       $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) $(LIBBPF_OUTPUT)libbpf.a
> +       $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) \
> +               DESTDIR=$(LIBBPF_DESTDIR) prefix= \
> +               $(LIBBPF_OUTPUT)libbpf.a install_headers

s/$(LIBBPF_OUTPUT)libbpf.a/$(LIBBPF)/ ?

> +       $(call QUIET_INSTALL, bpf/nlattr.h)
> +       $(Q)install -m 644 -t $(LIBBPF_INCLUDE)/bpf/ $(BPF_DIR)nlattr.h
>
>  $(LIBBPF_BOOTSTRAP): FORCE | $(LIBBPF_BOOTSTRAP_OUTPUT)
>         $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_BOOTSTRAP_OUTPUT) \

[...]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 3/9] tools: resolve_btfids: install libbpf headers when building
  2021-10-01 11:08 ` [PATCH bpf-next v2 3/9] tools: resolve_btfids: install libbpf headers when building Quentin Monnet
@ 2021-10-01 23:01   ` Andrii Nakryiko
  0 siblings, 0 replies; 25+ messages in thread
From: Andrii Nakryiko @ 2021-10-01 23:01 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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 resolve_btfids installs the
> headers properly when building.
>
> When descending from a parent Makefile, the specific output directories
> for building the library and exporting the headers are configurable with
> LIBBPF_OUT and LIBBPF_DESTDIR, respectively. This is in addition to
> OUTPUT, on top of which those variables are constructed by default.
>
> Also adjust the Makefile for the BPF selftests in order to point to the
> (target) libbpf shared with other tools, instead of building a version
> specific to resolve_btfids.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/resolve_btfids/Makefile    | 17 ++++++++++++-----
>  tools/bpf/resolve_btfids/main.c      |  4 ++--
>  tools/testing/selftests/bpf/Makefile |  7 +++++--
>  3 files changed, 19 insertions(+), 9 deletions(-)
>

Acked-by: Andrii Nakryiko <andrii@kernel.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 0/9] install libbpf headers when using the library
  2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
                   ` (8 preceding siblings ...)
  2021-10-01 11:08 ` [PATCH bpf-next v2 9/9] selftests/bpf: better clean up for runqslower in test_bpftool_build.sh Quentin Monnet
@ 2021-10-01 23:05 ` Andrii Nakryiko
  2021-10-01 23:27   ` Andrii Nakryiko
  2021-10-02 20:40   ` Quentin Monnet
  9 siblings, 2 replies; 25+ messages in thread
From: Andrii Nakryiko @ 2021-10-01 23:05 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Libbpf is used at several locations in the repository. Most of the time,
> the tools relying on it build the library in its own directory, and include
> the headers from there. This works, but this is not the cleanest approach.
> It generates objects outside of the directory of the tool which is being
> built, and it also increases the risk that developers include a header file
> internal to libbpf, which is not supposed to be exposed to user
> applications.
>
> This set adjusts all involved Makefiles to make sure that libbpf is built
> locally (with respect to the tool's directory or provided build directory),
> and by ensuring that "make install_headers" is run from libbpf's Makefile
> to export user headers properly.
>
> This comes at a cost: given that the libbpf was so far mostly compiled in
> its own directory by the different components using it, compiling it once
> would be enough for all those components. With the new approach, each
> component compiles its own version. To mitigate this cost, efforts were
> made to reuse the compiled library when possible:
>
> - Make the bpftool version in samples/bpf reuse the library previously
>   compiled for the selftests.
> - Make the bpftool version in BPF selftests reuse the library previously
>   compiled for the selftests.
> - Similarly, make resolve_btfids in BPF selftests reuse the same compiled
>   library.
> - Similarly, make runqslower in BPF selftests reuse the same compiled
>   library; and make it rely on the bpftool version also compiled from the
>   selftests (instead of compiling its own version).
> - runqslower, when compiled independently, needs its own version of
>   bpftool: make them share the same compiled libbpf.
>
> As a result:
>
> - Compiling the samples/bpf should compile libbpf just once.
> - Compiling the BPF selftests should compile libbpf just once.
> - Compiling the kernel (with BTF support) should now lead to compiling
>   libbpf twice: one for resolve_btfids, one for kernel/bpf/preload.
> - Compiling runqslower individually should compile libbpf just once. Same
>   thing for bpftool, resolve_btfids, and kernel/bpf/preload/iterators.

The whole sharing of libbpf build artifacts is great, I just want to
point out that it's also dangerous if those multiple Makefiles aren't
ordered properly. E.g., if you build runqslower and the rest of
selftests in parallel without making sure that libbpf already
completed its build, you might end up building libbpf in parallel in
two independent make instances and subsequently corrupting generated
object files. I haven't looked through all the changes (and I'll
confess that it's super hard to reason about dependencies and ordering
in Makefile) and I'll keep this in mind, but wanted to bring this up.
I suspect you already thought about that, but would be worth to call
out this explicitly.

>
> (Not accounting for the boostrap version of libbpf required by bpftool,
> which was already placed under a dedicated .../boostrap/libbpf/ directory,
> and for which the count remains unchanged.)
>
> A few commits in the series also contain drive-by clean-up changes for
> bpftool includes, samples/bpf/.gitignore, or test_bpftool_build.sh. Please
> refer to individual commit logs for details.
>
> v2: Declare an additional dependency on libbpf's headers for
>     iterators/iterators.o in kernel/preload/Makefile to make sure that
>     these headers are exported before we compile the object file (and not
>     just before we link it).
>
> Quentin Monnet (9):
>   tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h>
>   tools: bpftool: install libbpf headers instead of including the dir
>   tools: resolve_btfids: install libbpf headers when building
>   tools: runqslower: install libbpf headers when building
>   bpf: preload: install libbpf headers when building
>   bpf: iterators: install libbpf headers when building
>   samples/bpf: install libbpf headers when building
>   samples/bpf: update .gitignore
>   selftests/bpf: better clean up for runqslower in test_bpftool_build.sh
>
>  kernel/bpf/preload/Makefile                   | 25 ++++++++++---
>  kernel/bpf/preload/iterators/Makefile         | 18 ++++++----
>  samples/bpf/.gitignore                        |  3 ++
>  samples/bpf/Makefile                          | 36 +++++++++++++------
>  tools/bpf/bpftool/Makefile                    | 27 ++++++++------
>  tools/bpf/bpftool/gen.c                       |  1 -
>  tools/bpf/bpftool/prog.c                      |  1 -
>  tools/bpf/resolve_btfids/Makefile             | 17 ++++++---
>  tools/bpf/resolve_btfids/main.c               |  4 +--
>  tools/bpf/runqslower/Makefile                 | 12 ++++---
>  tools/testing/selftests/bpf/Makefile          | 22 ++++++++----
>  .../selftests/bpf/test_bpftool_build.sh       |  4 +++
>  12 files changed, 116 insertions(+), 54 deletions(-)
>
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building
  2021-10-01 11:08 ` [PATCH bpf-next v2 6/9] bpf: iterators: " Quentin Monnet
@ 2021-10-01 23:19   ` Andrii Nakryiko
  2021-10-02 20:27     ` Quentin Monnet
  0 siblings, 1 reply; 25+ messages in thread
From: Andrii Nakryiko @ 2021-10-01 23:19 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 7/9] samples/bpf: install libbpf headers when building
  2021-10-01 11:08 ` [PATCH bpf-next v2 7/9] samples/bpf: " Quentin Monnet
@ 2021-10-01 23:22   ` Andrii Nakryiko
  2021-10-02 20:30     ` Quentin Monnet
  0 siblings, 1 reply; 25+ messages in thread
From: Andrii Nakryiko @ 2021-10-01 23:22 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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 source
> directory. Instead, they should be exported with "make install_headers".
> Make sure that samples/bpf/Makefile installs the headers properly when
> building.
>
> The object compiled from and exported by libbpf are now placed into a
> subdirectory of sample/bpf/ instead of remaining in tools/lib/bpf/. We
> attempt to remove this directory on "make clean". However, the "clean"
> target re-enters the samples/bpf/ directory from the root of the
> repository ("$(MAKE) -C ../../ M=$(CURDIR) clean"), in such a way that
> $(srctree) and $(src) are not defined, making it impossible to use
> $(LIBBPF_OUTPUT) and $(LIBBPF_DESTDIR) in the recipe. So we only attempt
> to clean $(CURDIR)/libbpf, which is the default value.
>
> We also change the output directory for bpftool, to place the generated
> objects under samples/bpf/bpftool/ instead of building in bpftool's
> directory directly. Doing so, we make sure bpftool reuses the libbpf
> library previously compiled and installed.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  samples/bpf/Makefile | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 4dc20be5fb96..7de602c2c705 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -59,7 +59,11 @@ tprogs-y += xdp_redirect
>  tprogs-y += xdp_monitor
>
>  # Libbpf dependencies
> -LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
> +LIBBPF_SRC = $(TOOLS_PATH)/lib/bpf
> +LIBBPF_OUTPUT = $(abspath $(BPF_SAMPLES_PATH))/libbpf
> +LIBBPF_DESTDIR = $(LIBBPF_OUTPUT)
> +LIBBPF_INCLUDE = $(LIBBPF_DESTDIR)/include
> +LIBBPF = $(LIBBPF_OUTPUT)/libbpf.a
>
>  CGROUP_HELPERS := ../../tools/testing/selftests/bpf/cgroup_helpers.o
>  TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
> @@ -198,7 +202,7 @@ TPROGS_CFLAGS += -Wstrict-prototypes
>
>  TPROGS_CFLAGS += -I$(objtree)/usr/include
>  TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
> -TPROGS_CFLAGS += -I$(srctree)/tools/lib/
> +TPROGS_CFLAGS += -I$(LIBBPF_INCLUDE)
>  TPROGS_CFLAGS += -I$(srctree)/tools/include
>  TPROGS_CFLAGS += -I$(srctree)/tools/perf
>  TPROGS_CFLAGS += -DHAVE_ATTR_TEST=0
> @@ -268,16 +272,28 @@ all:
>  clean:
>         $(MAKE) -C ../../ M=$(CURDIR) clean
>         @find $(CURDIR) -type f -name '*~' -delete
> +       @/bin/rm -rf $(CURDIR)/libbpf $(CURDIR)/bpftool

should this be $(RM) -rf ? I've seen other makefiles don't hardcode
/bin/rm. And also below we are passing RM='rm -rf', not /bin/rm.
Inconsistent :)

>
> -$(LIBBPF): FORCE
> +$(LIBBPF): FORCE | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE)
>  # Fix up variables inherited from Kbuild that tools/ build system won't like
> -       $(MAKE) -C $(dir $@) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)" \
> -               LDFLAGS=$(TPROGS_LDFLAGS) srctree=$(BPF_SAMPLES_PATH)/../../ O=
> +       $(MAKE) -C $(LIBBPF_SRC) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)" \
> +               LDFLAGS=$(TPROGS_LDFLAGS) srctree=$(BPF_SAMPLES_PATH)/../../ \
> +               O= OUTPUT=$(LIBBPF_OUTPUT)/ DESTDIR=$(LIBBPF_DESTDIR) prefix= \
> +               $@ install_headers
>
>  BPFTOOLDIR := $(TOOLS_PATH)/bpf/bpftool
> -BPFTOOL := $(BPFTOOLDIR)/bpftool
> -$(BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)
> -           $(MAKE) -C $(BPFTOOLDIR) srctree=$(BPF_SAMPLES_PATH)/../../
> +BPFTOOL_OUTPUT := $(abspath $(BPF_SAMPLES_PATH))/bpftool
> +BPFTOOL := $(BPFTOOL_OUTPUT)/bpftool
> +$(BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \
> +           | $(BPFTOOL_OUTPUT)
> +           $(MAKE) -C $(BPFTOOLDIR) srctree=$(BPF_SAMPLES_PATH)/../../ \
> +               OUTPUT=$(BPFTOOL_OUTPUT)/ \
> +               LIBBPF_OUTPUT=$(LIBBPF_OUTPUT) \
> +               LIBBPF_DESTDIR=$(LIBBPF_DESTDIR)
> +
> +$(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) $(BPFTOOL_OUTPUT):
> +       $(call msg,MKDIR,$@)
> +       $(Q)mkdir -p $@
>
>  $(obj)/syscall_nrs.h:  $(obj)/syscall_nrs.s FORCE
>         $(call filechk,offsets,__SYSCALL_NRS_H__)
> @@ -367,7 +383,7 @@ $(obj)/%.bpf.o: $(src)/%.bpf.c $(obj)/vmlinux.h $(src)/xdp_sample.bpf.h $(src)/x
>         $(Q)$(CLANG) -g -O2 -target bpf -D__TARGET_ARCH_$(SRCARCH) \
>                 -Wno-compare-distinct-pointer-types -I$(srctree)/include \
>                 -I$(srctree)/samples/bpf -I$(srctree)/tools/include \
> -               -I$(srctree)/tools/lib $(CLANG_SYS_INCLUDES) \
> +               -I$(LIBBPF_INCLUDE) $(CLANG_SYS_INCLUDES) \
>                 -c $(filter %.bpf.c,$^) -o $@
>
>  LINKED_SKELS := xdp_redirect_cpu.skel.h xdp_redirect_map_multi.skel.h \
> @@ -404,7 +420,7 @@ $(obj)/%.o: $(src)/%.c
>         @echo "  CLANG-bpf " $@
>         $(Q)$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(BPF_EXTRA_CFLAGS) \
>                 -I$(obj) -I$(srctree)/tools/testing/selftests/bpf/ \
> -               -I$(srctree)/tools/lib/ \
> +               -I$(LIBBPF_INCLUDE) \
>                 -D__KERNEL__ -D__BPF_TRACING__ -Wno-unused-value -Wno-pointer-sign \
>                 -D__TARGET_ARCH_$(SRCARCH) -Wno-compare-distinct-pointer-types \
>                 -Wno-gnu-variable-sized-type-not-at-end \
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 0/9] install libbpf headers when using the library
  2021-10-01 23:05 ` [PATCH bpf-next v2 0/9] install libbpf headers when using the library Andrii Nakryiko
@ 2021-10-01 23:27   ` Andrii Nakryiko
  2021-10-02 20:40   ` Quentin Monnet
  1 sibling, 0 replies; 25+ messages in thread
From: Andrii Nakryiko @ 2021-10-01 23:27 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Fri, Oct 1, 2021 at 4:05 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >
> > Libbpf is used at several locations in the repository. Most of the time,
> > the tools relying on it build the library in its own directory, and include
> > the headers from there. This works, but this is not the cleanest approach.
> > It generates objects outside of the directory of the tool which is being
> > built, and it also increases the risk that developers include a header file
> > internal to libbpf, which is not supposed to be exposed to user
> > applications.
> >
> > This set adjusts all involved Makefiles to make sure that libbpf is built
> > locally (with respect to the tool's directory or provided build directory),
> > and by ensuring that "make install_headers" is run from libbpf's Makefile
> > to export user headers properly.
> >
> > This comes at a cost: given that the libbpf was so far mostly compiled in
> > its own directory by the different components using it, compiling it once
> > would be enough for all those components. With the new approach, each
> > component compiles its own version. To mitigate this cost, efforts were
> > made to reuse the compiled library when possible:
> >
> > - Make the bpftool version in samples/bpf reuse the library previously
> >   compiled for the selftests.
> > - Make the bpftool version in BPF selftests reuse the library previously
> >   compiled for the selftests.
> > - Similarly, make resolve_btfids in BPF selftests reuse the same compiled
> >   library.
> > - Similarly, make runqslower in BPF selftests reuse the same compiled
> >   library; and make it rely on the bpftool version also compiled from the
> >   selftests (instead of compiling its own version).
> > - runqslower, when compiled independently, needs its own version of
> >   bpftool: make them share the same compiled libbpf.
> >
> > As a result:
> >
> > - Compiling the samples/bpf should compile libbpf just once.
> > - Compiling the BPF selftests should compile libbpf just once.
> > - Compiling the kernel (with BTF support) should now lead to compiling
> >   libbpf twice: one for resolve_btfids, one for kernel/bpf/preload.
> > - Compiling runqslower individually should compile libbpf just once. Same
> >   thing for bpftool, resolve_btfids, and kernel/bpf/preload/iterators.
>
> The whole sharing of libbpf build artifacts is great, I just want to
> point out that it's also dangerous if those multiple Makefiles aren't
> ordered properly. E.g., if you build runqslower and the rest of
> selftests in parallel without making sure that libbpf already
> completed its build, you might end up building libbpf in parallel in
> two independent make instances and subsequently corrupting generated
> object files. I haven't looked through all the changes (and I'll
> confess that it's super hard to reason about dependencies and ordering
> in Makefile) and I'll keep this in mind, but wanted to bring this up.
> I suspect you already thought about that, but would be worth to call
> out this explicitly.
>

Ok, I looked through the patches. It all looks reasonable to me, but
as I mentioned, Makefile is pure magic and human brain can't keep
track of all those dependencies and their ordering. I tried these
changes locally and so far all the builds I usually do work fine. But
it would be great if a few more folks try these changes locally for
their normal workflows to help test this. If not, we can land this
early next week, so that we have a whole week in front of us to fix
whatever fallout there will be due to these changes.

Sounds good?


> >
> > (Not accounting for the boostrap version of libbpf required by bpftool,
> > which was already placed under a dedicated .../boostrap/libbpf/ directory,
> > and for which the count remains unchanged.)
> >
> > A few commits in the series also contain drive-by clean-up changes for
> > bpftool includes, samples/bpf/.gitignore, or test_bpftool_build.sh. Please
> > refer to individual commit logs for details.
> >
> > v2: Declare an additional dependency on libbpf's headers for
> >     iterators/iterators.o in kernel/preload/Makefile to make sure that
> >     these headers are exported before we compile the object file (and not
> >     just before we link it).
> >
> > Quentin Monnet (9):
> >   tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h>
> >   tools: bpftool: install libbpf headers instead of including the dir
> >   tools: resolve_btfids: install libbpf headers when building
> >   tools: runqslower: install libbpf headers when building
> >   bpf: preload: install libbpf headers when building
> >   bpf: iterators: install libbpf headers when building
> >   samples/bpf: install libbpf headers when building
> >   samples/bpf: update .gitignore
> >   selftests/bpf: better clean up for runqslower in test_bpftool_build.sh
> >
> >  kernel/bpf/preload/Makefile                   | 25 ++++++++++---
> >  kernel/bpf/preload/iterators/Makefile         | 18 ++++++----
> >  samples/bpf/.gitignore                        |  3 ++
> >  samples/bpf/Makefile                          | 36 +++++++++++++------
> >  tools/bpf/bpftool/Makefile                    | 27 ++++++++------
> >  tools/bpf/bpftool/gen.c                       |  1 -
> >  tools/bpf/bpftool/prog.c                      |  1 -
> >  tools/bpf/resolve_btfids/Makefile             | 17 ++++++---
> >  tools/bpf/resolve_btfids/main.c               |  4 +--
> >  tools/bpf/runqslower/Makefile                 | 12 ++++---
> >  tools/testing/selftests/bpf/Makefile          | 22 ++++++++----
> >  .../selftests/bpf/test_bpftool_build.sh       |  4 +++
> >  12 files changed, 116 insertions(+), 54 deletions(-)
> >
> > --
> > 2.30.2
> >

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building
  2021-10-01 23:19   ` Andrii Nakryiko
@ 2021-10-02 20:27     ` Quentin Monnet
  2021-10-02 22:11       ` Quentin Monnet
  0 siblings, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-02 20:27 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 7/9] samples/bpf: install libbpf headers when building
  2021-10-01 23:22   ` Andrii Nakryiko
@ 2021-10-02 20:30     ` Quentin Monnet
  0 siblings, 0 replies; 25+ messages in thread
From: Quentin Monnet @ 2021-10-02 20:30 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Sat, 2 Oct 2021 at 00:22, 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 source
> > directory. Instead, they should be exported with "make install_headers".
> > Make sure that samples/bpf/Makefile installs the headers properly when
> > building.
> >
> > The object compiled from and exported by libbpf are now placed into a
> > subdirectory of sample/bpf/ instead of remaining in tools/lib/bpf/. We
> > attempt to remove this directory on "make clean". However, the "clean"
> > target re-enters the samples/bpf/ directory from the root of the
> > repository ("$(MAKE) -C ../../ M=$(CURDIR) clean"), in such a way that
> > $(srctree) and $(src) are not defined, making it impossible to use
> > $(LIBBPF_OUTPUT) and $(LIBBPF_DESTDIR) in the recipe. So we only attempt
> > to clean $(CURDIR)/libbpf, which is the default value.
> >
> > We also change the output directory for bpftool, to place the generated
> > objects under samples/bpf/bpftool/ instead of building in bpftool's
> > directory directly. Doing so, we make sure bpftool reuses the libbpf
> > library previously compiled and installed.

> > @@ -268,16 +272,28 @@ all:
> >  clean:
> >         $(MAKE) -C ../../ M=$(CURDIR) clean
> >         @find $(CURDIR) -type f -name '*~' -delete
> > +       @/bin/rm -rf $(CURDIR)/libbpf $(CURDIR)/bpftool
>
> should this be $(RM) -rf ? I've seen other makefiles don't hardcode
> /bin/rm. And also below we are passing RM='rm -rf', not /bin/rm.
> Inconsistent :)

Inconsistent?! But I took it just a few lines above, from
BTF_LLVM_PROBE! :) But I also prefer "$(RM) -r" (the -f is included in
"$(RM)"), so sure, I'll address.
Quentin

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 0/9] install libbpf headers when using the library
  2021-10-01 23:05 ` [PATCH bpf-next v2 0/9] install libbpf headers when using the library Andrii Nakryiko
  2021-10-01 23:27   ` Andrii Nakryiko
@ 2021-10-02 20:40   ` Quentin Monnet
  2021-10-03 19:20     ` Quentin Monnet
  1 sibling, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-02 20:40 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

On Sat, 2 Oct 2021 at 00:05, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >
> > Libbpf is used at several locations in the repository. Most of the time,
> > the tools relying on it build the library in its own directory, and include
> > the headers from there. This works, but this is not the cleanest approach.
> > It generates objects outside of the directory of the tool which is being
> > built, and it also increases the risk that developers include a header file
> > internal to libbpf, which is not supposed to be exposed to user
> > applications.
> >
> > This set adjusts all involved Makefiles to make sure that libbpf is built
> > locally (with respect to the tool's directory or provided build directory),
> > and by ensuring that "make install_headers" is run from libbpf's Makefile
> > to export user headers properly.
> >
> > This comes at a cost: given that the libbpf was so far mostly compiled in
> > its own directory by the different components using it, compiling it once
> > would be enough for all those components. With the new approach, each
> > component compiles its own version. To mitigate this cost, efforts were
> > made to reuse the compiled library when possible:
> >
> > - Make the bpftool version in samples/bpf reuse the library previously
> >   compiled for the selftests.
> > - Make the bpftool version in BPF selftests reuse the library previously
> >   compiled for the selftests.
> > - Similarly, make resolve_btfids in BPF selftests reuse the same compiled
> >   library.
> > - Similarly, make runqslower in BPF selftests reuse the same compiled
> >   library; and make it rely on the bpftool version also compiled from the
> >   selftests (instead of compiling its own version).
> > - runqslower, when compiled independently, needs its own version of
> >   bpftool: make them share the same compiled libbpf.
> >
> > As a result:
> >
> > - Compiling the samples/bpf should compile libbpf just once.
> > - Compiling the BPF selftests should compile libbpf just once.
> > - Compiling the kernel (with BTF support) should now lead to compiling
> >   libbpf twice: one for resolve_btfids, one for kernel/bpf/preload.
> > - Compiling runqslower individually should compile libbpf just once. Same
> >   thing for bpftool, resolve_btfids, and kernel/bpf/preload/iterators.
>
> The whole sharing of libbpf build artifacts is great, I just want to
> point out that it's also dangerous if those multiple Makefiles aren't
> ordered properly. E.g., if you build runqslower and the rest of
> selftests in parallel without making sure that libbpf already
> completed its build, you might end up building libbpf in parallel in
> two independent make instances and subsequently corrupting generated
> object files. I haven't looked through all the changes (and I'll
> confess that it's super hard to reason about dependencies and ordering
> in Makefile) and I'll keep this in mind, but wanted to bring this up.

I'm not sure how Makefile handles this exactly, I don't know if it can
possibly build the two in parallel or if it's smart enough to realise
that the libbpf.a is the same object in both cases and should be built
only once. Same as you, I didn't hit any issue of this kind when
testing the patches.

> I suspect you already thought about that, but would be worth to call
> out this explicitly.

Ok, how would you like me to mention it? Comments in the Makefiles for
runqslower, the samples, and the selftests?

I'll post a new version addressing this, your other comments, and an
issue I found for the samples/bpf/ while doing more tests.

Thanks for the review and testing!
Quentin

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building
  2021-10-02 20:27     ` Quentin Monnet
@ 2021-10-02 22:11       ` Quentin Monnet
  2021-10-04 19:10         ` Andrii Nakryiko
  0 siblings, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-02 22:11 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 0/9] install libbpf headers when using the library
  2021-10-02 20:40   ` Quentin Monnet
@ 2021-10-03 19:20     ` Quentin Monnet
  0 siblings, 0 replies; 25+ messages in thread
From: Quentin Monnet @ 2021-10-03 19:20 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

2021-10-02 21:40 UTC+0100 ~ Quentin Monnet <quentin@isovalent.com>
> On Sat, 2 Oct 2021 at 00:05, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>
>> On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>
>>> Libbpf is used at several locations in the repository. Most of the time,
>>> the tools relying on it build the library in its own directory, and include
>>> the headers from there. This works, but this is not the cleanest approach.
>>> It generates objects outside of the directory of the tool which is being
>>> built, and it also increases the risk that developers include a header file
>>> internal to libbpf, which is not supposed to be exposed to user
>>> applications.
>>>
>>> This set adjusts all involved Makefiles to make sure that libbpf is built
>>> locally (with respect to the tool's directory or provided build directory),
>>> and by ensuring that "make install_headers" is run from libbpf's Makefile
>>> to export user headers properly.
>>>
>>> This comes at a cost: given that the libbpf was so far mostly compiled in
>>> its own directory by the different components using it, compiling it once
>>> would be enough for all those components. With the new approach, each
>>> component compiles its own version. To mitigate this cost, efforts were
>>> made to reuse the compiled library when possible:
>>>
>>> - Make the bpftool version in samples/bpf reuse the library previously
>>>   compiled for the selftests.
>>> - Make the bpftool version in BPF selftests reuse the library previously
>>>   compiled for the selftests.
>>> - Similarly, make resolve_btfids in BPF selftests reuse the same compiled
>>>   library.
>>> - Similarly, make runqslower in BPF selftests reuse the same compiled
>>>   library; and make it rely on the bpftool version also compiled from the
>>>   selftests (instead of compiling its own version).
>>> - runqslower, when compiled independently, needs its own version of
>>>   bpftool: make them share the same compiled libbpf.
>>>
>>> As a result:
>>>
>>> - Compiling the samples/bpf should compile libbpf just once.
>>> - Compiling the BPF selftests should compile libbpf just once.
>>> - Compiling the kernel (with BTF support) should now lead to compiling
>>>   libbpf twice: one for resolve_btfids, one for kernel/bpf/preload.
>>> - Compiling runqslower individually should compile libbpf just once. Same
>>>   thing for bpftool, resolve_btfids, and kernel/bpf/preload/iterators.
>>
>> The whole sharing of libbpf build artifacts is great, I just want to
>> point out that it's also dangerous if those multiple Makefiles aren't
>> ordered properly. E.g., if you build runqslower and the rest of
>> selftests in parallel without making sure that libbpf already
>> completed its build, you might end up building libbpf in parallel in
>> two independent make instances and subsequently corrupting generated
>> object files. I haven't looked through all the changes (and I'll
>> confess that it's super hard to reason about dependencies and ordering
>> in Makefile) and I'll keep this in mind, but wanted to bring this up.
> 
> I'm not sure how Makefile handles this exactly, I don't know if it can
> possibly build the two in parallel or if it's smart enough to realise
> that the libbpf.a is the same object in both cases and should be built
> only once. Same as you, I didn't hit any issue of this kind when
> testing the patches.

Testing further with make, I don't think it's smart enough to avoid
building the object twice in the same directory. This probably didn't
show in practice because the parallel build tends to build the different
object files in parallel rather than libbpf and bpftool in parallel. But
to remain on the safe side, I'll just add a dependency on libbpf for
bpftool/runqslower etc. to make sure they are not all built together
(most locations had the dependency already). v3 is coming.

Quentin


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building
  2021-10-02 22:11       ` Quentin Monnet
@ 2021-10-04 19:10         ` Andrii Nakryiko
  2021-10-04 21:30           ` Quentin Monnet
  0 siblings, 1 reply; 25+ messages in thread
From: Andrii Nakryiko @ 2021-10-04 19:10 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building
  2021-10-04 19:10         ` Andrii Nakryiko
@ 2021-10-04 21:30           ` Quentin Monnet
  2021-10-05 20:03             ` Quentin Monnet
  0 siblings, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-04 21:30 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building
  2021-10-04 21:30           ` Quentin Monnet
@ 2021-10-05 20:03             ` Quentin Monnet
  2021-10-06 17:44               ` Andrii Nakryiko
  0 siblings, 1 reply; 25+ messages in thread
From: Quentin Monnet @ 2021-10-05 20:03 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building
  2021-10-05 20:03             ` Quentin Monnet
@ 2021-10-06 17:44               ` Andrii Nakryiko
  0 siblings, 0 replies; 25+ messages in thread
From: Andrii Nakryiko @ 2021-10-06 17:44 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking, bpf

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-10-06 17:45 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet
2021-10-01 11:08 ` [PATCH bpf-next v2 1/9] tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h> Quentin Monnet
2021-10-01 11:08 ` [PATCH bpf-next v2 2/9] tools: bpftool: install libbpf headers instead of including the dir Quentin Monnet
2021-10-01 22:55   ` Andrii Nakryiko
2021-10-01 11:08 ` [PATCH bpf-next v2 3/9] tools: resolve_btfids: install libbpf headers when building Quentin Monnet
2021-10-01 23:01   ` Andrii Nakryiko
2021-10-01 11:08 ` [PATCH bpf-next v2 4/9] tools: runqslower: " Quentin Monnet
2021-10-01 11:08 ` [PATCH bpf-next v2 5/9] bpf: preload: " Quentin Monnet
2021-10-01 11:08 ` [PATCH bpf-next v2 6/9] bpf: iterators: " Quentin Monnet
2021-10-01 23:19   ` Andrii Nakryiko
2021-10-02 20:27     ` Quentin Monnet
2021-10-02 22:11       ` Quentin Monnet
2021-10-04 19:10         ` Andrii Nakryiko
2021-10-04 21:30           ` Quentin Monnet
2021-10-05 20:03             ` Quentin Monnet
2021-10-06 17:44               ` Andrii Nakryiko
2021-10-01 11:08 ` [PATCH bpf-next v2 7/9] samples/bpf: " Quentin Monnet
2021-10-01 23:22   ` Andrii Nakryiko
2021-10-02 20:30     ` Quentin Monnet
2021-10-01 11:08 ` [PATCH bpf-next v2 8/9] samples/bpf: update .gitignore Quentin Monnet
2021-10-01 11:08 ` [PATCH bpf-next v2 9/9] selftests/bpf: better clean up for runqslower in test_bpftool_build.sh Quentin Monnet
2021-10-01 23:05 ` [PATCH bpf-next v2 0/9] install libbpf headers when using the library Andrii Nakryiko
2021-10-01 23:27   ` Andrii Nakryiko
2021-10-02 20:40   ` Quentin Monnet
2021-10-03 19:20     ` Quentin Monnet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).