Messages in this thread Patch in this message | | | From | 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 |
| |
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 <manfred@colorfullife.com> --- 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
| |