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@xxxxxxxxxx> wrote:
> >
> > Adding test that attached/detaches multiple consumers on
> > single uprobe and verifies all were hit as expected.
> >
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
> > .../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
