netdev
[Top] [All Lists]

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

To: Arthur Kepner <akepner@xxxxxxx>
Subject: Re: [RFC/PATCH] lockless loopback patch for 2.6 (version 2)
From: Andi Kleen <ak@xxxxxxx>
Date: Mon, 14 Jun 2004 20:23:31 +0200
Cc: "David S. Miller" <davem@xxxxxxxxxx>, netdev@xxxxxxxxxxx
In-reply-to: <Pine.SGI.4.56.0406141000060.479900@xxxxxxxxxxxxxxxxxxx>
References: <Pine.SGI.4.56.0406141000060.479900@xxxxxxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
> +#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++; 

I would also remove the macros and do this directly.

> +     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. But why can the stats here be NULL anyways?

>  #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.

> +             spin_lock_bh(&dev->xmit_lock);          \
> +             dev->xmit_lock_owner = cpu;             \
> +     }                                               \
> +}
> +
> +#define HARD_TX_UNLOCK_BH(dev) {                     \
> +     if ( dev->features && NETIF_F_LLTX  == 0 ) {    \

Same.

> -     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.

-Andi


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