On Thu, Apr 04, 2024 at 03:35:32PM -0700, Andrii Nakryiko wrote: [...] > > > > > The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success) > > > > > will do duplicate work for *existing* bpf programs in their respective > > > > > files. I think we still prefer to have cgrp_kfunc_success tests > > > > > in cgrp_kfunc.c to make it easy to cross check. But in order to > > > > > remove duplicate work, one option is to make other non-RUN_TESTS > > > > > programs in those files not auto-loaded and their corresponding > > > > > prog_tests/*.c file need to explicitly enable loading the problem. > > > > > > > > Good point, and yes I agree with that approach of not auto-loading > > > > non-RUN_TESTS programs. Considering that we have a __success BTF tag to > > > > say, "this prog should successfully load", it seems odd that we'd also > > > > automatically load and validate progs that _didn't_ specify that tag as > > > > well. At that point, I'm not sure what value the tag is bringing. Also, > > > > > > Just more explicitness (if desired). Normally __success would be > > > augmented by __msg() or __retval(). I'd feel uncomfortable just > > > > But __success really has no actual purpose, right? Isn't it identical to > > if it's just left off? You don't need __success to specify __msg() or > > __retval() right? > > right, it's just a more explicit documentation-like annotation, if you will > > > > > > silently skipping programs that are not marked with __success, as it > > > would be too easy to accidentally forget to add it and not know that > > > the BPF program is not tested. > > > > > > I'd say that RUN_TESTS-based programs should be kept separate from any > > > other BPF programs that have a custom user-space testing part, though. > > > > IF we do go this way, maybe just a __skip or something tag would be > > sufficient? > > if we go this way we wouldn't need __skip, but if we do not go, then > sure, why not. But in general, __skip makes sense either way, I guess, > I have no problem with it. Sorry, by "if we go this way" what I meant was "if we continue to have RUN_TESTS() run all progs by default." Given that we're doing that, it sounds like we're on the same page page and that __skip is the way to go. > > > > > > About the patch itself. I don't really see much point in adding > > > *_KFUNC_LOAD_TEST macros. They are used once or twice in total, while > > > obscuring *what* is actually being tested. Unless you expect to add 5+ > > > more copies of them, I'd just inline them explicitly. > > > > It's not really important what's in the actual prog though -- the point > > is that we're verifying we can invoke some kfuncs in a certain prog > > type. But yes, it does obscure what's there, and I'm fine with > > copy-pasting them if that's your preference. The reason I went with a > > macro was to make it easy for us to quickly test new prog types as we > > add support for them, or to add other negative testcases for unsafe prog > > types. Right now we're just testing tracing progs. > > I'm always for less macro usage, if possible :) > > For the use case you are describing I'd just add static subprog that > exercises all the kfuncs of interest, and then call this subprog from > all the (explicitly defined) main entry program of desired program > types Will do for v2. Thanks!
Attachment:
signature.asc
Description: PGP signature
