To begin with, the whole point of this RFC was to show that moving the entire IRQ handler (or even a large part of it) to a tasklet would have been at least as simple as moving the givebacks alone. Now that I realize the hrtimer and unlink pathways would have to be changed too, it's not quite so simple as it seemed at first. Still, I think it would be no worse than the work you did. It also is more typical of the way people expect the work to be divided between a top half and a bottom half -- usually the top half does little more than acknowledge the interrupt. Since the proposal was meant only as a proof of principle, I'm not going to develope it any farther. But I will respond to the points you raised in your review. On Sat, 24 Aug 2013, Ming Lei wrote: > But the current interrupt from EHCI isn't be acked(USBSTS isn't cleared), > so EHCI HW don't know the irq has been handled, then it is reasonable > that the EHCI interrupt still interrupts CPU. > > EHCI spec(4.15) also states the point clearly: > > the host controller is not required to de-assert a currently active > interrupt condition when software sets the interrupt enables (in the > USBINR register, see Section 2.3.3) to a zero. The only reliable > method software should use for acknowledging an interrupt is by > transitioning the appropriate status bits in the USBSTS register > (Section 2.3.2) from a one to a zero. Ooh, you're right! I had completely forgotten about this. It's a serious error in the RFC patch. Fixing it would be pretty easy, however. > > Not handling interrupts right away is the price we pay for using a > > bottom half. Your tasklet implementation isn't very different. > > Although the interrupt handler may realize quickly that a transfer has > > finished, there will still be a delay before the URB's completion > > handler can run. And the length of that delay will depend on the > > tasklet schedule delay. > > In my tasklet implementation, the next EHCI interrupt can be handled > after the current hard interrupt is handled, but in this patch the EHCI > hard irq can't be handled until the tasklet is handled, right? Right. But in the end, it doesn't matter. Remember, more than 99% of the work an HCD does is handling URBs. (HCDs do a couple of other things, such as detecting connections and disconnection or handling wakeups, but they are very small compared to the number of URBs it handles.) Consider also that an URB isn't done until it is given back to the driver that submitted it. Therefore, when measuring performance or latency of an HCD, what matters is how long the driver has to wait from URB submission to URB completion. If any part of that processing takes place in a tasklet, the tasklet delay will add in to the total time. It doesn't matter whether the tasklet handles only the giveback or some of the HCD's internal work as well. > >> Thirdly, fatal error should have been handled immediately inside hard > >> interrupt handler. > > > > Why? What difference does it make? > > At least, EHCI HW generates the fatal irq ASAP, "This interrupt is not > delayed to the next interrupt threshold."(4.15.2.4), so it is reasonable for > software to handle it asap. That doesn't seem like a good reason. (Probably it just reflects the fact that once a fatal error has occurred, the controller hardware can't reliably determine when the next interrupt threshold will occur.) But of course, it would be easy to put that part of the code into the top half of the handler. > >> Finally, tasklet schedule should have been optimized here if the tasklet > >> is running on another CPU, otherwise the current CPU may spin on > >> the tasklet lock until the same tasklet is completed on another CPU. > > > > That could be added in. But doesn't the core kernel's tasklet > > implementation already take care of this? The tasklet_hi_action() > > routine uses tasklet_trylock(), so it doesn't spin. > > OK, but extra tasklet schedule delay might be caused. Like I said, this feature also could be added easily. > > There's no point in enabling interrupts before they can be handled. As > > long as the tasklet is running, it doesn't do any good to receive more > > IRQs. > > Why doesn't any good to receive more IRWQs? And there should be > other interrupts(non transfer complete irq, such as port change, fatal > error, ...) > comes. There's no reason to handle those events any more quickly than ordinary completion IRQs. Of course, the code _could_ be structured to leave interrupts unmasked while the tasklet runs. The tasklet would end up doing the same amount of work; the only difference would be that more interrupts occur while the tasklet runs, thereby wasting CPU time. To put it another way, which would you prefer: To have three interrupts while the tasklet is running, or one interrupt as soon as it finishes? > >> Also if the latest value of irqs_are_masked isn't see by ehci_irq > >> immediately, it will cause CPU interrupted by extra and unnecessary > >> irq signal. > >> > >> > + smp_wmb(); /* Make sure ehci_irq() sees that assignment */ > > > > That's why I put in this memory barrier. It guarantees that > > irqs_are_masked _will_ be seen by ehci_irq. > > memory barrier only orders memory load/store, and it can't > make sure CPU always sees the latest value, also what is > the two memory load/store to order with the smp_wmb()? And > where is the smp_rmb() pair? Whenever the CPU gets interrupted, the main system-level interrupt handler executes at least one memory barrier. That's what pairs with the barrier above. Thus: The smp_wmb() forces the irqs_are_masked assignment to be ordered before the write to the intr_enable register. If an interrupt then occurs, the memory barrier in the system's interrupt code will cause the assignment to visible to the interrupt service routine. On the other hand, I realized later that ehci_irq() can sometimes be called in other contexts, not from within a hardirq. This means the smp_wmb is insufficient after all, and a spinlock is indeed needed. > The same problem might exist for reading/writing 'irqs_are_masked' > in ehci_irq: the flag is set in ehci_irq() on CPU0, but it might not be > seen in another ehci_irq() on CPU1. An interrupt can't occur on CPU1 until the handler on CPU0 returns. The memory barriers in the system-level interrupt routines mean that everything done on CPU0 before the handler returns would be visible to the handler on CPU1. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
