arm64: spinlock: serialise spin_unlock_wait against concurrent lockers

Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics:

 | CPU 1                   CPU 2                     CPU 3
 | ==================      ====================      ==============
 |                                                   spin_unlock(&lock);
 |                         spin_lock(&lock):
 |                           r1 = *lock; // r1 == 0;
 |                         o = READ_ONCE(object); // reordered here
 | object = NULL;
 | smp_mb();
 | spin_unlock_wait(&lock);
 |                           *lock = 1;
 | smp_mb();
 | o->dead = true;
 |                         if (o) // true
 |                           BUG_ON(o->dead); // true!!

The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro).

Quoted 19 lines · Will Deacon

Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics:

 | CPU 1                   CPU 2                     CPU 3
 | ==================      ====================      ==============
 |                                                   spin_unlock(&lock);
 |                         spin_lock(&lock):
 |                           r1 = *lock; // r1 == 0;
 |                         o = READ_ONCE(object); // reordered here
 | object = NULL;
 | smp_mb();
 | spin_unlock_wait(&lock);
 |                           *lock = 1;
 | smp_mb();
 | o->dead = true;
 |                         if (o) // true
 |                           BUG_ON(o->dead); // true!!

Quoted 8 lines

The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro).

Do we want to go do a note with spin_unlock_wait() in include/linux/spinlock.h warning about these subtle issues for the next arch that thinks this is a 'trivial' thing to implement?

Quoted 19 lines · Will Deacon

Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics:

 | CPU 1                   CPU 2                     CPU 3
 | ==================      ====================      ==============
 |                                                   spin_unlock(&lock);
 |                         spin_lock(&lock):
 |                           r1 = *lock; // r1 == 0;
 |                         o = READ_ONCE(object); // reordered here
 | object = NULL;
 | smp_mb();
 | spin_unlock_wait(&lock);
 |                           *lock = 1;
 | smp_mb();
 | o->dead = true;
 |                         if (o) // true
 |                           BUG_ON(o->dead); // true!!

Quoted 8 lines

The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro).

Do we want to go do a note with spin_unlock_wait() in include/linux/spinlock.h warning about these subtle issues for the next arch that thinks this is a 'trivial' thing to implement?

Quoted 29 lines · Peter Zijlstra

Quoted 19 lines · Will Deacon

Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics:

 | CPU 1                   CPU 2                     CPU 3
 | ==================      ====================      ==============
 |                                                   spin_unlock(&lock);
 |                         spin_lock(&lock):
 |                           r1 = *lock; // r1 == 0;
 |                         o = READ_ONCE(object); // reordered here
 | object = NULL;
 | smp_mb();
 | spin_unlock_wait(&lock);
 |                           *lock = 1;
 | smp_mb();
 | o->dead = true;
 |                         if (o) // true
 |                           BUG_ON(o->dead); // true!!

Quoted 8 lines

The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro).

Quoted 3 lines

Do we want to go do a note with spin_unlock_wait() in include/linux/spinlock.h warning about these subtle issues for the next arch that thinks this is a 'trivial' thing to implement?

Could do, but I still need agreement from Paul on the solution before I can describe it in core code. At the moment, the semantics are, unfortunately, arch-specific.

Paul -- did you have any more thoughts about this? I ended up at:

https://lkml.org/lkml/2015/11/16/343

and then ran out of ideas.

This looks architecture-agnostic to me:

a. TSO systems have smp_mb__after_unlock_lock() be a no-op, and
have a read-only implementation for spin_unlock_wait().

b. Small-scale weakly ordered systems can also have
smp_mb__after_unlock_lock() be a no-op, but must instead have spin_unlock_wait() acquire the lock and immediately release it, or some optimized implementation of this.

c. Large-scale weakly ordered systems are required to define
smp_mb__after_unlock_lock() as smp_mb(), but can have a read-only implementation of spin_unlock_wait().

Quoted 5 lines · Paul E. McKenney

This looks architecture-agnostic to me:

a. TSO systems have smp_mb__after_unlock_lock() be a no-op, and
have a read-only implementation for spin_unlock_wait().

Quoted 8 lines

b. Small-scale weakly ordered systems can also have
smp_mb__after_unlock_lock() be a no-op, but must instead have spin_unlock_wait() acquire the lock and immediately release it, or some optimized implementation of this.

c. Large-scale weakly ordered systems are required to define
smp_mb__after_unlock_lock() as smp_mb(), but can have a read-only implementation of spin_unlock_wait().

This would still require all relevant spin_lock() sites to be annotated with smp_mb__after_unlock_lock(), which is going to be a painful (no warning when done wrong) exercise and expensive (added MBs all over the place).

But yes, I think the proposal is technically sound, just not quite sure how far we'll want to push this.

Hi Peter, Paul,

Firstly, thanks for writing that up. I agree that you have something that can work in theory, but see below.

Quoted 19 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

This looks architecture-agnostic to me:

a. TSO systems have smp_mb__after_unlock_lock() be a no-op, and
have a read-only implementation for spin_unlock_wait().

Quoted 8 lines

b. Small-scale weakly ordered systems can also have
smp_mb__after_unlock_lock() be a no-op, but must instead have spin_unlock_wait() acquire the lock and immediately release it, or some optimized implementation of this.

c. Large-scale weakly ordered systems are required to define
smp_mb__after_unlock_lock() as smp_mb(), but can have a read-only implementation of spin_unlock_wait().

This would still require all relevant spin_lock() sites to be annotated with smp_mb__after_unlock_lock(), which is going to be a painful (no warning when done wrong) exercise and expensive (added MBs all over the place).

Quoted 2 lines

But yes, I think the proposal is technically sound, just not quite sure how far we'll want to push this.

