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 <manf...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
