Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320214 > unrolled thread

Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in the debug code

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Byungchul Park <byungchul.park@lge.com> - 2016-01-28 03:00 +0100
    Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-28 03:40 +0100
      RE: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in the debug code "byungchul.park" <byungchul.park@lge.com> - 2016-01-28 06:00 +0100
        Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-28 07:10 +0100
          Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Byungchul Park <byungchul.park@lge.com> - 2016-01-28 09:30 +0100
            Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-28 11:50 +0100
              Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-28 12:00 +0100
                Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-28 16:50 +0100
                  Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in the  debug code Peter Hurley <peter@hurleysoftware.com> - 2016-01-29 00:10 +0100
                    Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Byungchul Park <byungchul.park@lge.com> - 2016-01-29 01:00 +0100
                      Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-29 02:00 +0100
                        Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Byungchul Park <byungchul.park@lge.com> - 2016-01-29 04:10 +0100
                          Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-29 05:10 +0100
                            Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Byungchul Park <byungchul.park@lge.com> - 2016-01-29 13:20 +0100
                    Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-29 01:30 +0100
                      Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in the  debug code Peter Hurley <peter@hurleysoftware.com> - 2016-01-29 05:40 +0100
                        Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-29 06:30 +0100
                          Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in the  debug code Peter Hurley <peter@hurleysoftware.com> - 2016-01-29 06:50 +0100
                            Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-29 07:20 +0100
                              Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-29 07:40 +0100
                                Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-31 13:30 +0100
                                  [PATCH 2/3] printk: introduce reset_console_drivers() Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-31 13:40 +0100
                                    Re: [PATCH 2/3] printk: introduce reset_console_drivers() kbuild test robot <lkp@intel.com> - 2016-01-31 13:50 +0100
                                  [PATCH 1/3] printk: introduce console_reset_on_panic() function Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-31 13:40 +0100
                                    [PATCH 3/3] spinlock_debug: panic on recursive lock spin_dump() Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-31 13:40 +0100
                                      Re: [PATCH 3/3] spinlock_debug: panic on recursive lock spin_dump() Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-02-01 17:20 +0100
                                        Re: [PATCH 3/3] spinlock_debug: panic on recursive lock spin_dump() Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-02-02 09:00 +0100
                                    Re: [PATCH 1/3] printk: introduce console_reset_on_panic() function kbuild test robot <lkp@intel.com> - 2016-01-31 13:50 +0100
                      Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Byungchul Park <byungchul.park@lge.com> - 2016-01-29 08:00 +0100
                        Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-01-29 08:20 +0100
                          Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in  the debug code Byungchul Park <byungchul.park@lge.com> - 2016-01-29 09:20 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1322632

On (01/29/16 15:37), Sergey Senozhatsky wrote:
> 
> panic()->console_panic_mode()->{for_each_console()->reset(), zap_locks()}->console_trelock()->console_unlock().

Hello,

This is not a final submission, just a RFC, so we can settle a better
plan. the patches are not signed off, have known problems (and likely
some unknown). I put a summary in here and send them out as a reply to
this email, so it'll be easier to review/comment/discuss.

patch 0001
***************
CPU stop IPI issued from panic() on CPUA, can leave console_sem locked
on CPUB if that cpu was holding the console_sem lock at the time when
IPI arrived. console_flush_on_panic() is trying to workaround it by
ignoring the return status of console_trylock() and unconditionally
executing console_unlock().

