All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs
@ 2021-11-08  6:13 Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 01/11] bpftool: normalize compile rules to specify output file last Andrii Nakryiko
                   ` (10 more replies)
  0 siblings, 11 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

This patch set continues the work of revamping libbpf APIs that are not
extensible, as they were added before we figured out all the intricacies of
building APIs that can preserve ABI compatibility (both backward and forward).

What makes them tricky is that (most of) these APIs are actively used by
multiple applications, so we need to be careful about refactoring them. See
individual patches for details, but the general approach is similar to
previous bpf_prog_load() API revamp. The biggest different and complexity is
in changing btf_dump__new(), because function overloading through macro magic
doesn't work based on number of arguments, as both new and old APIs have
4 arguments. Because of that, another overloading approach is taken; overload
happens based on argument types.

To validate that not just source code compatibility is preserved, but that
applications using libbpf as a shared library stay compatible as well, first
few patches extend selftests/bpf with another flavor of test_progs (-shared),
that compiles against libbpf.so. Using this flavor I've validated manually
that compiling "old application" (selftests before being adapted to using new
variants of revamped APIs) are compiled and successfully run against newest
libbpf version as well as the older libbpf version (provided no new variants
are used). All these scenarios seem to be working as expected.

Currently, the way that selftests are set up we'll build three variants of BPF
object files (alu32, no-alu32, and alu32 but for shared flavor), which is
suboptimal. It is possible to rework Makefile to only generate alu32 vs
no-alu32 variants and use them in appropriate flavors, minimizing unnecessary
work. But that was left for follow up patches as there are already a lot of
changes across selftests and libbpf.

Andrii Nakryiko (11):
  bpftool: normalize compile rules to specify output file last
  selftests/bpf: minor cleanups and normalization of Makefile
  selftests/bpf: allow to generate per-flavor list of tests
  selftests/bpf: add test_progs flavor using libbpf as a shared lib
  libbpf: turn btf_dedup_opts into OPTS-based struct
  libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof
  libbpf: make perf_buffer__new() use OPTS-based interface
  selftests/bpf: migrate all deprecated perf_buffer uses
  selftests/bpf: update btf_dump__new() uses to v1.0+ variant
  tools/runqslower: update perf_buffer__new() calls
  bpftool: update btf_dump__new() and perf_buffer__new_raw() calls

 tools/bpf/bpftool/Makefile                    | 16 ++--
 tools/bpf/bpftool/btf.c                       |  2 +-
 tools/bpf/bpftool/gen.c                       |  2 +-
 tools/bpf/bpftool/map_perf_ring.c             |  9 +-
 tools/bpf/runqslower/runqslower.c             |  6 +-
 tools/lib/bpf/btf.c                           | 46 +++++----
 tools/lib/bpf/btf.h                           | 72 ++++++++++++--
 tools/lib/bpf/btf_dump.c                      | 26 ++++--
 tools/lib/bpf/libbpf.c                        | 70 +++++++++++---
 tools/lib/bpf/libbpf.h                        | 86 ++++++++++++++---
 tools/lib/bpf/libbpf.map                      |  8 ++
 tools/lib/bpf/linker.c                        |  4 +-
 tools/testing/selftests/bpf/.gitignore        |  9 +-
 tools/testing/selftests/bpf/Makefile          | 93 +++++++++++--------
 .../selftests/bpf/benchs/bench_ringbufs.c     |  8 +-
 tools/testing/selftests/bpf/btf_helpers.c     |  4 +-
 tools/testing/selftests/bpf/prog_tests/btf.c  | 46 ++-------
 .../bpf/prog_tests/btf_dedup_split.c          |  6 +-
 .../selftests/bpf/prog_tests/btf_dump.c       | 33 +++----
 .../selftests/bpf/prog_tests/btf_split.c      |  4 +-
 .../bpf/prog_tests/get_stack_raw_tp.c         |  5 +-
 .../selftests/bpf/prog_tests/kfree_skb.c      |  6 +-
 .../selftests/bpf/prog_tests/perf_buffer.c    |  6 +-
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    |  7 +-
 tools/testing/selftests/bpf/test_maps.c       |  4 +-
 tools/testing/selftests/bpf/test_progs.c      |  6 +-
 .../selftests/bpf/test_tcpnotify_user.c       |  4 +-
 27 files changed, 363 insertions(+), 225 deletions(-)

-- 
2.30.2


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

* [PATCH bpf-next 01/11] bpftool: normalize compile rules to specify output file last
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 02/11] selftests/bpf: minor cleanups and normalization of Makefile Andrii Nakryiko
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

When dealing with verbose Makefile output, it's extremely confusing when
compiler invocation commands don't specify -o <output.o> as the last
argument. Normalize bpftool's Makefile to do just that, as most other
BPF-related Makefiles are already doing that.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/bpftool/Makefile | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index c0c30e56988f..b9515bdc836e 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -177,7 +177,8 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF)
 		-I$(if $(OUTPUT),$(OUTPUT),.) \
 		-I$(srctree)/tools/include/uapi/ \
 		-I$(LIBBPF_INCLUDE) \
-		-g -O2 -Wall -target bpf -c $< -o $@ && $(LLVM_STRIP) -g $@
+		-g -O2 -Wall -target bpf -c $< -o $@
+	$(Q)$(LLVM_STRIP) -g $@
 
 $(OUTPUT)%.skel.h: $(OUTPUT)%.bpf.o $(BPFTOOL_BOOTSTRAP)
 	$(QUIET_GEN)$(BPFTOOL_BOOTSTRAP) gen skeleton $< > $@
@@ -192,10 +193,10 @@ endif
 CFLAGS += $(if $(BUILD_BPF_SKELS),,-DBPFTOOL_WITHOUT_SKELETONS)
 
 $(BOOTSTRAP_OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
-	$(QUIET_CC)$(HOSTCC) $(CFLAGS) -c -MMD -o $@ $<
+	$(QUIET_CC)$(HOSTCC) $(CFLAGS) -c -MMD $< -o $@
 
 $(OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
-	$(QUIET_CC)$(CC) $(CFLAGS) -c -MMD -o $@ $<
+	$(QUIET_CC)$(CC) $(CFLAGS) -c -MMD $< -o $@
 
 $(OUTPUT)feature.o:
 ifneq ($(feature-zlib), 1)
@@ -203,17 +204,16 @@ ifneq ($(feature-zlib), 1)
 endif
 
 $(BPFTOOL_BOOTSTRAP): $(BOOTSTRAP_OBJS) $(LIBBPF_BOOTSTRAP)
-	$(QUIET_LINK)$(HOSTCC) $(CFLAGS) $(LDFLAGS) -o $@ $(BOOTSTRAP_OBJS) \
-		$(LIBS_BOOTSTRAP)
+	$(QUIET_LINK)$(HOSTCC) $(CFLAGS) $(LDFLAGS) $(BOOTSTRAP_OBJS) $(LIBS_BOOTSTRAP) -o $@
 
 $(OUTPUT)bpftool: $(OBJS) $(LIBBPF)
-	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(OBJS) $(LIBS)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $(OBJS) $(LIBS) -o $@
 
 $(BOOTSTRAP_OUTPUT)%.o: %.c $(LIBBPF_INTERNAL_HDRS) | $(BOOTSTRAP_OUTPUT)
-	$(QUIET_CC)$(HOSTCC) $(CFLAGS) -c -MMD -o $@ $<
+	$(QUIET_CC)$(HOSTCC) $(CFLAGS) -c -MMD $< -o $@
 
 $(OUTPUT)%.o: %.c
-	$(QUIET_CC)$(CC) $(CFLAGS) -c -MMD -o $@ $<
+	$(QUIET_CC)$(CC) $(CFLAGS) -c -MMD $< -o $@
 
 feature-detect-clean:
 	$(call QUIET_CLEAN, feature-detect)
-- 
2.30.2


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

* [PATCH bpf-next 02/11] selftests/bpf: minor cleanups and normalization of Makefile
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 01/11] bpftool: normalize compile rules to specify output file last Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 03/11] selftests/bpf: allow to generate per-flavor list of tests Andrii Nakryiko
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Few clean ups and single-line simplifications. Also split CLEAN command
into multiple $(RM) invocations as it gets dangerously close to too long
argument list. Make sure that -o <output.o> is used always as the last
argument for saner verbose make output.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/Makefile | 32 ++++++++++++++--------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 0468ea57650d..0470802c907c 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -45,10 +45,8 @@ ifneq ($(BPF_GCC),)
 TEST_GEN_PROGS += test_progs-bpf_gcc
 endif
 
-TEST_GEN_FILES = test_lwt_ip_encap.o \
-	test_tc_edt.o
-TEST_FILES = xsk_prereqs.sh \
-	$(wildcard progs/btf_dump_test_case_*.c)
+TEST_GEN_FILES = test_lwt_ip_encap.o test_tc_edt.o
+TEST_FILES = xsk_prereqs.sh $(wildcard progs/btf_dump_test_case_*.c)
 
 # Order correspond to 'make run_tests' order
 TEST_PROGS := test_kmod.sh \
@@ -107,7 +105,10 @@ endif
 OVERRIDE_TARGETS := 1
 override define CLEAN
 	$(call msg,CLEAN)
-	$(Q)$(RM) -r $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) $(EXTRA_CLEAN)
+	$(Q)$(RM) -r $(TEST_GEN_PROGS)
+	$(Q)$(RM) -r $(TEST_GEN_PROGS_EXTENDED)
+	$(Q)$(RM) -r $(TEST_GEN_FILES)
+	$(Q)$(RM) -r $(EXTRA_CLEAN)
 	$(Q)$(MAKE) -C bpf_testmod clean
 	$(Q)$(MAKE) docs-clean
 endef
@@ -169,7 +170,7 @@ $(OUTPUT)/%:%.c
 
 $(OUTPUT)/urandom_read: urandom_read.c
 	$(call msg,BINARY,,$@)
-	$(Q)$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS) -Wl,--build-id=sha1
+	$(Q)$(CC) $(LDFLAGS) $< $(LDLIBS) -Wl,--build-id=sha1 -o $@
 
 $(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(wildcard bpf_testmod/Makefile bpf_testmod/*.[ch])
 	$(call msg,MOD,,$@)
@@ -232,16 +233,16 @@ docs-clean:
 	            prefix= OUTPUT=$(OUTPUT)/ DESTDIR=$(OUTPUT)/ $@
 
 $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)		       \
-	   ../../../include/uapi/linux/bpf.h                                   \
+	   $(APIDIR)/linux/bpf.h					       \
 	   | $(BUILD_DIR)/libbpf
 	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
 		    EXTRA_CFLAGS='-g -O0'				       \
 		    DESTDIR=$(SCRATCH_DIR) prefix= all install_headers
 
 ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
-$(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)                \
-	   ../../../include/uapi/linux/bpf.h                                   \
-	   | $(HOST_BUILD_DIR)/libbpf
+$(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)		       \
+		$(APIDIR)/linux/bpf.h					       \
+		| $(HOST_BUILD_DIR)/libbpf
 	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR)                             \
 		    EXTRA_CFLAGS='-g -O0'				       \
 		    OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
@@ -305,12 +306,12 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
 # $3 - CFLAGS
 define CLANG_BPF_BUILD_RULE
 	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -mcpu=v3
+	$(Q)$(CLANG) $3 -O2 -target bpf -c $1 -mcpu=v3 -o $2
 endef
 # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
 define CLANG_NOALU32_BPF_BUILD_RULE
 	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -mcpu=v2
+	$(Q)$(CLANG) $3 -O2 -target bpf -c $1 -mcpu=v2 -o $2
 endef
 # Build BPF object using GCC
 define GCC_BPF_BUILD_RULE
@@ -472,13 +473,12 @@ TRUNNER_TESTS_DIR := prog_tests
 TRUNNER_BPF_PROGS_DIR := progs
 TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c	\
 			 network_helpers.c testing_helpers.c		\
-			 btf_helpers.c	flow_dissector_load.h
+			 btf_helpers.c flow_dissector_load.h
 TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko	\
 		       ima_setup.sh					\
 		       $(wildcard progs/btf_dump_test_case_*.c)
 TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
-TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
-TRUNNER_BPF_CFLAGS += -DENABLE_ATOMICS_TESTS
+TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
 $(eval $(call DEFINE_TEST_RUNNER,test_progs))
 
 # Define test_progs-no_alu32 test runner.
@@ -540,7 +540,7 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
 		 $(OUTPUT)/bench_ringbufs.o \
 		 $(OUTPUT)/bench_bloom_filter_map.o
 	$(call msg,BINARY,,$@)
-	$(Q)$(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
+	$(Q)$(CC) $(LDFLAGS) $(filter %.a %.o,$^) $(LDLIBS) -o $@
 
 EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)	\
 	prog_tests/tests.h map_tests/tests.h verifier/tests.h		\
-- 
2.30.2


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

* [PATCH bpf-next 03/11] selftests/bpf: allow to generate per-flavor list of tests
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 01/11] bpftool: normalize compile rules to specify output file last Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 02/11] selftests/bpf: minor cleanups and normalization of Makefile Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib Andrii Nakryiko
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Add per-flavor list of tests to test_progs/test_maps test runners. Allow
to filter out some tests. This is going to be used in the next patch to
filter out tests that are using internal libbpf APIs and thus are not
compatible with test_progs built with libbpf as a shared library.

The way this is achieved is by generating test headers with test
binary-specific name and passing it explicitly to compiled test runners
through TESTS_HDR compiler define.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/.gitignore   |  7 ++++---
 tools/testing/selftests/bpf/Makefile     | 25 +++++++++++-------------
 tools/testing/selftests/bpf/test_maps.c  |  4 ++--
 tools/testing/selftests/bpf/test_progs.c |  6 +++---
 4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 1dad8d617da8..b6bc56c8127a 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -9,9 +9,6 @@ test_tag
 FEATURE-DUMP.libbpf
 fixdep
 test_dev_cgroup
-/test_progs
-/test_progs-no_alu32
-/test_progs-bpf_gcc
 test_verifier_log
 feature
 test_sock
@@ -32,6 +29,10 @@ xdping
 test_cpp
 *.skel.h
 *.lskel.h
+*.tests.h
+/test_progs
+/test_progs-no_alu32
+/test_progs-bpf_gcc
 /no_alu32
 /bpf_gcc
 /tools
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 0470802c907c..33f153a9de4c 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -344,14 +344,15 @@ LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-d
 # $2 - test runner extra "flavor" (e.g., no_alu32, gcc-bpf, etc)
 define DEFINE_TEST_RUNNER
 
-TRUNNER_OUTPUT := $(OUTPUT)$(if $2,/)$2
+TRUNNER_OUTPUT := $(if $(OUTPUT),$(OUTPUT),.)$(if $2,/)$2
 TRUNNER_BINARY := $1$(if $2,-)$2
-TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,	\
-				 $$(notdir $$(wildcard $(TRUNNER_TESTS_DIR)/*.c)))
+TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,	       \
+				 $$(filter-out $(TRUNNER_TESTS_BLACKLIST),     \
+					       $$(notdir $$(wildcard $(TRUNNER_TESTS_DIR)/*.c))))
 TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o,		\
 				 $$(filter %.c,$(TRUNNER_EXTRA_SOURCES)))
 TRUNNER_EXTRA_HDRS := $$(filter %.h,$(TRUNNER_EXTRA_SOURCES))
-TRUNNER_TESTS_HDR := $(TRUNNER_TESTS_DIR)/tests.h
+TRUNNER_TESTS_HDR := $$(TRUNNER_OUTPUT)/$1.tests.h
 TRUNNER_BPF_SRCS := $$(notdir $$(wildcard $(TRUNNER_BPF_PROGS_DIR)/*.c))
 TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS))
 TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h,	\
@@ -418,16 +419,13 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$(Q)$$(BPFTOOL) gen skeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$@
 endif
 
-# ensure we set up tests.h header generation rule just once
-ifeq ($($(TRUNNER_TESTS_DIR)-tests-hdr),)
-$(TRUNNER_TESTS_DIR)-tests-hdr := y
-$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c
+$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c | $(TRUNNER_OUTPUT)
 	$$(call msg,TEST-HDR,$(TRUNNER_BINARY),$$@)
-	$$(shell (echo '/* Generated header, do not edit */';					\
+	$$(shell (echo '/* Generated header, do not edit */';						\
 		  sed -n -E 's/^void (serial_)?test_([a-zA-Z0-9_]+)\((void)?\).*/DEFINE_TEST(\2)/p'	\
-			$(TRUNNER_TESTS_DIR)/*.c | sort ;	\
+			$(filter-out $(addprefix $(TRUNNER_TESTS_DIR)/,$(TRUNNER_TESTS_BLACKLIST)),	\
+				     $(wildcard $(TRUNNER_TESTS_DIR)/*.c)) | sort ;			\
 		 ) > $$@)
-endif
 
 # compile individual test files
 # Note: we cd into output directory to ensure embedded BPF object is found
@@ -448,7 +446,7 @@ $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
 		       $(TRUNNER_TESTS_HDR)				\
 		       $$(BPFOBJ) | $(TRUNNER_OUTPUT)
 	$$(call msg,EXT-OBJ,$(TRUNNER_BINARY),$$@)
-	$(Q)$$(CC) $$(CFLAGS) -c $$< $$(LDLIBS) -o $$@
+	$(Q)$$(CC) $$(CFLAGS) -DTESTS_HDR=\"$(TRUNNER_TESTS_HDR)\" -c $$< $$(LDLIBS) -o $$@
 
 # non-flavored in-srctree builds receive special treatment, in particular, we
 # do not need to copy extra resources (see e.g. test_btf_dump_case())
@@ -543,8 +541,7 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
 	$(Q)$(CC) $(LDFLAGS) $(filter %.a %.o,$^) $(LDLIBS) -o $@
 
 EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)	\
-	prog_tests/tests.h map_tests/tests.h verifier/tests.h		\
-	feature								\
+	*.tests.h verifier/tests.h					\
 	$(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h no_alu32 bpf_gcc bpf_testmod.ko)
 
 .PHONY: docs docs-clean
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 8b31bc1a801d..daddaf4f3e5a 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -1887,7 +1887,7 @@ static void run_all_tests(void)
 }
 
 #define DEFINE_TEST(name) extern void test_##name(void);
-#include <map_tests/tests.h>
+#include TESTS_HDR
 #undef DEFINE_TEST
 
 int main(void)
@@ -1903,7 +1903,7 @@ int main(void)
 	run_all_tests();
 
 #define DEFINE_TEST(name) test_##name();
-#include <map_tests/tests.h>
+#include TESTS_HDR
 #undef DEFINE_TEST
 
 	printf("test_maps: OK, %d SKIPPED\n", skips);
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c65986bd9d07..7c078565ee9d 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -461,7 +461,7 @@ static int load_bpf_testmod(void)
 #define DEFINE_TEST(name)				\
 	extern void test_##name(void) __weak;		\
 	extern void serial_test_##name(void) __weak;
-#include <prog_tests/tests.h>
+#include TESTS_HDR
 #undef DEFINE_TEST
 
 static struct prog_test_def prog_test_defs[] = {
@@ -470,7 +470,7 @@ static struct prog_test_def prog_test_defs[] = {
 	.run_test = &test_##name,		\
 	.run_serial_test = &serial_test_##name,	\
 },
-#include <prog_tests/tests.h>
+#include TESTS_HDR
 #undef DEFINE_TEST
 };
 const int prog_test_cnt = ARRAY_SIZE(prog_test_defs);
@@ -1377,7 +1377,7 @@ int main(int argc, char **argv)
 
 		if ((test->run_test == NULL && test->run_serial_test == NULL) ||
 		    (test->run_test != NULL && test->run_serial_test != NULL)) {
-			fprintf(stderr, "Test %d:%s must have either test_%s() or serial_test_%sl() defined.\n",
+			fprintf(stderr, "Test %d:%s must have either test_%s() or serial_test_%s() defined.\n",
 				test->test_num, test->test_name, test->test_name, test->test_name);
 			exit(EXIT_ERR_SETUP_INFRA);
 		}
-- 
2.30.2


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

* [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2021-11-08  6:13 ` [PATCH bpf-next 03/11] selftests/bpf: allow to generate per-flavor list of tests Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-09  3:44   ` Alexei Starovoitov
  2021-11-08  6:13 ` [PATCH bpf-next 05/11] libbpf: turn btf_dedup_opts into OPTS-based struct Andrii Nakryiko
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Add test_progs-shared flavor to compile against libbpf as a shared
library. This is useful to make sure that libbpf's backwards/forward
compatibility guarantees are upheld. Currently this has to be checked
locally, but in the future we'll automate at least some scenarios as
part of libbpf CI runs.

