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

[Steven Rostedt]

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?

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);

--
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/