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

[Pranith Kumar]

On Thu, Nov 13, 2014 at 10:47 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Thu, 13 Nov 2014 22:21:21 -0500
> Pranith Kumar <bobby.prani@xxxxxxxxx> wrote:
>
>> Remove volatile type qualifier and use ACCESS_ONCE() in its place for each
>> access. Using volatile is not recommended as documented in
>> Documentation/volatile-considered-harmful.txt.
>>
>> Here logbuf_cpu is a local variable and it is not clear how it is being accessed
>> concurrently. We should remove volatile accesses entirely here, but for now make
>> a safer change of using ACCESS_ONCE().
>
> I'm a little confused by the above paragraph about it's not clear how
> it is being accessed concurrently. Do you mean the code is unclear, or
> your understanding of it is unclear?

It is definitely got to do with my understanding. recursion explains
how it can be concurrently accessed. Thanks!

>
> Regardless of your answer, this patch is correct. logbuf_cpu is used to
> determine if printk has recursed on itself before it takes the
> logbuf_lock and deadlocks. Your ACCESS_ONCE keeps the compiler from
> optimizing out the logbuf_cpu as it will see that this function is the
> only one that can touch it and may try to do weird things to it.
>
> Reviewed-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
>
> -- Steve
>
>
>>
>> Signed-off-by: Pranith Kumar <bobby.prani@xxxxxxxxx>
>> ---
>>  kernel/printk/printk.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index e748971..4790191 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1624,7 +1624,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>>       int printed_len = 0;
>>       bool in_sched = false;
>>       /* cpu currently holding logbuf_lock in this function */
>> -     static volatile unsigned int logbuf_cpu = UINT_MAX;
>> +     static unsigned int logbuf_cpu = UINT_MAX;
>>
>>       if (level == LOGLEVEL_SCHED) {
>>               level = LOGLEVEL_DEFAULT;
>> @@ -1641,7 +1641,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>>       /*
>>        * Ouch, printk recursed into itself!
>>        */
>> -     if (unlikely(logbuf_cpu == this_cpu)) {
>> +     if (unlikely(ACCESS_ONCE(logbuf_cpu) == this_cpu)) {
>>               /*
>>                * If a crash is occurring during printk() on this CPU,
>>                * then try to get the crash message out but make sure
>> @@ -1659,7 +1659,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>>
>>       lockdep_off();
>>       raw_spin_lock(&logbuf_lock);
>> -     logbuf_cpu = this_cpu;
>> +     ACCESS_ONCE(logbuf_cpu) = this_cpu;
>>
>>       if (unlikely(recursion_bug)) {
>>               static const char recursion_msg[] =
>> @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>>                                                dict, dictlen, text, text_len);
>>       }
>>
>> -     logbuf_cpu = UINT_MAX;
>> +     ACCESS_ONCE(logbuf_cpu) = UINT_MAX;
>>       raw_spin_unlock(&logbuf_lock);
>>       lockdep_on();
>>       local_irq_restore(flags);
>



-- 
Pranith
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/