When I said that the solution isn't architecture-agnostic, I was referring to the fact that spin_unlock_wait acts as a LOCK operation on all architectures other than those using case (c) above. You've resolved this by requiring smp_mb__after_unlock_lock() after every LOCK, but I share Peter's concerns that this isn't going to work in practice because:

1. The smp_mb__after_unlock_lock additions aren't necessarily in the code where the spin_unlock_wait() is being added, so it's going to require a lot of diligence for developers to get this right

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

3. We've now made the kernel memory model even more difficult to understand, so people might not even bother with this addition

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Quoted 4 lines

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

To be fair, those were in debug code and non critical for correctness per se. That is, at worst the debug print would've observed an incorrect value.

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

Quoted 3 lines

To be fair, those were in debug code and non critical for correctness per se. That is, at worst the debug print would've observed an incorrect value.

True enough, but there is still risk from people repurposing debug code for non-debug uses. Still, thank you, I don't feel -quite- so bad about rcutorture's failure to find these. ;-)

Quoted 14 lines · Paul E. McKenney

Quoted 12 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

To be fair, those were in debug code and non critical for correctness per se. That is, at worst the debug print would've observed an incorrect value.

Quoted 3 lines

True enough, but there is still risk from people repurposing debug code for non-debug uses. Still, thank you, I don't feel -quite- so bad about rcutorture's failure to find these. ;-)

It's the ones that it's yet to find that you should be worried about, and the debug code is all fixed ;)

Quoted 15 lines · Will Deacon

Quoted 14 lines · Paul E. McKenney

Quoted 12 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

To be fair, those were in debug code and non critical for correctness per se. That is, at worst the debug print would've observed an incorrect value.

Quoted 6 lines

Quoted 3 lines

True enough, but there is still risk from people repurposing debug code for non-debug uses. Still, thank you, I don't feel -quite- so bad about rcutorture's failure to find these. ;-)

It's the ones that it's yet to find that you should be worried about, and the debug code is all fixed ;)

Fortunately, when Peter sent the patch fixing the debug-only cases, he also created wrapper functions for the various types of lock acquisition for rnp->lock. Of course, the danger is that I might type "raw_spin_lock_irqsave(&rnp->lock, flags)" instead of "raw_spin_lock_irqsave_rcu_node(rnp, flags)" out of force of habit. So I must occasionally scan the RCU source code for "spin_lock.*->lock", which I just now did. ;-)

Thanx, Paul

Hi Paul,

Quoted 23 lines · Paul E. McKenney

Quoted 15 lines · Will Deacon

Quoted 14 lines · Paul E. McKenney

Quoted 12 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

To be fair, those were in debug code and non critical for correctness per se. That is, at worst the debug print would've observed an incorrect value.

Quoted 6 lines

Quoted 3 lines

True enough, but there is still risk from people repurposing debug code for non-debug uses. Still, thank you, I don't feel -quite- so bad about rcutorture's failure to find these. ;-)

It's the ones that it's yet to find that you should be worried about, and the debug code is all fixed ;)

Quoted 7 lines

Fortunately, when Peter sent the patch fixing the debug-only cases, he also created wrapper functions for the various types of lock acquisition for rnp->lock. Of course, the danger is that I might type "raw_spin_lock_irqsave(&rnp->lock, flags)" instead of "raw_spin_lock_irqsave_rcu_node(rnp, flags)" out of force of habit. So I must occasionally scan the RCU source code for "spin_lock.*->lock", which I just now did. ;-)

Maybe you can rename ->lock of rnp to ->lock_acquired_on_your_own_risk to avoid the force of habit ;-)

Quoted 34 lines · Boqun Feng

Hi Paul,

Quoted 30 lines · Paul E. McKenney

Quoted 15 lines · Will Deacon

Quoted 14 lines · Paul E. McKenney

Quoted 12 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

To be fair, those were in debug code and non critical for correctness per se. That is, at worst the debug print would've observed an incorrect value.

Quoted 6 lines

Quoted 3 lines

True enough, but there is still risk from people repurposing debug code for non-debug uses. Still, thank you, I don't feel -quite- so bad about rcutorture's failure to find these. ;-)

It's the ones that it's yet to find that you should be worried about, and the debug code is all fixed ;)

Fortunately, when Peter sent the patch fixing the debug-only cases, he also created wrapper functions for the various types of lock acquisition for rnp->lock. Of course, the danger is that I might type "raw_spin_lock_irqsave(&rnp->lock, flags)" instead of "raw_spin_lock_irqsave_rcu_node(rnp, flags)" out of force of habit. So I must occasionally scan the RCU source code for "spin_lock.*->lock", which I just now did. ;-)

Quoted 2 lines

Maybe you can rename ->lock of rnp to ->lock_acquired_on_your_own_risk to avoid the force of habit ;-)

Sold! Though with a shorter alternate name... And timing will be an issue. Probably needs to go into the first post-v4.5 set (due to the high expected conflict rate), and probably needs to create wrappers for the spin_unlock functions.

Thanx, Paul

Quoted 35 lines · Paul E. McKenney

Quoted 34 lines · Boqun Feng

Hi Paul,

Quoted 30 lines · Paul E. McKenney

Quoted 15 lines · Will Deacon

Quoted 14 lines · Paul E. McKenney

Quoted 12 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

To be fair, those were in debug code and non critical for correctness per se. That is, at worst the debug print would've observed an incorrect value.

Quoted 6 lines

Quoted 3 lines

True enough, but there is still risk from people repurposing debug code for non-debug uses. Still, thank you, I don't feel -quite- so bad about rcutorture's failure to find these. ;-)

It's the ones that it's yet to find that you should be worried about, and the debug code is all fixed ;)

Fortunately, when Peter sent the patch fixing the debug-only cases, he also created wrapper functions for the various types of lock acquisition for rnp->lock. Of course, the danger is that I might type "raw_spin_lock_irqsave(&rnp->lock, flags)" instead of "raw_spin_lock_irqsave_rcu_node(rnp, flags)" out of force of habit. So I must occasionally scan the RCU source code for "spin_lock.*->lock", which I just now did. ;-)

