Re: [PATCH bpf-next] selftests/bpf: Fix uprobe consumer test (again)

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Nov 06, 2024 at 04:26:11PM -0800, Andrii Nakryiko wrote:
> On Wed, Nov 6, 2024 at 2:40 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The new uprobe changes bring bit some new behaviour that we need
> 
> needs some proofreading, not sure what you were trying to say
> 
> > to reflect in the consumer test.
> >
> > There's special case when we have one of the existing uretprobes removed
> 
> see below, I don't like how special that case seems. It's actually not
> that special, we just have a rule under which uretprobe instance
> survives before->after transition, and we can express that pretty
> clearly and explicitly.
> 
> pw-bot: cr
> 
> > and at the same time we're adding the other uretprobe. In this case we get
> > hit on the new uretprobe consumer only if there was already another uprobe
> > existing so the uprobe object stayed valid for uprobe return instance.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/uprobe_multi_test.c    | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > 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 619b31cd24a1..545b91385749 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > @@ -873,10 +873,21 @@ static int consumer_test(struct uprobe_multi_consumers *skel,
> >                          * which means one of the 'return' uprobes was alive when probe was hit:
> >                          *
> >                          *   idxs: 2/3 uprobe return in 'installed' mask
> > +                        *
> > +                        * There's special case when we have one of the existing uretprobes removed
> > +                        * and at the same time we're adding the other uretprobe. In this case we get
> > +                        * hit on the new uretprobe consumer only if there was already another uprobe
> > +                        * existing so the uprobe object stayed valid for uprobe return instance.
> >                          */
> >                         unsigned long had_uretprobes  = before & 0b1100; /* is uretprobe installed */
> > +                       unsigned long b = before >> 2, a = after >> 2;
> > +                       bool hit = true;
> > +
> > +                       /* Match for following a/b cases: 01/10 10/01 */
> > +                       if ((a ^ b) == 0b11)
> > +                               hit = before & 0b11;
> >
> > -                       if (had_uretprobes && test_bit(idx, after))
> > +                       if (hit && had_uretprobes && test_bit(idx, after))
> 
> I found these changes very hard to reason about (not because of bit
> manipulations, but due to very specific 01/10 requirement, which seems
> too specific). So I came up with this:
> 
>     bool uret_stays = before & after & 0b1100;
>     bool uret_survives = (before & 0b1100) && (after & 0b1100) &&
> (before & 0b0011);
> 
>     if ((uret_stays || uret_survives) && test_bit(idx, after))
>         val++;
> 
> The idea being that uretprobe under test either stayed from before to
> after (uret_stays + test_bit) or uretprobe instance survived and we
> have uretprobe active in after (uret_survives + test_bit).
> 
> uret_survives just states that uretprobe survives if there are *any*
> uretprobes both before and after (overlapping or not, doesn't matter)
> and uprobe was attached before.
> 
> Does it make sense? Can you incorporate that into v2, if you agree?

ok, seems easier.. will send v2

thanks,
jirka