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

[Alex Elder <elder@linaro.org>]

On 11/13/2014 09:21 PM, Pranith Kumar 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().

Although logbuf_cpu is declared locally, it has static scope and
hence its value is persistent across calls to the function,
including concurrent calls on different CPUs.

This is a very interesting bit of code.  I have a
question, below.

					-Alex

> 
> 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 this is not volatile, can the compiler assume that it
can't change before the first access?  Put another way,
does this assignment need to be done more like this?

	static unsigned int ACCESS_ONCE(logbuf_cpu) = UINT_MAX;

(I haven't checked, but I don't believe that expands to valid code.)

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