[PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[David Howells]

From: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

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.

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_protect() 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>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/rcupdate.h |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 872a98e..a1b14b6 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -209,9 +209,43 @@ 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_protect()
+ * is a lighter-weight primitive for this use case.
+ */
+#define rcu_access_pointer(p) \
+	({ \
+		ACCESS_ONCE(p); \
+	})
+
+/**
+ * rcu_dereference_protected - fetch RCU pointer when updates prevented
+ *
+ * Return the value of the specified RCU-protected pointer, but omit
+ * both the smp_read_barrier_depends() and the ACCESS_ONCE().  This
+ * is useful in cases where update-side locks prevent the value of the
+ * pointer from changing.  Please note that this primitive does -not-
+ * prevent the compiler from repeating this reference or combining it
+ * with other references, so it should not be used without protection
+ * of appropriate locks.
+ */
+#define rcu_dereference_protected(p) \
+	({ \
+		(p); \
+	})
+
 #else /* #ifdef CONFIG_PROVE_RCU */
 
 #define rcu_dereference_check(p, c)	rcu_dereference_raw(p)
+#define rcu_access_pointer(p)		ACCESS_ONCE(p)
+#define rcu_dereference_protect(p)	(p)
 
 #endif /* #else #ifdef CONFIG_PROVE_RCU */
 

[PATCH 2/2] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[David Howells]

Fix a number of RCU warnings in nfs_inode_return_delegation_noreclaim().
nfs_inode_return_delegation_noreclaim() and nfs_inode_return_delegation() don't
need to use rcu_dereference() outside the spinlocked region as they merely
examin the pointer and don't follow it, thus rendering unnecessary the need to
impose a partial ordering over the one item of interest.

nfs_detach_delegation_locked() doesn't need rcu_derefence() because it can only
be called if nfs_client::cl_lock is held, and that guards against anyone
changing nfsi->delegation under it.  Furthermore, the barrier in
rcu_derefence() is superfluous, given that the spin_lock() is also a barrier.

nfs_free_delegation() should be using rcu_dereference_check() to validate the
state that the data is in (the delegation inode must have been cleared).  By
this point, the delegation is being released, so no one else should be
attempting to use the saved credentials, and they can be cleared.  However,
rcu_assign_pointer() should be used to clear them, and the delegation itself
must still use call_rcu() as the list of delegations could be being traversed.


[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
fs/nfs/delegation.c:332 invoked rcu_dereference_check() without protection!

other info that might help us debug this:


rcu_scheduler_active = 1, debug_locks = 0
2 locks held by mount.nfs4/2281:
 #0:  (&type->s_umount_key#34){+.+...}, at: [<ffffffff810b25b4>] deactivate_super+0x60/0x80
 #1:  (iprune_sem){+.+...}, at: [<ffffffff810c332a>] invalidate_inodes+0x39/0x13a

stack backtrace:
Pid: 2281, comm: mount.nfs4 Not tainted 2.6.34-rc1-cachefs #110
Call Trace:
 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b4591>] nfs_inode_return_delegation_noreclaim+0x5b/0xa0 [nfs]
 [<ffffffffa0095d63>] nfs4_clear_inode+0x11/0x1e [nfs]
 [<ffffffff810c2d92>] clear_inode+0x9e/0xf8
 [<ffffffff810c3028>] dispose_list+0x67/0x10e
 [<ffffffff810c340d>] invalidate_inodes+0x11c/0x13a
 [<ffffffff810b1dc1>] generic_shutdown_super+0x42/0xf4
 [<ffffffff810b1ebe>] kill_anon_super+0x11/0x4f
 [<ffffffffa009893c>] nfs4_kill_super+0x3f/0x72 [nfs]
 [<ffffffff810b25bc>] deactivate_super+0x68/0x80
 [<ffffffff810c6744>] mntput_no_expire+0xbb/0xf8
 [<ffffffff810c681b>] release_mounts+0x9a/0xb0
 [<ffffffff810c689b>] put_mnt_ns+0x6a/0x79
 [<ffffffffa00983a1>] nfs_follow_remote_path+0x5a/0x146 [nfs]
 [<ffffffffa0098334>] ? nfs_do_root_mount+0x82/0x95 [nfs]
 [<ffffffffa00985a9>] nfs4_try_mount+0x75/0xaf [nfs]
 [<ffffffffa0098874>] nfs4_get_sb+0x291/0x31a [nfs]
 [<ffffffff810b2059>] vfs_kern_mount+0xb8/0x177
 [<ffffffff810b2176>] do_kern_mount+0x48/0xe8
 [<ffffffff810c810b>] do_mount+0x782/0x7f9
 [<ffffffff810c8205>] sys_mount+0x83/0xbe
 [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b

Also on:

fs/nfs/delegation.c:215 invoked rcu_dereference_check() without protection!
 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b4223>] nfs_inode_set_delegation+0xfe/0x219 [nfs]
 [<ffffffffa00a9c6f>] nfs4_opendata_to_nfs4_state+0x2c2/0x30d [nfs]
 [<ffffffffa00aa15d>] nfs4_do_open+0x2a6/0x3a6 [nfs]
 ...

And:

fs/nfs/delegation.c:40 invoked rcu_dereference_check() without protection!
 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b3bef>] nfs_free_delegation+0x3d/0x6e [nfs]
 [<ffffffffa00b3e71>] nfs_do_return_delegation+0x26/0x30 [nfs]
 [<ffffffffa00b406a>] __nfs_inode_return_delegation+0x1ef/0x1fe [nfs]
 [<ffffffffa00b448a>] nfs_client_return_marked_delegations+0xc9/0x124 [nfs]
 ...


Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/nfs/delegation.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 1567124..8506bc0 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -38,7 +38,8 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
 {
 	struct rpc_cred *cred;
 
-	cred = rcu_dereference(delegation->cred);
+	cred = rcu_dereference_check(delegation->cred,
+				     delegation->inode == NULL);
 	rcu_assign_pointer(delegation->cred, NULL);
 	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
 	if (cred)
@@ -168,7 +169,7 @@ static struct inode *nfs_delegation_grab_inode(struct nfs_delegation *delegation
 
 static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid)
 {
-	struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation);
+	struct nfs_delegation *delegation = rcu_dereference_protected(nfsi->delegation);
 
 	if (delegation == NULL)
 		goto nomatch;
@@ -195,7 +196,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 {
 	struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
 	struct nfs_inode *nfsi = NFS_I(inode);
-	struct nfs_delegation *delegation;
+	struct nfs_delegation *delegation, *old_delegation;
 	struct nfs_delegation *freeme = NULL;
 	int status = 0;
 
@@ -213,10 +214,11 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	spin_lock_init(&delegation->lock);
 
 	spin_lock(&clp->cl_lock);
-	if (rcu_dereference(nfsi->delegation) != NULL) {
-		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
-					sizeof(delegation->stateid)) == 0 &&
-				delegation->type == nfsi->delegation->type) {
+	old_delegation = rcu_dereference_protected(nfsi->delegation);
+	if (old_delegation != NULL) {
+		if (memcmp(&delegation->stateid, &old_delegation->stateid,
+					sizeof(old_delegation->stateid)) == 0 &&
+				delegation->type == old_delegation->type) {
 			goto out;
 		}
 		/*
@@ -226,7 +228,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 		dfprintk(FILE, "%s: server %s handed out "
 				"a duplicate delegation!\n",
 				__func__, clp->cl_hostname);
-		if (delegation->type <= nfsi->delegation->type) {
+		if (delegation->type <= old_delegation->type) {
 			freeme = delegation;
 			delegation = NULL;
 			goto out;
@@ -330,7 +332,7 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode)
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_delegation *delegation;
 
-	if (rcu_dereference(nfsi->delegation) != NULL) {
+	if (rcu_access_pointer(nfsi->delegation) != NULL) {
 		spin_lock(&clp->cl_lock);
 		delegation = nfs_detach_delegation_locked(nfsi, NULL);
 		spin_unlock(&clp->cl_lock);
@@ -346,7 +348,7 @@ int nfs_inode_return_delegation(struct inode *inode)
 	struct nfs_delegation *delegation;
 	int err = 0;
 
-	if (rcu_dereference(nfsi->delegation) != NULL) {
+	if (rcu_access_pointer(nfsi->delegation) != NULL) {
 		spin_lock(&clp->cl_lock);
 		delegation = nfs_detach_delegation_locked(nfsi, NULL);
 		spin_unlock(&clp->cl_lock);

Re: [PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[Eric Dumazet]

Le mercredi 07 avril 2010 à 14:57 +0100, David Howells a écrit :
> From: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> 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.
> 
> 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_protect() 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>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  include/linux/rcupdate.h |   34 ++++++++++++++++++++++++++++++++++
>  1 files changed, 34 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 872a98e..a1b14b6 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -209,9 +209,43 @@ 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_protect()
> + * is a lighter-weight primitive for this use case.
> + */
> +#define rcu_access_pointer(p) \
> +	({ \
> +		ACCESS_ONCE(p); \
> +	})
> +
> +/**
> + * rcu_dereference_protected - fetch RCU pointer when updates prevented
> + *
> + * Return the value of the specified RCU-protected pointer, but omit
> + * both the smp_read_barrier_depends() and the ACCESS_ONCE().  This
> + * is useful in cases where update-side locks prevent the value of the
> + * pointer from changing.  Please note that this primitive does -not-
> + * prevent the compiler from repeating this reference or combining it
> + * with other references, so it should not be used without protection
> + * of appropriate locks.
> + */
> +#define rcu_dereference_protected(p) \
> +	({ \
> +		(p); \
> +	})
> +
>  #else /* #ifdef CONFIG_PROVE_RCU */
>  
>  #define rcu_dereference_check(p, c)	rcu_dereference_raw(p)
> +#define rcu_access_pointer(p)		ACCESS_ONCE(p)
> +#define rcu_dereference_protect(p)	(p)
>  
>  #endif /* #else #ifdef CONFIG_PROVE_RCU */
>  
> 
> --

This is not the version Paul posted. 

Removing checks just to shutup warnings ?

All the point is to get lockdep assistance, and you throw it away.

We want to explicit the condition, so that RCU users can explicitly
state what protects their data.

Re: [PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[David Howells]

Eric Dumazet <eric.dumazet@gmail.com> wrote:

> This is not the version Paul posted. 
> 
> Removing checks just to shutup warnings ?

No.  I don't see the point in the condition.

> All the point is to get lockdep assistance, and you throw it away.
> 
> We want to explicit the condition, so that RCU users can explicitly
> state what protects their data.

You've missed the point.

For rcu_access_pointer(), _nothing_ protects the data, not only that, we don't
care: we're only checking the pointer.

For rcu_dereference_protect[ed](), I don't see that the check helps.  You
don't need to be holding the RCU lock to call it, but you do need to hold all
the requisite locks required to exclude others modifying it.  That's a
precondition for calling this function, so is there any point in testing it
again?

For instance, consider the following pseudocode:

	do_something(struct foo *p)
	{
		struct bar *b;
		spin_lock(&foo->lock);
		b = rcu_dereference_protected(
			foo->bar, lockdep_is_held(&foo->lock));
		do_something_to_bar(b);
		spin_unlock(&foo->lock);
	}

is there any need for the condition?  Does lockdep_is_held() have any side
effects beyond those listed in the Documentation directory or on its attached
banner comments?


Furthermore, I think the condition in rcu_dereference_check() may well be
misused.  For instance, Paul suggested:

	cred = rcu_dereference_check(delegation->cred,
				     delegation->inode == NULL);

but if 'c' is supposed to be the locks that protect the data, is this a valid
check?

David

Re: [PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[Eric Dumazet]

Le mercredi 07 avril 2010 à 16:40 +0100, David Howells a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > This is not the version Paul posted. 
> > 
> > Removing checks just to shutup warnings ?
> 
> No.  I don't see the point in the condition.
> 
> > All the point is to get lockdep assistance, and you throw it away.
> > 
> > We want to explicit the condition, so that RCU users can explicitly
> > state what protects their data.
> 
> You've missed the point.
> 

You already claimed I dont understand RCU. I find this claim funny.


> For rcu_access_pointer(), _nothing_ protects the data, not only that, we don't
> care: we're only checking the pointer.

How can you state this ?

Thats pretty simple, "always true" is a fine condition.

What's the problem with this ?

> 
> For rcu_dereference_protect[ed](), I don't see that the check helps.  You
> don't need to be holding the RCU lock to call it, but you do need to hold all
> the requisite locks required to exclude others modifying it.  That's a
> precondition for calling this function, so is there any point in testing it
> again?
> 

If you dont see how the check can help, why dont you unset
CONFIG_PROVE_RCU ?


> For instance, consider the following pseudocode:
> 
> 	do_something(struct foo *p)
> 	{
> 		struct bar *b;
> 		spin_lock(&foo->lock);
> 		b = rcu_dereference_protected(
> 			foo->bar, lockdep_is_held(&foo->lock));
> 		do_something_to_bar(b);
> 		spin_unlock(&foo->lock);
> 	}
> 
> is there any need for the condition? 

Yes, this is what is needed to help to catch when a condition is not
met.

Of course, on trivial code like this one, its pretty obvious condition
will be always true.

In many cases, smp_processor_id() checks are obvious too, yet we perform
them. It can help us sometimes, because many developers forget the
obvious things.

>  Does lockdep_is_held() have any side
> effects beyond those listed in the Documentation directory or on its attached
> banner comments?
> 
> 
> Furthermore, I think the condition in rcu_dereference_check() may well be
> misused.  For instance, Paul suggested:
> 
> 	cred = rcu_dereference_check(delegation->cred,
> 				     delegation->inode == NULL);
> 
> but if 'c' is supposed to be the locks that protect the data, is this a valid
> check?

'c' is not a lock. Its a condition.

You as the author of this code, decide of the condition to check.

You therefore can answer yourself to this question.

Example of non trivial check :

static void __sk_free(struct sock *sk)
{
...
filter = rcu_dereference_check(sk->sk_filter,
			       atomic_read(&sk->sk_wmem_alloc) == 0);
...
}

In this check, there is no lock held.


commit a898def29e4119bc01ebe7ca97423181f4c0ea2d
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Feb 22 17:04:49 2010 -0800

    net: Add checking to rcu_dereference() primitives
    
    Update rcu_dereference() primitives to use new lockdep-based
    checking. The rcu_dereference() in __in6_dev_get() may be
    protected either by rcu_read_lock() or RTNL, per Eric Dumazet.
    The rcu_dereference() in __sk_free() is protected by the fact
    that it is never reached if an update could change it.  Check
    for this by using rcu_dereference_check() to verify that the
    struct sock's ->sk_wmem_alloc counter is zero.
    
    Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
    Acked-by: David S. Miller <davem@davemloft.net>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: laijs@cn.fujitsu.com
    Cc: dipankar@in.ibm.com
    Cc: mathieu.desnoyers@polymtl.ca
    Cc: josh@joshtriplett.org
    Cc: dvhltc@us.ibm.com
    Cc: niv@us.ibm.com
    Cc: peterz@infradead.org
    Cc: rostedt@goodmis.org
    Cc: Valdis.Kletnieks@vt.edu
    Cc: dhowells@redhat.com
    LKML-Reference:
<1266887105-1528-5-git-send-email-paulmck@linux.vnet.ibm.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

...
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1073,7 +1073,8 @@ static void __sk_free(struct sock *sk)
        if (sk->sk_destruct)
                sk->sk_destruct(sk);
 
-       filter = rcu_dereference(sk->sk_filter);
+       filter = rcu_dereference_check(sk->sk_filter,
+                                      atomic_read(&sk->sk_wmem_alloc) == 0);
        if (filter) {
                sk_filter_uncharge(sk, filter);
                rcu_assign_pointer(sk->sk_filter, NULL);

Re: [PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[David Howells]

Eric Dumazet <eric.dumazet@gmail.com> wrote:

> > You've missed the point.
> 
> You already claimed I dont understand RCU. I find this claim funny.
> 
> > For rcu_access_pointer(), _nothing_ protects the data, not only that, we
> > don't care: we're only checking the pointer.
> 
> How can you state this ?
> 
> Thats pretty simple, "always true" is a fine condition.
> 
> What's the problem with this ?

If the condition for rcu_access_pointer() is always "always true", then it's
redundant, right?  rcu_access_pointer() is for checking the pointer only, not
checking the payload that pointer might point to.  So, what condition are you
supposed to be checking?
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> > but if 'c' is supposed to be the locks that protect the data, is this a
> > valid check?
> 
> 'c' is not a lock. Its a condition.

Sorry, I meant the state of the relevant locking context.

To take your example:

> filter = rcu_dereference_check(sk->sk_filter,
> 			       atomic_read(&sk->sk_wmem_alloc) == 0);

what is the value of sk->sk_wmem_alloc to the lock context of sk->sk_filter?
Why would lockdep be interested in sk_wmem_alloc?

Surely, the assertion that the value of sk->sk_filter is related to
sk_wmem_alloc being 0 is independent of the need to dereference sk_filter for
RCU purposes.  So why are these being combined?

Why not:

	ASSERT(atomic_read(&sk->sk_wmem_alloc) == 0);
	filter = rcu_dereference(sk->sk_filter);

This is much clearer, and you're not combining an unrelated assertion with the
RCU dereference.

David

Re: [PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[Eric Dumazet]

Le mercredi 07 avril 2010 à 17:19 +0100, David Howells a écrit :

> Why not:
> 
> 	ASSERT(atomic_read(&sk->sk_wmem_alloc) == 0);
> 	filter = rcu_dereference(sk->sk_filter);
> 
> This is much clearer, and you're not combining an unrelated assertion with the
> RCU dereference.

1) Because we want the check being done only when CONFIG_PROVE_RCU is
set.

2) Because rcu_dereference() default condition is : 'Am I owning
rcu_read_lock() or equivalent'. 
In this context, I am _not_ owning rcu lock, so we will trigger a
warning.


So this is best done as is :)

I personally find this very clear and clean, this is why I acked Paul
patch :)

If we were 100% sure testing sk_wmem_alloc is not necessary, we would
have put :

filter = rcu_dereference_check(sk->sk_filter, 1);

Re: [PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[Eric Dumazet]

Le mercredi 07 avril 2010 à 17:19 +0100, David Howells a écrit :

> what is the value of sk->sk_wmem_alloc to the lock context of sk->sk_filter?
> Why would lockdep be interested in sk_wmem_alloc?
> 
> Surely, the assertion that the value of sk->sk_filter is related to
> sk_wmem_alloc being 0 is independent of the need to dereference sk_filter for
> RCU purposes.  So why are these being combined?

Because when sk->sk_filter is eventually written by some thread, this
thread _must_ own a reference on the socket, that is sk_wmem_alloc > 0

So when reading sk->sk_filter, the general condition is : 
- We own the rcu lock 
- But on the particular case of __sk_free(),
  we owned the very last reference to sk (we are going to kfree it), so
nobody can possibly change sk->sk_filter under us.

Re: [PATCH 1/2] rcu: add rcu_access_pointer and rcu_dereference_protect

[Paul E. McKenney]

On Wed, Apr 07, 2010 at 04:56:50PM +0200, Eric Dumazet wrote:
> Le mercredi 07 avril 2010 à 14:57 +0100, David Howells a écrit :
> > From: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > 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.
> > 
> > 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_protect() 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>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > ---
> > 
> >  include/linux/rcupdate.h |   34 ++++++++++++++++++++++++++++++++++
> >  1 files changed, 34 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 872a98e..a1b14b6 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -209,9 +209,43 @@ 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_protect()
> > + * is a lighter-weight primitive for this use case.
> > + */
> > +#define rcu_access_pointer(p) \
> > +	({ \
> > +		ACCESS_ONCE(p); \
> > +	})
> > +
> > +/**
> > + * rcu_dereference_protected - fetch RCU pointer when updates prevented
> > + *
> > + * Return the value of the specified RCU-protected pointer, but omit
> > + * both the smp_read_barrier_depends() and the ACCESS_ONCE().  This
> > + * is useful in cases where update-side locks prevent the value of the
> > + * pointer from changing.  Please note that this primitive does -not-
> > + * prevent the compiler from repeating this reference or combining it
> > + * with other references, so it should not be used without protection
> > + * of appropriate locks.
> > + */
> > +#define rcu_dereference_protected(p) \
> > +	({ \
> > +		(p); \
> > +	})
> > +
> >  #else /* #ifdef CONFIG_PROVE_RCU */
> >  
> >  #define rcu_dereference_check(p, c)	rcu_dereference_raw(p)
> > +#define rcu_access_pointer(p)		ACCESS_ONCE(p)
> > +#define rcu_dereference_protect(p)	(p)
> >  
> >  #endif /* #else #ifdef CONFIG_PROVE_RCU */
> >  
> > 
> > --
> 
> This is not the version Paul posted. 

I blew the name -- rcu_dereference_protected() is in fact a better name.

> Removing checks just to shutup warnings ?
> 
> All the point is to get lockdep assistance, and you throw it away.
> 
> We want to explicit the condition, so that RCU users can explicitly
> state what protects their data.

What Eric said!!!  ;-)

							Thanx, Paul