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?
> val++;
> fmt = "idx 2/3: uretprobe";
> }
> --
> 2.47.0
>
