Re: [PATCH] perf lock contention: Allow 'mmap_lock' in -L/--lock-filter

On Mon, May 04, 2026 at 06:27:43AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Combining `mmap_lock` with other lock names in the `-L/--lock-filter` option drops all events.
> - [High] Specifying only an unknown lock symbol in `-L/--lock-filter` silently traces all locks in the system instead of aborting.
> --
> 
> commit ba21190eae512a9060154fb29ca23ce62c636cc2
> Author: Namhyung Kim <[email protected]>
> Subject: perf lock contention: Allow 'mmap_lock' in -L/--lock-filter
> 
> This commit adds support for filtering lock contention events specifically for
> the 'mmap_lock' using the -L/--lock-filter option. It introduces a special
> flag to bypass the usual address-based filtering for mmap_lock.
> 
> > diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> > index cbd7435579fe..cd7ee5d1d1dd 100644
> > --- a/tools/perf/util/bpf_lock_contention.c
> > +++ b/tools/perf/util/bpf_lock_contention.c
> 
> [ ... ]
> 
> > @@ -264,7 +270,10 @@ int lock_contention_prepare(struct lock_contention *con)
> >  			con->filters->addrs = addrs;
> >  		}
> >  		naddrs = con->filters->nr_addrs;
> > -		skel->rodata->has_addr = 1;
> > +		if (naddrs > 0)
> > +			skel->rodata->has_addr = 1;
> > +		else
> > +			naddrs = 1;
> >  	}
> 
> Prior to this patch, providing an unknown symbol (e.g., -L non_existent)
> resulted in naddrs = 0 and has_addr = 1, which correctly caused libbpf to
> fail creating the map with 0 entries, gracefully aborting the process.
> 
> With this change, if naddrs is 0, has_addr remains 0 but naddrs is
> artificially set to 1. The BPF map creation succeeds, but since has_addr
> and has_mmap_lock are both 0, the BPF program skips the address filter
> entirely.
> 
> Does this unintentionally trace all locks in the system instead of gracefully
> aborting when an unknown lock name is provided?

Ok, I think this is a valid concern.  Will update in v2.

> 
> > diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> > index 96e7d853b9ed..45ec2fb73984 100644
> > --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> > +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> 
> [ ... ]
> 
> > @@ -295,6 +298,14 @@ static inline int can_record(u64 *ctx)
> >  			return 0;
> >  	}
> >  
> > +	if (has_mmap_lock) {
> > +		__u64 lock = ctx[0];
> > +		__u32 flag = ctx[1];
> > +
> > +		if (check_lock_type(lock, flag) != LCD_F_MMAP_LOCK)
> > +			return 0;
> > +	}
> > +
> >  	return 1;
> >  }
> 
> The -L option allows filtering by multiple lock names, but can_record()
> evaluates active filters sequentially. If a user specifies
> -L mmap_lock,other_lock, both has_mmap_lock = 1 and has_addr = 1 will be
> set in the BPF rodata.
> 
> In this scenario:
> 
> 1. An other_lock event passes the has_addr check but then reaches the
> has_mmap_lock check. Since its type is not LCD_F_MMAP_LOCK, the function
> returns 0.
> 
> 2. An mmap_lock event is evaluated against the has_addr check first.
> Because its dynamic address isn't in addr_filter, the lookup fails, and the
> function returns 0 before even reaching the has_mmap_lock check.
> 
> Will this cause all valid events to be silently dropped when combining
> mmap_lock with other lock names?

Good point, I'll handle the case mmap_lock and another lock are used
together.

Thanks,
Namhyung