Quoted 7 lines

Quoted 2 lines

Maybe you can rename ->lock of rnp to ->lock_acquired_on_your_own_risk to avoid the force of habit ;-)

Sold! Though with a shorter alternate name... And timing will be an issue. Probably needs to go into the first post-v4.5 set (due to the high expected conflict rate), and probably needs to create wrappers for the spin_unlock functions.

Or maybe, we introduce another address space of sparse like:

	# define __private	__attribute__((noderef, address_space(6)))

and macro to dereference private

	# define private_dereference(p) ((typeof(*p) *) p)

and define struct rcu_node like:

	struct rcu_node {
		raw_spinlock_t __private lock;	/* Root rcu_node's lock protects some */
		...
	};

and finally raw_spin_{un}lock_rcu_node() like:

	static inline void raw_spin_lock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_lock(private_dereference(&rnp->lock));
		smp_mb__after_unlock_lock();
	}

	static inline void raw_spin_unlock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_unlock(private_dereference(&rnp->lock));
	}

This __private mechanism also works for others who wants to private their fields of struct, which is not supported by C.

I will send two patches(one introduces __private and one uses it for rcu_node->lock) if you think this is not a bad idea ;-)

Quoted 72 lines · Boqun Feng

Quoted 35 lines · Paul E. McKenney

Quoted 34 lines · Boqun Feng

Hi Paul,

Quoted 30 lines · Paul E. McKenney

Quoted 15 lines · Will Deacon

Quoted 14 lines · Paul E. McKenney

Quoted 12 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Quoted 2 lines

2. Only PowerPC is going to see the (very occassional) failures, so

     testing this is nigh on impossible :(

Indeed, we clearly cannot rely on normal testing, witness rcutorture failing to find the missing smp_mb__after_unlock_lock() instances that Peter found by inspection. So I believe that augmented testing is required, perhaps as suggested above.

To be fair, those were in debug code and non critical for correctness per se. That is, at worst the debug print would've observed an incorrect value.

Quoted 6 lines

Quoted 3 lines

True enough, but there is still risk from people repurposing debug code for non-debug uses. Still, thank you, I don't feel -quite- so bad about rcutorture's failure to find these. ;-)

It's the ones that it's yet to find that you should be worried about, and the debug code is all fixed ;)

Fortunately, when Peter sent the patch fixing the debug-only cases, he also created wrapper functions for the various types of lock acquisition for rnp->lock. Of course, the danger is that I might type "raw_spin_lock_irqsave(&rnp->lock, flags)" instead of "raw_spin_lock_irqsave_rcu_node(rnp, flags)" out of force of habit. So I must occasionally scan the RCU source code for "spin_lock.*->lock", which I just now did. ;-)

Quoted 7 lines

Quoted 2 lines

Maybe you can rename ->lock of rnp to ->lock_acquired_on_your_own_risk to avoid the force of habit ;-)

Sold! Though with a shorter alternate name... And timing will be an issue. Probably needs to go into the first post-v4.5 set (due to the high expected conflict rate), and probably needs to create wrappers for the spin_unlock functions.

Or maybe, we introduce another address space of sparse like:

	# define __private	__attribute__((noderef, address_space(6)))

and macro to dereference private

	# define private_dereference(p) ((typeof(*p) *) p)

and define struct rcu_node like:

	struct rcu_node {
		raw_spinlock_t __private lock;	/* Root rcu_node's lock protects some */
		...
	};

and finally raw_spin_{un}lock_rcu_node() like:

	static inline void raw_spin_lock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_lock(private_dereference(&rnp->lock));
		smp_mb__after_unlock_lock();
	}

	static inline void raw_spin_unlock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_unlock(private_dereference(&rnp->lock));
	}

Quoted 5 lines

This __private mechanism also works for others who wants to private their fields of struct, which is not supported by C.

I will send two patches(one introduces __private and one uses it for rcu_node->lock) if you think this is not a bad idea ;-)

This approach reminds me of an old saying from my childhood: "Attacking a flea with a sledgehammer". ;-)

Thanx, Paul

Quoted 31 lines

Quoted 31 lines

Or maybe, we introduce another address space of sparse like:

	# define __private	__attribute__((noderef, address_space(6)))

and macro to dereference private

	# define private_dereference(p) ((typeof(*p) *) p)

and define struct rcu_node like:

	struct rcu_node {
		raw_spinlock_t __private lock;	/* Root rcu_node's lock protects some */
		...
	};

and finally raw_spin_{un}lock_rcu_node() like:

	static inline void raw_spin_lock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_lock(private_dereference(&rnp->lock));
		smp_mb__after_unlock_lock();
	}

	static inline void raw_spin_unlock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_unlock(private_dereference(&rnp->lock));
	}

This __private mechanism also works for others who wants to private their fields of struct, which is not supported by C.

Quoted 2 lines

Quoted 2 lines

I will send two patches(one introduces __private and one uses it for rcu_node->lock) if you think this is not a bad idea ;-)

If rcu_node->lock is the only user then this is probably a bad idea, but if others also want to have a way to privatize some fields of the structure, this may be not that bad?

Quoted 33 lines · Boqun Feng

Quoted 33 lines

Quoted 33 lines

Or maybe, we introduce another address space of sparse like:

	# define __private	__attribute__((noderef, address_space(6)))

and macro to dereference private

	# define private_dereference(p) ((typeof(*p) *) p)

and define struct rcu_node like:

	struct rcu_node {
		raw_spinlock_t __private lock;	/* Root rcu_node's lock protects some */
		...
	};

