Message-ID: <20191017160716.GA2090@mini-arch>
Date:   Thu, 17 Oct 2019 09:07:16 -0700
From:   Stanislav Fomichev <sdf@...ichev.me>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v4 bpf-next 5/7] selftests/bpf: replace test_progs and
 test_maps w/ general rule

On 10/16, Andrii Nakryiko wrote:
> On Wed, Oct 16, 2019 at 9:32 AM Stanislav Fomichev <sdf@...ichev.me> wrote:
> >
> > On 10/15, Andrii Nakryiko wrote:
> > > Define test runner generation meta-rule that codifies dependencies
> > > between test runner, its tests, and its dependent BPF programs. Use that
> > > for defining test_progs and test_maps test-runners. Also additionally define
> > > 2 flavors of test_progs:
> > > - alu32, which builds BPF programs with 32-bit registers codegen;
> > > - bpf_gcc, which build BPF programs using GCC, if it supports BPF target.
> > Question:
> >
> > Why not merge test_maps tests into test_progs framework and have a
> > single binary instead of doing all this makefile-related work?
> > We can independently address the story with alu32/gcc progs (presumably
> > in the same manner, with make defines).
> 
> test_maps wasn't a reason for doing this, alue2/bpf_gcc was. test_maps
> is a simple sub-case that was just easy to convert to. I dare you to
> try solve alu32/bpf_gcc with make defines (whatever you mean by that)
> and in a simpler manner ;)
I think my concern comes from the fact that I don't really understand why
we need all that complexity (and the problem you're solving for alu/gcc;
part of that is that you're replacing everything, so it's hard to
understand what's the real diff).

In particular, why do we need to compile test_progs 3 times for
normal/alu32/gcc? Isn't it the same test_progs? Can we just teach test_progs
to run the tests for 3 output dirs with different versions of BPF programs?
(kind of like you do in your first patch with -<flavor>, but just in a loop).

> > I can hardly follow the existing makefile and now with the evals it's
> > 10x more complicated for no good reason.
> 
> I agree that existing Makefile logic is hard to follow, especially
> given it's broken. But I think 10x more complexity is gross
> exaggeration and just means you haven't tried to follow rules' logic.
Not 10x, but it does raise a complexity bar. I tried to follow the
rules, but I admit that I didn't try too hard :-)

> The rules inside DEFINE_TEST_RUNNER_RULES are exactly (minus one or
> two ifs to prevent re-definition of target) the rules that should have
> been written for test_progs, test_progs-alu32, test_progs-bpf_gcc.
> They define a chain of BPF .c -> BPF .o -> tests .c -> tests .o ->
> final binary + test.h generation. Previously we were getting away with
> this for, e.g., test_progs-alu32, because we always also built
> test_progs in parallel, which generated necessary stuff. Now with
> recent changes to test_attach_probe.c which now embeds BPF .o file,
> this doesn't work anymore. And it's going to be more and more
> prevalent form, so we need to fix it.
> 
> Surely $(eval) and $(call) are not common for simple Makefiles, but
> just ignore it, we need that to only dynamically generate
> per-test-runner rules. DEFINE_TEST_RUNNER_RULES can be almost read
> like a normal Makefile definitions, module $$(VAR) which is turned
> into a normal $(VAR) upon $(call) evaluation.
> 
> But really, I'd like to be wrong and if there is simpler way to
> achieve the same - go for it, I'll gladly review and ack.
Again, it probably comes from the fact that I don't see the problem
you're solving. Can we start by removing 3 test_progs variations
(somthing like patch below)? If we can do it, then the leftover parts
that generate alu32/gcc bpf program don't look too bad and can probably
be tweaked without makefile codegen.

--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -157,26 +157,10 @@ TEST_VERIFIER_CFLAGS := -I. -I$(OUTPUT) -Iverifier
 
 ifneq ($(SUBREG_CODEGEN),)
 ALU32_BUILD_DIR = $(OUTPUT)/alu32
-TEST_CUSTOM_PROGS += $(ALU32_BUILD_DIR)/test_progs_32
 $(ALU32_BUILD_DIR):
 	mkdir -p $@
 
-$(ALU32_BUILD_DIR)/urandom_read: $(OUTPUT)/urandom_read | $(ALU32_BUILD_DIR)
-	cp $< $@
-
-$(ALU32_BUILD_DIR)/test_progs_32: test_progs.c $(OUTPUT)/libbpf.a\
-						$(ALU32_BUILD_DIR)/urandom_read \
-						| $(ALU32_BUILD_DIR)
-	$(CC) $(TEST_PROGS_CFLAGS) $(CFLAGS) \
-		-o $(ALU32_BUILD_DIR)/test_progs_32 \
-		test_progs.c test_stub.c cgroup_helpers.c trace_helpers.c prog_tests/*.c \
-		$(OUTPUT)/libbpf.a $(LDLIBS)
-
-$(ALU32_BUILD_DIR)/test_progs_32: $(PROG_TESTS_H)
-$(ALU32_BUILD_DIR)/test_progs_32: prog_tests/*.c
-
-$(ALU32_BUILD_DIR)/%.o: progs/%.c $(ALU32_BUILD_DIR)/test_progs_32 \
-					| $(ALU32_BUILD_DIR)
+$(ALU32_BUILD_DIR)/%.o: progs/%.c | $(ALU32_BUILD_DIR)
 	($(CLANG) $(BPF_CFLAGS) $(CLANG_CFLAGS) -O2 -target bpf -emit-llvm \
 		-c $< -o - || echo "clang failed") | \
 	$(LLC) -march=bpf -mcpu=probe -mattr=+alu32 $(LLC_FLAGS) \
@@ -194,19 +178,10 @@ MENDIAN=-mlittle-endian
 endif
 BPF_GCC_CFLAGS = $(GCC_SYS_INCLUDES) $(MENDIAN)
 BPF_GCC_BUILD_DIR = $(OUTPUT)/bpf_gcc
-TEST_CUSTOM_PROGS += $(BPF_GCC_BUILD_DIR)/test_progs_bpf_gcc
 $(BPF_GCC_BUILD_DIR):
 	mkdir -p $@
 
-$(BPF_GCC_BUILD_DIR)/urandom_read: $(OUTPUT)/urandom_read | $(BPF_GCC_BUILD_DIR)
-	cp $< $@
-
-$(BPF_GCC_BUILD_DIR)/test_progs_bpf_gcc: $(OUTPUT)/test_progs \
-					 | $(BPF_GCC_BUILD_DIR)
-	cp $< $@
-
-$(BPF_GCC_BUILD_DIR)/%.o: progs/%.c $(BPF_GCC_BUILD_DIR)/test_progs_bpf_gcc \
-			  | $(BPF_GCC_BUILD_DIR)
+$(BPF_GCC_BUILD_DIR)/%.o: progs/%.c | $(BPF_GCC_BUILD_DIR)
 	$(BPF_GCC) $(BPF_CFLAGS) $(BPF_GCC_CFLAGS) -O2 -c $< -o $@
 endif
 
> Please truncate irrelevant parts, easier to review.
Sure, will do, but I always forget because I don't have this problem.
In mutt I can press shift+s to jump to the next unquoted section.

Powered by blists - more mailing lists