Date: Thu, 3 Mar 2016 09:05:32 -0800
From: Darren Hart <dvhart@infradead.org>
To: Jianyu Zhan <nasa4836@gmail.com>
Cc: linux-kernel@vger.kernel.org, peterz@infradead.org, tglx@linutronix.de,
        dave@stgolabs.net, akpm@linux-foundation.org, mingo@kernel.org,
        linux@rasmusvillemoes.dk, dvhart@linux.intel.com,
        borntraeger@de.ibm.com, fengguang.wu@intel.com, bigeasy@linutronix.de
Subject: Re: [PATCH] futex: replace bare barrier() with more lightweight
 READ_ONCE()
Message-ID: <20160303170532.GC1092@dvhart-mobl5.amr.corp.intel.com>
References: <1457019485-26441-1-git-send-email-nasa4836@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1457019485-26441-1-git-send-email-nasa4836@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4351
Lines: 126

On Thu, Mar 03, 2016 at 11:38:05PM +0800, Jianyu Zhan wrote:
> Commit e91467ecd1ef ("bug in futex unqueue_me") introduces a barrier()
> in unqueue_me(), to address below problem.
> 
> The scenario is like this:
> 
> ====================
> original code:
> 
> retry:
>        lock_ptr = q->lock_ptr;
>        if (lock_ptr != 0)  {
>                   spin_lock(lock_ptr)
>                   if (unlikely(lock_ptr != q->lock_ptr)) {
>                         spin_unlock(lock_ptr);
>                          goto retry;
>                   }
>                    ...
>        }
> 
> ====================
> It was observed that compiler generates code that is equivalent to:
> 
> retry:
>        if (q->lock_ptr != 0)  {
>                   spin_lock(q->lock_ptr)
>                   if (unlikely(lock_ptr != q->lock_ptr)) {
>                         spin_unlock(lock_ptr);
>                          goto retry;
>                   }
>                    ...
>        }
> 
> since q->lock_ptr might change between the test of non-nullness and spin_lock(),
> the double load will cause trouble. So that commit uses a barrier() to prevent this.
> 
> This patch replaces this bare barrier() with a READ_ONCE().
> 
> The reasons are:
> 
> 1) READ_ONCE() is a more weak form of barrier() that affect only the specific
>    accesses, while barrier() is a more general compiler level memroy barrier.
>    READ_ONCE() was not available at that time when that patch was written.
> 
> 2) READ_ONCE() which could be more informative by its name, while a bare barrier()
>    without comment leads to quite a bit of perplexity.
> 
> Assembly code before(barrier version) and after this patch(READ_ONCE version) are the same:
> 
> ====================
> Before(barrier version):
> 
> unqueue_me():
> linux/kernel/futex.c:1930
>     1df6:       4c 8b bd 28 ff ff ff    mov    -0xd8(%rbp),%r15
> linux/kernel/futex.c:1932
>     1dfd:       4d 85 ff                test   %r15,%r15
>     1e00:       0f 84 5c 01 00 00       je     1f62 <futex_wait+0x292>
> spin_lock():
> linux/include/linux/spinlock.h:302
>     1e06:       4c 89 ff                mov    %r15,%rdi
>     1e09:       e8 00 00 00 00          callq  1e0e <futex_wait+0x13e>
> 
> ====================
> After(READ_ONCE version):
> 
> __read_once_size():
> linux/include/linux/compiler.h:218
>     1df6:       4c 8b bd 28 ff ff ff    mov    -0xd8(%rbp),%r15
> unqueue_me():
> linux/kernel/futex.c:1935
>     1dfd:       4d 85 ff                test   %r15,%r15
>     1e00:       0f 84 5c 01 00 00       je     1f62 <futex_wait+0x292>
> spin_lock():
> linux/include/linux/spinlock.h:302
>     1e06:       4c 89 ff                mov    %r15,%rdi
>     1e09:       e8 00 00 00 00          callq  1e0e <futex_wait+0x13e>
> 
> Code size is also the same.
> 
> Suggested-by: Darren Hart <dvhart@infradead.org>

A simple suggested-by may cause people to think this patch was my idea, which
would not be accurate. You can credit people for recommendations in the patch
changelog ("Since v1: ..." below the --- line).

> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
> ---
>  kernel/futex.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 5d6ce64..58c1bcc 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1927,8 +1927,11 @@ static int unqueue_me(struct futex_q *q)
>  
>  	/* In the common case we don't take the spinlock, which is nice. */
>  retry:
> -	lock_ptr = q->lock_ptr;
> -	barrier();
> +	 /*
> +	  * Prevent the compiler to read q->lock_ptr twice (if and spin_lock),
> +	  * or that would cause trouble since q->lock_ptr can change in between.
> +	  */

I thought I provided a corrected comment block.... maybe I didn't. We have been
working on improving the futex documentation, so we're paying close attention to
terminology as well as grammar. This one needs a couple minor tweaks. I suggest:

/*
 * Use READ_ONCE to forbid the compiler from reloading q->lock_ptr and
 * optimizing lock_ptr out of the logic below.
 */

The bit about q->lock_ptr possibly changing is already covered by the large
comment block below the spin_lock(lock_ptr) call.

With the above change, I'll add my Reviewed-by.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center