Re: [PATCH] ipc/sem.c: [RFC] memory barrier in sem_lock()

[Davidlohr Bueso]

On Tue, 2014-08-12 at 21:43 +0200, Manfred Spraul wrote:
> sem_lock right now contains an smp_mb().
> I think smp_rmb() would be sufficient - and performance of semop() with rmb()
> is up to 10% faster. It would be a pairing of rmb() with spin_unlock().
> 
> The race we must protect against is:
> 
> sem->lock is free
> sma->complex_count = 0
> sma->sem_perm.lock held by thread B
> 
> thread A:
> 
> A: spin_lock(&sem->lock)
> 
>   B: sma->complex_count++; (now 1)
>   B: spin_unlock(&sma->sem_perm.lock);
> 
> A: spin_is_locked(&sma->sem_perm.lock);
> A: X which memory barrier is necessary?
> A: if (sma->complex_count == 0)
> 
> Thread A must read the increased complex_count value, i.e. the read must
> not be reordered with the read of sem_perm.lock done by spin_is_locked().
> 
> But that's it, there are no writes that must be ordered.

Looks right. You might also replace the current (useless) comment with
the patch's changelog, describing why the barrier is there.

Thanks,
Davidlohr

--
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/

[PATCH] ipc/sem.c: [RFC] memory barrier in sem_lock()

[Manfred Spraul]

sem_lock right now contains an smp_mb().
I think smp_rmb() would be sufficient - and performance of semop() with rmb()
is up to 10% faster. It would be a pairing of rmb() with spin_unlock().

The race we must protect against is:

sem->lock is free
sma->complex_count = 0
sma->sem_perm.lock held by thread B

thread A:

A: spin_lock(&sem->lock)

B: sma->complex_count++; (now 1)
B: spin_unlock(&sma->sem_perm.lock);

A: spin_is_locked(&sma->sem_perm.lock);
A: X which memory barrier is necessary?
A: if (sma->complex_count == 0)

Thread A must read the increased complex_count value, i.e. the read must
not be reordered with the read of sem_perm.lock done by spin_is_locked().

But that's it, there are no writes that must be ordered.

---
 ipc/sem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 454f6c6..a5c8a77 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -327,7 +327,7 @@ static inline int sem_lock(struct sem_array *sma, struct 
sembuf *sops,
/* Then check that the global lock is free */
if (!spin_is_locked(&sma->sem_perm.lock)) {
/* spin_is_locked() is not a memory barrier */
-   smp_mb();
+   smp_rmb();
 
/* Now repeat the test of complex_count:
 * It can't change anymore until we drop sem->lock.
-- 
1.9.3

--
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/