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@xxxxxxxxxx> 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
