Le mercredi 07 avril 2010 à 16:40 +0100, David Howells a écrit :
> Eric Dumazet <eric.dumazet@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
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@xxxxxxxxx>
Acked-by: David S. Miller <davem@xxxxxxxxxxxxx>
Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
Cc: laijs@xxxxxxxxxxxxxx
Cc: dipankar@xxxxxxxxxx
Cc: mathieu.desnoyers@xxxxxxxxxx
Cc: josh@xxxxxxxxxxxxxxxx
Cc: dvhltc@xxxxxxxxxx
Cc: niv@xxxxxxxxxx
Cc: peterz@xxxxxxxxxxxxx
Cc: rostedt@xxxxxxxxxxx
Cc: Valdis.Kletnieks@xxxxxx
Cc: dhowells@xxxxxxxxxx
LKML-Reference:
<1266887105-1528-5-git-send-email-paulmck@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Ingo Molnar <mingo@xxxxxxx>
...
--- 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);
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
