Re: [PATCH tip/urgent] rcu: add rcu_access_pointer and rcu_dereference_protected

Previous thread: [PATCH] xtables: make XT_ALIGN() usable in exported headers by exporting __ALIGN_KERNEL() by Alexey Dobriyan on Wednesday, April 7, 2010 - 9:22 am. (7 messages)

Next thread: [PATCH 1/2] slab: Generify kernel pointer validation by Pekka Enberg on Wednesday, April 7, 2010 - 9:23 am. (5 messages)
From: Paul E. McKenney
Date: Wednesday, April 7, 2010 - 9:26 am

This patch adds variants of rcu_dereference() that handle situations
where the RCU-protected data structure cannot change, perhaps due to
our holding the update-side lock, or where the RCU-protected pointer is
only to be fetched, not dereferenced.  These are needed due to some
performance concerns with using rcu_dereference() where it is not
required, aside from the need for lockdep/sparse checking.

The new rcu_access_pointer() primitive is for the case where the pointer
is be fetch and not dereferenced.  This primitive may be used without
protection, RCU or otherwise, due to the fact that it uses ACCESS_ONCE().

The new rcu_dereference_protected() primitive is for the case where updates
are prevented, for example, due to holding the update-side lock.  This
primitive does neither ACCESS_ONCE() nor smp_read_barrier_depends(), so
can only be used when updates are somehow prevented.

Suggested-by: David Howells <dhowells@redhat.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

 rcupdate.h |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 872a98e..3f06b3d 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -209,9 +209,47 @@ static inline int rcu_read_lock_sched_held(void)
 		rcu_dereference_raw(p); \
 	})
 
+/**
+ * rcu_access_pointer - fetch RCU pointer with no dereferencing
+ *
+ * Return the value of the specified RCU-protected pointer, but omit the
+ * smp_read_barrier_depends() and keep the ACCESS_ONCE().  This is useful
+ * when the value of this pointer is accessed, but the pointer is not
+ * dereferenced, for example, when testing an RCU-protected pointer against
+ * NULL.  This may also be used in cases where update-side locks prevent
+ * the value of the pointer from changing, but rcu_dereference_protected()
+ * is a lighter-weight primitive for this use case.
+ */
+#define rcu_access_pointer(p, c) \
+	({ \
+		if ...
From: Eric Dumazet
Date: Wednesday, April 7, 2010 - 9:38 am

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>


--

From: Mathieu Desnoyers
Date: Wednesday, April 7, 2010 - 9:45 am

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

We should probably pull this in liburcu eventually too (renaming
ACCESS_ONCE() into LOAD_SHARED()).

Thanks,


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--

From: David Howells
Date: Wednesday, April 7, 2010 - 10:00 am

NAK.  This shouldn't have the conditional parameter 'c'.  Given that 'c' (by
analogy to rcu_dereference_check()) is there to describe the conditions under
which it's permitted to dereference the pointer, why is that relevant here?
What is it you're proving?

David
--

From: Paul E. McKenney
Date: Wednesday, April 7, 2010 - 10:13 am

In some cases, the value of 'c' will indeed be '1', and in those cases,
there needs to be a comment stating why it is OK, similar to those
required for smp_mb() and friends.

In other cases, there will be a reference counter or a "not yet fully
initialized" flag that can (and should) be tested.

							Thanx, Paul
--

From: David Howells
Date: Wednesday, April 7, 2010 - 10:20 am

Why would you be using rcu_access_pointer() there?  Why wouldn't you be using
rcu_dereference_protected()?


Also, one other thing:  Should the default versions of these functions make
some reference to 'c' to prevent compiler warnings?  Should:

	#define rcu_dereference_check(p, c)	rcu_dereference_raw(p)

for example, be:

	#define rcu_dereference_check(p, c)	\
	({					\
		if (1 || !(c))			\
			rcu_dereference_raw(p);	\
	})

I'm not sure it's necessary, but it's possible to envisage a situation where
someone calculates something specifically for use in 'c', which will cause an
warning from the compiler if c isn't then checked.

David
--

From: Paul E. McKenney
Date: Wednesday, April 7, 2010 - 4:00 pm

Excellent question.  I am writing up the documentation now, and will

I did try this.  The problem is that it breaks the build for non-lockdep
configurations due to the lockdep-check primitives not being defined.  :-(

							Thanx, Paul
--

From: Paul E. McKenney
Date: Thursday, April 8, 2010 - 9:46 am

And all of the examples I could come up with that had c!=1 were contorted,
even by my standards.  So you were right, and I will drop the "c" on my
next set of patches.

							Thanx, Paul
--

From: David Howells
Date: Thursday, April 8, 2010 - 12:04 pm

When it's done, I'll rebuild my keys and NFS patches on top of it.

David
--

From: Paul E. McKenney
Date: Thursday, April 8, 2010 - 12:53 pm

Testing in progress.  ;-)

							Thanx, Paul
--

Previous thread: [PATCH] xtables: make XT_ALIGN() usable in exported headers by exporting __ALIGN_KERNEL() by Alexey Dobriyan on Wednesday, April 7, 2010 - 9:22 am. (7 messages)

Next thread: [PATCH 1/2] slab: Generify kernel pointer validation by Pekka Enberg on Wednesday, April 7, 2010 - 9:23 am. (5 messages)