On Thu, 28 Sep 2006 15:13:01 +0200
Jarek Poplawski <[EMAIL PROTECTED]> wrote:
> On Thu, Sep 28, 2006 at 02:17:51PM +0200, Patrick McHardy wrote:
> > [My mail provider is down, so responding "manually"]
> >
> > Jarek Poplawski wrote:
> > > > [NET_SCHED]: Fix fallout from dev->qdisc RCU change
> > >
> > > Sorry again but I can't abstain from some doubts:
> > >
> > > ...
> > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > index 14de297..4d891be 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -1480,14 +1480,16 @@ #endif
> > > > if (q->enqueue) {
> > > > /* Grab device queue */
> > > > spin_lock(&dev->queue_lock);
> > > > + q = dev->qdisc;
> > >
> > > I don't get it. If it is some anti-race step according to
> > > rcu rules it should be again:
> > > q = rcu_dereference(dev->qdisc);
> >
> > At this point RCU protection is not needed anymore since we
> > have the lock. We simply want to avoid taking the lock for
> > devices that don't have a real qdisc attached (like loopback).
> > Thats what the test for q->enqueue is there for. RCU is only
> > needed to avoid races between testing q->enqueue and freeing
> > of the qdisc.
>
> But in Documentation/RCU/whatisRCU.txt I see this:
>
> /*
> * Return the value of field "a" of the current gbl_foo
> * structure. Use rcu_read_lock() and rcu_read_unlock()
> * to ensure that the structure does not get deleted out
> * from under us, and use rcu_dereference() to ensure that
> * we see the initialized version of the structure (important
> * for DEC Alpha and for people reading the code).
> */
> int foo_get_a(void)
> {
> int retval;
>
> rcu_read_lock();
> retval = rcu_dereference(gbl_foo)->a;
> rcu_read_unlock();
> return retval;
> }
>
The example uses rcu_read_lock() which is a weaker form of protection
than a real lock, so the rcu_dereference() is needed to do memory barriers.
In the qdisc case we have the proper spin_lock() so no additional
barrier is needed.
--
Stephen Hemminger <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
