On Fri, 23 Aug 2013, Ming Lei wrote: > 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? I haven't checked in detail. It seems likely that uhci-hcd and ohci-hcd will need changes. > 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. Wrong: For other HCDs, unlinks _do_ need to wait for the hardware. > >> 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. That's not true, at least, not for ehci-hcd. > OK, I give you a simple solution in which only one line change is > needed for these HCDs, see attachment patch. The patch is wrong. It keeps track of whether the URB is being resubmitted by the completion handler, not whether the endpoint queue has emptied out. What happens if the completion handler for URB0 submits URB1? To do it properly, you would need to count the number of URBs on the endpoint queue. The count would be decremented after the completion handler returned, which means it probably would have to be an atomic_t variable -- and I know that you hate those! > >> 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 Right. We don't have to change the isochronous code in _every_ HCD, but we do need to change it in _some_ of them. > >> 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. No, it isn't. See above. Besides, even if your patch was correct, it still wouldn't be sufficient. ehci-hcd relies on the fact that the entries in a non-empty isochronous queue expire on or after ehci->last_iso_frame. With your patch, that wouldn't be true any more. > >> - 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, It's a producer-consumer relationship. The IRQ handler produces events and the tasklet consumes them. It's well known that this sort of thing doesn't require locks. (It did require one memory barrier, though.) > 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. No, it doesn't. > >> - 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 Each of those uses about three lines of code. And you're wrong about qh->exception; that one is needed (or will be needed) for other reasons. > Doesn't it make HCD simpler? No, not very much. A little bit. > > (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. No, hrtimers run in hardirq context. Try replacing the "!in_irq()" expression in the 2/3 patch with "true" and see what happens. I crashed my computer a few times before I realized what the problem was. It turns out that what I did in the RFC wasn't a valid solution, though. It will be necessary to make sure that givebacks never happen in an unlink call or timer handler; the tasklet will have to take care of them instead. That will complicate the patch, unfortunately. > > 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, It's still rare. The watchdog timer has a period of 100 ms. It will complete only a very small fraction of all URBs. > also below problem might be caused: > > - HCD irq comes and URB0 will be scheduled to complete in tasklet The tasklet doesn't schedule URBs to complete -- it actually completes them. Or maybe you meant that the IRQ handler will schedule the URBs to be completed by the tasklet? It won't; the IRQ handler merely schedules the tasklet to run. > - 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 If the tasklet hasn't completed URB0 yet, then the I/O watchdog path will complete it before moving on to URB1. > Suppose URB0 and URB1 belong to same endpoint, so their completion > order is reversed. That can't happen. > With giveback URB in tasklet, usbcore will solve the race generally, > and each HCD needn't worry about them, that is another simplification. This isn't a race. 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
