Re: [PATCHv2 bpf-next 9/9] selftests/bpf: Add uprobe session consumers test

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Jul 02, 2024 at 03:10:55PM -0700, Andrii Nakryiko wrote:
> On Mon, Jul 1, 2024 at 9:44 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test that attached/detaches multiple consumers on
> > single uprobe and verifies all were hit as expected.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../bpf/prog_tests/uprobe_multi_test.c        | 203 ++++++++++++++++++
> >  .../progs/uprobe_multi_session_consumers.c    |  53 +++++
> >  2 files changed, 256 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_multi_session_consumers.c
> >
> 
> This is clever, though bit notation obscures the meaning of the code a
> bit. But thanks for the long comment explaining the overall idea.
> 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > index b521590fdbb9..83eac954cf00 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > @@ -9,6 +9,7 @@
> >  #include "uprobe_multi_session.skel.h"
> >  #include "uprobe_multi_session_cookie.skel.h"
> >  #include "uprobe_multi_session_recursive.skel.h"
> > +#include "uprobe_multi_session_consumers.skel.h"
> >  #include "bpf/libbpf_internal.h"
> >  #include "testing_helpers.h"
> >  #include "../sdt.h"
> > @@ -739,6 +740,206 @@ static void test_session_recursive_skel_api(void)
> >         uprobe_multi_session_recursive__destroy(skel);
> >  }
> >
> > +static int uprobe_attach(struct uprobe_multi_session_consumers *skel, int bit)
> > +{
> > +       struct bpf_program **prog = &skel->progs.uprobe_0 + bit;
> > +       struct bpf_link **link = &skel->links.uprobe_0 + bit;
> > +       LIBBPF_OPTS(bpf_uprobe_multi_opts, opts);
> > +
> > +       /*
> > +        * bit: 0,1 uprobe session
> > +        * bit: 2,3 uprobe entry
> > +        * bit: 4,5 uprobe return
> > +        */
> > +       opts.session = bit < 2;
> > +       opts.retprobe = bit == 4 || bit == 5;
> > +
> > +       *link = bpf_program__attach_uprobe_multi(*prog, 0, "/proc/self/exe",
> > +                                                "uprobe_session_consumer_test",
> > +                                                &opts);
> > +       if (!ASSERT_OK_PTR(*link, "bpf_program__attach_uprobe_multi"))
> > +               return -1;
> > +       return 0;
> > +}
> > +
> > +static void uprobe_detach(struct uprobe_multi_session_consumers *skel, int bit)
> > +{
> > +       struct bpf_link **link = &skel->links.uprobe_0 + bit;
> 
> ok, this is nasty, no one guarantees this should keep working,
> explicit switch would be preferable

I see, ok, will replace that with a switch

> 
> > +
> > +       bpf_link__destroy(*link);
> > +       *link = NULL;
> > +}
> > +
> > +static bool test_bit(int bit, unsigned long val)
> > +{
> > +       return val & (1 << bit);
> > +}
> > +
> > +noinline int
> > +uprobe_session_consumer_test(struct uprobe_multi_session_consumers *skel,
> > +                            unsigned long before, unsigned long after)
> > +{
> > +       int bit;
> > +
> > +       /* detach uprobe for each unset bit in 'before' state ... */
> > +       for (bit = 0; bit < 6; bit++) {
> 
> Does "bit" correspond to the uprobe_X program? Maybe call it an uprobe
> index or something, if that's the case? bits are just representations,
> but semantically meaningful is identifier of an uprobe program, right?

right.. so it corresponds to program 'uprobe_<bit>' so maybe 'idx' is better

thanks,
jirka