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
