Message-ID: <CAEf4BzYSUoN2Boy-iveFAFGiiAMta5S9SK8aGO1BMnd+q2FzbA@mail.gmail.com>
Date:   Thu, 17 Oct 2019 10:48:23 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Stanislav Fomichev <sdf@...ichev.me>
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 Thu, Oct 17, 2019 at 9:07 AM Stanislav Fomichev <sdf@...ichev.me> wrote:
>
> 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).

So that's a good question and the answer is "no, we can't". And that's
why I consider alu32/bpf_gcc broken. Check progs/test_attach_probe.c,
it does BPF_OBJECT_EMBED, which links BPF object into test object
file. This means that if we want to compile BPF objects differently
between default/alu32/gcc flavors, we need to compile test_progs
independently. Embedding objects is going to be prevalent way to
consume them (and it is already the only way we consume them in
production at FB), so we need to accommodate it. With some more
usability improvements that's on my TODO list, it will become also
much more convenient and easy to consume such BPF objects.

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

So see my explanation above about why we need to compile flavors
independently. Rules have to be like they are here. I'd like to make
dependencies between test objects and BPF objects a bit more granular,
but only after we land this, it's already quite a lot of changes at
once.

Beyond fixing the rules, $(eval)/$(call) is a new stuff for
selftests/bpf's Makefile, but it's semantics is well described in
documentation and you can gloss over it for now, it shouldn't break
with Makefile changes.

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

Yes, it probably is. See above, I tried to give more context.

I've fixed some other inconveniences with current Makefile set up
(e.g., on-demand bpf_helper_defs.h re-generation, etc), but those are
minor changes and it was hard to de-couple from the main change.

>
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -157,26 +157,10 @@ TEST_VERIFIER_CFLAGS := -I. -I$(OUTPUT) -Iverifier
>

[...]

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

no worries, but with a bit of recurring reminder it becomes easier, I
know from my own experience :)

Powered by blists - more mailing lists