[PATCH] Provide better printk() support for SMP machines

[PATCH] Provide better printk() support for SMP machines

[David Howells]

The attached patch prevents oopses interleaving with characters from other
printks on other machines by only zapping the locks if the oops is happening
on the machine holding the lock.

It might be better if the oops generator got the lock and then called an inner
vprintk routine that assumed the caller holds the lock, thus making oops
reports "atomic".

Signed-Off-By: David Howells <dhowells@redhat.com>
---
diff -uNrp linux-2.6.12-mm1/kernel/printk.c linux-2.6.12-mm1-cachefs-wander/kernel/printk.c
--- linux-2.6.12-mm1/kernel/printk.c	2005-06-22 13:54:08.000000000 +0100
+++ linux-2.6.12-mm1-cachefs-wander/kernel/printk.c	2005-06-22 13:57:02.000000000 +0100
@@ -514,6 +514,9 @@ asmlinkage int printk(const char *fmt, .
 	return r;
 }
 
+/* cpu currently holding logbuf_lock */
+static volatile int printk_cpu = -1;
+
 asmlinkage int vprintk(const char *fmt, va_list args)
 {
 	unsigned long flags;
@@ -522,11 +525,15 @@ asmlinkage int vprintk(const char *fmt, 
 	static char printk_buf[1024];
 	static int log_level_unknown = 1;
 
-	if (unlikely(oops_in_progress))
+	if (unlikely(oops_in_progress) && printk_cpu == smp_processor_id())
+		/* If a crash is occurring during printk() on this CPU,
+		 * make sure we can't deadlock */
 		zap_locks();
 
 	/* This stops the holder of console_sem just where we want him */
 	spin_lock_irqsave(&logbuf_lock, flags);
+	printk_cpu = smp_processor_id();
+	smp_wmb();
 
 	/* Emit the output into the temporary buffer */
 	printed_len = vscnprintf(printk_buf, sizeof(printk_buf), fmt, args);
@@ -595,6 +602,7 @@ asmlinkage int vprintk(const char *fmt, 
 		 * CPU until it is officially up.  We shouldn't be calling into
 		 * random console drivers on a CPU which doesn't exist yet..
 		 */
+		printk_cpu = -1;
 		spin_unlock_irqrestore(&logbuf_lock, flags);
 		goto out;
 	}
@@ -604,6 +612,7 @@ asmlinkage int vprintk(const char *fmt, 
 		 * We own the drivers.  We can drop the spinlock and let
 		 * release_console_sem() print the text
 		 */
+		printk_cpu = -1;
 		spin_unlock_irqrestore(&logbuf_lock, flags);
 		console_may_schedule = 0;
 		release_console_sem();
@@ -613,6 +622,7 @@ asmlinkage int vprintk(const char *fmt, 
 		 * allows the semaphore holder to proceed and to call the
 		 * console drivers with the output which we just produced.
 		 */
+		printk_cpu = -1;
 		spin_unlock_irqrestore(&logbuf_lock, flags);
 	}
 out:

Re: [PATCH] Provide better printk() support for SMP machines

[Andrew Morton]

David Howells <dhowells@redhat.com> wrote:
>
>  The attached patch prevents oopses interleaving with characters from other
>  printks on other machines by only zapping the locks if the oops is happening
>  on the machine holding the lock.

(s/machine/CPU/)

hm, I guess it adds a theoretical deadlock if some other CPU is in the
middle of printk and is trying to take some_lock and this CPU takes an oops
while holding some_lock.  Probably that's an acceptable tradeoff though.

>  --- linux-2.6.12-mm1/kernel/printk.c	2005-06-22 13:54:08.000000000 +0100
>  +++ linux-2.6.12-mm1-cachefs-wander/kernel/printk.c	2005-06-22 13:57:02.000000000 +0100
>  @@ -514,6 +514,9 @@ asmlinkage int printk(const char *fmt, .
>   	return r;
>   }
>   
>  +/* cpu currently holding logbuf_lock */
>  +static volatile int printk_cpu = -1;
>  +

Does this guy really need to be volatile?  Coud we use atomic_t and lose
that wmb()?

>   asmlinkage int vprintk(const char *fmt, va_list args)
>   {
>   	unsigned long flags;
>  @@ -522,11 +525,15 @@ asmlinkage int vprintk(const char *fmt, 
>   	static char printk_buf[1024];
>   	static int log_level_unknown = 1;
>   
>  -	if (unlikely(oops_in_progress))
>  +	if (unlikely(oops_in_progress) && printk_cpu == smp_processor_id())
>  +		/* If a crash is occurring during printk() on this CPU,
>  +		 * make sure we can't deadlock */

Methinks this should be raw_smp_processor_id().

Re: [PATCH] Provide better printk() support for SMP machines

[David Howells]

Andrew Morton <akpm@osdl.org> wrote:

> hm, I guess it adds a theoretical deadlock if some other CPU is in the
> middle of printk and is trying to take some_lock and this CPU takes an oops
> while holding some_lock.  Probably that's an acceptable tradeoff though.

What it perhaps needs is a maximum number of spins or something like that.

> Does this guy really need to be volatile?

atomic_t is volatile, so that makes no difference.

Actually, I shouldn't use an atomic_t or a signed int as the CPU ID is
unsigned:-/

I'm using -1 / UINT_MAX as a "not in use" thing, but I'm not sure that's
strictly viable.

> Coud we use atomic_t and lose that wmb()?

I talked it over with David Woodhouse, and he's convinced me that we don't
need the barrier: the only thing we care about is getting a printk within a
printk on the CPU because of an oops. THEN we have to break the lock. And in
that case we're looking for the CPU number being set to that of our CPU, and
only our CPU will ever set that.

> Methinks this should be raw_smp_processor_id().

Not only that, but the whole function needs wrapping in preemption
disablement, lest the processor ID change under us.

David

[PATCH] Provide better printk() support for SMP machines [try #2]

[David Howells]

The attached patch prevents oopses interleaving with characters from other
printks on other CPUs by only breaking the lock if the oops is happening on
the machine holding the lock.

It might be better if the oops generator got the lock and then called an inner
vprintk routine that assumed the caller holds the lock, thus making oops
reports "atomic".

Signed-Off-By: David Howells <dhowells@redhat.com>
---
warthog>diffstat -p1 ../printk-smp-2613rc2mm1-2.diff 
 kernel/printk.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff -uNrp linux-2.6.13-rc2-mm1/kernel/printk.c linux-2.6.13-rc2-mm1-cachefs/kernel/printk.c
--- linux-2.6.13-rc2-mm1/kernel/printk.c	2005-07-07 22:24:17.000000000 +0100
+++ linux-2.6.13-rc2-mm1-cachefs/kernel/printk.c	2005-07-08 12:35:51.000000000 +0100
@@ -514,6 +514,9 @@ asmlinkage int printk(const char *fmt, .
 	return r;
 }
 
+/* cpu currently holding logbuf_lock */
+static volatile unsigned int printk_cpu = UINT_MAX;
+
 asmlinkage int vprintk(const char *fmt, va_list args)
 {
 	unsigned long flags;
@@ -522,11 +525,15 @@ asmlinkage int vprintk(const char *fmt, 
 	static char printk_buf[1024];
 	static int log_level_unknown = 1;
 
-	if (unlikely(oops_in_progress))
+	preempt_disable();
+	if (unlikely(oops_in_progress) && printk_cpu == raw_smp_processor_id())
+		/* If a crash is occurring during printk() on this CPU,
+		 * make sure we can't deadlock */
 		zap_locks();
 
 	/* This stops the holder of console_sem just where we want him */
 	spin_lock_irqsave(&logbuf_lock, flags);
+	printk_cpu = raw_smp_processor_id();
 
 	/* Emit the output into the temporary buffer */
 	printed_len = vscnprintf(printk_buf, sizeof(printk_buf), fmt, args);
@@ -588,13 +595,14 @@ asmlinkage int vprintk(const char *fmt, 
 			log_level_unknown = 1;
 	}
 
-	if (!cpu_online(smp_processor_id())) {
+	if (!cpu_online(raw_smp_processor_id())) {
 		/*
 		 * Some console drivers may assume that per-cpu resources have
 		 * been allocated.  So don't allow them to be called by this
 		 * CPU until it is officially up.  We shouldn't be calling into
 		 * random console drivers on a CPU which doesn't exist yet..
 		 */
+		printk_cpu = UINT_MAX;
 		spin_unlock_irqrestore(&logbuf_lock, flags);
 		goto out;
 	}
@@ -604,6 +612,7 @@ asmlinkage int vprintk(const char *fmt, 
 		 * We own the drivers.  We can drop the spinlock and let
 		 * release_console_sem() print the text
 		 */
+		printk_cpu = UINT_MAX;
 		spin_unlock_irqrestore(&logbuf_lock, flags);
 		console_may_schedule = 0;
 		release_console_sem();
@@ -613,9 +622,11 @@ asmlinkage int vprintk(const char *fmt, 
 		 * allows the semaphore holder to proceed and to call the
 		 * console drivers with the output which we just produced.
 		 */
+		printk_cpu = UINT_MAX;
 		spin_unlock_irqrestore(&logbuf_lock, flags);
 	}
 out:
+	preempt_enable();
 	return printed_len;
 }
 EXPORT_SYMBOL(printk);

Re: [PATCH] Provide better printk() support for SMP machines [try #2]

[Lars Marowsky-Bree]

On 2005-07-08T13:36:12, David Howells <dhowells@redhat.com> wrote:

> The attached patch prevents oopses interleaving with characters from other
> printks on other CPUs by only breaking the lock if the oops is happening on
> the machine holding the lock.
> 
> It might be better if the oops generator got the lock and then called an inner
> vprintk routine that assumed the caller holds the lock, thus making oops
> reports "atomic".
> 
> Signed-Off-By: David Howells <dhowells@redhat.com>

After some discussion on IRC (me asking dumb questions) and reviewing
the code I'm in infinite favour of this patch. It clearly is a step in a
mucho desireable direction.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business	 -- Charles Darwin
"Ignorance more frequently begets confidence than does knowledge"