Messages in this thread | | | Date | Mon, 22 Sep 2003 23:16:17 -0500 | From | Matt Mackall <> | Subject | Re: [PATCH 1/3] netpoll-core |
| |
On Mon, Sep 22, 2003 at 08:58:02PM -0400, Jeff Garzik wrote: > Matt Mackall wrote: > > >+void netpoll_poll(struct netpoll *np) > >+{ > >+ int budget = 1; > >+ > >+ if(!np || !np->dev || !(np->dev->flags & IFF_UP)) > >+ return; > > should test netif_running() not IFF_UP, IMO
Sure. I modeled it on the ip autoconf code which wasn't exactly pretty, but now I think I have a firmer grasp on the net stack.
> > >+ disable_irq(np->dev->irq); > >+ > >+ /* Process pending work on NIC */ > >+ np->irqfunc(np->dev->irq, np->dev, 0); > >+ > >+ /* If scheduling is stopped, tickle NAPI bits */ > >+ if(trapped && np->dev->poll && > >+ test_bit(__LINK_STATE_RX_SCHED, &np->dev->state)) > >+ np->dev->poll(np->dev, &budget); > >+ > >+ enable_irq(np->dev->irq); > >+} > > Calling the irq function from two different places, netpoll code and > arch code, worries me.
Why? The interrupt handlers don't have a special calling convention to speak of.
> >+static void refill_skbs(void) > >+{ > >+ struct sk_buff *skb; > >+ unsigned long flags; > >+ > >+ spin_lock_irqsave(&skb_list_lock, flags); > >+ while (nr_skbs < MAX_SKBS) { > >+ skb = alloc_skb(MAX_SKB_SIZE, GFP_ATOMIC); > >+ if (!skb) > >+ break; > >+ > >+ skb->next = skbs; > >+ skbs = skb; > >+ nr_skbs++; > >+ } > >+ spin_unlock_irqrestore(&skb_list_lock, flags); > >+} > > if it's a simple list, why not lock, find out number of allocations, > unlock, allocate a bunch of skbs, then finally lock again and tie back > into list.
That could work too. The above is more or less the same as what's in the RH 2.4 netdump patches and works well enough.
> >+static void zap_completion_queue(void) [...] > > this should be somewhere in a more general place... otherwise, somebody > will inevitably change softnet and forget to change this code.
Fair enough. Suggestions?
> >+void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) > >+{ > >+ int status; > >+ > >+repeat: > >+ if(!np || !np->dev || !(np->dev->flags & IFF_UP)) { > > netif_running() > > > >+ spin_lock(&np->dev->xmit_lock); > >+ np->dev->xmit_lock_owner = smp_processor_id(); > >+ > >+ if (netif_queue_stopped(np->dev)) { > >+ np->dev->xmit_lock_owner = -1; > >+ spin_unlock(&np->dev->xmit_lock); > >+ > >+ netpoll_poll(np); > >+ zap_completion_queue(); > >+ goto repeat; > >+ } > >+ > >+ status = np->dev->hard_start_xmit(skb, np->dev); > >+ np->dev->xmit_lock_owner = -1; > >+ spin_unlock(&np->dev->xmit_lock); > > this worries be too, but I don't have any outright objections. Mainly > similar to the above comments -- this is intimate with the net stack > locking, and at the very least shouldn't be hidden in a little-used > corner of the code :)
Actually I think it's going to be effectively hidden to anyone who doesn't use the netpoll interface as no one else is going to have a use for this sort of functionality. There might be a more generic place for zapping completions but not for this.
> >+#ifdef CONFIG_NETPOLL > >+ if (skb->dev->rx_hook && skb->dev->rx_hook(skb)) { > >+ kfree_skb(skb); > >+ return NET_RX_DROP; > >+ } > >+#endif > > This allows wholesale replacement of the IPv4 net stack, something we've > traditionally avoided.
Heh. Here I was thinking I was doing the right thing by making it generic. I can just add a netpoll flag somewhere in netdevice instead.
-- Matt Mackall : http://www.selenic.com : of or relating to the moon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |