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
lockless_loopback.patch.3
Description: lockless loopback patch 3
|