and finally raw_spin_{un}lock_rcu_node() like:

	static inline void raw_spin_lock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_lock(private_dereference(&rnp->lock));
		smp_mb__after_unlock_lock();
	}

	static inline void raw_spin_unlock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_unlock(private_dereference(&rnp->lock));
	}

This __private mechanism also works for others who wants to private their fields of struct, which is not supported by C.

I will send two patches(one introduces __private and one uses it for rcu_node->lock) if you think this is not a bad idea ;-)

Quoted 3 lines

If rcu_node->lock is the only user then this is probably a bad idea, but if others also want to have a way to privatize some fields of the structure, this may be not that bad?

Thomas might also want this for things like irq_common_data::state_use_accessors for instance.

And I'm fairly sure there's more out there.

Quoted 35 lines · Peter Zijlstra

Quoted 33 lines · Boqun Feng

Quoted 33 lines

Quoted 33 lines

Or maybe, we introduce another address space of sparse like:

	# define __private	__attribute__((noderef, address_space(6)))

and macro to dereference private

	# define private_dereference(p) ((typeof(*p) *) p)

and define struct rcu_node like:

	struct rcu_node {
		raw_spinlock_t __private lock;	/* Root rcu_node's lock protects some */
		...
	};

and finally raw_spin_{un}lock_rcu_node() like:

	static inline void raw_spin_lock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_lock(private_dereference(&rnp->lock));
		smp_mb__after_unlock_lock();
	}

	static inline void raw_spin_unlock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_unlock(private_dereference(&rnp->lock));
	}

This __private mechanism also works for others who wants to private their fields of struct, which is not supported by C.

I will send two patches(one introduces __private and one uses it for rcu_node->lock) if you think this is not a bad idea ;-)

Quoted 6 lines

Quoted 3 lines

If rcu_node->lock is the only user then this is probably a bad idea, but if others also want to have a way to privatize some fields of the structure, this may be not that bad?

Thomas might also want this for things like irq_common_data::state_use_accessors for instance.

Quoted 2 lines

And I'm fairly sure there's more out there.

If Thomas takes it, I will consider also applying it to RCU.

Quoted 36 lines · Paul E. McKenney

Quoted 35 lines · Peter Zijlstra

Quoted 33 lines · Boqun Feng

Quoted 33 lines

Quoted 33 lines

Or maybe, we introduce another address space of sparse like:

	# define __private	__attribute__((noderef, address_space(6)))

and macro to dereference private

	# define private_dereference(p) ((typeof(*p) *) p)

and define struct rcu_node like:

	struct rcu_node {
		raw_spinlock_t __private lock;	/* Root rcu_node's lock protects some */
		...
	};

and finally raw_spin_{un}lock_rcu_node() like:

	static inline void raw_spin_lock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_lock(private_dereference(&rnp->lock));
		smp_mb__after_unlock_lock();
	}

	static inline void raw_spin_unlock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_unlock(private_dereference(&rnp->lock));
	}

This __private mechanism also works for others who wants to private their fields of struct, which is not supported by C.

I will send two patches(one introduces __private and one uses it for rcu_node->lock) if you think this is not a bad idea ;-)

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

If rcu_node->lock is the only user then this is probably a bad idea, but if others also want to have a way to privatize some fields of the structure, this may be not that bad?

Thomas might also want this for things like irq_common_data::state_use_accessors for instance.

Good to know! Thank you, Peter ;-)

Quoted 4 lines

Quoted 2 lines

And I'm fairly sure there's more out there.

If Thomas takes it, I will consider also applying it to RCU.

Paul, so I played with sparse a little more today, and found out that the address_space(6) attribute actually doesn't work here. However, the *noderef* attribute does work here, though the warning information is not very verbose, as there is no number of the address space, for example:

kernel/rcu/tree.c:4453:25: warning: incorrect type in argument 1 (different modifiers)
kernel/rcu/tree.c:4453:25: expected struct raw_spinlock [usertype] *lock
kernel/rcu/tree.c:4453:25: got struct raw_spinlock [noderef] *<noident>

In this example, I made rnp->lock __private and wrap *_{lock,unlock}() and this warning refers the raw_spin_lock_init() in rcu_init_one(). If we really want to privatize ->lock, we'd better also wrap this, I simply make an example here.

Thoughts?

The reason why address_space(6) doesn't work is that it's designed as an attribute of a pointer other than any type, and sparse will replace the members' address spaces with the address spaces of "parents" (objects of that struct).

Quoted 61 lines · Boqun Feng

Quoted 36 lines · Paul E. McKenney

Quoted 35 lines · Peter Zijlstra

Quoted 33 lines · Boqun Feng

Quoted 33 lines

Quoted 33 lines

Or maybe, we introduce another address space of sparse like:

	# define __private	__attribute__((noderef, address_space(6)))

and macro to dereference private

	# define private_dereference(p) ((typeof(*p) *) p)

and define struct rcu_node like:

	struct rcu_node {
		raw_spinlock_t __private lock;	/* Root rcu_node's lock protects some */
		...
	};

and finally raw_spin_{un}lock_rcu_node() like:

	static inline void raw_spin_lock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_lock(private_dereference(&rnp->lock));
		smp_mb__after_unlock_lock();
	}

	static inline void raw_spin_unlock_rcu_node(struct rcu_node *rnp)
	{
		raw_spin_unlock(private_dereference(&rnp->lock));
	}

This __private mechanism also works for others who wants to private their fields of struct, which is not supported by C.

I will send two patches(one introduces __private and one uses it for rcu_node->lock) if you think this is not a bad idea ;-)

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

If rcu_node->lock is the only user then this is probably a bad idea, but if others also want to have a way to privatize some fields of the structure, this may be not that bad?

Thomas might also want this for things like irq_common_data::state_use_accessors for instance.

Good to know! Thank you, Peter ;-)

Quoted 4 lines

Quoted 2 lines