console_unlock() has a dependency on at least one more
lock - `logbuf_lock', which can be corrupted, for example, thus
console_unlock() may not be able to print anything afterall.

Introduce console_reset_on_panic() function to zap (re-init) printk
locks and call this function from panic().

WARNING
=======
This must be improved. console_reset_on_panic() is called before
smp_send_stop(), so:
a) we can have several CPU looping in console_unlock(), which is not
so critical.
b) we can re-init logbuf_lock while other CPU is holding it. Which
is more serious and needs to fixed.

The reason why console_reset_on_panic() is called this early is that
panicing CPU does pr_emerg("Kernel panic...") and dump_stack()
before it sends out smp_send_stop(). So if console_sem or logbug_lock,
or some console device driver lock is/are corrupted then panic() may
never smp_send_stop().




patch 0002
***************
Console driver(-s) can be in any state when CPU stop IPI
arrives from panic() issued on another CPU, so
console_flush_on_panic()->console_unlock() can call
con->write() callback on a locked console driver.

Introduce reset_console_drivers() that attempts to reset()
every console in via a console driver specific ->reset()
call.

Invoke reset_console_drivers() from console_reset_on_panic().

WARNING
=======
console_reset_on_panic() needs to be fixed.




patch 0003 -- detect recursive spin_dump() and panic() the system
***************
spin_dump() calls printk() which can attempt to reacquire the
'buggy' lock (one of printk's lock, or console device driver lock,
etc.) and thus spin_dump() will recursive into itself. Steal most
significant bit of spin_lock->owner_cpu to keep there a mark
that spin_dump() is in progress for that particular spin_lock.
spin_dump() will now set SPIN_DUMP_IN_PROGRESS bit at the
beginning of spin_dump() and clear it at the end, so it's
possible to detect recursive spin_dump() calls by checking if
lock's owner_cpu already has SPIN_DUMP_IN_PROGRESS bit already
set. panic() the system when spin_dump() recursion occurs.



	-ss

[toc] | [prev] | [next] | [standalone]

#1322637 — [PATCH 2/3] printk: introduce reset_console_drivers()

Console driver(-s) can be in any state when CPU stop IPI
arrives from panic() issued on another CPU, so
console_flush_on_panic()->console_unlock() can call
con->write() callback on a locked console driver.

Introduce reset_console_drivers() that attempts to reset()
every console in via a console driver specific ->reset()
call.

Invoke reset_console_drivers() from console_reset_on_panic().

WARNING
=======
console_reset_on_panic() needs to be fixed.

Not-Yet-Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 include/linux/console.h |  1 +
 kernel/printk/printk.c  | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/console.h b/include/linux/console.h
index 6edc2ea..f745ffe 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -125,6 +125,7 @@ struct console {
 	void	(*unblank)(void);
 	int	(*setup)(struct console *, char *);
 	int	(*match)(struct console *, char *name, int idx, char *options);
+	void	(*reset)(struct console *);
 	short	flags;
 	short	index;
 	int	cflag;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 34642f7..e245f9f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1462,6 +1462,15 @@ static void call_console_drivers(int level,
 	}
 }
 
+static void reset_console_drivers(void)
+{
+	struct console *con;
+
+	for_each_console(con)
+		if ((con->flags & CON_ENABLED) && con->reset)
+			con->reset(con);
+}
+
 /*
  * Zap console related locks when oopsing.
  * To leave time for slow consoles to print a full oops,
@@ -2397,6 +2406,7 @@ void console_flush_on_panic(void)
 void console_reset_on_panic(void)
 {
 	zap_locks();
+	reset_console_drivers();
 }
 
 /*
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1322643 — Re: [PATCH 2/3] printk: introduce reset_console_drivers()

[Multipart message — attachments visible in raw view] — view raw

Hi Sergey,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.5-rc1 next-20160129]
[cannot apply to tip/core/locking]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/printk-introduce-console_reset_on_panic-function/20160131-203605
config: i386-tinyconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/printk/printk.c: In function 'console_reset_on_panic':
   kernel/printk/printk.c:2426:2: error: implicit declaration of function 'zap_locks' [-Werror=implicit-function-declaration]
     zap_locks();
     ^
>> kernel/printk/printk.c:2427:2: error: implicit declaration of function 'reset_console_drivers' [-Werror=implicit-function-declaration]
     reset_console_drivers();
     ^
   cc1: some warnings being treated as errors

vim +/reset_console_drivers +2427 kernel/printk/printk.c

  2420		console_may_schedule = 0;
  2421		console_unlock();
  2422	}
  2423	
  2424	void console_reset_on_panic(void)
  2425	{
> 2426		zap_locks();
> 2427		reset_console_drivers();
  2428	}
  2429	
  2430	/*

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1322638 — [PATCH 1/3] printk: introduce console_reset_on_panic() function

CPU stop IPI issued from panic() on CPUA, can leave console_sem locked
on CPUB if that cpu was holding the console_sem lock at the time when
IPI arrived. console_flush_on_panic() is trying to workaround it by
ignoring the return status of console_trylock() and unconditionally
executing console_unlock().

console_unlock() has a dependency on at least one more
lock - `logbuf_lock', which can be corrupted, for example, thus
console_unlock() may not be able to print anything afterall.

Introduce console_reset_on_panic() function to zap (re-init) printk
locks and call this function from panic().

WARNING
=======
This must be improved. console_reset_on_panic() is called before
smp_send_stop(), so:
a) we can have several CPU looping in console_unlock(), which is not
so critical.
b) we can re-init logbuf_lock while other CPU is holding it. Which
is more serious and needs to fixed.

The reason why console_reset_on_panic() is called this early is that
panicing CPU does pr_emerg("Kernel panic...") and dump_stack()
before it sends out smp_send_stop(). So if console_sem or logbug_lock,
or some console device driver lock is/are corrupted then panic() may
never smp_send_stop().

