Message-ID: <20191017204430.GC2090@mini-arch>
Date:   Thu, 17 Oct 2019 13:44:30 -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/17, Andrii Nakryiko wrote:
> 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.
Thanks for the explanation, I missed that EMBED_FILE in attach_probe.c

I was going to suggest to move it out of test_progs (or use open() instead
of embedding?) if you want to test bpf_object__open_mem (to avoid all that
complexity and make test_progs work with any bpf alu32/gcc/clang subdir),
but it seems like you have pretty much settled on the embedding.

It's interesting that we try to do it a bit different here, maybe
even going as far as rolling out individual BPF .o files without
updating the control plane :-)

> > > > 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.
Again, thanks for the explanation, I did indeed miss that EMBED_FILE
case. But I'd still vote for moving that test into a dedicated binary,
have single test_progs that works with a flavored BPF subdir and simplify
the makefile instead of adding more stuff into it.

These are my 2c, feel free to ignore :-) We have our own simplified
reimplementation anyway to fit into our testing system, so I don't
have a horse in this race.

> 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