bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stanislav Fomichev <sdf@fomichev.me>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v4 bpf-next 5/7] selftests/bpf: replace test_progs and test_maps w/ general rule
Date: Thu, 17 Oct 2019 13:44:30 -0700	[thread overview]
Message-ID: <20191017204430.GC2090@mini-arch> (raw)
In-Reply-To: <CAEf4BzYSUoN2Boy-iveFAFGiiAMta5S9SK8aGO1BMnd+q2FzbA@mail.gmail.com>

On 10/17, Andrii Nakryiko wrote:
> On Thu, Oct 17, 2019 at 9:07 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 10/16, Andrii Nakryiko wrote:
> > > On Wed, Oct 16, 2019 at 9:32 AM Stanislav Fomichev <sdf@fomichev.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 :)

  reply	other threads:[~2019-10-17 20:44 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-16  6:00 [PATCH v4 bpf-next 0/7] Fix, clean up, and revamp selftests/bpf Makefile Andrii Nakryiko
2019-10-16  6:00 ` [PATCH v4 bpf-next 1/7] selftests/bpf: teach test_progs to cd into subdir Andrii Nakryiko
2019-10-16  6:00 ` [PATCH v4 bpf-next 2/7] selftests/bpf: make CO-RE reloc test impartial to test_progs flavor Andrii Nakryiko
2019-10-16  6:00 ` [PATCH v4 bpf-next 3/7] selftests/bpf: switch test_maps to test_progs' test.h format Andrii Nakryiko
2019-10-16  6:00 ` [PATCH v4 bpf-next 4/7] selftests/bpf: add simple per-test targets to Makefile Andrii Nakryiko
2019-10-16  6:00 ` [PATCH v4 bpf-next 5/7] selftests/bpf: replace test_progs and test_maps w/ general rule Andrii Nakryiko
2019-10-16 16:32   ` Stanislav Fomichev
2019-10-16 20:47     ` Andrii Nakryiko
2019-10-17 16:07       ` Stanislav Fomichev
2019-10-17 17:48         ` Andrii Nakryiko
2019-10-17 20:44           ` Stanislav Fomichev [this message]
2019-10-17 17:50   ` Andrii Nakryiko
2019-10-17 17:54     ` Alexei Starovoitov
2019-10-17 18:19       ` Andrii Nakryiko
2020-05-12 20:16   ` Yauheni Kaliuta
2020-05-12 22:13     ` Andrii Nakryiko
2020-05-13  1:58       ` Yauheni Kaliuta
2019-10-16  6:00 ` [PATCH v4 bpf-next 6/7] selftests/bpf: move test_queue_stack_map.h into progs/ where it belongs Andrii Nakryiko
2019-10-16  6:00 ` [PATCH v4 bpf-next 7/7] selftest/bpf: remove test_libbpf.sh and test_libbpf_open Andrii Nakryiko
2019-10-17  4:27 ` [PATCH v4 bpf-next 0/7] Fix, clean up, and revamp selftests/bpf Makefile Alexei Starovoitov
2019-10-17  6:52   ` Andrii Nakryiko
2019-10-17  8:08     ` Jakub Sitnicki
2019-10-17 19:18       ` Alexei Starovoitov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191017204430.GC2090@mini-arch \
    --to=sdf@fomichev.me \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).