Re: [PATCH] Prevent netpoll hanging when link is down

[Matt Mackall <mpm@selenic.com>]

On Thu, Oct 07, 2004 at 11:28:46AM -0700, David S. Miller wrote:
> On Thu, 7 Oct 2004 16:05:32 +0200
> Colin Leroy <colin@colino.net> wrote:
> 
> > First, my newbie question: is it possible to deadlock a spinlock on a
> > Uniprocessor kernel ? For example, there's something I find suspect in
> > netpoll/sungem interaction:
> > 
> 
> Oh yes, it appears that netpoll doesn't support NETIF_F_LLTX locking,
> crap :(
> 
> When a device has NETIF_F_LLTX set, it means that the driver's
> dev->hard_start_xmit() routine is what takes the xmit_lock, not
> the caller one level up.
> 
> Andi Kleen didn't fix up netpoll when he did his LLTX changes, oops.
> 
> So, netpoll needs to have the NETIF_F_LLTX stuff added to it.
> Basically:
> 
> 1) If NETIF_F_LLTX is clear, same as before
> 2) If NETIF_F_LLTX is set:
> 	a) Do not take xmit_lock
> 	b) Check ->hard_start_xmit() return value,
> 	   if it is NETDEV_TX_LOCKED, then
> 	   spin_trylock(&dev->xmit_lock) failed
>            in ->hard_start_xmit()

Colin, feeling adventurous enough to take a stab at this? It looks
pretty straightforward but I'm going to be even more useless than
usual for the next two weeks.

> 
> The best example is in net/sched/sch_generic.c:qdisc_restart()
> 
> 		unsigned nolock = (dev->features & NETIF_F_LLTX);
> 		/*
> 		 * When the driver has LLTX set it does its own locking
> 		 * in start_xmit. No need to add additional overhead by
> 		 * locking again. These checks are worth it because
> 		 * even uncongested locks can be quite expensive.
> 		 * The driver can do trylock like here too, in case
> 		 * of lock congestion it should return -1 and the packet
> 		 * will be requeued.
> 		 */
> 		if (!nolock) {
> 			if (!spin_trylock(&dev->xmit_lock)) {
> 			collision:
> 				/* So, someone grabbed the driver. */
> 				
> 				/* It may be transient configuration error,
> 				   when hard_start_xmit() recurses. We detect
> 				   it by checking xmit owner and drop the
> 				   packet when deadloop is detected.
> 				*/
> 				if (dev->xmit_lock_owner == smp_processor_id()) {
> 					kfree_skb(skb);
> 					if (net_ratelimit())
> 						printk(KERN_DEBUG "Dead loop on netdevice %s, fix it urgently!\n", dev->name);
> 					return -1;
> 				}
> 				__get_cpu_var(netdev_rx_stat).cpu_collision++;
> 				goto requeue;
> 			}
> 			/* Remember that the driver is grabbed by us. */
> 			dev->xmit_lock_owner = smp_processor_id();
> 		}
> 		
> 		{
> 			/* And release queue */
> 			spin_unlock(&dev->queue_lock);
> 
> 			if (!netif_queue_stopped(dev)) {
> 				int ret;
> 				if (netdev_nit)
> 					dev_queue_xmit_nit(skb, dev);
> 
> 				ret = dev->hard_start_xmit(skb, dev);
> 				if (ret == NETDEV_TX_OK) { 
> 					if (!nolock) {
> 						dev->xmit_lock_owner = -1;
> 						spin_unlock(&dev->xmit_lock);
> 					}
> 					spin_lock(&dev->queue_lock);
> 					return -1;
> 				}
> 				if (ret == NETDEV_TX_LOCKED && nolock) {
> 					spin_lock(&dev->queue_lock);
> 					goto collision; 
> 				}
> 			}
> 
> 			/* NETDEV_TX_BUSY - we need to requeue */
> 			/* Release the driver */
> 			if (!nolock) { 
> 				dev->xmit_lock_owner = -1;
> 				spin_unlock(&dev->xmit_lock);
> 			} 
> 			spin_lock(&dev->queue_lock);
> 			q = dev->qdisc;
> 		}

-- 
Mathematics is the supreme nostalgia of our time.