[RFC] kernel/locking: introduce stack_handle for tracing the callstack
Commit Message
From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
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@unisoc.com>
---
include/linux/rwsem.h | 9 ++++++++-
kernel/locking/rwsem.c | 18 ++++++++++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
Comments
On 1/20/23 03:48, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
>
> 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@unisoc.com>
> ---
> 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
@@ -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
+
#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
@@ -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
}
static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
@@ -397,6 +402,19 @@ enum rwsem_wake_type {
return false;
}
+#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