Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753014AbbBYTg7 (ORCPT ); Wed, 25 Feb 2015 14:36:59 -0500 Received: from mail-wi0-f176.google.com ([209.85.212.176]:63954 "EHLO mail-wi0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752728AbbBYTg6 (ORCPT ); Wed, 25 Feb 2015 14:36:58 -0500 From: Manfred Spraul To: Oleg Nesterov , "Paul E. McKenney" Cc: LKML , 1vier1@web.de, Peter Zijlstra , Kirill Tkhai , Ingo Molnar , Josh Poimboeuf , Manfred Spraul Subject: [RFC PATCH] ipc/sem.c: Add one more memory barrier to sem_lock(). Date: Wed, 25 Feb 2015 20:36:49 +0100 Message-Id: <1424893009-27191-1-git-send-email-manfred@colorfullife.com> X-Mailer: git-send-email 2.1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2746 Lines: 98 Hi, What do you think about the following patch for sem_lock()? Other options: 1) I don't like #define smp_mb__after_unlock_wait() smp_rmb() I think it is too specific: the last block in sem_lock uses if (sma->complex_count == 0) { smp_rmb(); return; } 2) What about #define smp_aquire__after_control_barrier() smp_rmb() Best regards, Manfred xxxxx sem_lock() does not properly pair memory barriers. Theoretially an acquire barrier would the right operation. But since the existing control boundary is a write memory barrier, it is cheaper use an smp_rmb(). Signed-off-by: Manfred Spraul --- ipc/sem.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/ipc/sem.c b/ipc/sem.c index 9284211..d43011d 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -267,6 +267,10 @@ static void sem_wait_array(struct sem_array *sma) if (sma->complex_count) { /* The thread that increased sma->complex_count waited on * all sem->lock locks. Thus we don't need to wait again. + * + * The is no need for memory barriers: with + * complex_count>0, all threads acquire/release + * sem_perm.lock, thus spin_lock/unlock is the barrier. */ return; } @@ -275,6 +279,20 @@ static void sem_wait_array(struct sem_array *sma) sem = sma->sem_base + i; spin_unlock_wait(&sem->lock); } + /* + * We own sem_perm.lock, all owners of sma->sem_base[i].lock have + * dropped their locks. But we still need a memory barrier: + * - The lock dropping thread did a spin_unlock(), which is the + * release memory barrier. + * - But the spin_unlock(&sma->sem_base[i].lock) might have + * happened after this thread did spin_lock(&sma->sem_perm.lock), + * thus the acquire memory barrier in this thread is missing. + * - spin_unlock_wait() is internally a loop, thus we have a control + * boundary. As writes are not speculated, we have already a barrier + * for writes. Reads can be performed speculatively, therefore a + * smp_rmb() is necessary. + */ + smp_rmb(); } /* @@ -341,7 +359,13 @@ static inline int sem_lock(struct sem_array *sma, struct sembuf *sops, * Thus: if is now 0, then it will stay 0. */ if (sma->complex_count == 0) { - /* fast path successful! */ + /* + * Fast path successful! + * We only need a final memory barrier. + * (see sem_wait_array() for details). + */ + smp_rmb(); + return sops->sem_num; } } -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/