Re: [RFC PATCH] kernel/locking: introduce stack_handle for tracing the callstack

[Waiman Long]

On 1/20/23 03:48, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
>
> When deadlock happens, sometimes it is hard to know how the owner get the lock
> especially the owner is running when snapshot(ramdump). Introduce stack_handle
> to record the trace.
>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> ---
>   include/linux/rwsem.h  |  9 ++++++++-
>   kernel/locking/rwsem.c | 18 ++++++++++++++++++
>   2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
> index efa5c32..ad4c591 100644
> --- a/include/linux/rwsem.h
> +++ b/include/linux/rwsem.h
> @@ -16,6 +16,11 @@
>   #include <linux/atomic.h>
>   #include <linux/err.h>
>   
> +#define CONFIG_TRACE_RWSEMS
> +#ifdef CONFIG_TRACE_RWSEMS
> +typedef u32 depot_stack_handle_t;
> +#endif
> +

We don't define CONFIG_* macro in header file like that. We define them 
in Kconfig files. There is a CONFIG_DEBUG_RWSEMS defined. Maybe you can 
use it for the time being. You should also include dependency on 
CONFIG_STACKDEPOT too.

Moreover, I believe depot_stack_handle_t has already been defined in 
include/linux/stackdepot.h. You should include this header file instead 
of defining your own.


>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   # define __RWSEM_DEP_MAP_INIT(lockname)			\
>   	.dep_map = {					\
> @@ -31,7 +36,6 @@
>   #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
>   #include <linux/osq_lock.h>
>   #endif
> -
>   /*
>    * For an uncontended rwsem, count and owner are the only fields a task
>    * needs to touch when acquiring the rwsem. So they are put next to each
> @@ -60,6 +64,9 @@ struct rw_semaphore {
>   #ifdef CONFIG_DEBUG_RWSEMS
>   	void *magic;
>   #endif
> +#ifdef CONFIG_TRACE_RWSEMS
> +	depot_stack_handle_t trace_handle;
> +#endif
>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
>   	struct lockdep_map	dep_map;
>   #endif
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index 4487359..a12766e 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -28,6 +28,7 @@
>   #include <linux/rwsem.h>
>   #include <linux/atomic.h>
>   #include <trace/events/lock.h>
> +#include <linux/stacktrace.h>
>   
>   #ifndef CONFIG_PREEMPT_RT
>   #include "lock_events.h"
> @@ -74,6 +75,7 @@
>   		list_empty(&(sem)->wait_list) ? "" : "not "))	\
>   			debug_locks_off();			\
>   	} while (0)
> +#define MAX_TRACE		16
>   #else
>   # define DEBUG_RWSEMS_WARN_ON(c, sem)
>   #endif
> @@ -174,6 +176,9 @@ static inline void __rwsem_set_reader_owned(struct rw_semaphore *sem,
>   		(atomic_long_read(&sem->owner) & RWSEM_NONSPINNABLE);
>   
>   	atomic_long_set(&sem->owner, val);
> +#ifdef CONFIG_TRACE_RWSEMS
> +	sem->trace_handle = owner ? set_track_prepare() : NULL;
> +#endif
>   }

The problem with tracking readers is that a rwsem can have many readers 
at the same time. We can store information for only one of the readers 
and it will be costly if we want to track them all.

>   
>   static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
> @@ -397,6 +402,19 @@ enum rwsem_wake_type {
>   	return false;
>   }
>   stack_depot_save
>
> +#ifdef CONFIG_TRACE_RWSEMS
> +static inline depot_stack_handle_t set_track_prepare(void)
> +{
> +	depot_stack_handle_t trace_handle;
> +	unsigned long entries[MAX_TRACE];
> +	unsigned int nr_entries;
> +
> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 3);
> +	trace_handle = stack_depot_save(entries, nr_entries, GFP_NOWAIT);
> +
> +	return trace_handle;
> +}
> +#endif
>   /*
>    * handle the lock release when processes blocked on it that can now run
>    * - if we come here from up_xxxx(), then the RWSEM_FLAG_WAITERS bit must

Similar set_track_prepare() is defined in mm/slub.c and mm/kmemleak.c. I 
would suggest consolidate them into a common library function and use it 
instead.

Even if we save the stack trace, where are you planning to have this 
information used. It is not clear from this patch.

Cheers,
Longman