bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Stanislav Fomichev <sdf@fomichev.me>
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 10:48:23 -0700	[thread overview]
Message-ID: <CAEf4BzYSUoN2Boy-iveFAFGiiAMta5S9SK8aGO1BMnd+q2FzbA@mail.gmail.com> (raw)
In-Reply-To: <20191017160716.GA2090@mini-arch>

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.

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

  reply	other threads:[~2019-10-17 17:48 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 [this message]
2019-10-17 20:44           ` Stanislav Fomichev
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=CAEf4BzYSUoN2Boy-iveFAFGiiAMta5S9SK8aGO1BMnd+q2FzbA@mail.gmail.com \
    --to=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 \
    --cc=sdf@fomichev.me \
    /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).