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. > 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. > 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. > 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. > 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. > - 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. > >> 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. > 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. (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 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 impossible, and when it happens we are forced to leave interrupts disabled during the entire giveback.) 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. 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
