Re: [PATCH 1/3] netpoll-core

[Matt Mackall <mpm@selenic.com>]

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