On 09/02/2016 09:22 PM, Peter Zijlstra wrote:
On Fri, Sep 02, 2016 at 08:35:55AM +0200, Manfred Spraul wrote:
On 09/01/2016 06:41 PM, Peter Zijlstra wrote:
On Thu, Sep 01, 2016 at 04:30:39PM +0100, Will Deacon wrote:
On Thu, Sep 01, 2016 at 05:27:52PM +0200, Manfred Spraul wrote:
Since spin_unlock_wait() is defined as equivalent to spin_lock();
spin_unlock(), the memory barrier before spin_unlock_wait() is
also not required.
Note that ACQUIRE+RELEASE isn't a barrier.
Both are semi-permeable and things can cross in the middle, like:
x = 1;
LOCK
UNLOCK
r = y;
can (validly) get re-ordered like:
LOCK
r = y;
x = 1;
UNLOCK
So if you want things ordered, as I think you do, I think the smp_mb()
is still needed.
CPU1:
x=1; /* without WRITE_ONCE */
LOCK(l);
UNLOCK(l);
<do_semop>
smp_store_release(x,0)
CPU2;
LOCK(l)
if (smp_load_acquire(x)==1) goto slow_path
<do_semop>
UNLOCK(l)
Ordering is enforced because both CPUs access the same lock.
x=1 can't be reordered past the UNLOCK(l), I don't see that further
guarantees are necessary.
Correct?
Correct, sadly implementations do not comply :/ In fact, even x86 is
broken here.
I spoke to Will earlier today and he suggests either making
spin_unlock_wait() stronger to avoids any and all such surprises or just
getting rid of the thing.
I'm not sure which way we should go, but please hold off on these two
patches until I've had a chance to audit all of those implementations
again.
For me, it doesn't really matter.
spin_unlock_wait() as "R", as "RAcq" or as "spin_lock(); spin_lock();" -
I just want a usable definition for ipc/sem.c
So (just to keep Andrew updated):
Ready for merging is: (Bugfixes, safe just with spin_unlock_wait() as "R"):
- 45a449340cd1 ("ipc/sem.c: fix complex_count vs. simple op race")
Cc stable, back to 3.10 ...
- 7fd5653d9986 ("net/netfilter/nf_conntrack_core: Fix memory barriers.")
Cc stable, back to ~4.5
--
Manfred
