lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150205002059.GA28282@kria> Date: Thu, 5 Feb 2015 01:20:59 +0100 From: Sabrina Dubroca <sd@...asysnail.net> To: Thomas Gleixner <tglx@...utronix.de> Cc: David Miller <davem@...emloft.net>, netdev@...r.kernel.org, peterz@...radead.org Subject: Re: [RFC PATCH net-next 00/11] net: remove disable_irq() from ->ndo_poll_controller 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@...r.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists