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