And I'm fairly sure there's more out there.

If Thomas takes it, I will consider also applying it to RCU.

Paul, so I played with sparse a little more today, and found out that the address_space(6) attribute actually doesn't work here. However, the *noderef* attribute does work here, though the warning information is not very verbose, as there is no number of the address space, for example:

kernel/rcu/tree.c:4453:25: warning: incorrect type in argument 1 (different modifiers)
kernel/rcu/tree.c:4453:25: expected struct raw_spinlock [usertype] *lock
kernel/rcu/tree.c:4453:25: got struct raw_spinlock [noderef] *<noident>

Quoted 6 lines

In this example, I made rnp->lock __private and wrap *_{lock,unlock}() and this warning refers the raw_spin_lock_init() in rcu_init_one(). If we really want to privatize ->lock, we'd better also wrap this, I simply make an example here.

Thoughts?

I don't have any particular objection to noderef.

Quoted 4 lines

The reason why address_space(6) doesn't work is that it's designed as an attribute of a pointer other than any type, and sparse will replace the members' address spaces with the address spaces of "parents" (objects of that struct).

IIRC, we do an artificial dereference in rcu_dereference() and friends to get around this. But if the noderef attribute is more natural, why not go with it? For one thing, you can have something that is

both __rcu and noderef, which would not be possible with sparse address space 6.

Probably worth trying it out in a number of use cases, and perhaps you already tried it out on an int or some such.

On the added MBs,
the only alternative I have been able to come up with has even more MBs, as in on every lock acquisition. If I am missing something, please do not keep it a secret!

Quoted 4 lines · Paul E. McKenney

On the added MBs,
the only alternative I have been able to come up with has even more MBs, as in on every lock acquisition. If I am missing something, please do not keep it a secret!

You're right. And I suppose mpe is still on the fence wrt switching PPC over to RCsc lock order.. which would be all those extra MBs you talk about.

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines · Paul E. McKenney

On the added MBs,
the only alternative I have been able to come up with has even more MBs, as in on every lock acquisition. If I am missing something, please do not keep it a secret!

Quoted 3 lines

You're right. And I suppose mpe is still on the fence wrt switching PPC over to RCsc lock order.. which would be all those extra MBs you talk about.

Yes, I would like to avoid forcing that choice on him.

Quoted 25 lines · Will Deacon

Hi Peter, Paul,

Firstly, thanks for writing that up. I agree that you have something that can work in theory, but see below.

Quoted 19 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

This looks architecture-agnostic to me:

a. TSO systems have smp_mb__after_unlock_lock() be a no-op, and
have a read-only implementation for spin_unlock_wait().

Quoted 8 lines

b. Small-scale weakly ordered systems can also have
smp_mb__after_unlock_lock() be a no-op, but must instead have spin_unlock_wait() acquire the lock and immediately release it, or some optimized implementation of this.

c. Large-scale weakly ordered systems are required to define
smp_mb__after_unlock_lock() as smp_mb(), but can have a read-only implementation of spin_unlock_wait().

This would still require all relevant spin_lock() sites to be annotated with smp_mb__after_unlock_lock(), which is going to be a painful (no warning when done wrong) exercise and expensive (added MBs all over the place).

On the lack of warning, agreed, but please see below. On the added MBs, the only alternative I have been able to come up with has even more MBs, as in on every lock acquisition. If I am missing something, please do not keep it a secret!

Hi Paul,

Quoted 27 lines · Paul E. McKenney

Quoted 25 lines · Will Deacon

Hi Peter, Paul,

Firstly, thanks for writing that up. I agree that you have something that can work in theory, but see below.

Quoted 19 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

This looks architecture-agnostic to me:

a. TSO systems have smp_mb__after_unlock_lock() be a no-op, and
have a read-only implementation for spin_unlock_wait().

Quoted 8 lines

b. Small-scale weakly ordered systems can also have
smp_mb__after_unlock_lock() be a no-op, but must instead have spin_unlock_wait() acquire the lock and immediately release it, or some optimized implementation of this.

c. Large-scale weakly ordered systems are required to define
smp_mb__after_unlock_lock() as smp_mb(), but can have a read-only implementation of spin_unlock_wait().

This would still require all relevant spin_lock() sites to be annotated with smp_mb__after_unlock_lock(), which is going to be a painful (no warning when done wrong) exercise and expensive (added MBs all over the place).

Quoted 5 lines

On the lack of warning, agreed, but please see below. On the added MBs, the only alternative I have been able to come up with has even more MBs, as in on every lock acquisition. If I am missing something, please do not keep it a secret!

Maybe we can treat this problem as a problem of data accesses other than one of locks?

Let's take the example of tsk->flags in do_exit() and tsk->pi_lock, we don't need to add a full barrier for every lock acquisition of ->pi_lock, because some critical sections of ->pi_lock don't access the PF_EXITING bit of ->flags at all. What we only need is to add a full barrier before reading the PF_EXITING bit in a critical section of ->pi_lock. To achieve this, we could introduce a primitive like smp_load_in_lock():

(on PPC and ARM64v8)

	#define smp_load_in_lock(x, lock) 		\
		({ 					\
			smp_mb();			\
			READ_ONCE(x);			\
		})

(on other archs)

	#define smp_load_in_lock(x, lock) READ_ONCE(x)

And call it every time we read a data which is not protected by the current lock critical section but whose updaters synchronize with the current lock critical section with spin_unlock_wait().

I admit the name may be bad and the second parameter @lock is for a way to diagnosing the usage which I haven't come up with yet ;-)

Thoughts?

Quoted 64 lines · Boqun Feng

Hi Paul,

Quoted 27 lines · Paul E. McKenney

Quoted 25 lines · Will Deacon

Hi Peter, Paul,

Firstly, thanks for writing that up. I agree that you have something that can work in theory, but see below.

Quoted 19 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

