Re: [RFC PATCH] printk: Use ACCESS_ONCE() instead of a volatile type

[Alex Elder <elder@linaro.org>]

On 11/13/2014 11:24 PM, Steven Rostedt wrote:
> On Thu, 13 Nov 2014 23:57:22 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>> That assignment is what it is initialized to at boot up. I can't see
>> any optimization that would cause gcc to modify that. Especially since
>> we are hiding its accesses within the ACCESS_ONCE(). That alone should
>> confuse gcc enough to leave it a hell alone J.
> 
> 
> I'm actually wondering if the ACCESS_ONCE or volatile is even needed.
> 
> static variables are used to maintain state, and that goes for
> recursive functions. gcc should not touch it.

I think you're right.

Here's some extra analysis.  I may be wrong on a detail or
two but see if it makes sense.

The logbuf_cpu variable has static storage duration, so will
be initialized before program startup.

This function (vprintk_emit()) can be called on multiple
CPUs concurrently.  So we can assume that there is more than
one thread executing in window from the start of the function
until the raw_spin_lock(&logbuf_lock) call is made.

The only writes to logbuf_lock are made under protection
of the spinlock.  It is initially UINT_MAX; it is changed
to the current processor id right after taking the lock;
and it is reverted to UINT_MAX right before releasing the
lock.  So logbuf_cpu will either contain UINT_MAX, or will
hold the processor id of the CPU that is holding logbuf_lock.
The spinlock barrier ensures that the only value a CPU will
see is UINT_MAX, unless it is the CPU that holds the spinlock.

There is only one read of logbuf_cpu:
        if (unlikely(logbuf_cpu == this_cpu)) {
This is called only while local interrupts are disabled, so
if this condition holds it cannot be due to an interrupt--it
must be due to simple recursion into printk() while inside
the spinlock-protected critical section.

We *can* recurse into printk() via a function call within
the protected section--through vscnprintf(), which can
descend into printk() via WARN() calls in format_decode().
(There may be others after that point, but up to there it
looks like no other function call in that section can fail.)
So it *is* possible to hit this recursion (I wanted to
verify that...).

OK.  So back to the original issue...  How do we ensure
the value of logbuf_cpu is in fact the last set value,
and is not affected by any compiler reordering?

If its value is anything other than UINT_MAX, it will
be the current CPU's processor id, which will have been
set by the current CPU.  There are no issues related to
caches or barriers.

Since vprintk_emit() is a public entry point there's no
magic inter-function optimization or inlining that could
allow the value of the static logbuf_cpu to be preserved
between calls.  So the first read of logbuf_cpu in a given
function call will have to fetch its current value from memory
(regardless of whether there's a "volatile" qualifier).

And therefore the one read of that value will involve
fetching the "real" value from memory, and it will
either be UINT_MAX or the CPU's own processor id.

So there should be no need to declare the variable
volatile, nor to access it with ACCESS_ONCE().

QED.  (Well, please correct me where I'm wrong...)

					-Alex

> Now perhaps it can see that there is no recursion for logbuf_cpu to be
> set to the current cpu (which would be interesting since the
> smp_processor_id() call is also hidden from gcc), and it might optimize
> it out. But that would not protect us from NMIs doing a printk().
> Although this code doesn't protect us from that anyway if an NMI were
> to come in right after taking the logbuf_lock and before setting
> logbuf_cpu. In that case, logbuf_cpu will not be set to this_cpu and a
> deadlock can still occur. This code only makes the race window smaller.
> 
> I'm thinking the correct change is to nuke all of it. Perhaps the only
> reason using volatile here was not a bug is because volatile wasn't
> needed in the first place!
> 
> -- Steve
>