Re: [PATCH v4 1/7] perf lock contention: Filter events before creating lock_stat

[Namhyung Kim]

On Mon, Oct 19, 2026 at 03:09:09PM -0700, Ian Rogers wrote:
> In report_lock_contention_begin_event(), lock_stat_findnew() was
> called before checking filters.nr_types (-Y), filters.nr_addrs (-L),
> and match_callstack_filter() (-S).
> 
> When a filtered-out lock event (such as a mutex contention when
> running with -Y spinlock) shared the same aggregation key as a
> subsequent matching event (for example, contention on the mutex's
> internal wait_lock spinlock from the same caller), the first event
> created the lock_stat entry with the filtered-out lock's flags and the
> second event reused that entry, causing non-spinlock entries to appear
> in -Y spinlock output.
> 
> Check the type, address, and callstack filters before looking up or
> creating the lock_stat entry.
> 
> Fixes: b4a7eff93c39 ("perf lock contention: Add -Y/--type-filter option")
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>

Reviewed-by: Namhyung Kim <namhyung@xxxxxxxxxx>

Thanks,
Namhyung

> ---
>  tools/perf/builtin-lock.c | 61 +++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index d5c0d55cb82d..778923c92f42 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -975,6 +975,7 @@ static int report_lock_contention_begin_event(struct perf_sample *sample)
>  	struct machine *machine = &session->machines.host;
>  	struct map *kmap;
>  	struct symbol *sym;
> +	u64 *callstack = NULL;
>  
>  	ret = get_key_by_aggr_mode(&key, addr, sample);
>  	if (ret < 0)
> @@ -1010,33 +1011,6 @@ static int report_lock_contention_begin_event(struct perf_sample *sample)
>  		}
>  	}
>  
> -	ls = lock_stat_find(key);
> -	if (!ls) {
> -		char buf[128];
> -		const char *name = "";
> -
> -		switch (aggr_mode) {
> -		case LOCK_AGGR_ADDR:
> -			sym = machine__find_kernel_symbol(machine, key, &kmap);
> -			if (sym)
> -				name = sym->name;
> -			break;
> -		case LOCK_AGGR_CALLER:
> -			name = buf;
> -			if (lock_contention_caller(sample, buf, sizeof(buf)) < 0)
> -				name = "Unknown";
> -			break;
> -		case LOCK_AGGR_CGROUP:
> -		case LOCK_AGGR_TASK:
> -		default:
> -			break;
> -		}
> -
> -		ls = lock_stat_findnew(key, name, flags);
> -		if (!ls)
> -			return -ENOMEM;
> -	}
> -
>  	if (filters.nr_types) {
>  		bool found = false;
>  
> @@ -1066,7 +1040,7 @@ static int report_lock_contention_begin_event(struct perf_sample *sample)
>  	}
>  
>  	if (needs_callstack()) {
> -		u64 *callstack = get_callstack(sample, max_stack_depth);
> +		callstack = get_callstack(sample, max_stack_depth);
>  		if (callstack == NULL)
>  			return 0;
>  
> @@ -1074,7 +1048,38 @@ static int report_lock_contention_begin_event(struct perf_sample *sample)
>  			free(callstack);
>  			return 0;
>  		}
> +	}
> +
> +	ls = lock_stat_find(key);
> +	if (!ls) {
> +		char buf[128];
> +		const char *name = "";
> +
> +		switch (aggr_mode) {
> +		case LOCK_AGGR_ADDR:
> +			sym = machine__find_kernel_symbol(machine, key, &kmap);
> +			if (sym)
> +				name = sym->name;
> +			break;
> +		case LOCK_AGGR_CALLER:
> +			name = buf;
> +			if (lock_contention_caller(sample, buf, sizeof(buf)) < 0)
> +				name = "Unknown";
> +			break;
> +		case LOCK_AGGR_CGROUP:
> +		case LOCK_AGGR_TASK:
> +		default:
> +			break;
> +		}
> +
> +		ls = lock_stat_findnew(key, name, flags);
> +		if (!ls) {
> +			free(callstack);
> +			return -ENOMEM;
> +		}
> +	}
>  
> +	if (callstack) {
>  		if (ls->callstack == NULL)
>  			ls->callstack = callstack;
>  		else
> -- 
> 2.56.0.212.g129ede20d9-goog
>