Re: [PATCH bpf-next 2/2] selftests/bpf: Verify calling core kfuncs from BPF_PROG_TYPE_SYCALL

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 3420 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]