Messages in this thread Patch in this message | | | Date | Tue, 2 Feb 2016 16:59:40 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [PATCH 3/3] spinlock_debug: panic on recursive lock spin_dump() |
| |
On (02/02/16 01:14), Sergey Senozhatsky wrote: > how about splitting ->owner_cpu 4 bytes as: > > | | > 1 byte spin bug recursion | 1 byte spin_dump recursion counter | 2 bytes owner cpu > | |
after some thinking... no, this will not do the trick. one byte is not enough for recursion counter -- we can have 8K CPUs on the system and 8K-1 cpus can "suspect a lockup". so, a slightly different approach:
1) split ->owner_cpu 4 bytes in struct raw_spinlock unsigned short owner_cpu; unsigned short recursion;
I still can use only ->owner_cpu, but it's much easier when they are apart. with a single 4 byte variable for recursion and cpu owner we need to take extra care of higher 2 bytes every time we touch the ->owner_cpu
CPU1 CPU2 spin_dump ->owner_cpu[recursion_bits] += 1 spin_unlock ->owner_cpu = -1 ^^^ need to store cpu_id in lower 2 bytes, avoiding overwrite of 2 higher bytes, etc. ->owner_cpu[recursion_bits] -= 1
which is fragile and ugly.
2) ->recursion has most significant bit for spin_bug() bit, the remaining bits are for recursion counter.
spin_bug() does set SPIN_BUG bit (most significant bit) spin_dump clear SPIN_BUG bit
spin_dump() does read SPIN_BUG bit inc ->recursion do_checks printk... dec ->recursion
and the do_checks is:
-- "if the SPIN_BUG bit is set AND recursion counter > NR_CPUS" then we have a spin_bug() recursion on at least one of the CPUs and we need to panic the system
printk spin_lock spin_bug spin_dump printk spin_lock spin_bug spin_dump ...
-- "if the SPIN_BUG bit is clear AND recursion counter >= SHRT_MAX/2" then we have spin_dump() recursion (16K calls.. can be bigger) and we need to panic the system. if recursion counter < SHRT_MAX/2 - keep going. "suspected soft lockup" potentially can be resolved (the lock owner unlocks the lock), so we need to have a big enough limit before we declare panic().
printk spin_lock spin_dump printk spin_lock spin_dump ...
I guess I'll I'll start a new thread with the next submission, to refresh it.
RFC, any opinions are appreciated. not yet tested code.
--- include/linux/spinlock_types.h | 4 +++- kernel/locking/spinlock_debug.c | 40 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/include/linux/spinlock_types.h b/include/linux/spinlock_types.h index 73548eb..c8f6b56 100644 --- a/include/linux/spinlock_types.h +++ b/include/linux/spinlock_types.h @@ -23,7 +23,9 @@ typedef struct raw_spinlock { unsigned int break_lock; #endif #ifdef CONFIG_DEBUG_SPINLOCK - unsigned int magic, owner_cpu; + unsigned int magic; + unsigned short owner_cpu; + unsigned short recursion; void *owner; #endif #ifdef CONFIG_DEBUG_LOCK_ALLOC diff --git a/kernel/locking/spinlock_debug.c b/kernel/locking/spinlock_debug.c index 0374a59..f838fe9 100644 --- a/kernel/locking/spinlock_debug.c +++ b/kernel/locking/spinlock_debug.c @@ -13,6 +13,8 @@ #include <linux/delay.h> #include <linux/export.h> +#define SPIN_BUG_RECURSION (1 << 15) + void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name, struct lock_class_key *key) { @@ -26,7 +28,8 @@ void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name, lock->raw_lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; lock->magic = SPINLOCK_MAGIC; lock->owner = SPINLOCK_OWNER_INIT; - lock->owner_cpu = -1; + lock->owner_cpu = USHRT_MAX; + lock->recursion = 0; } EXPORT_SYMBOL(__raw_spin_lock_init); @@ -49,9 +52,31 @@ void __rwlock_init(rwlock_t *lock, const char *name, EXPORT_SYMBOL(__rwlock_init); +static void spin_recursion_panic(raw_spinlock_t *lock, const char *msg) +{ + panic("lock: %pS %s recursion on CPU#%d, %s/%d\n", + lock, msg, raw_smp_processor_id(), + current->comm, task_pid_nr(current)); +} + static void spin_dump(raw_spinlock_t *lock, const char *msg) { struct task_struct *owner = NULL; + unsigned short dump_counter; + bool spin_bug; + + spin_bug = lock->recursion & SPIN_BUG_RECURSION; + dump_counter = lock->recursion & SHRT_MAX; + smp_rmb(); + + smp_wmb(); + lock->recursion += 1; + dump_counter++; + + if (spin_bug && dump_counter > NR_CPUS) /* num_online_cpus() */ + spin_recursion_panic(lock, "spin_bug()"); + if (dump_counter >= (SHRT_MAX >> 1)) + spin_recursion_panic(lock, "spin_dump()"); if (lock->owner && lock->owner != SPINLOCK_OWNER_INIT) owner = lock->owner; @@ -63,8 +88,11 @@ static void spin_dump(raw_spinlock_t *lock, const char *msg) lock, lock->magic, owner ? owner->comm : "<none>", owner ? task_pid_nr(owner) : -1, - lock->owner_cpu); + lock->owner_cpu == USHRT_MAX ? -1 : lock->owner_cpu); dump_stack(); + + smp_wmb(); + lock->recursion -= 1; } static void spin_bug(raw_spinlock_t *lock, const char *msg) @@ -72,7 +100,13 @@ static void spin_bug(raw_spinlock_t *lock, const char *msg) if (!debug_locks_off()) return; + smp_wmb(); + lock->recursion |= SPIN_BUG_RECURSION; + spin_dump(lock, msg); + + smp_wmb(); + lock->recursion &= ~SPIN_BUG_RECURSION; } #define SPIN_BUG_ON(cond, lock, msg) if (unlikely(cond)) spin_bug(lock, msg) @@ -100,7 +134,7 @@ static inline void debug_spin_unlock(raw_spinlock_t *lock) SPIN_BUG_ON(lock->owner_cpu != raw_smp_processor_id(), lock, "wrong CPU"); lock->owner = SPINLOCK_OWNER_INIT; - lock->owner_cpu = -1; + lock->owner_cpu = USHRT_MAX; } static void __spin_lock_debug(raw_spinlock_t *lock) -- 2.7.0
| |