Biggest change is how either libbpf.a or libbpf.so is passed to the
compiler, which is controled on per-flavor through a new TRUNNER_LIBBPF
parameter. All the places that depend on libbpf artifacts (headers,
library itself, etc) to be built are moved to order-only dependency on
$(BPFOBJ). rpath is used to specify relative location to where libbpf.so
should be so that when test_progs-shared is run under QEMU, libbpf.so is
still going to be discovered correctly.

Few selftests are using or testing internal libbpf APIs, so are not
compatible with shared library use of libbpf. Filter them out for shared
flavor.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/.gitignore |  2 ++
 tools/testing/selftests/bpf/Makefile   | 36 +++++++++++++++++++-------
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index b6bc56c8127a..5adfac44c478 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -32,8 +32,10 @@ test_cpp
 *.tests.h
 /test_progs
 /test_progs-no_alu32
+/test_progs-shared
 /test_progs-bpf_gcc
 /no_alu32
+/shared
 /bpf_gcc
 /tools
 /runqslower
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 33f153a9de4c..42bc7913cc1a 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -32,16 +32,19 @@ ifneq ($(LLVM),)
 CFLAGS += -Wno-unused-command-line-argument
 endif
 
+TEST_RUNNERS = test_maps test_progs test_progs-no_alu32 test_progs-shared
+
 # Order correspond to 'make run_tests' order
-TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
+TEST_GEN_PROGS = test_verifier test_tag test_lru_map test_lpm_map \
 	test_verifier_log test_dev_cgroup \
 	test_sock test_sockmap get_cgroup_id_user \
 	test_cgroup_storage \
 	test_tcpnotify_user test_sysctl \
-	test_progs-no_alu32
+	$(TEST_RUNNERS)
 
 # Also test bpf-gcc, if present
 ifneq ($(BPF_GCC),)
+TEST_RUNNERS += test_progs-bpf_gcc
 TEST_GEN_PROGS += test_progs-bpf_gcc
 endif
 
@@ -190,7 +193,9 @@ $(OUTPUT)/runqslower: $(BPFOBJ) | $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT)
 
 TEST_GEN_PROGS_EXTENDED += $(DEFAULT_BPFTOOL)
 
