Re: [PATCH v9 07/29] perf lock: Only pass sample to handlers

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] get_callstack() incorrectly caches an empty callstack if the first sample lacks a callchain, permanently hiding the lock's callstack.
--

commit 49c5d291bcdc6ef932bccd0415609f2f21a5cbc8
Author: Ian Rogers <irogers@google.com>

perf lock: Only pass sample to handlers

The evsel is within the sample, so only the sample needs to be passed
to the handlers. This commit removes the evsel parameter from handlers
and fixes call sites. It also adds an additional NULL check in
get_callstack().

> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index e2b585e528aca..2a6fc4c97dcdb 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
[ ... ]
> @@ -949,8 +939,8 @@ static u64 *get_callstack(struct perf_sample *sample, int max_stack)
>  	int c;
>  
>  	callstack = calloc(max_stack, sizeof(*callstack));
> -	if (callstack == NULL)
> -		return NULL;
> +	if (callstack == NULL || !sample->callchain)
> +		return callstack;
>  
>  	for (i = 0, c = 0; i < sample->callchain->nr && c < max_stack; i++) {
>  		u64 ip = sample->callchain->ips[i];

Will this cause get_callstack() to return an empty allocated array when
the sample lacks a callchain?

If the first sample for a lock doesn't have a callchain,
report_lock_contention_begin_event() will permanently assign this empty
array to ls->callstack:

	if (needs_callstack()) {
		u64 *callstack = get_callstack(sample, max_stack_depth);
		if (callstack == NULL)
			return -ENOMEM;

		...

		if (ls->callstack == NULL)
			ls->callstack = callstack;
		else
			free(callstack);

Would later samples with valid callchains then be freed instead of updating
the empty ls->callstack, permanently hiding the callstack for that lock?

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