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

sashiko-bot
2026-04-23T22:36:34Z

Sashiko AI review found 1 potential issue(s):
- [Medium] Incomplete spinlock and rwlock contentions are silently dropped under cgroup aggregation because of a leftover early return.
--

commit 5bccbbefb61cd7effb0fec5a7f0bee5add3ba114
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, recording it at
contention_begin for cgroup aggregation mode. It also updates update_lock_stat()
to use this recorded cgroup_id, enabling accounting for incomplete contentions
that start but never reach a contention_end event.

> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index cbd7435579feaf..1a5bd2ff8ee4c2 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;
> default:
> return;
> }

Now that update_lock_stat() handles cgroup aggregation, does
account_end_timestamp() also need an update to stop returning early?

Looking at account_end_timestamp() in tools/perf/util/bpf_lock_contention.c:

/* 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);

Because of this early return, will incomplete spinlock and rwlock contentions
(which use the tstamp_cpu map) still be dropped when using cgroup
aggregation?

[ ... ]

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