[PATCH] printk: Remove superfluous memory barriers from printk_safe

[PATCH] printk: Remove superfluous memory barriers from printk_safe

[Steven Rostedt]

From: Steven Rostedt (VMware) <rostedt@goodmis.org>

The variable printk_safe_irq_ready is set and never cleared at system
boot up, when there's only one CPU active. It is set before other
CPUs come on line. Also, it is extremely unlikely that an NMI would
trigger this early in boot up (which I wonder why we even have this
variable at all).

Also mark the printk_safe_irq_ready as read mostly, as it is set at
system boot up, and never touched again.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index 3cdaeae..724d929 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -39,7 +39,7 @@
  * There are situations when we want to make sure that all buffers
  * were handled or when IRQs are blocked.
  */
-static int printk_safe_irq_ready;
+static int printk_safe_irq_ready __read_mostly;
 
 #define SAFE_LOG_BUF_LEN ((1 << CONFIG_PRINTK_SAFE_LOG_BUF_SHIFT) -	\
 				sizeof(atomic_t) -			\
@@ -63,11 +63,8 @@ static DEFINE_PER_CPU(struct printk_safe_seq_buf, nmi_print_seq);
 /* Get flushed in a more safe context. */
 static void queue_flush_work(struct printk_safe_seq_buf *s)
 {
-	if (printk_safe_irq_ready) {
-		/* Make sure that IRQ work is really initialized. */
-		smp_rmb();
+	if (printk_safe_irq_ready)
 		irq_work_queue(&s->work);
-	}
 }
 
 /*
@@ -398,8 +395,12 @@ void __init printk_safe_init(void)
 #endif
 	}
 
-	/* Make sure that IRQ works are initialized before enabling. */
-	smp_wmb();
+	/*
+	 * In the highly unlikely event that a NMI were to trigger at
+	 * this moment. Make sure IRQ work is set up before this
+	 * variable is set.
+	 */
+	barrier();
 	printk_safe_irq_ready = 1;
 
 	/* Flush pending messages that did not have scheduled IRQ works. */

Re: [PATCH] printk: Remove superfluous memory barriers from printk_safe

[Petr Mladek]

On Wed 2017-10-11 12:46:47, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> The variable printk_safe_irq_ready is set and never cleared at system
> boot up, when there's only one CPU active. It is set before other
> CPUs come on line. Also, it is extremely unlikely that an NMI would
> trigger this early in boot up (which I wonder why we even have this
> variable at all).
> 
> Also mark the printk_safe_irq_ready as read mostly, as it is set at
> system boot up, and never touched again.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Great catch! It makes perfect sense:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH] printk: Remove superfluous memory barriers from printk_safe

[Sergey Senozhatsky]

On (10/11/17 12:46), Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> The variable printk_safe_irq_ready is set and never cleared at system
> boot up, when there's only one CPU active. It is set before other
> CPUs come on line. Also, it is extremely unlikely that an NMI would
> trigger this early in boot up (which I wonder why we even have this
> variable at all).

it's not only NMI related, printk() recursion can happen at any stages,
including... um... wait a second. ... including the "before we set up
per-CPU areas" stage? hmm... smells like a bug?

do we need to move per-CPU printk_safe buffers out of per-CPU and turn
it into a global static buffer? like logbuf, and just give every CPU a
starting offset of its printk_safe_logbuf part.

IOW,

char printk_safe_logbuf[number of cpus * sizeof printk safe buffer];

cpu0 offset 0, up to sizeof printk safe buffer
cpu1 offset sizeof printk safe buffer, up to 2 * sizeof printk safe buffer

etc.

or... at least. avoid stoing to per-CPU printk-safe/printk-nmi buffers
unless we've got per-CPU areas set up? or am I hallucinating?

	-ss

Re: [PATCH] printk: Remove superfluous memory barriers from printk_safe

[Steven Rostedt]

On Sat, 14 Oct 2017 18:21:29 +0900
Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> On (10/11/17 12:46), Steven Rostedt wrote:
> > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> > The variable printk_safe_irq_ready is set and never cleared at system
> > boot up, when there's only one CPU active. It is set before other
> > CPUs come on line. Also, it is extremely unlikely that an NMI would
> > trigger this early in boot up (which I wonder why we even have this
> > variable at all).  
> 
> it's not only NMI related, printk() recursion can happen at any stages,
> including... um... wait a second. ... including the "before we set up
> per-CPU areas" stage? hmm... smells like a bug?

I think this was just being overly paranoid.

> 
> do we need to move per-CPU printk_safe buffers out of per-CPU and turn
> it into a global static buffer? like logbuf, and just give every CPU a
> starting offset of its printk_safe_logbuf part.
> 
> IOW,
> 
> char printk_safe_logbuf[number of cpus * sizeof printk safe buffer];
> 
> cpu0 offset 0, up to sizeof printk safe buffer
> cpu1 offset sizeof printk safe buffer, up to 2 * sizeof printk safe buffer