This looks architecture-agnostic to me:

a. TSO systems have smp_mb__after_unlock_lock() be a no-op, and
have a read-only implementation for spin_unlock_wait().

Quoted 8 lines

b. Small-scale weakly ordered systems can also have
smp_mb__after_unlock_lock() be a no-op, but must instead have spin_unlock_wait() acquire the lock and immediately release it, or some optimized implementation of this.

c. Large-scale weakly ordered systems are required to define
smp_mb__after_unlock_lock() as smp_mb(), but can have a read-only implementation of spin_unlock_wait().

This would still require all relevant spin_lock() sites to be annotated with smp_mb__after_unlock_lock(), which is going to be a painful (no warning when done wrong) exercise and expensive (added MBs all over the place).

Quoted 5 lines

On the lack of warning, agreed, but please see below. On the added MBs, the only alternative I have been able to come up with has even more MBs, as in on every lock acquisition. If I am missing something, please do not keep it a secret!

Maybe we can treat this problem as a problem of data accesses other than one of locks?

Let's take the example of tsk->flags in do_exit() and tsk->pi_lock, we don't need to add a full barrier for every lock acquisition of ->pi_lock, because some critical sections of ->pi_lock don't access the PF_EXITING bit of ->flags at all. What we only need is to add a full barrier before reading the PF_EXITING bit in a critical section of ->pi_lock. To achieve this, we could introduce a primitive like smp_load_in_lock():

(on PPC and ARM64v8)

	#define smp_load_in_lock(x, lock) 		\
		({ 					\
			smp_mb();			\
			READ_ONCE(x);			\
		})

(on other archs)

	#define smp_load_in_lock(x, lock) READ_ONCE(x)

And call it every time we read a data which is not protected by the current lock critical section but whose updaters synchronize with the current lock critical section with spin_unlock_wait().

Quoted 4 lines

I admit the name may be bad and the second parameter @lock is for a way to diagnosing the usage which I haven't come up with yet ;-)

Thoughts?

In other words, dispense with smp_mb__after_unlock_lock() in those cases, and use smp_load_in_lock() to get the desired effect?

If so, one concern is how to check for proper use of smp_load_in_lock().

Another concern is redundant smp_mb() instances in case of multiple accesses to the data under a given critical section.

Or am I missing your point?

Thanx, Paul

Quoted 65 lines · Paul E. McKenney

Quoted 64 lines · Boqun Feng

Hi Paul,

Quoted 27 lines · Paul E. McKenney

Quoted 25 lines · Will Deacon

Hi Peter, Paul,

Firstly, thanks for writing that up. I agree that you have something that can work in theory, but see below.

Quoted 19 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

This looks architecture-agnostic to me:

a. TSO systems have smp_mb__after_unlock_lock() be a no-op, and
have a read-only implementation for spin_unlock_wait().

Quoted 8 lines

b. Small-scale weakly ordered systems can also have
smp_mb__after_unlock_lock() be a no-op, but must instead have spin_unlock_wait() acquire the lock and immediately release it, or some optimized implementation of this.

c. Large-scale weakly ordered systems are required to define
smp_mb__after_unlock_lock() as smp_mb(), but can have a read-only implementation of spin_unlock_wait().

This would still require all relevant spin_lock() sites to be annotated with smp_mb__after_unlock_lock(), which is going to be a painful (no warning when done wrong) exercise and expensive (added MBs all over the place).

Quoted 5 lines

On the lack of warning, agreed, but please see below. On the added MBs, the only alternative I have been able to come up with has even more MBs, as in on every lock acquisition. If I am missing something, please do not keep it a secret!

Maybe we can treat this problem as a problem of data accesses other than one of locks?

Let's take the example of tsk->flags in do_exit() and tsk->pi_lock, we don't need to add a full barrier for every lock acquisition of ->pi_lock, because some critical sections of ->pi_lock don't access the PF_EXITING bit of ->flags at all. What we only need is to add a full barrier before reading the PF_EXITING bit in a critical section of ->pi_lock. To achieve this, we could introduce a primitive like smp_load_in_lock():

(on PPC and ARM64v8)

	#define smp_load_in_lock(x, lock) 		\
		({ 					\
			smp_mb();			\
			READ_ONCE(x);			\
		})

(on other archs)

	#define smp_load_in_lock(x, lock) READ_ONCE(x)

And call it every time we read a data which is not protected by the current lock critical section but whose updaters synchronize with the current lock critical section with spin_unlock_wait().

Quoted 8 lines

Quoted 4 lines

I admit the name may be bad and the second parameter @lock is for a way to diagnosing the usage which I haven't come up with yet ;-)

Thoughts?

In other words, dispense with smp_mb__after_unlock_lock() in those cases, and use smp_load_in_lock() to get the desired effect?

Exactly.

Quoted 1 line

If so, one concern is how to check for proper use of smp_load_in_lock().

I also propose that on the updaters' side, we merge STORE and smp_mb() into another primitive, maybe smp_store_out_of_lock(). After that we make sure a smp_store_out_of_lock() plus a spin_unlock_wait() pairs with a spin_lock plus a smp_load_in_lock() in the following way:

	CPU 0				CPU 1
	==============================================================
	smp_store_out_of_lock(o, NULL, lock);
	<other stores or reads>
	spin_unlock_wait(lock);		spin_lock(lock);
					<other stores or reads>
					obj = smp_load_in_lock(o, lock);

Their names and this pairing pattern could help us check their usages. And we can also try to come up with a way to use lockdep to check their usages automatically. Anyway, I don't think that is more difficult to check the usage of smp_mb__after_unlock_lock() for the same purpose of ordering "Prior Write" with "Current Read" ;-)

Quoted 3 lines

Another concern is redundant smp_mb() instances in case of multiple accesses to the data under a given critical section.