-$(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(BPFOBJ)
+# Add libbpf.a dependency to all the test binaries but those in $(TEST_RUNNERS)
+$(filter-out $(addprefix %,$(TEST_RUNNERS)),$(TEST_GEN_PROGS)): $(BPFOBJ)
+$(TEST_GEN_PROGS_EXTENDED): $(BPFOBJ)
 
 $(OUTPUT)/test_dev_cgroup: cgroup_helpers.c testing_helpers.o
 $(OUTPUT)/test_skb_cgroup_id_user: cgroup_helpers.c testing_helpers.o
@@ -436,7 +441,7 @@ $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o:			\
 		      $(TRUNNER_BPF_SKELS)				\
 		      $(TRUNNER_BPF_LSKELS)				\
 		      $(TRUNNER_BPF_SKELS_LINKED)			\
-		      $$(BPFOBJ) | $(TRUNNER_OUTPUT)
+		      | $$(BPFOBJ) $(TRUNNER_OUTPUT)
 	$$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
 	$(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
 
@@ -444,7 +449,7 @@ $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
 		       %.c						\
 		       $(TRUNNER_EXTRA_HDRS)				\
 		       $(TRUNNER_TESTS_HDR)				\
-		       $$(BPFOBJ) | $(TRUNNER_OUTPUT)
+		       | $$(BPFOBJ) $(TRUNNER_OUTPUT)
 	$$(call msg,EXT-OBJ,$(TRUNNER_BINARY),$$@)
 	$(Q)$$(CC) $$(CFLAGS) -DTESTS_HDR=\"$(TRUNNER_TESTS_HDR)\" -c $$< $$(LDLIBS) -o $$@
 
@@ -457,11 +462,11 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
 endif
 
 $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)			\
-			     $(TRUNNER_EXTRA_OBJS) $$(BPFOBJ)		\
+			     $(TRUNNER_EXTRA_OBJS)			\
 			     $(RESOLVE_BTFIDS)				\
-			     | $(TRUNNER_BINARY)-extras
+			     | $$(BPF_OBJ) $(TRUNNER_BINARY)-extras
 	$$(call msg,BINARY,,$$@)
-	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
+	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $(TRUNNER_LIBBPF) $$(LDLIBS) $(TRUNNER_EXTRA_CFLAGS) -o $$@
 	$(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.o $$@
 
 endef
@@ -477,17 +482,29 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko	\
 		       $(wildcard progs/btf_dump_test_case_*.c)
 TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
+TRUNNER_LIBBPF := $(BPFOBJ)
 $(eval $(call DEFINE_TEST_RUNNER,test_progs))
 
 # Define test_progs-no_alu32 test runner.
 TRUNNER_BPF_BUILD_RULE := CLANG_NOALU32_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
+TRUNNER_LIBBPF := $(BPFOBJ)
 $(eval $(call DEFINE_TEST_RUNNER,test_progs,no_alu32))
 
+# Define test_progs-shared test runner.
+TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
+TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
+TRUNNER_EXTRA_CFLAGS := -Wl,-rpath=$(subst $(CURDIR)/,,$(dir $(BPFOBJ)))
+TRUNNER_LIBBPF := $(patsubst %libbpf.a,%libbpf.so,$(BPFOBJ))
+TRUNNER_TESTS_BLACKLIST := cpu_mask.c hashmap.c perf_buffer.c raw_tp_test_run.c
+$(eval $(call DEFINE_TEST_RUNNER,test_progs,shared))
+TRUNNER_TESTS_BLACKLIST :=
+
 # Define test_progs BPF-GCC-flavored test runner.
 ifneq ($(BPF_GCC),)
 TRUNNER_BPF_BUILD_RULE := GCC_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(call get_sys_includes,gcc)
+TRUNNER_LIBBPF := $(BPFOBJ)
 $(eval $(call DEFINE_TEST_RUNNER,test_progs,bpf_gcc))
 endif
 
@@ -542,6 +559,7 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
 
 EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)	\
 	*.tests.h verifier/tests.h					\
-	$(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h no_alu32 bpf_gcc bpf_testmod.ko)
+	$(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h)			\
+	$(addprefix $(OUTPUT)/,no_alu32 shared bpf_gcc bpf_testmod.ko)
 
 .PHONY: docs docs-clean
-- 
2.30.2


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

* [PATCH bpf-next 05/11] libbpf: turn btf_dedup_opts into OPTS-based struct
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
                   ` (3 preceding siblings ...)
  2021-11-08  6:13 ` [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof Andrii Nakryiko
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

btf__dedup() and struct btf_dedup_opts were added before we figured out
OPTS mechanism. As such, btf_dedup_opts is non-extensible without
breaking an ABI and potentially crashing user application.

Unfortunately, btf__dedup() and btf_dedup_opts are short and succinct
names that would be great to preserve and use going forward. So we use
___libbpf_override() macro approach, used previously for bpf_prog_load()
API, to define a new btf__dedup() variant that accepts only struct btf *
and struct btf_dedup_opts * arguments, and rename the old btf__dedup()
implementation into btf__dedup_deprecated(). This keeps both source and
binary compatibility with old and new applications.

The biggest problem was struct btf_dedup_opts, which wasn't OPTS-based,
and as such doesn't have `size_t sz;` as a first field. But btf__dedup()
is a pretty rarely used API and I believe that the only currently known
users (besides selftests) are libbpf's own bpf_linker and pahole.
Neither use case actually uses options and just passes NULL. So instead
of doing extra hacks, just rewrite struct btf_dedup_opts into OPTS-based
one, move btf_ext argument into those opts (only bpf_linker needs to
dedup btf_ext, so it's not a typical thing to specify), and drop never
used `dont_resolve_fwds` option (it was never used anywhere, AFAIK, it
makes BTF dedup much less useful and efficient).

Just in case, for old implementation, btf__dedup_deprecated(), detect
non-NULL options and error out with helpful message, to help users
migrate, if there are any user playing with btf__dedup().

The last remaining piece is dedup_table_size, which is another
anachronism from very early days of BTF dedup. Since then it has been
reduced to the only valid value, 1, to request forced hash collisions.
This is only used during testing. So instead introduce a bool flag to
force collisions explicitly.

This patch also adapts selftests to new btf__dedup() and btf_dedup_opts
use to avoid selftests breakage.

  [0] Closes: https://github.com/libbpf/libbpf/issues/281

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.c                           | 46 +++++++++++--------
 tools/lib/bpf/btf.h                           | 20 ++++++--
 tools/lib/bpf/libbpf.map                      |  2 +
 tools/lib/bpf/linker.c                        |  4 +-
 tools/testing/selftests/bpf/prog_tests/btf.c  | 46 +++----------------
 .../bpf/prog_tests/btf_dedup_split.c          |  6 +--
 6 files changed, 58 insertions(+), 66 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 7e4c5586bd87..fcec27622e7a 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -2846,8 +2846,7 @@ __u32 btf_ext__line_info_rec_size(const struct btf_ext *btf_ext)
 
 struct btf_dedup;
 
-static struct btf_dedup *btf_dedup_new(struct btf *btf, struct btf_ext *btf_ext,
-				       const struct btf_dedup_opts *opts);
+static struct btf_dedup *btf_dedup_new(struct btf *btf, const struct btf_dedup_opts *opts);
 static void btf_dedup_free(struct btf_dedup *d);
 static int btf_dedup_prep(struct btf_dedup *d);
 static int btf_dedup_strings(struct btf_dedup *d);
@@ -2994,12 +2993,17 @@ static int btf_dedup_remap_types(struct btf_dedup *d);
  * deduplicating structs/unions is described in greater details in comments for
  * `btf_dedup_is_equiv` function.
  */
-int btf__dedup(struct btf *btf, struct btf_ext *btf_ext,
-	       const struct btf_dedup_opts *opts)
+
+DEFAULT_VERSION(btf__dedup_v0_6_0, btf__dedup, LIBBPF_0.6.0)
+int btf__dedup_v0_6_0(struct btf *btf, const struct btf_dedup_opts *opts)
 {
-	struct btf_dedup *d = btf_dedup_new(btf, btf_ext, opts);
+	struct btf_dedup *d;
 	int err;
 
+	if (!OPTS_VALID(opts, btf_dedup_opts))
+		return libbpf_err(-EINVAL);
+
+	d = btf_dedup_new(btf, opts);
 	if (IS_ERR(d)) {
 		pr_debug("btf_dedup_new failed: %ld", PTR_ERR(d));
 		return libbpf_err(-EINVAL);
@@ -3051,6 +3055,19 @@ int btf__dedup(struct btf *btf, struct btf_ext *btf_ext,
 	return libbpf_err(err);
 }
 
+COMPAT_VERSION(bpf__dedup_deprecated, btf__dedup, LIBBPF_0.0.2)
+int btf__dedup_deprecated(struct btf *btf, struct btf_ext *btf_ext, const void *unused_opts)
+{
+	LIBBPF_OPTS(btf_dedup_opts, opts, .btf_ext = btf_ext);
+
+	if (unused_opts) {
+		pr_warn("please use new version of btf__dedup() that supports options\n");
+		return libbpf_err(-ENOTSUP);
+	}
+
+	return btf__dedup(btf, &opts);
+}
+
 #define BTF_UNPROCESSED_ID ((__u32)-1)
 #define BTF_IN_PROGRESS_ID ((__u32)-2)
 
@@ -3163,8 +3180,7 @@ static bool btf_dedup_equal_fn(const void *k1, const void *k2, void *ctx)
 	return k1 == k2;
 }
 
-static struct btf_dedup *btf_dedup_new(struct btf *btf, struct btf_ext *btf_ext,
-				       const struct btf_dedup_opts *opts)
+static struct btf_dedup *btf_dedup_new(struct btf *btf, const struct btf_dedup_opts *opts)
 {
 	struct btf_dedup *d = calloc(1, sizeof(struct btf_dedup));
 	hashmap_hash_fn hash_fn = btf_dedup_identity_hash_fn;
@@ -3173,13 +3189,11 @@ static struct btf_dedup *btf_dedup_new(struct btf *btf, struct btf_ext *btf_ext,
 	if (!d)
 		return ERR_PTR(-ENOMEM);
 
-	d->opts.dont_resolve_fwds = opts && opts->dont_resolve_fwds;
-	/* dedup_table_size is now used only to force collisions in tests */
-	if (opts && opts->dedup_table_size == 1)
+	if (OPTS_GET(opts, force_collisions, false))
 		hash_fn = btf_dedup_collision_hash_fn;
 
 	d->btf = btf;
-	d->btf_ext = btf_ext;
+	d->btf_ext = OPTS_GET(opts, btf_ext, NULL);
 
 	d->dedup_table = hashmap__new(hash_fn, btf_dedup_equal_fn, NULL);
 	if (IS_ERR(d->dedup_table)) {
@@ -3708,8 +3722,6 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
 				new_id = cand_id;
 				break;
 			}
-			if (d->opts.dont_resolve_fwds)
-				continue;
 			if (btf_compat_enum(t, cand)) {
 				if (btf_is_enum_fwd(t)) {
 					/* resolve fwd to full enum */
@@ -3952,8 +3964,7 @@ static int btf_dedup_is_equiv(struct btf_dedup *d, __u32 cand_id,
 		return 0;
 
 	/* FWD <--> STRUCT/UNION equivalence check, if enabled */
-	if (!d->opts.dont_resolve_fwds
-	    && (cand_kind == BTF_KIND_FWD || canon_kind == BTF_KIND_FWD)
+	if ((cand_kind == BTF_KIND_FWD || canon_kind == BTF_KIND_FWD)
 	    && cand_kind != canon_kind) {
 		__u16 real_kind;
 		__u16 fwd_kind;
@@ -3979,10 +3990,7 @@ static int btf_dedup_is_equiv(struct btf_dedup *d, __u32 cand_id,
 		return btf_equal_int_tag(cand_type, canon_type);
 
 	case BTF_KIND_ENUM:
-		if (d->opts.dont_resolve_fwds)
-			return btf_equal_enum(cand_type, canon_type);
-		else
-			return btf_compat_enum(cand_type, canon_type);
+		return btf_compat_enum(cand_type, canon_type);
 
 	case BTF_KIND_FWD:
 	case BTF_KIND_FLOAT:
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index bc005ba3ceec..6aae4f62ee0b 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -245,12 +245,24 @@ LIBBPF_API int btf__add_decl_tag(struct btf *btf, const char *value, int ref_typ
 			    int component_idx);
 
 struct btf_dedup_opts {
-	unsigned int dedup_table_size;
-	bool dont_resolve_fwds;
+	size_t sz;
+	/* optional .BTF.ext info to dedup along the main BTF info */
+	struct btf_ext *btf_ext;
+	/* force hash collisions (used for testing) */
+	bool force_collisions;
+	size_t :0;
 };
+#define btf_dedup_opts__last_field force_collisions
+
+LIBBPF_API int btf__dedup(struct btf *btf, const struct btf_dedup_opts *opts);
+
+LIBBPF_API int btf__dedup_v0_6_0(struct btf *btf, const struct btf_dedup_opts *opts);
 
-LIBBPF_API int btf__dedup(struct btf *btf, struct btf_ext *btf_ext,
-			  const struct btf_dedup_opts *opts);
+LIBBPF_DEPRECATED_SINCE(0, 7, "use btf__dedup() instead")
+LIBBPF_API int btf__dedup_deprecated(struct btf *btf, struct btf_ext *btf_ext, const void *opts);
+#define btf__dedup(...) ___libbpf_overload(___btf_dedup, __VA_ARGS__)
+#define ___btf_dedup3(btf, btf_ext, opts) btf__dedup_deprecated(btf, btf_ext, opts)
+#define ___btf_dedup2(btf, opts) btf__dedup(btf, opts)
 
 struct btf_dump;
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index b895861a13c0..edc40bc16f19 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -401,6 +401,8 @@ LIBBPF_0.6.0 {
 		bpf_program__insns;
 		btf__add_btf;
 		btf__add_decl_tag;
+		btf__dedup;
+		btf__dedup_deprecated;
 		btf__raw_data;
 		btf__type_cnt;
 } LIBBPF_0.5.0;
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index f677dccdeae4..594b206fa674 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -2650,6 +2650,7 @@ static int emit_elf_data_sec(struct bpf_linker *linker, const char *sec_name,
 
 static int finalize_btf(struct bpf_linker *linker)
 {
+	LIBBPF_OPTS(btf_dedup_opts, opts);
 	struct btf *btf = linker->btf;
 	const void *raw_data;
 	int i, j, id, err;
@@ -2686,7 +2687,8 @@ static int finalize_btf(struct bpf_linker *linker)
 		return err;
 	}
 
-	err = btf__dedup(linker->btf, linker->btf_ext, NULL);
+	opts.btf_ext = linker->btf_ext;
+	err = btf__dedup(linker->btf, &opts);
 	if (err) {
 		pr_warn("BTF dedup failed: %d\n", err);
 		return err;
diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index ebd1aa4d09d6..1e8b36d74df2 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -6627,7 +6627,7 @@ struct btf_dedup_test {
 	struct btf_dedup_opts opts;
 };
 
-const struct btf_dedup_test dedup_tests[] = {
+static struct btf_dedup_test dedup_tests[] = {
 
 {
 	.descr = "dedup: unused strings filtering",
@@ -6647,9 +6647,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0int\0long"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: strings deduplication",
@@ -6672,9 +6669,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0int\0long int"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: struct example #1",
@@ -6755,9 +6749,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0a\0b\0c\0d\0int\0float\0next\0s"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: struct <-> fwd resolution w/ hash collision",
@@ -6800,8 +6791,7 @@ const struct btf_dedup_test dedup_tests[] = {
 		BTF_STR_SEC("\0s\0x"),
 	},
 	.opts = {
-		.dont_resolve_fwds = false,
-		.dedup_table_size = 1, /* force hash collisions */
+		.force_collisions = true, /* force hash collisions */
 	},
 },
 {
@@ -6847,8 +6837,7 @@ const struct btf_dedup_test dedup_tests[] = {
 		BTF_STR_SEC("\0s\0x"),
 	},
 	.opts = {
-		.dont_resolve_fwds = false,
-		.dedup_table_size = 1, /* force hash collisions */
+		.force_collisions = true, /* force hash collisions */
 	},
 },
 {
@@ -6911,9 +6900,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0A\0B\0C\0D\0E\0F\0G\0H\0I\0J\0K\0L\0M\0N\0O\0P\0Q"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: no int/float duplicates",
@@ -6965,9 +6951,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0int\0some other int\0float"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: enum fwd resolution",
@@ -7009,9 +6992,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0e1\0e1_val\0e2\0e2_val"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: datasec and vars pass-through",
@@ -7054,8 +7034,7 @@ const struct btf_dedup_test dedup_tests[] = {
 		BTF_STR_SEC("\0.bss\0t"),
 	},
 	.opts = {
-		.dont_resolve_fwds = false,
-		.dedup_table_size = 1
+		.force_collisions = true
 	},
 },
 {
@@ -7099,9 +7078,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0t\0a1\0a2\0f\0tag"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: func/func_param tags",
@@ -7152,9 +7128,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0a1\0a2\0f\0tag1\0tag2\0tag3"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: struct/struct_member tags",
@@ -7200,9 +7173,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0t\0m1\0m2\0tag1\0tag2\0tag3"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 {
 	.descr = "dedup: typedef tags",
@@ -7233,9 +7203,6 @@ const struct btf_dedup_test dedup_tests[] = {
 		},
 		BTF_STR_SEC("\0t\0tag1\0tag2\0tag3"),
 	},
-	.opts = {
-		.dont_resolve_fwds = false,
-	},
 },
 
 };
@@ -7293,7 +7260,7 @@ static void dump_btf_strings(const char *strs, __u32 len)
 
 static void do_test_dedup(unsigned int test_num)
 {
-	const struct btf_dedup_test *test = &dedup_tests[test_num - 1];
+	struct btf_dedup_test *test = &dedup_tests[test_num - 1];
 	__u32 test_nr_types, expect_nr_types, test_btf_size, expect_btf_size;
 	const struct btf_header *test_hdr, *expect_hdr;
 	struct btf *test_btf = NULL, *expect_btf = NULL;
@@ -7337,7 +7304,8 @@ static void do_test_dedup(unsigned int test_num)
 		goto done;
 	}
 
-	err = btf__dedup(test_btf, NULL, &test->opts);
+	test->opts.sz = sizeof(test->opts);
+	err = btf__dedup(test_btf, &test->opts);
 	if (CHECK(err, "btf_dedup failed errno:%d", err)) {
 		err = -1;
 		goto done;
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dedup_split.c b/tools/testing/selftests/bpf/prog_tests/btf_dedup_split.c
index 64554fd33547..9d3b8d7a1537 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dedup_split.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dedup_split.c
@@ -92,7 +92,7 @@ struct s2 {\n\
 	int *f3;\n\
 };\n\n", "c_dump");
 
-	err = btf__dedup(btf2, NULL, NULL);
+	err = btf__dedup(btf2, NULL);
 	if (!ASSERT_OK(err, "btf_dedup"))
 		goto cleanup;
 
@@ -186,7 +186,7 @@ static void test_split_fwd_resolve() {
 		"\t'f1' type_id=7 bits_offset=0\n"
 		"\t'f2' type_id=9 bits_offset=64");
 
-	err = btf__dedup(btf2, NULL, NULL);
+	err = btf__dedup(btf2, NULL);
 	if (!ASSERT_OK(err, "btf_dedup"))
 		goto cleanup;
 
@@ -283,7 +283,7 @@ static void test_split_struct_duped() {
 		"[13] STRUCT 's3' size=8 vlen=1\n"
 		"\t'f1' type_id=12 bits_offset=0");
 
-	err = btf__dedup(btf2, NULL, NULL);
+	err = btf__dedup(btf2, NULL);
 	if (!ASSERT_OK(err, "btf_dedup"))
 		goto cleanup;
 
-- 
2.30.2


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

* [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
                   ` (4 preceding siblings ...)
  2021-11-08  6:13 ` [PATCH bpf-next 05/11] libbpf: turn btf_dedup_opts into OPTS-based struct Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-09  3:38   ` Alexei Starovoitov
  2021-11-08  6:13 ` [PATCH bpf-next 07/11] libbpf: make perf_buffer__new() use OPTS-based interface Andrii Nakryiko
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Change btf_dump__new() and corresponding struct btf_dump_ops structure
to be extensible by using OPTS "framework" ([0]). Given we don't change
the names, we use a similar approach as with bpf_prog_load(), but this
time we ended up with two APIs with the same name and same number of
arguments, so overloading based on number of arguments with
___libbpf_override() doesn't work.

Instead, use "overloading" based on types. In this particular case,
print callback has to be specified, so we detect which argument is
a callback. If it's 4th (last) argument, old implementation of API is
used by user code. If not, it must be 2nd, and thus new implementation
is selected. The rest is handled by the same symbol versioning approach.

btf_ext argument is dropped as it was never used and isn't necessary
either. If in the future we'll need btf_ext, that will be added into
OPTS-based struct btf_dump_opts.

struct btf_dump_opts is reused for both old API and new APIs. ctx field
is marked deprecated in v0.7+ and it's put at the same memory location
as OPTS's sz field. Any user of new-style btf_dump__new() will have to
set sz field and doesn't/shouldn't use ctx, as ctx is now passed along
the callback as mandatory input argument, following the other APIs in
libbpf that accept callbacks consistently.

Again, this is quite ugly in implementation, but is done in the name of
backwards compatibility and uniform and extensible future APIs (at the
same time, sigh). And it will be gone in libbpf 1.0.

  [0] Closes: https://github.com/libbpf/libbpf/issues/283

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.h      | 52 ++++++++++++++++++++++++++++++++++++----
 tools/lib/bpf/btf_dump.c | 26 +++++++++++++-------
 tools/lib/bpf/libbpf.map |  2 ++
 3 files changed, 67 insertions(+), 13 deletions(-)

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 6aae4f62ee0b..a6e528faebf9 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -267,15 +267,59 @@ LIBBPF_API int btf__dedup_deprecated(struct btf *btf, struct btf_ext *btf_ext, c
 struct btf_dump;
 
 struct btf_dump_opts {
-	void *ctx;
+	union {
+		size_t sz;
+		void *ctx; /* DEPRECATED: will be gone in v1.0 */
+	};
 };
 
 typedef void (*btf_dump_printf_fn_t)(void *ctx, const char *fmt, va_list args);
 
 LIBBPF_API struct btf_dump *btf_dump__new(const struct btf *btf,
-					  const struct btf_ext *btf_ext,
-					  const struct btf_dump_opts *opts,
-					  btf_dump_printf_fn_t printf_fn);
+					  btf_dump_printf_fn_t printf_fn,
+					  void *ctx,
+					  const struct btf_dump_opts *opts);
+
+LIBBPF_API struct btf_dump *btf_dump__new_v0_6_0(const struct btf *btf,
+						 btf_dump_printf_fn_t printf_fn,
+						 void *ctx,
+						 const struct btf_dump_opts *opts);
+
+LIBBPF_API struct btf_dump *btf_dump__new_deprecated(const struct btf *btf,
+						     const struct btf_ext *btf_ext,
+						     const struct btf_dump_opts *opts,
+						     btf_dump_printf_fn_t printf_fn);
+
+/* Choose either btf_dump__new() or btf_dump__new_deprecated() based on the
+ * type of 4th argument. If it's btf_dump's print callback, use deprecated
+ * API; otherwise, choose the new btf_dump__new(). ___libbpf_override()
+ * doesn't work here because both variants have 4 input arguments.
+ *
+ * (void *) casts are necessary to avoid compilation warnings about type
+ * mismatches, because even though __builtin_choose_expr() only ever evaluates
+ * one side the other side still has to satisfy type constraints (this is
+ * compiler implementation limitation which might be lifted eventually,
+ * according to the documentation). So passing struct btf_ext in place of
+ * btf_dump_printf_fn_t would be generating compilation warning.  Casting to
+ * void * avoids this issue.
+ *
+ * Also, two type compatibility checks for a function and function pointer are
+ * required because passing function reference into btf_dump__new() as
+ * btf_dump__new(..., my_callback, ...) and as btf_dump__new(...,
+ * &my_callback, ...) (not explicit ampersand in the latter case) actually
+ * differs as far as __builtin_types_compatible_p() is concerned. Thus two
+ * checks are combined to detect callback argument.
+ *
+ * The rest works just like in case of ___libbpf_override() usage with symbol
+ * versioning.
+ */
+#define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr(		      \
+	__builtin_types_compatible_p(typeof(a4), btf_dump_printf_fn_t) +      \
+	__builtin_types_compatible_p(typeof(a4),			      \
+				     void(void *, const char *, va_list)),    \
+	btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4),\
+	btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4))
+
 LIBBPF_API void btf_dump__free(struct btf_dump *d);
 
 LIBBPF_API int btf_dump__dump_type(struct btf_dump *d, __u32 id);
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 17db62b5002e..0ed9c2f93322 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -77,9 +77,8 @@ struct btf_dump_data {
 
 struct btf_dump {
 	const struct btf *btf;
-	const struct btf_ext *btf_ext;
 	btf_dump_printf_fn_t printf_fn;
-	struct btf_dump_opts opts;
+	void *cb_ctx;
 	int ptr_sz;
 	bool strip_mods;
 	bool skip_anon_defs;
@@ -138,17 +137,18 @@ static void btf_dump_printf(const struct btf_dump *d, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	d->printf_fn(d->opts.ctx, fmt, args);
+	d->printf_fn(d->cb_ctx, fmt, args);
 	va_end(args);
 }
 
 static int btf_dump_mark_referenced(struct btf_dump *d);
 static int btf_dump_resize(struct btf_dump *d);
 
-struct btf_dump *btf_dump__new(const struct btf *btf,
-			       const struct btf_ext *btf_ext,
-			       const struct btf_dump_opts *opts,
-			       btf_dump_printf_fn_t printf_fn)
+DEFAULT_VERSION(btf_dump__new_v0_6_0, btf_dump__new, LIBBPF_0.6.0)
+struct btf_dump *btf_dump__new_v0_6_0(const struct btf *btf,
+				      btf_dump_printf_fn_t printf_fn,
+				      void *ctx,
+				      const struct btf_dump_opts *opts)
 {
 	struct btf_dump *d;
 	int err;
@@ -158,9 +158,8 @@ struct btf_dump *btf_dump__new(const struct btf *btf,
 		return libbpf_err_ptr(-ENOMEM);
 
 	d->btf = btf;
-	d->btf_ext = btf_ext;
 	d->printf_fn = printf_fn;
-	d->opts.ctx = opts ? opts->ctx : NULL;
+	d->cb_ctx = ctx;
 	d->ptr_sz = btf__pointer_size(btf) ? : sizeof(void *);
 
 	d->type_names = hashmap__new(str_hash_fn, str_equal_fn, NULL);
@@ -186,6 +185,15 @@ struct btf_dump *btf_dump__new(const struct btf *btf,
 	return libbpf_err_ptr(err);
 }
 
+COMPAT_VERSION(btf_dump__new_deprecated, btf_dump__new, LIBBPF_0.0.4)
+struct btf_dump *btf_dump__new_deprecated(const struct btf *btf,
+					  const struct btf_ext *btf_ext,
+					  const struct btf_dump_opts *opts,
+					  btf_dump_printf_fn_t printf_fn)
+{
+	return btf_dump__new_v0_6_0(btf, printf_fn, opts ? opts->ctx : NULL, opts);
+}
+
 static int btf_dump_resize(struct btf_dump *d)
 {
 	int err, last_id = btf__type_cnt(d->btf) - 1;
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index edc40bc16f19..c51411d46c9e 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -405,4 +405,6 @@ LIBBPF_0.6.0 {
 		btf__dedup_deprecated;
 		btf__raw_data;
 		btf__type_cnt;
+		btf_dump__new;
+		btf_dump__new_deprecated;
 } LIBBPF_0.5.0;
-- 
2.30.2


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

* [PATCH bpf-next 07/11] libbpf: make perf_buffer__new() use OPTS-based interface
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
                   ` (5 preceding siblings ...)
  2021-11-08  6:13 ` [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 08/11] selftests/bpf: migrate all deprecated perf_buffer uses Andrii Nakryiko
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Add new variants of perf_buffer__new() and perf_buffer__new_raw() that
use OPTS-based options for future extensibility ([0]). Given all the
currently used API names are best fits, re-use them and use
___libbpf_override() approach and symbol versioning to preserve ABI and
source code compatibility. struct perf_buffer_opts and struct
perf_buffer_raw_opts are kept as well, but they are restructured such
that they are OPTS-based when used with new APIs. For struct
perf_buffer_raw_opts we keep few fields intact, so we have to also
preserve the memory location of them both when used as OPTS and for
legacy API variants. This is achieved with anonymous padding for OPTS
"incarnation" of the struct.  These pads can be eventually used for new
options.

  [0] Closes: https://github.com/libbpf/libbpf/issues/311

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c   | 70 +++++++++++++++++++++++++-------
 tools/lib/bpf/libbpf.h   | 86 ++++++++++++++++++++++++++++++++++------
 tools/lib/bpf/libbpf.map |  4 ++
 3 files changed, 132 insertions(+), 28 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d869ebee1e27..cc08d677c5c0 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10564,11 +10564,18 @@ perf_buffer__open_cpu_buf(struct perf_buffer *pb, struct perf_event_attr *attr,
 static struct perf_buffer *__perf_buffer__new(int map_fd, size_t page_cnt,
 					      struct perf_buffer_params *p);
 
-struct perf_buffer *perf_buffer__new(int map_fd, size_t page_cnt,
-				     const struct perf_buffer_opts *opts)
+DEFAULT_VERSION(perf_buffer__new_v0_6_0, perf_buffer__new, LIBBPF_0.6.0)
+struct perf_buffer *perf_buffer__new_v0_6_0(int map_fd, size_t page_cnt,
+					    perf_buffer_sample_fn sample_cb,
+					    perf_buffer_lost_fn lost_cb,
+					    void *ctx,
+					    const struct perf_buffer_opts *opts)
 {
 	struct perf_buffer_params p = {};
-	struct perf_event_attr attr = { 0, };
+	struct perf_event_attr attr = {};
+
+	if (!OPTS_VALID(opts, perf_buffer_opts))
+		return libbpf_err_ptr(-EINVAL);
 
 	attr.config = PERF_COUNT_SW_BPF_OUTPUT;
 	attr.type = PERF_TYPE_SOFTWARE;
@@ -10577,29 +10584,62 @@ struct perf_buffer *perf_buffer__new(int map_fd, size_t page_cnt,
 	attr.wakeup_events = 1;
 
 	p.attr = &attr;
-	p.sample_cb = opts ? opts->sample_cb : NULL;
-	p.lost_cb = opts ? opts->lost_cb : NULL;
-	p.ctx = opts ? opts->ctx : NULL;
+	p.sample_cb = sample_cb;
+	p.lost_cb = lost_cb;
+	p.ctx = ctx;
 
 	return libbpf_ptr(__perf_buffer__new(map_fd, page_cnt, &p));
 }
 
-struct perf_buffer *
-perf_buffer__new_raw(int map_fd, size_t page_cnt,
-		     const struct perf_buffer_raw_opts *opts)
+COMPAT_VERSION(perf_buffer__new_deprecated, perf_buffer__new, LIBBPF_0.0.4)
+struct perf_buffer *perf_buffer__new_deprecated(int map_fd, size_t page_cnt,
+						const struct perf_buffer_opts *opts)
+{
+	return perf_buffer__new_v0_6_0(map_fd, page_cnt,
+				       opts ? opts->sample_cb : NULL,
+				       opts ? opts->lost_cb : NULL,
+				       opts ? opts->ctx : NULL,
+				       NULL);
+}
+
+DEFAULT_VERSION(perf_buffer__new_raw_v0_6_0, perf_buffer__new_raw, LIBBPF_0.6.0)
+struct perf_buffer *perf_buffer__new_raw_v0_6_0(int map_fd, size_t page_cnt,
+						struct perf_event_attr *attr,
+						perf_buffer_event_fn event_cb, void *ctx,
+						const struct perf_buffer_raw_opts *opts)
 {
 	struct perf_buffer_params p = {};
 
-	p.attr = opts->attr;
-	p.event_cb = opts->event_cb;
-	p.ctx = opts->ctx;
-	p.cpu_cnt = opts->cpu_cnt;
-	p.cpus = opts->cpus;
-	p.map_keys = opts->map_keys;
+	if (page_cnt == 0 || !attr)
+		return libbpf_err_ptr(-EINVAL);
+
+	if (!OPTS_VALID(opts, perf_buffer_raw_opts))
+		return libbpf_err_ptr(-EINVAL);
+
+	p.attr = attr;
+	p.event_cb = event_cb;
+	p.ctx = ctx;
+	p.cpu_cnt = OPTS_GET(opts, cpu_cnt, 0);
+	p.cpus = OPTS_GET(opts, cpus, NULL);
+	p.map_keys = OPTS_GET(opts, map_keys, NULL);
 
 	return libbpf_ptr(__perf_buffer__new(map_fd, page_cnt, &p));
 }
 
+COMPAT_VERSION(perf_buffer__new_raw_deprecated, perf_buffer__new_raw, LIBBPF_0.0.4)
+struct perf_buffer *perf_buffer__new_raw_deprecated(int map_fd, size_t page_cnt,
+						    const struct perf_buffer_raw_opts *opts)
+{
+	LIBBPF_OPTS(perf_buffer_raw_opts, inner_opts,
+		.cpu_cnt = opts->cpu_cnt,
+		.cpus = opts->cpus,
+		.map_keys = opts->map_keys,
+	);
+
+	return perf_buffer__new_raw_v0_6_0(map_fd, page_cnt, opts->attr,
+					   opts->event_cb, opts->ctx, &inner_opts);
+}
+
 static struct perf_buffer *__perf_buffer__new(int map_fd, size_t page_cnt,
 					      struct perf_buffer_params *p)
 {
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 039058763173..47d4585a79bb 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -775,18 +775,52 @@ typedef void (*perf_buffer_lost_fn)(void *ctx, int cpu, __u64 cnt);
 
 /* common use perf buffer options */
 struct perf_buffer_opts {
-	/* if specified, sample_cb is called for each sample */
-	perf_buffer_sample_fn sample_cb;
-	/* if specified, lost_cb is called for each batch of lost samples */
-	perf_buffer_lost_fn lost_cb;
-	/* ctx is provided to sample_cb and lost_cb */
-	void *ctx;
+	union {
+		size_t sz;
+		struct { /* DEPRECATED: will be removed in v1.0 */
+			/* if specified, sample_cb is called for each sample */
+			perf_buffer_sample_fn sample_cb;
+			/* if specified, lost_cb is called for each batch of lost samples */
+			perf_buffer_lost_fn lost_cb;
+			/* ctx is provided to sample_cb and lost_cb */
+			void *ctx;
+		};
+	};
 };
+#define perf_buffer_opts__last_field sz
 
+/**
+ * @brief **perf_buffer__new()** creates BPF perfbuf manager for a specified
+ * BPF_PERF_EVENT_ARRAY map
+ * @param map_fd FD of BPF_PERF_EVENT_ARRAY BPF map that will be used by BPF
+ * code to send data over to user-space
+ * @param page_cnt number of memory pages allocated for each per-CPU buffer
+ * @param sample_cb function called on each received data record
+ * @param lost_cb function called when record loss has occurred
+ * @param ctx user-provided extra context passed into *sample_cb* and *lost_cb*
+ * @return a new instance of struct perf_buffer on success, NULL on error with
+ * *errno* containing an error code
+ */
 LIBBPF_API struct perf_buffer *
 perf_buffer__new(int map_fd, size_t page_cnt,
+		 perf_buffer_sample_fn sample_cb, perf_buffer_lost_fn lost_cb, void *ctx,
 		 const struct perf_buffer_opts *opts);
 
+LIBBPF_API struct perf_buffer *
+perf_buffer__new_v0_6_0(int map_fd, size_t page_cnt,
+			perf_buffer_sample_fn sample_cb, perf_buffer_lost_fn lost_cb, void *ctx,
+			const struct perf_buffer_opts *opts);
+
+LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 7, "use new variant of perf_buffer__new() instead")
+struct perf_buffer *perf_buffer__new_deprecated(int map_fd, size_t page_cnt,
+						const struct perf_buffer_opts *opts);
+
+#define perf_buffer__new(...) ___libbpf_overload(___perf_buffer_new, __VA_ARGS__)
+#define ___perf_buffer_new6(map_fd, page_cnt, sample_cb, lost_cb, ctx, opts) \
+	perf_buffer__new(map_fd, page_cnt, sample_cb, lost_cb, ctx, opts)
+#define ___perf_buffer_new3(map_fd, page_cnt, opts) \
+	perf_buffer__new_deprecated(map_fd, page_cnt, opts)
+
 enum bpf_perf_event_ret {
 	LIBBPF_PERF_EVENT_DONE	= 0,
 	LIBBPF_PERF_EVENT_ERROR	= -1,
@@ -800,12 +834,21 @@ typedef enum bpf_perf_event_ret
 
 /* raw perf buffer options, giving most power and control */
 struct perf_buffer_raw_opts {
-	/* perf event attrs passed directly into perf_event_open() */
-	struct perf_event_attr *attr;
-	/* raw event callback */
-	perf_buffer_event_fn event_cb;
-	/* ctx is provided to event_cb */
-	void *ctx;
+	union {
+		struct {
+			size_t sz;
+			long :0;
+			long :0;
+		};
+		struct { /* DEPRECATED: will be removed in v1.0 */
+			/* perf event attrs passed directly into perf_event_open() */
+			struct perf_event_attr *attr;
+			/* raw event callback */
+			perf_buffer_event_fn event_cb;
+			/* ctx is provided to event_cb */
+			void *ctx;
+		};
+	};
 	/* if cpu_cnt == 0, open all on all possible CPUs (up to the number of
 	 * max_entries of given PERF_EVENT_ARRAY map)
 	 */
@@ -815,11 +858,28 @@ struct perf_buffer_raw_opts {
 	/* if cpu_cnt > 0, map_keys specify map keys to set per-CPU FDs for */
 	int *map_keys;
 };
+#define perf_buffer_raw_opts__last_field map_keys
 
 LIBBPF_API struct perf_buffer *
-perf_buffer__new_raw(int map_fd, size_t page_cnt,
+perf_buffer__new_raw(int map_fd, size_t page_cnt, struct perf_event_attr *attr,
+		     perf_buffer_event_fn event_cb, void *ctx,
 		     const struct perf_buffer_raw_opts *opts);
 
+LIBBPF_API struct perf_buffer *
+perf_buffer__new_raw_v0_6_0(int map_fd, size_t page_cnt, struct perf_event_attr *attr,
+			    perf_buffer_event_fn event_cb, void *ctx,
+			    const struct perf_buffer_raw_opts *opts);
+
+LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 7, "use new variant of perf_buffer__new_raw() instead")
+struct perf_buffer *perf_buffer__new_raw_deprecated(int map_fd, size_t page_cnt,
+						    const struct perf_buffer_raw_opts *opts);
+
+#define perf_buffer__new_raw(...) ___libbpf_overload(___perf_buffer_new_raw, __VA_ARGS__)
+#define ___perf_buffer_new_raw6(map_fd, page_cnt, attr, event_cb, ctx, opts) \
+	perf_buffer__new_raw(map_fd, page_cnt, attr, event_cb, ctx, opts)
+#define ___perf_buffer_new_raw3(map_fd, page_cnt, opts) \
+	perf_buffer__new_raw_deprecated(map_fd, page_cnt, opts)
+
 LIBBPF_API void perf_buffer__free(struct perf_buffer *pb);
 LIBBPF_API int perf_buffer__epoll_fd(const struct perf_buffer *pb);
 LIBBPF_API int perf_buffer__poll(struct perf_buffer *pb, int timeout_ms);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index c51411d46c9e..19396e1c59f9 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -407,4 +407,8 @@ LIBBPF_0.6.0 {
 		btf__type_cnt;
 		btf_dump__new;
 		btf_dump__new_deprecated;
+		perf_buffer__new;
+		perf_buffer__new_deprecated;
+		perf_buffer__new_raw;
+		perf_buffer__new_raw_deprecated;
 } LIBBPF_0.5.0;
-- 
2.30.2


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

* [PATCH bpf-next 08/11] selftests/bpf: migrate all deprecated perf_buffer uses
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
                   ` (6 preceding siblings ...)
  2021-11-08  6:13 ` [PATCH bpf-next 07/11] libbpf: make perf_buffer__new() use OPTS-based interface Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 09/11] selftests/bpf: update btf_dump__new() uses to v1.0+ variant Andrii Nakryiko
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Migrate all old-style perf_buffer__new() and perf_buffer__new_raw()
calls to new v1.0+ variants.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/benchs/bench_ringbufs.c       | 8 ++------
 tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c | 5 ++---
 tools/testing/selftests/bpf/prog_tests/kfree_skb.c        | 6 ++----
 tools/testing/selftests/bpf/prog_tests/perf_buffer.c      | 6 ++----
 tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c      | 7 ++-----
 tools/testing/selftests/bpf/test_tcpnotify_user.c         | 4 +---
 6 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
index d167bffac679..52d4a2f91dbd 100644
--- a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
+++ b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c
@@ -394,11 +394,6 @@ static void perfbuf_libbpf_setup()
 {
 	struct perfbuf_libbpf_ctx *ctx = &perfbuf_libbpf_ctx;
 	struct perf_event_attr attr;
-	struct perf_buffer_raw_opts pb_opts = {
-		.event_cb = perfbuf_process_sample_raw,
-		.ctx = (void *)(long)0,
-		.attr = &attr,
-	};
 	struct bpf_link *link;
 
 	ctx->skel = perfbuf_setup_skeleton();
@@ -423,7 +418,8 @@ static void perfbuf_libbpf_setup()
 	}
 
 	ctx->perfbuf = perf_buffer__new_raw(bpf_map__fd(ctx->skel->maps.perfbuf),
-					    args.perfbuf_sz, &pb_opts);
+					    args.perfbuf_sz, &attr,
+					    perfbuf_process_sample_raw, NULL, NULL);
 	if (!ctx->perfbuf) {
 		fprintf(stderr, "failed to create perfbuf\n");
 		exit(1);
diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
index 569fcc6ed660..4184c399d4c6 100644
--- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
@@ -85,7 +85,6 @@ void test_get_stack_raw_tp(void)
 	const char *file_err = "./test_get_stack_rawtp_err.o";
 	const char *prog_name = "raw_tracepoint/sys_enter";
 	int i, err, prog_fd, exp_cnt = MAX_CNT_RAWTP;
-	struct perf_buffer_opts pb_opts = {};
 	struct perf_buffer *pb = NULL;
 	struct bpf_link *link = NULL;
 	struct timespec tv = {0, 10};
@@ -124,8 +123,8 @@ void test_get_stack_raw_tp(void)
 	if (!ASSERT_OK_PTR(link, "attach_raw_tp"))
 		goto close_prog;
 
-	pb_opts.sample_cb = get_stack_print_output;
-	pb = perf_buffer__new(bpf_map__fd(map), 8, &pb_opts);
+	pb = perf_buffer__new(bpf_map__fd(map), 8, get_stack_print_output,
+			      NULL, NULL, NULL);
 	if (!ASSERT_OK_PTR(pb, "perf_buf__new"))
 		goto close_prog;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
index 885413ed5c96..2a49f8fcde06 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
@@ -66,7 +66,6 @@ void serial_test_kfree_skb(void)
 	struct bpf_map *perf_buf_map, *global_data;
 	struct bpf_program *prog, *fentry, *fexit;
 	struct bpf_object *obj, *obj2 = NULL;
-	struct perf_buffer_opts pb_opts = {};
 	struct perf_buffer *pb = NULL;
 	int err, kfree_skb_fd;
 	bool passed = false;
@@ -112,9 +111,8 @@ void serial_test_kfree_skb(void)
 		goto close_prog;
 
 	/* set up perf buffer */
-	pb_opts.sample_cb = on_sample;
-	pb_opts.ctx = &passed;
-	pb = perf_buffer__new(bpf_map__fd(perf_buf_map), 1, &pb_opts);
+	pb = perf_buffer__new(bpf_map__fd(perf_buf_map), 1,
+			      on_sample, NULL, &passed, NULL);
 	if (!ASSERT_OK_PTR(pb, "perf_buf__new"))
 		goto close_prog;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/perf_buffer.c b/tools/testing/selftests/bpf/prog_tests/perf_buffer.c
index 4e32f3586a75..5fc2b3a0711e 100644
--- a/tools/testing/selftests/bpf/prog_tests/perf_buffer.c
+++ b/tools/testing/selftests/bpf/prog_tests/perf_buffer.c
@@ -47,7 +47,6 @@ void serial_test_perf_buffer(void)
 {
 	int err, on_len, nr_on_cpus = 0, nr_cpus, i, j;
 	int zero = 0, my_pid = getpid();
-	struct perf_buffer_opts pb_opts = {};
 	struct test_perf_buffer *skel;
 	cpu_set_t cpu_seen;
 	struct perf_buffer *pb;
@@ -82,9 +81,8 @@ void serial_test_perf_buffer(void)
 		goto out_close;
 
 	/* set up perf buffer */
-	pb_opts.sample_cb = on_sample;
-	pb_opts.ctx = &cpu_seen;
-	pb = perf_buffer__new(bpf_map__fd(skel->maps.perf_buf_map), 1, &pb_opts);
+	pb = perf_buffer__new(bpf_map__fd(skel->maps.perf_buf_map), 1,
+			      on_sample, NULL, &cpu_seen, NULL);
 	if (!ASSERT_OK_PTR(pb, "perf_buf__new"))
 		goto out_close;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c
index 3bd5904b4db5..f99386d1dc4c 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c
@@ -49,7 +49,6 @@ void test_xdp_bpf2bpf(void)
 	struct vip key4 = {.protocol = 6, .family = AF_INET};
 	struct bpf_program *prog;
 	struct perf_buffer *pb = NULL;
-	struct perf_buffer_opts pb_opts = {};
 
 	/* Load XDP program to introspect */
 	pkt_skel = test_xdp__open_and_load();
@@ -86,10 +85,8 @@ void test_xdp_bpf2bpf(void)
 		goto out;
 
 	/* Set up perf buffer */
-	pb_opts.sample_cb = on_sample;
-	pb_opts.ctx = &passed;
-	pb = perf_buffer__new(bpf_map__fd(ftrace_skel->maps.perf_buf_map),
-			      1, &pb_opts);
+	pb = perf_buffer__new(bpf_map__fd(ftrace_skel->maps.perf_buf_map), 1,
+			      on_sample, NULL, &passed, NULL);
 	if (!ASSERT_OK_PTR(pb, "perf_buf__new"))
 		goto out;
 
diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
index 63111cb082fe..4c5114765b23 100644
--- a/tools/testing/selftests/bpf/test_tcpnotify_user.c
+++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
@@ -72,7 +72,6 @@ int main(int argc, char **argv)
 {
 	const char *file = "test_tcpnotify_kern.o";
 	struct bpf_map *perf_map, *global_map;
-	struct perf_buffer_opts pb_opts = {};
 	struct tcpnotify_globals g = {0};
 	struct perf_buffer *pb = NULL;
 	const char *cg_path = "/foo";
@@ -117,8 +116,7 @@ int main(int argc, char **argv)
 		return -1;
 	}
 
-	pb_opts.sample_cb = dummyfn;
-	pb = perf_buffer__new(bpf_map__fd(perf_map), 8, &pb_opts);
+	pb = perf_buffer__new(bpf_map__fd(perf_map), 8, dummyfn, NULL, NULL, NULL);
 	if (!pb)
 		goto err;
 
-- 
2.30.2


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

* [PATCH bpf-next 09/11] selftests/bpf: update btf_dump__new() uses to v1.0+ variant
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
                   ` (7 preceding siblings ...)
  2021-11-08  6:13 ` [PATCH bpf-next 08/11] selftests/bpf: migrate all deprecated perf_buffer uses Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 10/11] tools/runqslower: update perf_buffer__new() calls Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 11/11] bpftool: update btf_dump__new() and perf_buffer__new_raw() calls Andrii Nakryiko
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Update to-be-deprecated forms of btf_dump__new().

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/btf_helpers.c     |  4 +--
 .../selftests/bpf/prog_tests/btf_dump.c       | 33 ++++++++-----------
 .../selftests/bpf/prog_tests/btf_split.c      |  4 +--
 3 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/bpf/btf_helpers.c b/tools/testing/selftests/bpf/btf_helpers.c
index 3d1a748d09d8..acb59202486d 100644
--- a/tools/testing/selftests/bpf/btf_helpers.c
+++ b/tools/testing/selftests/bpf/btf_helpers.c
@@ -238,7 +238,6 @@ const char *btf_type_c_dump(const struct btf *btf)
 	static char buf[16 * 1024];
 	FILE *buf_file;
 	struct btf_dump *d = NULL;
-	struct btf_dump_opts opts = {};
 	int err, i;
 
 	buf_file = fmemopen(buf, sizeof(buf) - 1, "w");
@@ -247,8 +246,7 @@ const char *btf_type_c_dump(const struct btf *btf)
 		return NULL;
 	}
 
-	opts.ctx = buf_file;
-	d = btf_dump__new(btf, NULL, &opts, btf_dump_printf);
+	d = btf_dump__new(btf, btf_dump_printf, buf_file, NULL);
 	if (libbpf_get_error(d)) {
 		fprintf(stderr, "Failed to create btf_dump instance: %ld\n", libbpf_get_error(d));
 		goto err_out;
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index a04961942dfa..d6272013a5a3 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -13,25 +13,23 @@ static struct btf_dump_test_case {
 	const char *name;
 	const char *file;
 	bool known_ptr_sz;
-	struct btf_dump_opts opts;
 } btf_dump_test_cases[] = {
-	{"btf_dump: syntax", "btf_dump_test_case_syntax", true, {}},
-	{"btf_dump: ordering", "btf_dump_test_case_ordering", false, {}},
-	{"btf_dump: padding", "btf_dump_test_case_padding", true, {}},
-	{"btf_dump: packing", "btf_dump_test_case_packing", true, {}},
-	{"btf_dump: bitfields", "btf_dump_test_case_bitfields", true, {}},
-	{"btf_dump: multidim", "btf_dump_test_case_multidim", false, {}},
-	{"btf_dump: namespacing", "btf_dump_test_case_namespacing", false, {}},
+	{"btf_dump: syntax", "btf_dump_test_case_syntax", true},
+	{"btf_dump: ordering", "btf_dump_test_case_ordering", false},
+	{"btf_dump: padding", "btf_dump_test_case_padding", true},
+	{"btf_dump: packing", "btf_dump_test_case_packing", true},
+	{"btf_dump: bitfields", "btf_dump_test_case_bitfields", true},
+	{"btf_dump: multidim", "btf_dump_test_case_multidim", false},
+	{"btf_dump: namespacing", "btf_dump_test_case_namespacing", false},
 };
 
-static int btf_dump_all_types(const struct btf *btf,
-			      const struct btf_dump_opts *opts)
+static int btf_dump_all_types(const struct btf *btf, void *ctx)
 {
 	size_t type_cnt = btf__type_cnt(btf);
 	struct btf_dump *d;
 	int err = 0, id;
 
-	d = btf_dump__new(btf, NULL, opts, btf_dump_printf);
+	d = btf_dump__new(btf, btf_dump_printf, ctx, NULL);
 	err = libbpf_get_error(d);
 	if (err)
 		return err;
@@ -88,8 +86,7 @@ static int test_btf_dump_case(int n, struct btf_dump_test_case *t)
 		goto done;
 	}
 
-	t->opts.ctx = f;
-	err = btf_dump_all_types(btf, &t->opts);
+	err = btf_dump_all_types(btf, f);
 	fclose(f);
 	close(fd);
 	if (CHECK(err, "btf_dump", "failure during C dumping: %d\n", err)) {
@@ -137,7 +134,6 @@ static void test_btf_dump_incremental(void)
 {
 	struct btf *btf = NULL;
 	struct btf_dump *d = NULL;
-	struct btf_dump_opts opts;
 	int id, err, i;
 
 	dump_buf_file = open_memstream(&dump_buf, &dump_buf_sz);
@@ -146,8 +142,7 @@ static void test_btf_dump_incremental(void)
 	btf = btf__new_empty();
 	if (!ASSERT_OK_PTR(btf, "new_empty"))
 		goto err_out;
-	opts.ctx = dump_buf_file;
-	d = btf_dump__new(btf, NULL, &opts, btf_dump_printf);
+	d = btf_dump__new(btf, btf_dump_printf, dump_buf_file, NULL);
 	if (!ASSERT_OK(libbpf_get_error(d), "btf_dump__new"))
 		goto err_out;
 
@@ -815,7 +810,6 @@ static void test_btf_datasec(struct btf *btf, struct btf_dump *d, char *str,
 static void test_btf_dump_datasec_data(char *str)
 {
 	struct btf *btf;
-	struct btf_dump_opts opts = { .ctx = str };
 	char license[4] = "GPL";
 	struct btf_dump *d;
 
@@ -823,7 +817,7 @@ static void test_btf_dump_datasec_data(char *str)
 	if (!ASSERT_OK_PTR(btf, "xdping_kern.o BTF not found"))
 		return;
 
-	d = btf_dump__new(btf, NULL, &opts, btf_dump_snprintf);
+	d = btf_dump__new(btf, btf_dump_snprintf, str, NULL);
 	if (!ASSERT_OK_PTR(d, "could not create BTF dump"))
 		goto out;
 
@@ -837,7 +831,6 @@ static void test_btf_dump_datasec_data(char *str)
 
 void test_btf_dump() {
 	char str[STRSIZE];
-	struct btf_dump_opts opts = { .ctx = str };
 	struct btf_dump *d;
 	struct btf *btf;
 	int i;
@@ -857,7 +850,7 @@ void test_btf_dump() {
 	if (!ASSERT_OK_PTR(btf, "no kernel BTF found"))
 		return;
 
-	d = btf_dump__new(btf, NULL, &opts, btf_dump_snprintf);
+	d = btf_dump__new(btf, btf_dump_snprintf, str, NULL);
 	if (!ASSERT_OK_PTR(d, "could not create BTF dump"))
 		return;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_split.c b/tools/testing/selftests/bpf/prog_tests/btf_split.c
index b1ffe61f2aa9..eef1158676ed 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_split.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_split.c
@@ -13,7 +13,6 @@ static void btf_dump_printf(void *ctx, const char *fmt, va_list args)
 }
 
 void test_btf_split() {
-	struct btf_dump_opts opts;
 	struct btf_dump *d = NULL;
 	const struct btf_type *t;
 	struct btf *btf1, *btf2;
@@ -68,8 +67,7 @@ void test_btf_split() {
 	dump_buf_file = open_memstream(&dump_buf, &dump_buf_sz);
 	if (!ASSERT_OK_PTR(dump_buf_file, "dump_memstream"))
 		return;
-	opts.ctx = dump_buf_file;
-	d = btf_dump__new(btf2, NULL, &opts, btf_dump_printf);
+	d = btf_dump__new(btf2, btf_dump_printf, dump_buf_file, NULL);
 	if (!ASSERT_OK_PTR(d, "btf_dump__new"))
 		goto cleanup;
 	for (i = 1; i < btf__type_cnt(btf2); i++) {
-- 
2.30.2


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

* [PATCH bpf-next 10/11] tools/runqslower: update perf_buffer__new() calls
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
                   ` (8 preceding siblings ...)
  2021-11-08  6:13 ` [PATCH bpf-next 09/11] selftests/bpf: update btf_dump__new() uses to v1.0+ variant Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  2021-11-08  6:13 ` [PATCH bpf-next 11/11] bpftool: update btf_dump__new() and perf_buffer__new_raw() calls Andrii Nakryiko
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Use v1.0+ compatible variant of perf_buffer__new() call to prepare for
deprecation.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/runqslower/runqslower.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/bpf/runqslower/runqslower.c b/tools/bpf/runqslower/runqslower.c
index d89715844952..2414cc764461 100644
--- a/tools/bpf/runqslower/runqslower.c
+++ b/tools/bpf/runqslower/runqslower.c
@@ -123,7 +123,6 @@ int main(int argc, char **argv)
 		.parser = parse_arg,
 		.doc = argp_program_doc,
 	};
-	struct perf_buffer_opts pb_opts;
 	struct perf_buffer *pb = NULL;
 	struct runqslower_bpf *obj;
 	int err;
@@ -165,9 +164,8 @@ int main(int argc, char **argv)
 	printf("Tracing run queue latency higher than %llu us\n", env.min_us);
 	printf("%-8s %-16s %-6s %14s\n", "TIME", "COMM", "PID", "LAT(us)");
 
-	pb_opts.sample_cb = handle_event;
-	pb_opts.lost_cb = handle_lost_events;
-	pb = perf_buffer__new(bpf_map__fd(obj->maps.events), 64, &pb_opts);
+	pb = perf_buffer__new(bpf_map__fd(obj->maps.events), 64,
+			      handle_event, handle_lost_events, NULL, NULL);
 	err = libbpf_get_error(pb);
 	if (err) {
 		pb = NULL;
-- 
2.30.2


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

* [PATCH bpf-next 11/11] bpftool: update btf_dump__new() and perf_buffer__new_raw() calls
  2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
                   ` (9 preceding siblings ...)
  2021-11-08  6:13 ` [PATCH bpf-next 10/11] tools/runqslower: update perf_buffer__new() calls Andrii Nakryiko
@ 2021-11-08  6:13 ` Andrii Nakryiko
  10 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-08  6:13 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Use v1.0-compatible variants of btf_dump and perf_buffer "constructors".
This is also a demonstration of reusing struct perf_buffer_raw_opts as
OPTS-style option struct for new perf_buffer__new_raw() API.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/bpftool/btf.c           | 2 +-
 tools/bpf/bpftool/gen.c           | 2 +-
 tools/bpf/bpftool/map_perf_ring.c | 9 +++------
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 015d2758f826..223ac7676027 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -418,7 +418,7 @@ static int dump_btf_c(const struct btf *btf,
 	struct btf_dump *d;
 	int err = 0, i;
 
-	d = btf_dump__new(btf, NULL, NULL, btf_dump_printf);
+	d = btf_dump__new(btf, btf_dump_printf, NULL, NULL);
 	if (IS_ERR(d))
 		return PTR_ERR(d);
 
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 5c18351290f0..89f0e828bbfa 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -218,7 +218,7 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
 	char sec_ident[256], map_ident[256];
 	int i, err = 0;
 
-	d = btf_dump__new(btf, NULL, NULL, codegen_btf_dump_printf);
+	d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
 	if (IS_ERR(d))
 		return PTR_ERR(d);
 
diff --git a/tools/bpf/bpftool/map_perf_ring.c b/tools/bpf/bpftool/map_perf_ring.c
index b98ea702d284..6b0c410152de 100644
--- a/tools/bpf/bpftool/map_perf_ring.c
+++ b/tools/bpf/bpftool/map_perf_ring.c
@@ -124,7 +124,7 @@ int do_event_pipe(int argc, char **argv)
 		.wakeup_events = 1,
 	};
 	struct bpf_map_info map_info = {};
-	struct perf_buffer_raw_opts opts = {};
+	LIBBPF_OPTS(perf_buffer_raw_opts, opts);
 	struct event_pipe_ctx ctx = {
 		.all_cpus = true,
 		.cpu = -1,
@@ -190,14 +190,11 @@ int do_event_pipe(int argc, char **argv)
 		ctx.idx = 0;
 	}
 
-	opts.attr = &perf_attr;
-	opts.event_cb = print_bpf_output;
-	opts.ctx = &ctx;
 	opts.cpu_cnt = ctx.all_cpus ? 0 : 1;
 	opts.cpus = &ctx.cpu;
 	opts.map_keys = &ctx.idx;
-
-	pb = perf_buffer__new_raw(map_fd, MMAP_PAGE_CNT, &opts);
+	pb = perf_buffer__new_raw(map_fd, MMAP_PAGE_CNT, &perf_attr,
+				  print_bpf_output, &ctx, &opts);
 	err = libbpf_get_error(pb);
 	if (err) {
 		p_err("failed to create perf buffer: %s (%d)",
-- 
2.30.2


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

* Re: [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof
  2021-11-08  6:13 ` [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof Andrii Nakryiko
@ 2021-11-09  3:38   ` Alexei Starovoitov
  2021-11-09 15:37     ` Andrii Nakryiko
  0 siblings, 1 reply; 20+ messages in thread
From: Alexei Starovoitov @ 2021-11-09  3:38 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, ast, daniel, kernel-team

On Sun, Nov 07, 2021 at 10:13:11PM -0800, Andrii Nakryiko wrote:
> +#define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr(		      \
> +	__builtin_types_compatible_p(typeof(a4), btf_dump_printf_fn_t) +      \
> +	__builtin_types_compatible_p(typeof(a4),			      \
> +				     void(void *, const char *, va_list)),    \
> +	btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4),\
> +	btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4))

why '+' in the above? The return type of __builtin_types_compatible_p() is bool.
What is bool + bool ?
It suppose to be logical 'OR', right?

Maybe checking for ops type would be more robust ?
Like:
#define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr(		      \
	__builtin_types_compatible_p(typeof(a4), const struct btf_dump_opts*), \
	btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4),        \
	btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4))

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

* Re: [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib
  2021-11-08  6:13 ` [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib Andrii Nakryiko
@ 2021-11-09  3:44   ` Alexei Starovoitov
  2021-11-09 15:42     ` Andrii Nakryiko
  0 siblings, 1 reply; 20+ messages in thread
From: Alexei Starovoitov @ 2021-11-09  3:44 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, ast, daniel, kernel-team

On Sun, Nov 07, 2021 at 10:13:09PM -0800, Andrii Nakryiko wrote:
> Add test_progs-shared flavor to compile against libbpf as a shared
> library. This is useful to make sure that libbpf's backwards/forward
> compatibility guarantees are upheld. Currently this has to be checked
> locally, but in the future we'll automate at least some scenarios as
> part of libbpf CI runs.
> 
> Biggest change is how either libbpf.a or libbpf.so is passed to the
> compiler, which is controled on per-flavor through a new TRUNNER_LIBBPF
> parameter. All the places that depend on libbpf artifacts (headers,
> library itself, etc) to be built are moved to order-only dependency on
> $(BPFOBJ). rpath is used to specify relative location to where libbpf.so
> should be so that when test_progs-shared is run under QEMU, libbpf.so is
> still going to be discovered correctly.
> 
> Few selftests are using or testing internal libbpf APIs, so are not
> compatible with shared library use of libbpf. Filter them out for shared
> flavor.
...
> +# Define test_progs-shared test runner.
> +TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
> +TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
> +TRUNNER_EXTRA_CFLAGS := -Wl,-rpath=$(subst $(CURDIR)/,,$(dir $(BPFOBJ)))
> +TRUNNER_LIBBPF := $(patsubst %libbpf.a,%libbpf.so,$(BPFOBJ))
> +TRUNNER_TESTS_BLACKLIST := cpu_mask.c hashmap.c perf_buffer.c raw_tp_test_run.c
> +$(eval $(call DEFINE_TEST_RUNNER,test_progs,shared))
> +TRUNNER_TESTS_BLACKLIST :=

It's a good idea to add libbpf.so test, but going through test_progs is imo overkill.
No reason to run more than one test with shared lib.
If it links fine it's pretty much certain that it will work.
Maybe convert test_maps into shared only? CI runs it already.

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

* Re: [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof
  2021-11-09  3:38   ` Alexei Starovoitov
@ 2021-11-09 15:37     ` Andrii Nakryiko
  2021-11-09 17:40       ` Alexei Starovoitov
  0 siblings, 1 reply; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-09 15:37 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Mon, Nov 8, 2021 at 7:38 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Nov 07, 2021 at 10:13:11PM -0800, Andrii Nakryiko wrote:
> > +#define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr(               \
> > +     __builtin_types_compatible_p(typeof(a4), btf_dump_printf_fn_t) +      \
> > +     __builtin_types_compatible_p(typeof(a4),                              \
> > +                                  void(void *, const char *, va_list)),    \
> > +     btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4),\
> > +     btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4))
>
> why '+' in the above? The return type of __builtin_types_compatible_p() is bool.
> What is bool + bool ?
> It suppose to be logical 'OR', right?

__builtin_types_compatible_p() is defined as returning 0 or 1 (not
true/false). And __builtin_choose_expr() is also defined as comparing
first argument against 0, not as true/false. But in practice it
doesn't matter because bool is converted to 0 or 1 in arithmetic
operations. So I can switch to || with no effect. Let me know if you
still prefer logical || and I'll change.

But yes to your last question, it's logical OR.

>
> Maybe checking for ops type would be more robust ?

opts can be NULL. At which point it's actually only compatible with
void *. The only non-null args are btf (same for both variants, so no
go) or callback. So I have to check the callback. From my
experimentations with various possible invocations (locally, not in
any test), this check is quite robust. I need to check two types:
pointer to func and func itself (C allows that, turns out; I learned
it from [0]).

  [0] https://github.com/cilium/ebpf/issues/214

> Like:
> #define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr(                  \
>         __builtin_types_compatible_p(typeof(a4), const struct btf_dump_opts*), \
>         btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4),        \
>         btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4))

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

* Re: [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib
  2021-11-09  3:44   ` Alexei Starovoitov
@ 2021-11-09 15:42     ` Andrii Nakryiko
  2021-11-09 17:53       ` Alexei Starovoitov
  0 siblings, 1 reply; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-09 15:42 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Mon, Nov 8, 2021 at 7:44 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Nov 07, 2021 at 10:13:09PM -0800, Andrii Nakryiko wrote:
> > Add test_progs-shared flavor to compile against libbpf as a shared
> > library. This is useful to make sure that libbpf's backwards/forward
> > compatibility guarantees are upheld. Currently this has to be checked
> > locally, but in the future we'll automate at least some scenarios as
> > part of libbpf CI runs.
> >
> > Biggest change is how either libbpf.a or libbpf.so is passed to the
> > compiler, which is controled on per-flavor through a new TRUNNER_LIBBPF
> > parameter. All the places that depend on libbpf artifacts (headers,
> > library itself, etc) to be built are moved to order-only dependency on
> > $(BPFOBJ). rpath is used to specify relative location to where libbpf.so
> > should be so that when test_progs-shared is run under QEMU, libbpf.so is
> > still going to be discovered correctly.
> >
> > Few selftests are using or testing internal libbpf APIs, so are not
> > compatible with shared library use of libbpf. Filter them out for shared
> > flavor.
> ...
> > +# Define test_progs-shared test runner.
> > +TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
> > +TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
> > +TRUNNER_EXTRA_CFLAGS := -Wl,-rpath=$(subst $(CURDIR)/,,$(dir $(BPFOBJ)))
> > +TRUNNER_LIBBPF := $(patsubst %libbpf.a,%libbpf.so,$(BPFOBJ))
> > +TRUNNER_TESTS_BLACKLIST := cpu_mask.c hashmap.c perf_buffer.c raw_tp_test_run.c
> > +$(eval $(call DEFINE_TEST_RUNNER,test_progs,shared))
> > +TRUNNER_TESTS_BLACKLIST :=
>
> It's a good idea to add libbpf.so test, but going through test_progs is imo overkill.
> No reason to run more than one test with shared lib.
> If it links fine it's pretty much certain that it will work.
> Maybe convert test_maps into shared only? CI runs it already.

If some APIs are not used from a user app, their symbols won't be used
during dynamic linking, so they won't be tested neither for linking
nor functionally. E.g., in this case all the btf_dump__new(),
btf__dedup(), etc, invocations won't be verified even at dynamic
linking time. At least that's my understanding from looking at
generated ELF binaries. And that was the sole motivator (at least
initially) to add -shared flavor, so that I can make sure it works.
Learned a bit about symbol versioning and symbol visibility from that,
so definitely not a waste of time.

But more broadly, what's the concern? User-space part compilation time
for test_progs is extremely fast, it's the BPF object compilation that
is slow. I'll send another Makefile change to eliminate the third
compilation variant for BPF source code in the next few days (will
roll it into this patch set unless it lands first), so that will go
away.

It seems useful to have a full-featured testing ground for libbpf.so,
especially that I don't use (and thus don't test and validate
constantly) it in practice in my own applications.

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

* Re: [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof
  2021-11-09 15:37     ` Andrii Nakryiko
@ 2021-11-09 17:40       ` Alexei Starovoitov
  2021-11-10  0:10         ` Andrii Nakryiko
  0 siblings, 1 reply; 20+ messages in thread
From: Alexei Starovoitov @ 2021-11-09 17:40 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Nov 09, 2021 at 07:37:48AM -0800, Andrii Nakryiko wrote:
> On Mon, Nov 8, 2021 at 7:38 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Sun, Nov 07, 2021 at 10:13:11PM -0800, Andrii Nakryiko wrote:
> > > +#define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr(               \
> > > +     __builtin_types_compatible_p(typeof(a4), btf_dump_printf_fn_t) +      \
> > > +     __builtin_types_compatible_p(typeof(a4),                              \
> > > +                                  void(void *, const char *, va_list)),    \
> > > +     btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4),\
> > > +     btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4))
> >
> > why '+' in the above? The return type of __builtin_types_compatible_p() is bool.
> > What is bool + bool ?
> > It suppose to be logical 'OR', right?
> 
> __builtin_types_compatible_p() is defined as returning 0 or 1 (not
> true/false). And __builtin_choose_expr() is also defined as comparing
> first argument against 0, not as true/false. But in practice it
> doesn't matter because bool is converted to 0 or 1 in arithmetic
> operations. So I can switch to || with no effect. Let me know if you
> still prefer logical || and I'll change.
> 
> But yes to your last question, it's logical OR.

Interesting. Looking at LLVM code it does indeed returns 'int'.
At least typeof(_builtin_types_compatible_p(..)) seems to be 'int'.

At the same type LLVM tests are using this macro:
#define check_same_type(type1, type2) __builtin_types_compatible_p(type1, type2) && __builtin_types_compatible_p(type1 *, type2 *)

While kernel has this macro:
#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))

Guessing that extra typeof() may resolve the difference between fn and &fn ?
Not sure why LLVM took that approach.

Anyway it will be removed once we hit 1.0, so no need to dig too deep.
I think changing + to || is still worth doing.

> >
> > Maybe checking for ops type would be more robust ?
> 
> opts can be NULL. At which point it's actually only compatible with
> void *. 

Assuming that fn pointer in btf_dump__new_deprecated() will never be NULL?

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

* Re: [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib
  2021-11-09 15:42     ` Andrii Nakryiko
@ 2021-11-09 17:53       ` Alexei Starovoitov
  2021-11-10  0:52         ` Andrii Nakryiko
  0 siblings, 1 reply; 20+ messages in thread
From: Alexei Starovoitov @ 2021-11-09 17:53 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Nov 09, 2021 at 07:42:29AM -0800, Andrii Nakryiko wrote:
> On Mon, Nov 8, 2021 at 7:44 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Sun, Nov 07, 2021 at 10:13:09PM -0800, Andrii Nakryiko wrote:
> > > Add test_progs-shared flavor to compile against libbpf as a shared
> > > library. This is useful to make sure that libbpf's backwards/forward
> > > compatibility guarantees are upheld. Currently this has to be checked
> > > locally, but in the future we'll automate at least some scenarios as
> > > part of libbpf CI runs.
> > >
> > > Biggest change is how either libbpf.a or libbpf.so is passed to the
> > > compiler, which is controled on per-flavor through a new TRUNNER_LIBBPF
> > > parameter. All the places that depend on libbpf artifacts (headers,
> > > library itself, etc) to be built are moved to order-only dependency on
> > > $(BPFOBJ). rpath is used to specify relative location to where libbpf.so
> > > should be so that when test_progs-shared is run under QEMU, libbpf.so is
> > > still going to be discovered correctly.
> > >
> > > Few selftests are using or testing internal libbpf APIs, so are not
> > > compatible with shared library use of libbpf. Filter them out for shared
> > > flavor.
> > ...
> > > +# Define test_progs-shared test runner.
> > > +TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
> > > +TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
> > > +TRUNNER_EXTRA_CFLAGS := -Wl,-rpath=$(subst $(CURDIR)/,,$(dir $(BPFOBJ)))
> > > +TRUNNER_LIBBPF := $(patsubst %libbpf.a,%libbpf.so,$(BPFOBJ))
> > > +TRUNNER_TESTS_BLACKLIST := cpu_mask.c hashmap.c perf_buffer.c raw_tp_test_run.c
> > > +$(eval $(call DEFINE_TEST_RUNNER,test_progs,shared))
> > > +TRUNNER_TESTS_BLACKLIST :=
> >
> > It's a good idea to add libbpf.so test, but going through test_progs is imo overkill.
> > No reason to run more than one test with shared lib.
> > If it links fine it's pretty much certain that it will work.
> > Maybe convert test_maps into shared only? CI runs it already.
> 
> If some APIs are not used from a user app, their symbols won't be used
> during dynamic linking, so they won't be tested neither for linking
> nor functionally. E.g., in this case all the btf_dump__new(),
> btf__dedup(), etc, invocations won't be verified even at dynamic
> linking time. At least that's my understanding from looking at
> generated ELF binaries. And that was the sole motivator (at least
> initially) to add -shared flavor, so that I can make sure it works.
> Learned a bit about symbol versioning and symbol visibility from that,
> so definitely not a waste of time.
> 
> But more broadly, what's the concern? User-space part compilation time
> for test_progs is extremely fast, it's the BPF object compilation that
> is slow. I'll send another Makefile change to eliminate the third
> compilation variant for BPF source code in the next few days (will
> roll it into this patch set unless it lands first), so that will go
> away.

exactly. Rebuilding the tests for 3rd variant is a build time waste.
Even linking test_progs as shared is misleading.
There is no need to run such flavor.
I suspect test_progs doesn't use 100% of libbpf api.
test_maps is using less than test_progs. If the goal is to cover
all of libbpf then how about adding all calls from libbpf.map
to test_maps or standalone new binary to check link+run sanity.
I understand the appeal to follow the pattern of different
test_progs flavors, but shared doesn't fit.
Different flavors are used when tests themselves are compiled differently.
Not when test_progs runner is different.
imo standalone binary that calls 100% of func from libbpf.map will
serve us better in the long run from maintainability pov.
That will be an accurate unit test for .so functionality.

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

* Re: [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof
  2021-11-09 17:40       ` Alexei Starovoitov
@ 2021-11-10  0:10         ` Andrii Nakryiko
  0 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-10  0:10 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Nov 9, 2021 at 9:40 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 09, 2021 at 07:37:48AM -0800, Andrii Nakryiko wrote:
> > On Mon, Nov 8, 2021 at 7:38 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Sun, Nov 07, 2021 at 10:13:11PM -0800, Andrii Nakryiko wrote:
> > > > +#define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr(               \
> > > > +     __builtin_types_compatible_p(typeof(a4), btf_dump_printf_fn_t) +      \
> > > > +     __builtin_types_compatible_p(typeof(a4),                              \
> > > > +                                  void(void *, const char *, va_list)),    \
> > > > +     btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4),\
> > > > +     btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4))
> > >
> > > why '+' in the above? The return type of __builtin_types_compatible_p() is bool.
> > > What is bool + bool ?
> > > It suppose to be logical 'OR', right?
> >
> > __builtin_types_compatible_p() is defined as returning 0 or 1 (not
> > true/false). And __builtin_choose_expr() is also defined as comparing
> > first argument against 0, not as true/false. But in practice it
> > doesn't matter because bool is converted to 0 or 1 in arithmetic
> > operations. So I can switch to || with no effect. Let me know if you
> > still prefer logical || and I'll change.
> >
> > But yes to your last question, it's logical OR.
>
> Interesting. Looking at LLVM code it does indeed returns 'int'.
> At least typeof(_builtin_types_compatible_p(..)) seems to be 'int'.
>
> At the same type LLVM tests are using this macro:
> #define check_same_type(type1, type2) __builtin_types_compatible_p(type1, type2) && __builtin_types_compatible_p(type1 *, type2 *)
>
> While kernel has this macro:
> #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>
> Guessing that extra typeof() may resolve the difference between fn and &fn ?

no, btf_dump_printf_fn_t is already a type and typeof() doesn't seem
to change anything. I had a test like below. It produces the same
results with or without typeof.


static void test1(btf_dump_printf_fn_t arg, int val1, int val2, int
val3, int val4)
{
        fprintf(stderr, "TEST1 VAL %d %d %d %d\n", val1, val2, val3, val4);
}

static void test2(struct btf *arg, int val1, int val2, int val3, int val4)
{
        fprintf(stderr, "TEST2 VAL %d %d %d %d\n", val1, val2, val3, val4);
}

#define test_variad(arg) \
        __builtin_choose_expr(\
                __builtin_types_compatible_p(typeof(arg),
typeof(btf_dump_printf_fn_t)) + \
                __builtin_types_compatible_p(typeof(arg),
typeof(void(void *, const char *, va_list))),\
                test1((void *)arg,\
                      __builtin_types_compatible_p(typeof(arg),
typeof(btf_dump_printf_fn_t)),\
                      __builtin_types_compatible_p(typeof(arg),
typeof(void(void *, const char *, va_list))),\
                      __builtin_types_compatible_p(typeof(arg), struct btf *),\
                      __builtin_types_compatible_p(typeof(arg), void *)\
                ), \
                test2((void *)arg,\
                      __builtin_types_compatible_p(typeof(arg),
typeof(btf_dump_printf_fn_t)),\
                      __builtin_types_compatible_p(typeof(arg),
typeof(void(void *, const char *, va_list))),\
                      __builtin_types_compatible_p(typeof(arg), struct btf *),\
                      __builtin_types_compatible_p(typeof(arg), void *)\
                )\
        )

And then I call

        test_variad(codegen_btf_dump_printf);
        test_variad(&codegen_btf_dump_printf);
        test_variad(btf);
        test_variad(NULL);


I always get this (both with and without extra typeof()):

TEST1 VAL 0 1 0 0
TEST1 VAL 1 0 0 0
TEST2 VAL 0 0 1 0
TEST2 VAL 0 0 0 1


> Not sure why LLVM took that approach.

I think kernel's __same_type() doesn't handle this well. I've changed
kernel/bpf/hashtab.c like this:

        BUILD_BUG_ON(!__same_type(&__htab_map_lookup_elem,
-                    (void *(*)(struct bpf_map *map, void *key))NULL));
+                    void *(struct bpf_map *map, void *key)));

And it triggered compilation assertion.

Then I "fixed" it like this:

-       BUILD_BUG_ON(!__same_type(&__htab_map_lookup_elem,
-                    (void *(*)(struct bpf_map *map, void *key))NULL));
+       BUILD_BUG_ON(!__same_type(__htab_map_lookup_elem,
+                    void *(struct bpf_map *map, void *key)));

And it compiled just fine. So __same_type() is sensitive to &.

>
> Anyway it will be removed once we hit 1.0, so no need to dig too deep.
> I think changing + to || is still worth doing.

ok, I'll update in next revision

>
> > >
> > > Maybe checking for ops type would be more robust ?
> >
> > opts can be NULL. At which point it's actually only compatible with
> > void *.
>
> Assuming that fn pointer in btf_dump__new_deprecated() will never be NULL?

If fn pointer is NULL, btf_dump APIs will crash in runtime. I'll add
an explicit check and error out in such case with -EINVAL.

The way the check is written right now, if someone passes NULL we'll
choose non-deprecated btf_dump__new() variant, which seems to be a
good "default" (even though it will still crash later when calling a
callback).

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

* Re: [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib
  2021-11-09 17:53       ` Alexei Starovoitov
@ 2021-11-10  0:52         ` Andrii Nakryiko
  0 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2021-11-10  0:52 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Tue, Nov 9, 2021 at 9:53 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 09, 2021 at 07:42:29AM -0800, Andrii Nakryiko wrote:
> > On Mon, Nov 8, 2021 at 7:44 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Sun, Nov 07, 2021 at 10:13:09PM -0800, Andrii Nakryiko wrote:
> > > > Add test_progs-shared flavor to compile against libbpf as a shared
> > > > library. This is useful to make sure that libbpf's backwards/forward
> > > > compatibility guarantees are upheld. Currently this has to be checked
> > > > locally, but in the future we'll automate at least some scenarios as
> > > > part of libbpf CI runs.
> > > >
> > > > Biggest change is how either libbpf.a or libbpf.so is passed to the
> > > > compiler, which is controled on per-flavor through a new TRUNNER_LIBBPF
> > > > parameter. All the places that depend on libbpf artifacts (headers,
> > > > library itself, etc) to be built are moved to order-only dependency on
> > > > $(BPFOBJ). rpath is used to specify relative location to where libbpf.so
> > > > should be so that when test_progs-shared is run under QEMU, libbpf.so is
> > > > still going to be discovered correctly.
> > > >
> > > > Few selftests are using or testing internal libbpf APIs, so are not
> > > > compatible with shared library use of libbpf. Filter them out for shared
> > > > flavor.
> > > ...
> > > > +# Define test_progs-shared test runner.
> > > > +TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
> > > > +TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
> > > > +TRUNNER_EXTRA_CFLAGS := -Wl,-rpath=$(subst $(CURDIR)/,,$(dir $(BPFOBJ)))
> > > > +TRUNNER_LIBBPF := $(patsubst %libbpf.a,%libbpf.so,$(BPFOBJ))
> > > > +TRUNNER_TESTS_BLACKLIST := cpu_mask.c hashmap.c perf_buffer.c raw_tp_test_run.c
> > > > +$(eval $(call DEFINE_TEST_RUNNER,test_progs,shared))
> > > > +TRUNNER_TESTS_BLACKLIST :=
> > >
> > > It's a good idea to add libbpf.so test, but going through test_progs is imo overkill.
> > > No reason to run more than one test with shared lib.
> > > If it links fine it's pretty much certain that it will work.
> > > Maybe convert test_maps into shared only? CI runs it already.
> >
> > If some APIs are not used from a user app, their symbols won't be used
> > during dynamic linking, so they won't be tested neither for linking
> > nor functionally. E.g., in this case all the btf_dump__new(),
> > btf__dedup(), etc, invocations won't be verified even at dynamic
> > linking time. At least that's my understanding from looking at
> > generated ELF binaries. And that was the sole motivator (at least
> > initially) to add -shared flavor, so that I can make sure it works.
> > Learned a bit about symbol versioning and symbol visibility from that,
> > so definitely not a waste of time.
> >
> > But more broadly, what's the concern? User-space part compilation time
> > for test_progs is extremely fast, it's the BPF object compilation that
> > is slow. I'll send another Makefile change to eliminate the third
> > compilation variant for BPF source code in the next few days (will
> > roll it into this patch set unless it lands first), so that will go
> > away.
>
> exactly. Rebuilding the tests for 3rd variant is a build time waste.

I've built selftests with and without this patch set. With -j8 and
-j80 (because I can ;) ). I forced re-building only the user-space
part (e.g., by `touch flow_dissector_load.h && make -j8`).

WITH SHARED
===========
make -j8  40.47s user 12.85s system 757% cpu 7.037 total
make -j80  65.21s user 22.47s system 4370% cpu 2.006 total

WITHOUT SHARED
==============
make -j8  27.82s user 8.49s system 747% cpu 4.858 total
make -j80  43.56s user 17.95s system 4290% cpu 1.433 total

2.2 seconds and 0.6 seconds build time difference. Hardly that big of a deal.

> Even linking test_progs as shared is misleading.

Because flavor *has to* denote the flavor of BPF? Why is that so?

> There is no need to run such flavor.
> I suspect test_progs doesn't use 100% of libbpf api.

test_progs is the best we have in terms of libbpf API coverage,
though. Not perfect and not 100%, but by far the most complete. And
where all the usability features go into, including parallelization,
error summaries, etc. test_maps is not going to get most (or any) of
that.

> test_maps is using less than test_progs. If the goal is to cover
> all of libbpf then how about adding all calls from libbpf.map
> to test_maps or standalone new binary to check link+run sanity.

I'd rather have end-to-end (including runtime correctness) testing,
especially that it doesn't require lots of new code to be written or
an added ongoing maintenance.

> I understand the appeal to follow the pattern of different
> test_progs flavors, but shared doesn't fit.

It doesn't have to denote *BPF* flavor specifically. It's just a
flavor. We don't have specified anywhere that it has to only specify
differences in how BPF code is built.

> Different flavors are used when tests themselves are compiled differently.
> Not when test_progs runner is different.

*Tests* are compiled differently. Even test_progs.c itself is compiled
differently (it also uses libbpf APIs). Static libbpf vs shared
libbpf. You are talking about BPF programs, but that's only part of
the test, not *the test* in isolation.

> imo standalone binary that calls 100% of func from libbpf.map will
> serve us better in the long run from maintainability pov.

There are literally zero changes to prog_tests/*.c, I fail to see how
that induces maintainability cost.

> That will be an accurate unit test for .so functionality.

Only for dynamic linking part. Not for runtime correctness and proving
that the right versions of symbols were resolved.


I don't think any of the above arguments are really relevant. The
bigger problem is that we will now have a 3rd flavor of test_progs,
which you think everyone will have to run every single time, so that
adds a minute of extra testing time. From my perspective, when I was
working on adding -shared flavor I didn't assume we'll require anyone
to run it, unless they want/need. And I certainly wasn't going to run
it 100% of the time when testing locally.

BPF CI is supposed to run all the flavors, all the time. That's what
I'm optimizing for.

Having said that, the rest of the patch set is way more important. We
can always go back to -shared flavor later, if the need arises. I'll
drop first few patches and resubmit with the + -> || change.

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

end of thread, other threads:[~2021-11-10  0:52 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-08  6:13 [PATCH bpf-next 00/11] Future-proof more tricky libbpf APIs Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 01/11] bpftool: normalize compile rules to specify output file last Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 02/11] selftests/bpf: minor cleanups and normalization of Makefile Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 03/11] selftests/bpf: allow to generate per-flavor list of tests Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 04/11] selftests/bpf: add test_progs flavor using libbpf as a shared lib Andrii Nakryiko
2021-11-09  3:44   ` Alexei Starovoitov
2021-11-09 15:42     ` Andrii Nakryiko
2021-11-09 17:53       ` Alexei Starovoitov
2021-11-10  0:52         ` Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 05/11] libbpf: turn btf_dedup_opts into OPTS-based struct Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 06/11] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof Andrii Nakryiko
2021-11-09  3:38   ` Alexei Starovoitov
2021-11-09 15:37     ` Andrii Nakryiko
2021-11-09 17:40       ` Alexei Starovoitov
2021-11-10  0:10         ` Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 07/11] libbpf: make perf_buffer__new() use OPTS-based interface Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 08/11] selftests/bpf: migrate all deprecated perf_buffer uses Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 09/11] selftests/bpf: update btf_dump__new() uses to v1.0+ variant Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 10/11] tools/runqslower: update perf_buffer__new() calls Andrii Nakryiko
2021-11-08  6:13 ` [PATCH bpf-next 11/11] bpftool: update btf_dump__new() and perf_buffer__new_raw() calls Andrii Nakryiko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.