Re: [PATCH v2 1/6] perf lock contention: Share common BPF and lock type definitions in lock_data.h

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf lock: get_lock_owner() conflates waiter and owner rwsem flags
--

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

perf lock contention: Share common BPF and lock type definitions in lock_data.h

This commit moves lock_type_table, LCB_F_* flags, CO-RE structs, and common
BPF lock helpers into tools/perf/util/bpf_skel/lock_data.h so they can be
shared with BPF sample filtering. It also extends get_lock_owner() to support
rt_mutex and check writer-ownership and handoff flags.

> diff --git a/tools/perf/util/bpf_skel/lock_data.h b/tools/perf/util/bpf_skel/lock_data.h
> index 652e114e6b877..bf39128e0be15 100644
> --- a/tools/perf/util/bpf_skel/lock_data.h
> +++ b/tools/perf/util/bpf_skel/lock_data.h
> @@ -76,4 +84,320 @@ struct slab_cache_data {

[ ... ]

> +static __always_inline struct task_struct *get_lock_owner(__u64 lock, __u32 flags)
> +{
> +	struct task_struct *task;
> +	__u64 owner = 0;
> +
> +	if (flags & LCB_F_MUTEX) {
> +		struct mutex *mutex = (void *)lock;
> +
> +		if (bpf_core_field_exists(mutex->owner.counter)) {
> +			owner = BPF_CORE_READ(mutex, owner.counter);
> +			if (owner & 0x4UL)
> +				return NULL;
> +		} else if (bpf_core_type_exists(struct rt_mutex_base)) {
> +			struct rt_mutex_base *rtm = (void *)lock;
> +
> +			owner = (__u64)BPF_CORE_READ(rtm, owner);
> +			return (void *)(owner & ~1UL);
> +		}
> +	} else if (flags == LCB_F_READ || flags == LCB_F_WRITE) {
> +		struct rw_semaphore___new *rwsem = (void *)lock;
> +
> +		if (flags == LCB_F_WRITE && bpf_core_field_exists(rwsem->count.counter)) {
> +			__u64 count = BPF_CORE_READ(rwsem, count.counter);
> +
> +			/* Must have RWSEM_WRITER_LOCKED (bit 0) set */
> +			if (!(count & 1UL))
> +				return NULL;
> +		}
> +	/*
> +	 * Support for the BPF_TYPE_MATCHES argument to the
> +	 * __builtin_preserve_type_info builtin was added at some point during
> +	 * development of clang 15 and it's what is needed for
> +	 * bpf_core_type_matches.
> +	 */
> +#if __has_builtin(__builtin_preserve_type_info) && __clang_major__ >= 15
> +		if (bpf_core_type_matches(struct rw_semaphore___old)) {
> +			struct rw_semaphore___old *old = (void *)lock;
> +
> +			owner = (unsigned long)BPF_CORE_READ(old, owner);
> +		} else if (bpf_core_type_matches(struct rw_semaphore___new)) {
> +			owner = BPF_CORE_READ(rwsem, owner.counter);
> +		}
> +#else
> +		/* assume new struct */
> +		if (bpf_core_field_exists(rwsem->owner.counter))
> +			owner = BPF_CORE_READ(rwsem, owner.counter);
> +#endif
> +		/* RWSEM_READER_OWNED is bit 0 of owner */
> +		if (flags == LCB_F_WRITE && (owner & 1UL))
> +			return NULL;

[Severity: Medium]
In tools/perf/util/bpf_skel/lock_contention.bpf.c, contention_begin() calls
get_lock_owner(pelem->lock, pelem->flags) for both owner stack tracing and
LOCK_AGGR_TASK owner PID attribution:

tools/perf/util/bpf_skel/lock_contention.bpf.c:contention_begin() {
	...
	task = get_lock_owner(pelem->lock, pelem->flags);
	if (!task)
		goto skip_owner;
	...
	if (lock_owner) {
		task = get_lock_owner(pelem->lock, pelem->flags);
	...
}

Here pelem->flags comes from trace_contention_begin() and represents the
contending waiter's mode (LCB_F_READ in rwsem_down_read_slowpath() or
LCB_F_WRITE in rwsem_down_write_slowpath()), rather than the lock owner's
mode.

When a writer contends on a reader-owned rw_semaphore, pelem->flags is
LCB_F_WRITE while rwsem->count does not have RWSEM_WRITER_LOCKED (bit 0) set
and rwsem->owner has RWSEM_READER_OWNED (bit 0) set. Conversely, when a
reader contends (flags == LCB_F_READ), both checks are bypassed.

Will checking flags == LCB_F_WRITE in get_lock_owner() cause it to always
return NULL whenever a writer contends on a reader-owned rw_semaphore,
dropping owner attribution and owner callstacks in perf lock contention -b -o?

[ ... ]

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