Not-Yet-Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 include/linux/console.h | 1 +
 kernel/panic.c          | 1 +
 kernel/printk/printk.c  | 5 +++++
 3 files changed, 7 insertions(+)

diff --git a/include/linux/console.h b/include/linux/console.h
index ea731af..6edc2ea 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -151,6 +151,7 @@ extern void console_unlock(void);
 extern void console_conditional_schedule(void);
 extern void console_unblank(void);
 extern void console_flush_on_panic(void);
+extern void console_reset_on_panic(void);
 extern struct tty_driver *console_device(int *);
 extern void console_stop(struct console *);
 extern void console_start(struct console *);
diff --git a/kernel/panic.c b/kernel/panic.c
index d96469d..71d43f6 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -119,6 +119,7 @@ void panic(const char *fmt, ...)
 
 	console_verbose();
 	bust_spinlocks(1);
+	console_reset_on_panic();
 	va_start(args, fmt);
 	vsnprintf(buf, sizeof(buf), fmt, args);
 	va_end(args);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 097ca8b..34642f7 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2394,6 +2394,11 @@ void console_flush_on_panic(void)
 	console_unlock();
 }
 
+void console_reset_on_panic(void)
+{
+	zap_locks();
+}
+
 /*
  * Return the console tty driver structure and its associated index
  */
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1322640 — [PATCH 3/3] spinlock_debug: panic on recursive lock spin_dump()

spin_dump() calls printk() which can attempt to reacquire the
'buggy' lock (one of printk's lock, or console device driver lock,
etc.) and thus spin_dump() will recursive into itself. Steal most
significant bit of spin_lock->owner_cpu to keep there a mark
that spin_dump() is in progress for that particular spin_lock.
spin_dump() will now set SPIN_DUMP_IN_PROGRESS bit at the
beginning of spin_dump() and clear it at the end, so it's
possible to detect recursive spin_dump() calls by checking if
lock's owner_cpu already has SPIN_DUMP_IN_PROGRESS bit already
set. panic() the system when spin_dump() recursion occurs.

Not-Yet-Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/locking/spinlock_debug.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/spinlock_debug.c b/kernel/locking/spinlock_debug.c
index 0374a59..1a4a864 100644
--- a/kernel/locking/spinlock_debug.c
+++ b/kernel/locking/spinlock_debug.c
@@ -13,6 +13,9 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 
+#define SPIN_DUMP_IN_PROGRESS	(1 << 31)
+#define DEFAULT_OWNER_CPU	(INT_MAX >> 1)
+
 void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
 			  struct lock_class_key *key)
 {
@@ -26,7 +29,7 @@ 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 = DEFAULT_OWNER_CPU;
 }
 
 EXPORT_SYMBOL(__raw_spin_lock_init);
@@ -44,7 +47,7 @@ void __rwlock_init(rwlock_t *lock, const char *name,
 	lock->raw_lock = (arch_rwlock_t) __ARCH_RW_LOCK_UNLOCKED;
 	lock->magic = RWLOCK_MAGIC;
 	lock->owner = SPINLOCK_OWNER_INIT;
-	lock->owner_cpu = -1;
+	lock->owner_cpu = DEFAULT_OWNER_CPU;
 }
 
 EXPORT_SYMBOL(__rwlock_init);
@@ -53,6 +56,13 @@ static void spin_dump(raw_spinlock_t *lock, const char *msg)
 {
 	struct task_struct *owner = NULL;
 
+	if (lock->owner_cpu & SPIN_DUMP_IN_PROGRESS)
+		panic("BUG: lock: %pS spin dump recursion on CPU#%d, %s/%d\n",
+				lock, raw_smp_processor_id(),
+				current->comm, task_pid_nr(current));
+
+	lock->owner_cpu |= SPIN_DUMP_IN_PROGRESS;
+
 	if (lock->owner && lock->owner != SPINLOCK_OWNER_INIT)
 		owner = lock->owner;
 	printk(KERN_EMERG "BUG: spinlock %s on CPU#%d, %s/%d\n",
@@ -63,8 +73,10 @@ 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 == DEFAULT_OWNER_CPU ? -1 : lock->owner_cpu);
 	dump_stack();
+
+	lock->owner_cpu &= ~SPIN_DUMP_IN_PROGRESS;
 }
 
 static void spin_bug(raw_spinlock_t *lock, const char *msg)
