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

[sashiko-bot@kernel.org]

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