Re: [PATCH bpf-next 6/7] selftests/bpf: Add kprobe multi session test

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Apr 23, 2024 at 05:27:14PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:13 AM Jiri Olsa <jolsa@kernel.org> wrote:

SNIP

> > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > index 51628455b6f5..d1f116665551 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> > @@ -4,6 +4,7 @@
> >  #include "trace_helpers.h"
> >  #include "kprobe_multi_empty.skel.h"
> >  #include "kprobe_multi_override.skel.h"
> > +#include "kprobe_multi_session.skel.h"
> >  #include "bpf/libbpf_internal.h"
> >  #include "bpf/hashmap.h"
> >
> > @@ -326,6 +327,52 @@ static void test_attach_api_fails(void)
> >         kprobe_multi__destroy(skel);
> >  }
> >
> > +static void test_session_skel_api(void)
> > +{
> > +       struct kprobe_multi_session *skel = NULL;
> > +       LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
> > +       LIBBPF_OPTS(bpf_test_run_opts, topts);
> > +       struct bpf_link *link = NULL;
> > +       int err, prog_fd;
> > +
> > +       skel = kprobe_multi_session__open_and_load();
> > +       if (!ASSERT_OK_PTR(skel, "kprobe_multi_session__open_and_load"))
> > +               goto cleanup;
> 
> return?

ok

> 
> > +
> > +       skel->bss->pid = getpid();
> > +
> > +       err =  kprobe_multi_session__attach(skel);
> 
> nit: extra space
> 
> > +       if (!ASSERT_OK(err, " kprobe_multi_session__attach"))
> > +               goto cleanup;
> > +
> > +       prog_fd = bpf_program__fd(skel->progs.trigger);
> > +       err = bpf_prog_test_run_opts(prog_fd, &topts);
> > +       ASSERT_OK(err, "test_run");
> > +       ASSERT_EQ(topts.retval, 0, "test_run");
> > +
> > +       ASSERT_EQ(skel->bss->kprobe_test1_result, 1, "kprobe_test1_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test2_result, 1, "kprobe_test2_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test3_result, 1, "kprobe_test3_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test4_result, 1, "kprobe_test4_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test5_result, 1, "kprobe_test5_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test6_result, 1, "kprobe_test6_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test7_result, 1, "kprobe_test7_result");
> > +       ASSERT_EQ(skel->bss->kprobe_test8_result, 1, "kprobe_test8_result");
> > +
> > +       ASSERT_EQ(skel->bss->kretprobe_test1_result, 0, "kretprobe_test1_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test2_result, 1, "kretprobe_test2_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test3_result, 0, "kretprobe_test3_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test4_result, 1, "kretprobe_test4_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test5_result, 0, "kretprobe_test5_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test6_result, 1, "kretprobe_test6_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test7_result, 0, "kretprobe_test7_result");
> > +       ASSERT_EQ(skel->bss->kretprobe_test8_result, 1, "kretprobe_test8_result");
> 
> see below, even if array of ksym ptrs idea doesn't work out, at least
> results can be an array (which is cleaner to work with both on BPF and
> user space sides)

I recall in past we used to do that and we switched to specific values
to be more explicit I guess.. but it might make sense in here, will try it 

SNIP

> > +static int session_check(void *ctx, bool is_return)
> > +{
> > +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> > +               return 1;
> > +
> > +       __u64 addr = bpf_get_func_ip(ctx);
> > +
> > +#define SET(__var, __addr) ({                  \
> > +       if ((const void *) addr == __addr)      \
> > +               __var = 1;                      \
> > +})
> > +
> > +       if (is_return) {
> > +               SET(kretprobe_test1_result, &bpf_fentry_test1);
> > +               SET(kretprobe_test2_result, &bpf_fentry_test2);
> > +               SET(kretprobe_test3_result, &bpf_fentry_test3);
> > +               SET(kretprobe_test4_result, &bpf_fentry_test4);
> > +               SET(kretprobe_test5_result, &bpf_fentry_test5);
> > +               SET(kretprobe_test6_result, &bpf_fentry_test6);
> > +               SET(kretprobe_test7_result, &bpf_fentry_test7);
> > +               SET(kretprobe_test8_result, &bpf_fentry_test8);
> > +       } else {
> > +               SET(kprobe_test1_result, &bpf_fentry_test1);
> > +               SET(kprobe_test2_result, &bpf_fentry_test2);
> > +               SET(kprobe_test3_result, &bpf_fentry_test3);
> > +               SET(kprobe_test4_result, &bpf_fentry_test4);
> > +               SET(kprobe_test5_result, &bpf_fentry_test5);
> > +               SET(kprobe_test6_result, &bpf_fentry_test6);
> > +               SET(kprobe_test7_result, &bpf_fentry_test7);
> > +               SET(kprobe_test8_result, &bpf_fentry_test8);
> > +       }
> > +
> > +#undef SET
> 
> curious, have you tried implementing this through a proper for loop? I
> wonder if something like
> 
> void *kfuncs[] = { &bpf_fentry_test1, ..., &bpf_fentry_test8 };
> 
> and then generic loop over this array would work. Can you please try?

yep, will try, let's see if it gets nicer

jirka