Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757452AbcCCPiO (ORCPT ); Thu, 3 Mar 2016 10:38:14 -0500 Received: from mail-pf0-f173.google.com ([209.85.192.173]:33060 "EHLO mail-pf0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753213AbcCCPiM (ORCPT ); Thu, 3 Mar 2016 10:38:12 -0500 From: Jianyu Zhan To: 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 Cc: nasa4836@gmail.com Subject: [PATCH] futex: replace bare barrier() with more lightweight READ_ONCE() Date: Thu, 3 Mar 2016 23:38:05 +0800 Message-Id: <1457019485-26441-1-git-send-email-nasa4836@gmail.com> X-Mailer: git-send-email 2.4.3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3380 Lines: 106 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 spin_lock(): linux/include/linux/spinlock.h:302 1e06: 4c 89 ff mov %r15,%rdi 1e09: e8 00 00 00 00 callq 1e0e ==================== 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 spin_lock(): linux/include/linux/spinlock.h:302 1e06: 4c 89 ff mov %r15,%rdi 1e09: e8 00 00 00 00 callq 1e0e Code size is also the same. Suggested-by: Darren Hart Acked-by: Christian Borntraeger Signed-off-by: Jianyu Zhan --- 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. + */ + lock_ptr = READ_ONCE(q->lock_ptr); if (lock_ptr != NULL) { spin_lock(lock_ptr); /* -- 2.4.3