lkml.org 
[lkml]   [2023]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC PATCH] kernel/locking: introduce stack_handle for tracing the callstack
    From
    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

    \
     
     \ /
      Last update: 2023-03-26 23:49    [W:3.975 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site