netdev
[Top] [All Lists]

Re: [Fwd: [RFT] NAPI for 8139too]

To: Stephen Hemminger <shemminger@xxxxxxxx>
Subject: Re: [Fwd: [RFT] NAPI for 8139too]
From: OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
Date: Sat, 18 Oct 2003 21:56:20 +0900
Cc: Jeff Garzik <jgarzik@xxxxxxxxx>, netdev@xxxxxxxxxxx
In-reply-to: <3F9070B6.9090306@pobox.com>
References: <3F9070B6.9090306@pobox.com>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.3
Hi,

I also working for it (patches attached). So I have several comment.

Jeff Garzik <jgarzik@xxxxxxxxx> writes:

> +     dev->weight = 16;

just question: Is there basis for believing that this value is
rightness?  Although I use 64, I don't have a reason for having chosen
it at all.

>       dev->stop = rtl8139_close;
>       dev->get_stats = rtl8139_get_stats;
>       dev->set_multicast_list = rtl8139_set_rx_mode;
> @@ -1879,24 +1877,28 @@
>  #endif
>  }
>  
> -static void rtl8139_rx_interrupt (struct net_device *dev,
> -                               struct rtl8139_private *tp, void *ioaddr)
> +/*
> + * NAPI poll routine.
> + */
> +static int rtl8139_poll(struct net_device *dev, int *budget)
>  {
> +     struct rtl8139_private *tp = dev->priv;
> +     void *ioaddr = tp->mmio_addr;
>       unsigned char *rx_ring;
>       u16 cur_rx;
> +     int rx, status;
> +     unsigned long flags;
>  
> -     assert (dev != NULL);
> -     assert (tp != NULL);
> -     assert (ioaddr != NULL);
> -
> +     spin_lock_irqsave(&tp->lock, flags);

I don't think lock of interrupt related path needed here. And this
thinks that it has a bad influence on TX path.

Instead of it, I think we need synchronize with ->tx_timeout because
->tx_timeout touch IntrMask.

> + rescan:
>       rx_ring = tp->rx_ring;
>       cur_rx = tp->cur_rx;
> -
[...]
>                       RTL_W16_F (IntrStatus, RxAckBits);
> +
> +             if (++rx >= dev->quota)

Shouldn't we use "min(dev->quota, *budget)" for this limit? Users can
change "budget" via sysctl.

> +     /* If received interrupt, then disable furthur interrupts
> +      * and enable NAPI polling.
> +      */
> +     if (netif_running (dev) && (status & RxAckBits)) {
> +             /* disable more receive interrupts */
> +             RTL_W16 (IntrMask, rtl8139_intr_mask & ~RxAckBits);
> +             netif_rx_schedule(dev);
> +     }

This style have problem on shutdowning interface path. Please see
8139too-napi-stop-fix.patch.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>

Attachment: 8139too.tar.gz
Description: Binary data

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