Re: [PATCH v2 5/6] 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: rcu_state_addr collides with rcu_state.node[0].lock in BPF
--

commit 66aefea7c5ca7c7c5ee421770f454d75f5c24b84
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
support for filtering by RCU and SRCU lock types and names.

> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index c20bd075664e1..fa7df9187b08b 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
[ ... ]
> @@ -180,6 +181,129 @@ static void init_numa_data(struct lock_contention *con)
[ ... ]
> +static void configure_rcu_contention_progs(struct lock_contention *con,
> +					   bool has_rcu_lock)
> +{
[ ... ]
> +	sym = machine__find_kernel_symbol_by_name(con->machine, "rcu_state", &kmap);
> +	if (sym) {
> +		skel->rodata->rcu_state_addr = map__unmap_ip(kmap, sym->start);
> +		map__put(kmap);
> +	} else {
> +		skel->rodata->rcu_state_addr = 1ULL;
> +	}
[ ... ]
> diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> index 0f2118006005a..58d45c43150b5 100644
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
[ ... ]
> @@ -613,14 +639,19 @@ int contention_end(u64 *ctx)
>  	 */
>  	pelem = bpf_map_lookup_elem(&tstamp_cpu, &idx);
>  	if (pelem && pelem->lock) {
> -		if (pelem->lock != ctx[0])
> +		if (pelem->lock != lock)
>  			return 0;
>  	} else {
>  		pid = bpf_get_current_pid_tgid();
>  		pelem = bpf_map_lookup_elem(&tstamp, &pid);
> -		if (!pelem || pelem->lock != ctx[0])
> -			return 0;
> -		need_delete = true;
> +		if (pelem && pelem->lock == lock) {
> +			ts_map = &tstamp;
> +		} else {
> +			pelem = bpf_map_lookup_elem(&tstamp_rcu, &pid);
> +			if (!pelem || pelem->lock != lock)
> +				return 0;
> +			ts_map = &tstamp_rcu;
> +		}
>  	}

[Severity: Medium]
Can setting rcu_state_addr to the start address of rcu_state and sharing
do_contention_end() between tp_btf/contention_end and the RCU/SRCU fexit
hooks cause lock address collisions and corrupted wait tracking?

In kernel/rcu/tree.h, struct rcu_state begins at offset 0 with
node[NUM_RCU_NODES], and struct rcu_node begins at offset 0 with its
raw_spinlock_t lock:

kernel/rcu/tree.h:
    struct rcu_node {
        raw_spinlock_t __private lock;
        ...
    };

    struct rcu_state {
        struct rcu_node node[NUM_RCU_NODES];
        ...
    };

Because of this layout, &rcu_state.node[0].lock has the exact same address
as rcu_state (rcu_state_addr). Together with do_contention_end() not
distinguishing whether its caller is tp_btf/contention_end or an RCU/SRCU
fexit program, three issues can occur:

1. When filtering for RCU (for example with '-Y rcu', '-Y rcu:W', or
'-L rcu'), a task executing synchronize_rcu(), synchronize_rcu_expedited(),
or rcu_barrier() records an active entry in tstamp_rcu[pid] with
pelem->lock == rcu_state_addr. If that task contends on
&rcu_state.node[0].lock during grace-period initialization or waiting,
tp_btf/contention_begin(lock = rcu_state_addr, flags = LCB_F_SPIN) is
rejected by can_record(), leaving tstamp_cpu empty. When the spinlock is
acquired and tp_btf/contention_end(lock = rcu_state_addr) fires,
do_contention_end() falls through tstamp_cpu and tstamp into
tstamp_rcu[pid], matches pelem->lock == rcu_state_addr, records the short
spinlock wait duration as the RCU wait, and deletes tstamp_rcu[pid] before
the fexit hook runs.

2. If an RCU or SRCU fexit program runs on a CPU where tstamp_cpu happens
to have an active spinlock entry (pelem->lock != 0 and pelem->lock != lock),
do_contention_end() returns 0 at the tstamp_cpu check without looking up or
deleting tstamp_rcu[pid]. That leaves a stale entry in tstamp_rcu[pid] that
blocks subsequent RCU/SRCU waits for that task in get_tstamp_elem() and
charges end_ts - pelem->timestamp in account_end_timestamp().

3. In LOCK_AGGR_ADDR mode ('perf lock con -a -b -l'), both
do_contention_end() and lock_contention_read() key lock_stat entries by
lock address:

tools/perf/util/bpf_skel/lock_contention.bpf.c:do_contention_end()
    case LOCK_AGGR_ADDR:
        key.lock_addr_or_cgroup = pelem->lock;

Because &rcu_state.node[0].lock and rcu_state_addr share the same address,
root rcu_node spinlock contentions (LCB_F_SPIN) and RCU grace-period waits
(LCB_F_RCU | LCB_F_WRITE) merge into a single lock_stat entry under
whichever lock type was recorded first.

Would it make sense to pass a distinct synthetic address or offset for RCU
waits (or disambiguate the lock key), and pass an explicit flag to
do_contention_end() so that RCU/SRCU fexit hooks only inspect tstamp_rcu
while tp_btf/contention_end only inspects tstamp_cpu and tstamp?

[ ... ]
> @@ -795,11 +825,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);
> +}
> +
> +SEC("fexit/synchronize_rcu")
> +int BPF_PROG(fexit_synchronize_rcu)
> +{
> +	return do_contention_end(rcu_state_addr, 0);
> +}
[ ... ]

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