[PATCH] arm64: spinlock: serialise spin_unlock_wait against concurrent lockers

[Boqun Feng]

On Tue, Dec 08, 2015 at 11:17:46AM -0800, Paul E. McKenney wrote:
> On Tue, Dec 08, 2015 at 04:42:59PM +0800, Boqun Feng wrote:
> > On Mon, Dec 07, 2015 at 07:45:14AM -0800, Paul E. McKenney wrote:
> > > On Mon, Dec 07, 2015 at 11:34:55AM +0100, Peter Zijlstra wrote:
> > > > On Mon, Dec 07, 2015 at 08:45:04AM +0800, Boqun Feng wrote:
> > > > > > > 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 ;-)
> > > > 
> > > > > 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 ;-)
> > 
> > > > 
> > > > 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?
> 
> I don't have any particular objection to noderef.
> 
> > 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

Agreed. I think noderef is more appropriate for __private.

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

Yep, and I will cook a patchset which takes rcu_node::lock and
irq_common_data::state_use_accessors as examples, so that we have
something concrete to discuss ;-)

Regards,
Boqun
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151209/b29e91e3/attachment-0001.sig>