Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751896AbcCGBct (ORCPT ); Sun, 6 Mar 2016 20:32:49 -0500 Received: from mail-pa0-f43.google.com ([209.85.220.43]:34211 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751236AbcCGBck (ORCPT ); Sun, 6 Mar 2016 20:32:40 -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 v3] futex: replace bare barrier() with more lightweight READ_ONCE() Date: Mon, 7 Mar 2016 09:32:24 +0800 Message-Id: <1457314344-5685-1-git-send-email-nasa4836@gmail.com> X-Mailer: git-send-email 2.4.3 In-Reply-To: <20160304210524.GF1092@dvhart-mobl5.amr.corp.intel.com> References: <20160304210524.GF1092@dvhart-mobl5.amr.corp.intel.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3461 Lines: 109 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. Many thanks to Darren Hart for reviewing and suggestion. Acked-by: Christian Borntraeger Signed-off-by: Jianyu Zhan --- kernel/futex.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index 5d6ce64..25dbfed 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -1927,8 +1927,12 @@ 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(); + /* + * q->lock_ptr can change between this read and the following spin_lock. + * Use READ_ONCE to forbid the compiler from reloading q->lock_ptr and + * optimizing lock_ptr out of the logic below. + */ + lock_ptr = READ_ONCE(q->lock_ptr); if (lock_ptr != NULL) { spin_lock(lock_ptr); /* -- 2.4.3