Messages in this thread | | | From | Pranith Kumar <> | Date | Thu, 13 Nov 2014 23:02:07 -0500 | Subject | Re: [RFC PATCH] printk: Use ACCESS_ONCE() instead of a volatile type |
| |
On Thu, Nov 13, 2014 at 10:47 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > On Thu, 13 Nov 2014 22:21:21 -0500 > Pranith Kumar <bobby.prani@gmail.com> 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@goodmis.org> > > -- Steve > > >> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> --- >> 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
| |