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