On Wed, Aug 21, 2013 at 11:23 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 21 Aug 2013, Ming Lei wrote:
>
>> > By the way, even though it's a little late to ask this... Why did you
>> > decide to move only the giveback routine into a tasklet, instead of
>> > moving the entire interrupt handler?
>>
>> Looks below reasons in my mind before preparing the tasklet patch:
>>
>> 1, from my observation, on ARM, the most time-consuming part is dma
>> mapping, unmapping and data copy over coherent memory, so moving
>> giveback out of interrupt handler can decrease USB irq handling time a lot.
>
> Of course, this would remain true either way.
>
>> 2, moving giveback out of irq handler can be done in usbcore, so changes
>> can be minimized.
>
> This seems to be the most important factor. When you think about it,
> though, does it really minimize the changes? Consider all the other
> adjustments we had to make to ehci-hcd: the interrupt QH unlink change
> and the isochronous stream stuff (which also affects the usb-audio
> drivers).
For other HCDs, this changes on interrupt QH unlink may not need at all
if there is no much cost about relink.
About isochronous stream stuff, the only change with moving giveback
in tasklet is that iso_stream_schedule() may see list_empty(&stream->td_list)
when underrun, and we can solve it easily, can't we?
Also I remembered that you said the isochronous stream stuff needn't to
consider on other HCDs.
So maybe the change on ehci HCD is a bit much, but for other HCDs,
the change is just a little.
Another good point with moving giveback only to tasklet is deadlock
immune during resubmissions, as you mentioned in below link:
http://www.spinics.net/lists/linux-usb/msg88710.html
>
> I'm starting to think that moving the entire handler to a tasklet would
> have been better.
Not sure, if so:
- one thing is that the HCD private lock can't be held in interrupt handler any
more, so new lock need to introduce, not mention each hard irq handler need to
split to two parts.
- also it should have been better to avoid resubmissions in its
giveback handler.
>
>> 3, driver's complete() may do many driver specific things which may increase
>> irq handling time randomly, so moving complete() to tasklet can help to
>> decrease HCD irq handling time.
>
> This is like the first reason.
>
>> Also moving only the giveback routine into a tasklet can avoid dropping
>> HCD private lock during irq handler, which may simplify HCD code, and
>> I have figured out ehci cleanup patches for this.
>
> I don't think we should make these changes. It's okay to keep the
> private lock during the giveback call, but let's not remove the other
> things.
>
> My feeling is that at some point we may indeed want to move the entire
> handler to a tasklet or a threaded IRQ. If that happens, all those
> simplifications would need to be undone. Better not to do them in the
> first place, since they add very little overhead.
I think making HCDs simpler should have been welcome, :-)
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
