netdev
[Top] [All Lists]

Re: [RFC/PATCH] lockless loopback patch for 2.6 (version 2)

To: Andi Kleen <ak@xxxxxxx>
Subject: Re: [RFC/PATCH] lockless loopback patch for 2.6 (version 2)
From: Arthur Kepner <akepner@xxxxxxx>
Date: Fri, 18 Jun 2004 13:12:23 -0700
Cc: "David S. Miller" <davem@xxxxxxxxxx>, netdev@xxxxxxxxxxx
In-reply-to: <20040614182331.GA11862@xxxxxxxxxxxxx>
References: <Pine.SGI.4.56.0406141000060.479900@xxxxxxxxxxxxxxxxxxx> <20040614182331.GA11862@xxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
On Mon, 14 Jun 2004, Andi Kleen wrote:

> > +#define LOOPBACK_STAT_INC(field)                           \
> > +   (per_cpu_ptr(loopback_stats, smp_processor_id())->field++)
> > +#define LOOPBACK_STAT_ADD(field, n)                                \
> > +   (per_cpu_ptr(loopback_stats, smp_processor_id())->field += n)
>
> This is too complicated and not preempt safe. Use
> __get_cpu_var(loopback_stats).field++;

This is too complicated (and preemptible). But I think you mean to use:

get_cpu_ptr(loopback_stats)->field1 += blah;
get_cpu_ptr(loopback_stats)->field2++;
....
put_cpu_var(loopback_stats);

(loopback_stats is allocated with alloc_percpu().)

>
> I would also remove the macros and do this directly.

With this simplification the macros are not helpful. I'll kill 'em.

>
> > +   struct net_device_stats *stats = dev->priv;
> > +   int i;
> > +
> > +   if (unlikely(!stats)) {
>
> Tests for NULL don't need an unlikely, because gcc does that by
> default for itself.

OK, I didn't know that.

> But why can the stats here be NULL anyways?
>

stats can be NULL. loopback_init() might have failed when it tried to
kmalloc() it.

> >  #ifdef CONFIG_NET_RADIO
> >  #include <linux/wireless.h>                /* Note : will define 
> > WIRELESS_EXT */
> >  #include <net/iw_handler.h>
> > @@ -1277,6 +1278,20 @@
> >     return 0;
> >  }
> >
> > +#define HARD_TX_LOCK_BH(dev, cpu) {                        \
> > +   if ( dev->features && NETIF_F_LLTX  == 0 ) {    \
>
> && instead of & and missing brackets.
> ....

Fixed (both instances.)

>
> > -   if (ops->reset)
> > -           ops->reset(qdisc);
> > -   if (ops->destroy)
> > -           ops->destroy(qdisc);
> > -   module_put(ops->owner);
> > -   if (!(qdisc->flags&TCQ_F_BUILTIN))
> > -           kfree(qdisc);
> > +
> > +   call_rcu(&qdisc->q_rcu, __qdisc_destroy, qdisc);
>
> I think you need at least a wmb() after
>
>            if (q == qdisc) {
>                        *qp = q->next;
>                                break;
>                       }
>
> Otherwise the order of updates to the readers is no guaranteed.
> Also if you want to support alpha there will need to be
> smp_read_barrier_depends() in the reader walking this list.

Hmmm, I'm not sure.

The only place where the struct Qdisc is accessed locklessly
is in dev_queue_xmit(). (Or at least that's the only place
where the locking behavior when accessing the Qdisc _changes_
with this patch.)

It *does* look as if a smp_read_barrier_depends() is appropriate
in dev_queue_xmit() as follows:

  rcu_read_lock();
  ....
  q = dev->qdisc;
+ smp_read_barrier_depends();
  if (q->enqueue) {
     ....
     spin_lock_bh(&dev->queue_lock);
     rc = q->enqueue(skb, q);
     qdisc_run(dev);
     spin_unlock_bh(&dev->queue_lock);
     ....
  }
  rcu_read_unlock();

Also, there should be a memory barrier after the Qdisc's
"enqueue" pointer is modified but before the netdevice's qdisc
pointer is modified (although it looks as if there's always a
spinlock acquisition which does this implicitly.) But I've
added the smp_wmb()s to qdisc_create() and qdisc_create_dflt()
to be safe.

A patch with these changes is attached.

--

Arthur

Attachment: lockless_loopback.patch.3
Description: lockless loopback patch 3

<Prev in Thread] Current Thread [Next in Thread>