Re: [RFC PATCH net-next 00/11] net: remove disable_irq() from ->ndo_poll_controller

[Sabrina Dubroca <sd@queasysnail.net>]

Thomas, ping?

thread is over there if you need it:
https://marc.info/?l=linux-netdev&m=141833435000554&w=2

2015-01-05, 15:31:26 +0100, Sabrina Dubroca wrote:
> 2014-12-12, 23:01:28 +0100, Thomas Gleixner wrote:
> > On Thu, 11 Dec 2014, Sabrina Dubroca wrote:
> > > 2014-12-09, 21:44:33 -0500, David Miller wrote:
> > > > 
> > > > Adding a new spinlock to every interrupt service routine is
> > > > simply a non-starter.
> > > > 
> > > > You will certainly have to find a way to fix this in a way
> > > > that doesn't involve adding any new overhead to the normal
> > > > operational paths of these drivers.
> > > 
> > > Okay. Here is another idea.
> > > 
> > > Since the issue is with the wait_event() part of synchronize_irq(),
> > > and it only takes care of threaded handlers, maybe we could try not
> > > waiting for threaded handlers.
> > > 
> > > Introduce disable_irq_nosleep() that returns true if it successfully
> > > synchronized against all handlers (there was no threaded handler
> > > running), false if it left some threads running.  And in
> > > ->ndo_poll_controller, only call the interrupt handler if
> > > synchronization was successful.
> > > 
> > > Both users of the poll controllers retry their action (alloc/xmit an
> > > skb) several times, with calls to the device's poll controller between
> > > attempts.  And hopefully, if the first attempt fails, we will still
> > > manage to get through?
> > 
> > Hopefully is not a good starting point. Is the poll controller
> > definitely retrying? Otherwise you might end up with the following:
> > 
> > Interrupt line is shared between your network device and a
> > device which requested a threaded interrupt handler.
> > 
> >   CPU0	       		   	    CPU1
> >   interrupt()
> >     your_device_handler()
> >       return NONE;
> >     shared_device_handler()
> >       return WAKE_THREAD;
> >       --> atomic_inc(threads_active);
> > 				    poll()
> > 				      disable_irq_nosleep()
> > 					sync_hardirq()
> > 					return atomic_read(threads_active);
> > 
> > So if you do not have a reliable retry then you might just go into a
> > stale state. And this can happen if the interrupt type is edge because
> > we do not disable the interrupt when we wakeup the thread for obvious
> > reasons.
> 
> We do have loops retrying to run the netpoll controller, and trying to
> do the work even if the controller doesn't help.  And by hopefully I
> mean: even if we fail, we tried our best and netpoll isn't 100%
> reliable.
> 
> 
> static struct sk_buff *find_skb(struct netpoll *np, int len, int reserve)
> {
> 	...
> repeat:
> 
> 	skb = alloc_skb(len, GFP_ATOMIC);
> 	if (!skb)
> 		skb = skb_dequeue(&skb_pool);
> 
> 	if (!skb) {
> 		if (++count < 10) {
> 			netpoll_poll_dev(np->dev);
> 			goto repeat;
> 		}
> 		return NULL;
> 	}
> 
> 	...
> }
> 
> void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
> 			     struct net_device *dev)
> {
> 	...
> 
> 		/* try until next clock tick */
> 		for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
> 		     tries > 0; --tries) {
> 			if (HARD_TX_TRYLOCK(dev, txq)) {
> 				if (!netif_xmit_stopped(txq))
> 					status = netpoll_start_xmit(skb, dev, txq);
> 
> 				HARD_TX_UNLOCK(dev, txq);
> 
> 				if (status == NETDEV_TX_OK)
> 					break;
> 
> 			}
> 
> 			/* tickle device maybe there is some cleanup */
> 			netpoll_poll_dev(np->dev);
> 
> 			udelay(USEC_PER_POLL);
> 		}
> 
> 	...
> }
> 
> 
> 
> > Aside of that I think that something like this is a reasonable
> > approach to the problem.
> > 
> > The only other nitpicks I have are:
> > 
> >     - The name of the function sucks, though my tired braain can't
> >       come up with something reasonable right now
> 
> I couldn't think of anything better.  Maybe 'disable_irq_trysync' or
> 'disable_irq_hardsync'?
> 
> Or maybe you prefer something that works like spin_trylock, and
> reenables the irq before returning if we can't sync?  Maybe the risk
> of abuse would be a bit lower this way?
> 
> I made synchronize_irq_nosleep static, but maybe it should be
> EXPORT_SYMBOL'ed as well.  I didn't need that for e1000, but that
> would be more consistent.
> 
> 
> >     - The lack of extensive documentation how this interface is
> >       supposed to be used and the pitfals of abusage, both in the
> >       function documentation and the changelog.
> > 
> >       Merlily copying the existing documentation of the other
> >       interface is not sufficient.
> 
> 
> Yes, my email wasn't really a changelog, just a description and RFC.
> 
> 
> Modified documentation:
> 
> -----
> disable_irq_nosleep - disable an irq and wait for completion of hard IRQ handlers
> @irq: Interrupt to disable
> 
> Disable the selected interrupt line.  Enables and Disables are
> nested.
> This function does not sleep, and is safe to call in atomic context.
> 
> This function waits for any pending hard IRQ handlers for this
> interrupt to complete before returning. If you use this
> function while holding a resource the IRQ handler may need you
> will deadlock.
> 
> This function does not wait for threaded IRQ handlers.
> Returns true if synchronized, false if there are threaded
> handlers pending.
> 
> If false is returned, the caller must assume that synchronization
> didn't occur, and that it is NOT safe to proceed.
> The caller MUST reenable the interrupt by calling enable_irq in all
> cases.
> 
> This function may be called - with care - from IRQ context.
> -----
> 
> 
> Thanks.
> 
> --
> Sabrina
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html