Re: [PATCH v1 5/5] perf lock contention: Extend BPF lock contention to RCU and SRCU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf lock contention: wrong caller stack skip in do_contention_begin() for fentry
--

commit 9ca74d6f51b3408e9a151b639dd644cd10562df1
Author: Ian Rogers <irogers@google.com>

perf lock contention: Extend BPF lock contention to RCU and SRCU

This commit extends 'perf lock contention --use-bpf' to track RCU and SRCU
grace-period and barrier waits using fentry/fexit BPF programs and adds
lock type and name filtering support for rcu and srcu.

> diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> index 0f2118006005a..257e3402e8151 100644
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
[ ... ]
> @@ -474,21 +489,20 @@ static inline void update_owner_stat(u32 id, u64 duration, u32 flags)
>  	}
>  }
>  
> -SEC("tp_btf/contention_begin")
> -int contention_begin(u64 *ctx)
> +static __always_inline int do_contention_begin(void *ctx, __u64 lock, __u32 flags)
>  {
>  	struct tstamp_data *pelem;
>  
> -	if (!enabled || !can_record(ctx))
> +	if (!enabled || !can_record(lock, flags))
>  		return 0;
>  
> -	pelem = get_tstamp_elem(ctx[1]);
> +	pelem = get_tstamp_elem(flags);
>  	if (pelem == NULL)
>  		return 0;
>  
>  	pelem->timestamp = bpf_ktime_get_ns();
> -	pelem->lock = (__u64)ctx[0];
> -	pelem->flags = (__u32)ctx[1];
> +	pelem->lock = lock;
> +	pelem->flags = flags;
>  	if (aggr_mode == LOCK_AGGR_CGROUP)
>  		pelem->cgroup_id = get_current_cgroup_id();
>  
[ ... ]
> @@ -795,11 +812,109 @@ int contention_end(u64 *ctx)
>  		check_lock_delay(pelem->lock);
>  
>  	pelem->lock = 0;
> -	if (need_delete)
> -		bpf_map_delete_elem(&tstamp, &pid);
> +	if (ts_map)
> +		bpf_map_delete_elem(ts_map, &pid);
>  	return 0;
>  }
>  
> +SEC("tp_btf/contention_begin")
> +int contention_begin(u64 *ctx)
> +{
> +	return do_contention_begin(ctx, (__u64)ctx[0], (__u32)ctx[1]);
> +}
> +
> +SEC("tp_btf/contention_end")
> +int contention_end(u64 *ctx)
> +{
> +	return do_contention_end((__u64)ctx[0], (__s32)ctx[1]);
> +}
> +
> +/* RCU grace-period synchronize / barrier contention hooks */
> +SEC("fentry/synchronize_rcu")
> +int BPF_PROG(fentry_synchronize_rcu)
> +{
> +	return do_contention_begin(ctx, rcu_state_addr, LCB_F_RCU | LCB_F_WRITE);
> +}

[Severity: Medium]
In do_contention_begin() in tools/perf/util/bpf_skel/lock_contention.bpf.c,
bpf_get_stackid() uses stack_skip (which defaults to CONTENTION_STACK_SKIP =
4) unconditionally for both tp_btf/contention_begin and the new fentry RCU
and SRCU programs:

skip_owner:
		pelem->stack_id = bpf_get_stackid(ctx, &stacks,
						  BPF_F_FAST_STACK_CMP | stack_skip);

For tp_btf/contention_begin, the tracepoint fires inside a lock slowpath via
tracepoint dispatch:

  caller -> mutex_lock -> __mutex_lock_slowpath -> __mutex_lock ->
  trace_contention_begin -> __bpf_trace_contention_begin -> bpf_trace_run2 ->
  bpf_prog

Skipping 4 frames lands inside the locking functions (.sched.text,
.spinlock.text, or trace_contention_begin), which lock_contention_get_name()
in tools/perf/util/bpf_lock_contention.c then skips via
machine__is_lock_function() to reach the direct caller:

	/* LOCK_AGGR_CALLER: skip lock internal functions */
	while (machine__is_lock_function(machine, stack_trace[idx]) &&
	       idx < con->max_stack - 1)
		idx++;

	addr = stack_trace[idx];

In contrast, fentry_synchronize_rcu() (and the other six RCU/SRCU fentry
programs) attach directly to the top-level API functions. In an fentry
program, the callchain captured by bpf_get_stackid_raw_tp() ->
perf_callchain_kernel() only has two frames before the traced function
itself: ip[0] is the BPF prog, ip[1] is the BPF trampoline, ip[2] is the
traced function (e.g., synchronize_rcu), ip[3] is the direct caller, and
ip[4] is the caller's caller.

Won't skipping stack_skip = 4 frames in bpf_get_stackid() skip past ip[3],
dropping the direct caller from the recorded stack trace, attributing RCU
and SRCU contention in LOCK_AGGR_CALLER mode to the caller's caller (ip[4]),
and causing callstack filtering (-S) in match_callstack_filter() to miss the
direct caller?

Also, if the fentry stack skip is adjusted so that the traced RCU/SRCU
function itself can appear at stack_trace[0], machine__is_lock_function() in
tools/perf/util/machine.c only checks .sched.text, .spinlock.text,
__traceiter_contention_begin, and trace_contention_begin. Since
synchronize_rcu*(), synchronize_srcu*(), rcu_barrier(), and srcu_barrier()
are not in .sched.text or .spinlock.text, should machine__is_lock_function()
or the fentry skip depth account for those functions as well?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017054555.1655407-1-irogers@google.com?part=5