On Fri, Aug 23, 2013 at 4:36 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 22 Aug 2013, Ming Lei wrote:
>
>> > 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.
>
> But for some HCDs, it will be needed.
Which HCDs need them?
Even for EHCI HCD, the interrupt QH unlink change isn't necessary, and
the patch is only for improving the situation, isn't it? For other HCDs,
basically unlink interrupt queue need't the wait time, so don't need the change.
>
>> 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?
>
> No, it's not so easy. I have done some work on it, and it's more
> complicated than you might think. Not to mention that the
> snd-usb-audio driver (and perhaps others) will also need to be
> modified.
As I said, the only change introduced with running giveback in tasklet
is that iso_stream_schedule() may see list_empty(&stream->td_list)
when underrun, and we can handle it inside HCD and usbcore, nothing
to do with drivers.
OK, I give you a simple solution in which only one line change is
needed for these HCDs, see attachment patch.
>
>> Also I remembered that you said the isochronous stream stuff needn't to
>> consider on other HCDs.
>
> No, I didn't say that. It will have to be considered for ohci-hcd and
> uhci-hcd.
Maybe I understand it incorrectly:
We also don't have to change the isochronous code in every HCD
http://www.spinics.net/lists/linux-usb/msg89826.html
>
>> So maybe the change on ehci HCD is a bit much, but for other HCDs,
>> the change is just a little.
>
> The other HCDs will have to change too. Maybe not quite as much as
> ehci-hcd, but more than you seem to think.
Per my solution, only one line change for the affected HCD is enough.
>
>> 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
>
> That referred to giveback during submissions. It turns out that they
> aren't necessary in any case, although I didn't realize it at the time.
>
>> > 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.
>
> No new lock is needed -- the interrupt handler runs okay without one.
> Yes, the handler has to be split into two parts. But that's small and
> self-contained, and very few other changes are needed.
How can you make sure the lockless hw operations in hard interrupt handler
won't cause race with all other hw operations on host controller, anyway,
previously hcd lock is held to operate on host controller, and no such race.
We should be careful since the change might depends on each
hardware internal things.
>
>> - also it should have been better to avoid resubmissions in its
>> giveback handler.
>
> Why? We are all set up to allow them now. Ruling them out won't make
> things much easier than they are already.
Only for one example, we needn't consider race between qh unlink and
completion inside HCD anymore, so for ehci, the below variable and related
code can be removed:
ehci->async_unlinking
ehci->intr_unlinking
qh->exception
QH_STATE_COMPLETING
Doesn't it make HCD simpler?
>
>> >> 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, :-)
>
> But it isn't simpler! Look at the extra code you had to add to
> ehci-hcd. It is now more complicated than it used to be, and the
> isochronous changes will make it even more so. Ruling out submissions
> during givebacks will make very little difference.
See above.
>
>> Also one potential big problem of moving entire handler to tasklet is the
>> hardware race, previously the entire hander is run with interrupt disabled
>> and the big HCD private lock held, but after the conversion, both are not
>> true, so each HCD change for this conversion might be big.
>>
>> If only moving giveback into tasklet, there is no such problem.
>
> I don't understand what you mean. In any case, the necessary
> conversion is not big. I wrote it (for ehci-hcd) today. I will post
> it as a series of 3 RFCs following this email, so you can see how
> straightforward it is. The patches apply to a kernel with none of the
> existing tasklet changes.
OK, I will give my comments on these patches.
>
> (There is one awkward aspect. It turns out that ehci_urb_done() can
> sometimes be called in hardirq context. This can happen when the I/O
timer handler is run in softirq context.
> watchdog hrtimer routine completes an URB, or when an URB is unlinked
> while waiting for a Clear-TT-Buffer request. It's rare, but not
It may not be rare, for some buggy controller, URBs are often called
in io watchdog timer handler, also below problem might be caused:
- HCD irq comes and URB0 will be scheduled to complete in tasklet
- HCD lock is released before URB0 completion inside tasklet path
- io watchdog expired and the HCD lock is held
- URB1 completion handler completed inside io watchdog path
Suppose URB0 and URB1 belong to same endpoint, so their completion
order is reversed.
With giveback URB in tasklet, usbcore will solve the race generally,
and each HCD needn't worry about them, that is another simplification.
> impossible, and when it happens we are forced to leave interrupts
> disabled during the entire giveback.)
It isn't good to disable interrupt for a bit long randomly.
>
> Would you like to try testing these patches? I don't many high-speed
> devices here right now, but you can compare the performance with the
> measurements you made earlier.
I have some comments on these patches before the test, :-)
Thanks,
--
Ming Lei
Attachment:
tracing-compleing-urb.patch
Description: Binary data
