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>
8139too.tar.gz
Description: Binary data
|