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:

quoted

On Mon, Dec 07, 2015 at 07:45:14AM -0800, Paul E. McKenney wrote:

quoted

On Mon, Dec 07, 2015 at 11:34:55AM +0100, Peter Zijlstra wrote:

quoted

On Mon, Dec 07, 2015 at 08:45:04AM +0800, Boqun Feng wrote:

quoted

quoted

quoted

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

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

quoted

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.

quoted

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>