@@ -100,7 +112,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 = DEFAULT_OWNER_CPU;
 }
 
 static void __spin_lock_debug(raw_spinlock_t *lock)
@@ -244,7 +256,7 @@ static inline void debug_write_unlock(rwlock_t *lock)
 	RWLOCK_BUG_ON(lock->owner_cpu != raw_smp_processor_id(),
 							lock, "wrong CPU");
 	lock->owner = SPINLOCK_OWNER_INIT;
-	lock->owner_cpu = -1;
+	lock->owner_cpu = DEFAULT_OWNER_CPU;
 }
 
 #if 0		/* This can cause lockups */
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1323293 — Re: [PATCH 3/3] spinlock_debug: panic on recursive lock spin_dump()

On (01/31/16 21:33), Sergey Senozhatsky wrote:
> spin_dump() calls printk() which can attempt to reacquire the
> 'buggy' lock (one of printk's lock, or console device driver lock,
> etc.) and thus spin_dump() will recurse into itself.

how about splitting ->owner_cpu 4 bytes as:

                            |                                   |
  1 byte spin bug recursion | 1 byte spin_dump recursion counter | 2 bytes owner cpu
                            |                                   |

?

so we have 2 bytes to store the lock owner's smp_processor_id().
(I just don't want to extend spin lock with another 4 bytes to
fix the recursion.)


spin_dump() will increment lock's spin_dump recursion counter and decrement it
upon the exit from spin_dump(). if lock's recursion counter is equal to
U8_MAX -- panic() the system, we have recursed spin_dump() into spin_dump()
enough... (U8_MAX recursive printk->spin_dump->printk calls look big enough).

for spin_bug() recursion we don't have to maintain any counter: set
SPIN_BUG_RECURSION bit at the beginning of spin_bug() and clear it at
the end. if the lock already has SPIN_BUG_RECURSION bit set -- panic()
the system.

compile tested only.
will test it tomorrow.

---
 kernel/locking/spinlock_debug.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/spinlock_debug.c b/kernel/locking/spinlock_debug.c
index 0374a59..54bcc3b 100644
--- a/kernel/locking/spinlock_debug.c
+++ b/kernel/locking/spinlock_debug.c
@@ -13,6 +13,9 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 
+#define SPIN_DUMP_RECURSION	(1 << 16)
+#define SPIN_BUG_RECURSION	(1 << 31)
+
 void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
 			  struct lock_class_key *key)
 {
@@ -26,7 +29,7 @@ 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;
 }
 
 EXPORT_SYMBOL(__raw_spin_lock_init);
@@ -44,14 +47,26 @@ void __rwlock_init(rwlock_t *lock, const char *name,
 	lock->raw_lock = (arch_rwlock_t) __ARCH_RW_LOCK_UNLOCKED;
 	lock->magic = RWLOCK_MAGIC;
 	lock->owner = SPINLOCK_OWNER_INIT;
-	lock->owner_cpu = -1;
+	lock->owner_cpu = USHRT_MAX;
 }
 
 EXPORT_SYMBOL(__rwlock_init);
 
+static void spin_recursion_panic(raw_spinlock_t *lock, const char *msg)
+{
+	panic("BUG: 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;
+	u8 recursion_counter = (u8)(lock->owner_cpu >> 16);
+
+	lock->owner_cpu += SPIN_DUMP_RECURSION;
+	if ((recursion_counter + 1) == U8_MAX)
+		spin_recursion_panic(lock, "spin_dump()");
 
 	if (lock->owner && lock->owner != SPINLOCK_OWNER_INIT)
 		owner = lock->owner;
@@ -63,8 +78,10 @@ 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();
+
+	lock->owner_cpu -= SPIN_DUMP_RECURSION;
 }
 
 static void spin_bug(raw_spinlock_t *lock, const char *msg)
@@ -72,7 +89,12 @@ static void spin_bug(raw_spinlock_t *lock, const char *msg)
 	if (!debug_locks_off())
 		return;
 
+	if (lock->owner_cpu & SPIN_BUG_RECURSION)
+		spin_recursion_panic(lock, "spin_bug()");
+
+	lock->owner_cpu |= SPIN_BUG_RECURSION;
 	spin_dump(lock, msg);
+	lock->owner_cpu &= ~SPIN_BUG_RECURSION;
 }
 
 #define SPIN_BUG_ON(cond, lock, msg) if (unlikely(cond)) spin_bug(lock, msg)
@@ -100,7 +122,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)
@@ -244,7 +266,7 @@ static inline void debug_write_unlock(rwlock_t *lock)
 	RWLOCK_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;
 }
 
 #if 0		/* This can cause lockups */
