On Thu, Apr 4, 2024 at 9:33 AM David Vernet <void@xxxxxxxxxxxxx> wrote: > > On Thu, Apr 04, 2024 at 09:04:19AM -0700, Yonghong Song wrote: > > > > On 4/3/24 6:03 PM, David Vernet wrote: > > > Now that we can call some kfuncs from BPF_PROG_TYPE_SYSCALL progs, let's > > > add some selftests that verify as much. As a bonus, let's also verify > > > that we can't call the progs from raw tracepoints. > > > > > > Signed-off-by: David Vernet <void@xxxxxxxxxxxxx> > > > > Ack with some comments below. > > > > Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx> > > Thanks for the review. It looks like accidentally replied directly to > me, so I'll re-add the missing cc's. > And dropped bpf@vger :) adding back > > > > > --- > > > .../selftests/bpf/prog_tests/cgrp_kfunc.c | 1 + > > > .../selftests/bpf/prog_tests/task_kfunc.c | 1 + > > > .../selftests/bpf/progs/cgrp_kfunc_common.h | 21 +++++++++++++++++++ > > > .../selftests/bpf/progs/cgrp_kfunc_failure.c | 4 ++++ > > > .../selftests/bpf/progs/cgrp_kfunc_success.c | 4 ++++ > > > .../selftests/bpf/progs/cpumask_common.h | 19 +++++++++++++++++ > > > .../selftests/bpf/progs/cpumask_failure.c | 4 ++++ > > > .../selftests/bpf/progs/cpumask_success.c | 3 +++ > > > .../selftests/bpf/progs/task_kfunc_common.h | 18 ++++++++++++++++ > > > .../selftests/bpf/progs/task_kfunc_failure.c | 4 ++++ > > > .../selftests/bpf/progs/task_kfunc_success.c | 4 ++++ > > > 11 files changed, 83 insertions(+) > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c > > > index adda85f97058..73f0ec4f4eb7 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c > > > @@ -102,6 +102,7 @@ void test_cgrp_kfunc(void) > > > run_success_test(success_tests[i]); > > > } > > > + RUN_TESTS(cgrp_kfunc_success); > > > RUN_TESTS(cgrp_kfunc_failure); > > > cleanup: > > > diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c > > > index d4579f735398..3db4c8601b70 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c > > > @@ -94,5 +94,6 @@ void test_task_kfunc(void) > > > run_success_test(success_tests[i]); > > > } > > > + RUN_TESTS(task_kfunc_success); > > > RUN_TESTS(task_kfunc_failure); > > > } > > > > 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 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. 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. > that was the expected behavior before RUN_TESTS() was introduced, so it > hopefully shouldn't cause much if any churn. > > > Maybe the current patch is okay even with duplicated work as it > > should not take much time to verify those tiny problems. > > IMO it should be fine for now as the overhead for validating and loading > these progs is low, but it'd definitely be good to address this problem > in a follow-up. I don't think it should take too much effort -- AFAICT > we'd just have to mark a test spec as invalid if it didn't have any BTF > test tags. Ideally I'd like to separate that from this patch set, but I > can do it here if folks want. > > Thanks, > David