First, I don't think there would be too many cases which a lock critical section needs to access multiple variables, which are modified outside the critical section and synchronized with spin_unlock_wait(). Because using spin_unlock_wait() to synchronize with lock critical sections is itself an very weird usage(you could just take the lock).

Second, even if we have redundant smp_mb()s, we avoid to:

1. use a ll/sc loop on updaters' sides as Will proposed

or

2. put a full barrier *just* following spin_lock() as you proposed,
which could forbid unrelated data accesses to be moved before the store part of spin_lock().

Whether these two perform better than redundant smp_mb()s in a lock critical section is uncertain, right?

Third, even if this perform worse than Will's or your proposal, we don't need to maintain two quite different ways to solve the same problem for PPC and ARM64v8, that's one benefit of this.

In conclusion, we have more than a half of uses working well already, and each of the fix-needed ones has only one related critical section and only one related data access in it. So on PPC, I think my proposal won't have more smp_mb() instances to fix all current use cases than adding smp_mb__after_unlock_lock() after the lock acquisition in each related lock critical section.

Of course, my proposal needs the buy-ins of both PPC and ARM64v8, so Paul and Will, what do you think? ;-)

Quoted 7 lines · Boqun Feng

In conclusion, we have more than a half of uses working well already, and each of the fix-needed ones has only one related critical section and only one related data access in it. So on PPC, I think my proposal won't have more smp_mb() instances to fix all current use cases than adding smp_mb__after_unlock_lock() after the lock acquisition in each related lock critical section.

Quoted 2 lines

Of course, my proposal needs the buy-ins of both PPC and ARM64v8, so Paul and Will, what do you think? ;-)

I already queued the change promoting it to LOCK for arm64. It makes the semantics easy to understand and I've failed to measure any difference in performance. It's also robust against any future users of the macro and matches what other architectures do.

Quoted 7 lines · Boqun Feng

In conclusion, we have more than a half of uses working well already, and each of the fix-needed ones has only one related critical section and only one related data access in it. So on PPC, I think my proposal won't have more smp_mb() instances to fix all current use cases than adding smp_mb__after_unlock_lock() after the lock acquisition in each related lock critical section.

Quoted 2 lines

Of course, my proposal needs the buy-ins of both PPC and ARM64v8, so Paul and Will, what do you think? ;-)

I already queued the change promoting it to LOCK for arm64. It makes the semantics easy to understand and I've failed to measure any difference in performance. It's also robust against any future users of the macro and matches what other architectures do.

Quoted 11 lines · Will Deacon

Quoted 9 lines · Boqun Feng

In conclusion, we have more than a half of uses working well already, and each of the fix-needed ones has only one related critical section and only one related data access in it. So on PPC, I think my proposal won't have more smp_mb() instances to fix all current use cases than adding smp_mb__after_unlock_lock() after the lock acquisition in each related lock critical section.

Of course, my proposal needs the buy-ins of both PPC and ARM64v8, so Paul and Will, what do you think? ;-)

Quoted 4 lines

I already queued the change promoting it to LOCK for arm64. It makes the semantics easy to understand and I've failed to measure any difference in performance. It's also robust against any future users of the macro and matches what other architectures do.

What size system did you do your performance testing on?

Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics:

 | CPU 1                   CPU 2                     CPU 3
 | ==================      ====================      ==============
 |                                                   spin_unlock(&lock);
 |                         spin_lock(&lock):
 |                           r1 = *lock; // r1 == 0;
 |                         o = READ_ONCE(object); // reordered here
 | object = NULL;
 | smp_mb();
 | spin_unlock_wait(&lock);
 |                           *lock = 1;
 | smp_mb();
 | o->dead = true;
 |                         if (o) // true
 |                           BUG_ON(o->dead); // true!!

The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it

to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro).

This patch implements spin_unlock_wait using an LL/SC sequence with acquire semantics on arm64. For v8.1 systems with the LSE atomics, the

exclusive writeback is omitted, since the spin_lock operation is indivisible and no intermediate state can be observed.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/spinlock.h | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

arch/arm64/include/asm/spinlock.h +21 -2

index c85e96d174a5..fc9682bfe002 100644
@@ -26,9 +26,28 @@
  * The memory barriers are implicit with the load-acquire and store-release
  * instructions.
  */
+static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
+{
+	unsigned int tmp;
+	arch_spinlock_t lockval;
 
-#define arch_spin_unlock_wait(lock) \
-	do { while (arch_spin_is_locked(lock)) cpu_relax(); } while (0)
+	asm volatile(
+"	sevl\n"
+"1:	wfe\n"
+"2:	ldaxr	%w0, %2\n"
+"	eor	%w1, %w0, %w0, ror #16\n"
+"	cbnz	%w1, 1b\n"
+	ARM64_LSE_ATOMIC_INSN(
+	/* LL/SC */
+"	stxr	%w1, %w0, %2\n"
+"	cbnz	%w1, 2b\n", /* Serialise against any concurrent lockers */
+	/* LSE atomics */
+"	nop\n"
+"	nop\n")
+	: "=&r" (lockval), "=&r" (tmp), "+Q" (*lock)
+	:
+	: "memory");
+}
 
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 

Signature · 1 line

2.1.4

Hi Will,

Quoted 19 lines

Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics:

 | CPU 1                   CPU 2                     CPU 3
 | ==================      ====================      ==============
 |                                                   spin_unlock(&lock);
 |                         spin_lock(&lock):
 |                           r1 = *lock; // r1 == 0;
 |                         o = READ_ONCE(object); // reordered here
 | object = NULL;
 | smp_mb();
 | spin_unlock_wait(&lock);
 |                           *lock = 1;
 | smp_mb();
 | o->dead = true;
 |                         if (o) // true
 |                           BUG_ON(o->dead); // true!!

Quoted 3 lines

The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it

