From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753887AbcBBH6b (ORCPT ); Tue, 2 Feb 2016 02:58:31 -0500 Received: from mail-pa0-f68.google.com ([209.85.220.68]:34875 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753029AbcBBH63 (ORCPT ); Tue, 2 Feb 2016 02:58:29 -0500 Date: Tue, 2 Feb 2016 16:59:40 +0900 From: Sergey Senozhatsky To: Peter Hurley Cc: Byungchul Park , akpm@linux-foundation.org, mingo@kernel.org, linux-kernel@vger.kernel.org, akinobu.mita@gmail.com, jack@suse.cz, torvalds@linux-foundation.org, Sergey Senozhatsky , Sergey Senozhatsky Subject: Re: [PATCH 3/3] spinlock_debug: panic on recursive lock spin_dump() Message-ID: <20160202075940.GA499@swordfish> References: <20160131123041.GA1306@swordfish> <1454243623-15109-1-git-send-email-sergey.senozhatsky@gmail.com> <1454243623-15109-3-git-send-email-sergey.senozhatsky@gmail.com> <20160201161458.GA609@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160201161458.GA609@swordfish> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 #include +#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 : "", 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