On Sun, Aug 25, 2013 at 10:45 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.
I'd like to compare the two implementations when it is 'basically ready'.
>
> 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.
OK, I will ask Greg to put back the patch 'USB: EHCI: disable
IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled'
into his tree.
>
> 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.
It is hard to say easy:
- the USBSTS has to be cleared in top half, which means the status has
to be stored somewhere(suppose it is ehci->irq_status)
- ehci->irq_status need to be read in bottom half for handling irq, so one
lock might be required for synchronizing the access on the variable
- also, once the current irq is Acked by clearing USBSTS, then later
interrupts can come, so the irq status should have been saved into one
queue instead of single variable(see below why disabling ehci irq isn't good)
So when the above implementation is required in each HCDs
change, I am wondering if it is 'pretty easy'.
>
>> > 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.
I am not sure if the idea of disabling EHCI irq inside ehci_irq()
is good: one completed transfer may not be observed inside current
tasklet handler since hardware interrupt is disabled, so the transfer
can't be handled until next tasklet scheduled, then extra delay for
these URBs is introduced.
>
>> >> 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.
More things done in top half, the change will become more complicated
since more synchronizations need to consider.
>
>> >> 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.
Maybe not, see above.
>
>> > 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.
At least for 'giveback URB in tasklet' patch, no CPU time is wasted
when enabling irq during tasklet running.
> 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?
I prefer to enabling EHCI interrupt during tasklet handling.
>> >> 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.
OK, got it, thanks for your explanation, but I am afraid that intr_enable(0)
shouldn't have been done inside ehci_irq.
>
> 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.
I understand it might still depend on irqs_are_masked assignment to be
ordered before the write to the intr_enable register, but not true for
shared interrupts.
Suppose there are two barriers around irq handler, so
CPU0 CPU1
smp_mb()
read A
write A
smp_mb()
smp_mb()
read A
write A
smp_mb()
Looks we can't make sure 'read A' on CPU1 may observe
the latest value of A updated in CPU0, if there isn't one 'write C'
after the 2nd barrier on CPU0 and one 'read C' before the
1st barrier on CPU1.
Even I am not sure if there are implicit memory barriers inside system
irq handling path since Documentation/memory-barriers.txt doesn't
mention the point.
Thanks,
--
Ming Lei
--
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
