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

[Pranith Kumar <bobby.prani@gmail.com>]

On 11/14/2014 11:39 AM, Alex Elder wrote:
> 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...)
>

Thanks Alex, for the in-depth analysis. Please drop my patch in favour of removing volatile and without ACCESS_ONCE(). Will you send in such a patch?