Re: [PATCH] perf/lock: enable end-timestamp accounting for cgroup aggregation

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Incomplete spinlock and rwlock contentions are silently dropped for LOCK_AGGR_CGROUP because account_end_timestamp() skips the per-CPU tstamp map.
- [Medium] Inconsistent cgroup attribution between complete and incomplete lock contentions when a task migrates cgroups.
--

commit 7ba25dc647534f97eb1ae9f34c6c8d38e7fef074
Author: Suchit Karunakaran <suchitkarunakaran@gmail.com>

perf/lock: enable end-timestamp accounting for cgroup aggregation

This commit adds a cgroup_id field to struct tstamp_data to properly account
for incomplete lock contention events when aggregating by cgroup. It records
the cgroup ID at contention_begin and uses it to build the aggregation key
in userspace.

> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index cbd7435579fea..1a5bd2ff8ee4c 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -463,8 +463,8 @@ static void update_lock_stat(int map_fd, int pid, u64 end_ts,
>  		stat_key.lock_addr_or_cgroup = ts_data->lock;
>  		break;
>  	case LOCK_AGGR_CGROUP:
> -		/* TODO */
> -		return;
> +		stat_key.lock_addr_or_cgroup = ts_data->cgroup_id;
> +		break;

Does this fully enable end-timestamp accounting for cgroup aggregation
on per-CPU locks like spinlocks and rwlocks?

Looking at account_end_timestamp() in tools/perf/util/bpf_lock_contention.c,
there is an early return that skips the per-CPU tstamp map:

static void account_end_timestamp(struct lock_contention *con)
{
	...
	/* Now it'll check per-cpu tstamp map which doesn't have TID. */
	if (aggr_mode == LOCK_AGGR_TASK || aggr_mode == LOCK_AGGR_CGROUP)
		return;

	total_cpus = cpu__max_cpu().cpu;
	ts_fd = bpf_map__fd(skel->maps.tstamp_cpu);
	...
}

Since account_end_timestamp() skips iterating over skel->maps.tstamp_cpu
when aggr_mode is LOCK_AGGR_CGROUP, are all incomplete contentions for
spinlocks and rwlocks still silently dropped?

[ ... ]

> diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> index 96e7d853b9edf..d0e2cad02fa36 100644
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> @@ -536,6 +536,8 @@ int contention_begin(u64 *ctx)
>  	pelem->timestamp = bpf_ktime_get_ns();
>  	pelem->lock = (__u64)ctx[0];
>  	pelem->flags = (__u32)ctx[1];
> +	if (aggr_mode == LOCK_AGGR_CGROUP)
> +		pelem->cgroup_id = get_current_cgroup_id();

If a task migrates to a different cgroup while blocked on a lock, could this
lead to inconsistent attribution between complete and incomplete contentions?

The cgroup ID is statically recorded here at contention_begin() and used by
userspace for incomplete events.

However, contention_end() continues to dynamically resolve the cgroup ID:

SEC("tp_btf/contention_end")
int contention_end(u64 *ctx)
{
	...
	case LOCK_AGGR_CGROUP:
		key.lock_addr_or_cgroup = get_current_cgroup_id();
		break;
	...
}

Would it be more consistent for contention_end() to also use the statically
recorded pelem->cgroup_id, ensuring that both completed and incomplete events
attribute the contention time to the exact same cgroup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420184656.331306-1-suchitkarunakaran@gmail.com?part=1