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

[David Howells <dhowells@redhat.com>]

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