I wonder whether upgrading it to a LOCK operation is necessary. Please see below.

Quoted 8 lines

to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro).

This patch implements spin_unlock_wait using an LL/SC sequence with acquire semantics on arm64. For v8.1 systems with the LSE atomics, the

IIUC, you implement this with acquire semantics because a LOCK requires acquire semantics, right? I get that spin_unlock_wait() becoming a LOCK will surely simply our analysis, because LOCK->LOCK is always globally ordered. But for this particular problem, seems only a relaxed LL/SC loop suffices, and the use of spin_unlock_wait() in do_exit() only requires a control dependency which could be fulfilled by a relaxed LL/SC loop. So the acquire semantics may be not necessary here.

Am I missing something subtle here which is the reason you want to upgrading spin_unlock_wait() to a LOCK?

Quoted 1 line · Boqun Feng

Hi Will,

Hi Boqun,

Quoted 44 lines · Will Deacon

Quoted 19 lines

Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics:

 | CPU 1                   CPU 2                     CPU 3
 | ==================      ====================      ==============
 |                                                   spin_unlock(&lock);
 |                         spin_lock(&lock):
 |                           r1 = *lock; // r1 == 0;
 |                         o = READ_ONCE(object); // reordered here
 | object = NULL;
 | smp_mb();
 | spin_unlock_wait(&lock);
 |                           *lock = 1;
 | smp_mb();
 | o->dead = true;
 |                         if (o) // true
 |                           BUG_ON(o->dead); // true!!

Quoted 3 lines

The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it

I wonder whether upgrading it to a LOCK operation is necessary. Please see below.

Quoted 8 lines

to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro).

This patch implements spin_unlock_wait using an LL/SC sequence with acquire semantics on arm64. For v8.1 systems with the LSE atomics, the

IIUC, you implement this with acquire semantics because a LOCK requires acquire semantics, right? I get that spin_unlock_wait() becoming a LOCK will surely simply our analysis, because LOCK->LOCK is always globally ordered. But for this particular problem, seems only a relaxed LL/SC loop suffices, and the use of spin_unlock_wait() in do_exit() only requires a control dependency which could be fulfilled by a relaxed LL/SC loop. So the acquire semantics may be not necessary here.

Quoted 2 lines

Am I missing something subtle here which is the reason you want to upgrading spin_unlock_wait() to a LOCK?

There's two things going on here:

(1) Having spin_unlock_wait do a complete LL/SC sequence (i.e. writing to the lock, like a LOCK operation would)

(2) Giving it ACQUIRE semantics

(2) is not necessary to fix the problem you described. However, LOCK operations do imply ACQUIRE and it's not clear to me that what the ordering semantics are for spin_unlock_wait.

I'd really like to keep this as simple as possible. Introducing yet another magic barrier macro that isn't well understood or widely used feels like a major step backwards to me, so I opted to upgrade spin_unlock_wait to LOCK on arm64 so that I no longer have to worry about it.

Quoted 63 lines · Will Deacon

Quoted 1 line · Boqun Feng

Hi Will,

Hi Boqun,

Quoted 44 lines · Will Deacon

Quoted 19 lines

Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics:

 | CPU 1                   CPU 2                     CPU 3
 | ==================      ====================      ==============
 |                                                   spin_unlock(&lock);
 |                         spin_lock(&lock):
 |                           r1 = *lock; // r1 == 0;
 |                         o = READ_ONCE(object); // reordered here
 | object = NULL;
 | smp_mb();
 | spin_unlock_wait(&lock);
 |                           *lock = 1;
 | smp_mb();
 | o->dead = true;
 |                         if (o) // true
 |                           BUG_ON(o->dead); // true!!

Quoted 3 lines

The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it

I wonder whether upgrading it to a LOCK operation is necessary. Please see below.

Quoted 8 lines

to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro).

This patch implements spin_unlock_wait using an LL/SC sequence with acquire semantics on arm64. For v8.1 systems with the LSE atomics, the

IIUC, you implement this with acquire semantics because a LOCK requires acquire semantics, right? I get that spin_unlock_wait() becoming a LOCK will surely simply our analysis, because LOCK->LOCK is always globally ordered. But for this particular problem, seems only a relaxed LL/SC loop suffices, and the use of spin_unlock_wait() in do_exit() only requires a control dependency which could be fulfilled by a relaxed LL/SC loop. So the acquire semantics may be not necessary here.

Quoted 2 lines

Am I missing something subtle here which is the reason you want to upgrading spin_unlock_wait() to a LOCK?

There's two things going on here:

(1) Having spin_unlock_wait do a complete LL/SC sequence (i.e. writing to the lock, like a LOCK operation would)

(2) Giving it ACQUIRE semantics

(2) is not necessary to fix the problem you described. However, LOCK operations do imply ACQUIRE and it's not clear to me that what the ordering semantics are for spin_unlock_wait.

Quoted 6 lines

I'd really like to keep this as simple as possible. Introducing yet another magic barrier macro that isn't well understood or widely used feels like a major step backwards to me, so I opted to upgrade spin_unlock_wait to LOCK on arm64 so that I no longer have to worry about it.

Fair enough ;-)

Another thing about the problem is that it happens only if spin_unlock_wait() observes lock is *unlocked* at the first time it checks that, IOW, if spin_unlock_wait() observes the transition of the state of a lock from locked to unlocked, AFAICS, the problem won't happen. Maybe we can use this for a little optimization like(in pseudo code):

	while (1) { // ll/sc loop
		r1 = load_link(*lock);
		if (!is_locked(r1)) {
			res = store_conditional(lock, r1);
			if (res == SUCC) // store succeeded.
				return;
		}
		else
			break;
	}

	smp_rmb();
	do {
		r1 = *lock;	
	} while(is_locked(r1));

I think this works, and will test this using a PPC litmus in herd to verify it.