Let's not make this any more complicated than it has to be.

> 
> etc.
> 
> or... at least. avoid stoing to per-CPU printk-safe/printk-nmi buffers
> unless we've got per-CPU areas set up? or am I hallucinating?

We can keep the flag as is, it's highly unlikely to be a problem.

-- Steve

Re: [PATCH] printk: Remove superfluous memory barriers from printk_safe

[Petr Mladek]

On Sun 2017-10-15 20:27:15, Steven Rostedt wrote:
> On Sat, 14 Oct 2017 18:21:29 +0900
> Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:
> 
> > On (10/11/17 12:46), Steven Rostedt wrote:
> > > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > 
> > > The variable printk_safe_irq_ready is set and never cleared at system
> > > boot up, when there's only one CPU active. It is set before other
> > > CPUs come on line. Also, it is extremely unlikely that an NMI would
> > > trigger this early in boot up (which I wonder why we even have this
> > > variable at all).  
> > 
> > it's not only NMI related, printk() recursion can happen at any stages,
> > including... um... wait a second. ... including the "before we set up
> > per-CPU areas" stage? hmm... smells like a bug?
> 
> I think this was just being overly paranoid.

I was curious because it was not only about reading the per-CPU
variables. We set and clear the printk_context per-CPU variable
in every printk() call. I wondered if we accessed some
non-initialized stuff.

Fortunately, it seems that we are on the safe side.

If I get it correctly, the per-CPU variables are set up in
setup_per_cpu_areas(). But some per-CPU variables are used even
before, see

  boot_cpu_init()
    smp_processor_id()
      raw_smp_processor_id()
	this_cpu_read(cpu_number)

IMHO, the trick is the following code in setup_per_cpu_areas()
from arch/x86/kernel/setup_percpu.c:

		/*
		 * Up to this point, the boot CPU has been using .init.data
		 * area.  Reload any changed state for the boot CPU.
		 */
		if (!cpu)
			switch_to_new_gdt(cpu);

IMHO, this means that per-CPU variable for the first boot-CPU
can be used at any time. And all the interesting functions:
boot_cpu_init(), setup_per_cpu_areas(), printk_safe_init() are
still called in the single-CPU mode.

Best Regards,
Petr

Re: [PATCH] printk: Remove superfluous memory barriers from printk_safe

[Sergey Senozhatsky]

On (10/16/17 10:12), Petr Mladek wrote:
[..]
> > > it's not only NMI related, printk() recursion can happen at any stages,
> > > including... um... wait a second. ... including the "before we set up
> > > per-CPU areas" stage? hmm... smells like a bug?
> > 
> > I think this was just being overly paranoid.
> 
> I was curious because it was not only about reading the per-CPU
> variables. We set and clear the printk_context per-CPU variable
> in every printk() call. I wondered if we accessed some
> non-initialized stuff.
> 
> Fortunately, it seems that we are on the safe side.
> 
> If I get it correctly, the per-CPU variables are set up in
> setup_per_cpu_areas(). But some per-CPU variables are used even
> before, see
> 
>   boot_cpu_init()
>     smp_processor_id()
>       raw_smp_processor_id()
> 	this_cpu_read(cpu_number)
> 
> IMHO, the trick is the following code in setup_per_cpu_areas()
> from arch/x86/kernel/setup_percpu.c:
> 
> 		/*
> 		 * Up to this point, the boot CPU has been using .init.data
> 		 * area.  Reload any changed state for the boot CPU.
> 		 */
> 		if (!cpu)
> 			switch_to_new_gdt(cpu);
> 
> IMHO, this means that per-CPU variable for the first boot-CPU
> can be used at any time. And all the interesting functions:
> boot_cpu_init(), setup_per_cpu_areas(), printk_safe_init() are
> still called in the single-CPU mode.

hm... not so sure, need to check more.

we can access DECLARE_EARLY_PER_CPU() things, which we must copy after
per-cpu pages are set up. `cpu_number' is DECLARE_EARLY_PER_CPU(), btw.

	-ss

Re: [PATCH] printk: Remove superfluous memory barriers from printk_safe

[Sergey Senozhatsky]

On (10/15/17 20:27), Steven Rostedt wrote:
> > On (10/11/17 12:46), Steven Rostedt wrote:
> > > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > 
> > > The variable printk_safe_irq_ready is set and never cleared at system
> > > boot up, when there's only one CPU active. It is set before other
> > > CPUs come on line. Also, it is extremely unlikely that an NMI would
> > > trigger this early in boot up (which I wonder why we even have this
> > > variable at all).  
> > 
> > it's not only NMI related, printk() recursion can happen at any stages,
> > including... um... wait a second. ... including the "before we set up
> > per-CPU areas" stage? hmm... smells like a bug?
> 
> I think this was just being overly paranoid.

hm, printk recursion is pretty easy to trigger. vscnprintf() can WARN_ON(),
for instance. just pass (mistakenly) unknown printk specifier, e.g.
%A, and this will do.

	-ss