-- 
2.7.0.75.g3ee1e0f

[toc] | [prev] | [next] | [standalone]

#1323863 — 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

[toc] | [prev] | [next] | [standalone]

#1322642 — Re: [PATCH 1/3] printk: introduce console_reset_on_panic() function

[Multipart message — attachments visible in raw view] — view raw

Hi Sergey,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.5-rc1 next-20160129]
[cannot apply to tip/core/locking]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/printk-introduce-console_reset_on_panic-function/20160131-203605
config: i386-tinyconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/printk/printk.c: In function 'console_reset_on_panic':
>> kernel/printk/printk.c:2417:2: error: implicit declaration of function 'zap_locks' [-Werror=implicit-function-declaration]
     zap_locks();
     ^
   cc1: some warnings being treated as errors

vim +/zap_locks +2417 kernel/printk/printk.c

  2411		console_may_schedule = 0;
  2412		console_unlock();
  2413	}
  2414	
  2415	void console_reset_on_panic(void)
  2416	{
> 2417		zap_locks();
  2418	}
  2419	
  2420	/*

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1321515

On Fri, Jan 29, 2016 at 09:27:03AM +0900, Sergey Senozhatsky wrote:
> 
> well, the stack is surely limited, but on every
> spin_dump()->spin_lock() recursive call it does another
> round of
> 
> 	u64 loops = loops_per_jiffy * HZ;
> 
> 	for (i = 0; i < loops; i++) {
> 		if (arch_spin_trylock(&lock->raw_lock))
> 			return;
> 		__delay(1);
> 	}
> 
> so if you have 1000 spin_dump()->spin_lock() then, well,
> something has been holding the lock for '1000 * loops_per_jiffy * HZ'.

Or the printk() is heavily called and the lock is congested.

[toc] | [prev] | [next] | [standalone]

#1321520

On (01/29/16 15:54), Byungchul Park wrote:
> On Fri, Jan 29, 2016 at 09:27:03AM +0900, Sergey Senozhatsky wrote:
> > 
> > well, the stack is surely limited, but on every
> > spin_dump()->spin_lock() recursive call it does another
> > round of
> > 
> > 	u64 loops = loops_per_jiffy * HZ;
> > 
> > 	for (i = 0; i < loops; i++) {
> > 		if (arch_spin_trylock(&lock->raw_lock))
> > 			return;
> > 		__delay(1);
> > 	}
> > 
> > so if you have 1000 spin_dump()->spin_lock() then, well,
> > something has been holding the lock for '1000 * loops_per_jiffy * HZ'.
> 
> Or the printk() is heavily called and the lock is congested.

well, isn't it the case that ticket-based locking assumes at least
some sort of fairness? how many cpus do you have there? you can
have `num_online_cpus() - 1' tasks spinning on the spin lock and
1 owning the spin lock... if your lock is in correct state (no
before/after spinlock debug errors) even most unlucky task should
get the lock eventually...

	-ss

[toc] | [prev] | [next] | [standalone]

#1321543

On Fri, Jan 29, 2016 at 04:13:30PM +0900, Sergey Senozhatsky wrote:
> On (01/29/16 15:54), Byungchul Park wrote:
> > On Fri, Jan 29, 2016 at 09:27:03AM +0900, Sergey Senozhatsky wrote:
> > > 
> > > well, the stack is surely limited, but on every
> > > spin_dump()->spin_lock() recursive call it does another
> > > round of
> > > 
> > > 	u64 loops = loops_per_jiffy * HZ;
> > > 
> > > 	for (i = 0; i < loops; i++) {
> > > 		if (arch_spin_trylock(&lock->raw_lock))
                       ^^^
		       this is a trylock.

> > > 			return;
> > > 		__delay(1);
> > > 	}
> > > 
> > > so if you have 1000 spin_dump()->spin_lock() then, well,
> > > something has been holding the lock for '1000 * loops_per_jiffy * HZ'.
> > 
> > Or the printk() is heavily called and the lock is congested.
> 
> well, isn't it the case that ticket-based locking assumes at least
> some sort of fairness? how many cpus do you have there? you can

It's true for a congestion between arch_spin_lock()s, not trylock(). And I
cannot remember how many online cpus there are, since I frequently change
the number. Sorry. But the range is from 2 to 4.

> have `num_online_cpus() - 1' tasks spinning on the spin lock and
> 1 owning the spin lock... if your lock is in correct state (no
> before/after spinlock debug errors) even most unlucky task should
> get the lock eventually...
> 
> 	-ss

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web