On Fri, Aug 23, 2013 at 11:40 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > 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. Are you sure? If the change is absolutely required, there is certainly bug in them, since HCDs still need to support submitting interrupt URB in tasklet scheduled by its complete() handler? > >> 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. Just take a quick look at UHCI code, looks there is no much difference about unlinking qh between async qh and intr qh, which means the change might not need for uhci since usb-storage is common case to support. Correct me if it is wrong. > >> >> 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. Could you explain it in detail? I mean we only discuss the change on isoc schedule in case of underrun when giveback in tasklet. > >> 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 list_empty(&stream->td_list) is already checked in iso_stream_schedule(), so I am wondering what is the 'wrong' you mean. We need to know if the empty endpoint queue is caused by the last completed URB in queue, which will be resubmitted later. > has emptied out. What happens if the completion handler for URB0 > submits URB1? Do you have such example? It is possible for the two URBs which belong to different endpoint and let one of URBs to start the stream, but I am wondering it is reasonable that the two belong to same endpoint? Even for this case, we can handle it easily by checking if the two URBs belong to same endpoint. > > 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! percpu variable should be ok since no preemption is possible during completing? and no irq handler may operate the variable. > >> >> 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. Sorry, I don't understand your point, as I said, the only change on ISOC schedule introduced with running giveback in tasklet is that iso_stream_schedule() may see list_empty(&stream->td_list) when underrun, and the above discussed patch should address this one. If you mean the delay introduced by tasklet, I don't think there is no irq handling delay. If you mean other things, could you explain in a bit detail please? > >> >> - 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.) I have commented on this memory barrier, and it might not be enough. > >> 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. See my comment about clearing USBSTS, and your change violates EHCI spec. > >> >> - 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. No, with my patches, about 50 line codes can be saved. Also it may be not related code decrease only, it is about race-killing, and you know people always hate races, :-) > >> 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. Yes, imagine other HCDs still need to deal with similar & complicated things, so why don't we deal with most of things in usb core like only giveback in tasklet patches? >> > 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. It isn't only related with period, and one shot may handle dozens of URBs or more, and the number depends on bus bandwidth and average URB size too. > >> 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. Right. > >> - 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. Yes, but it is implemented in each HCD. > >> Suppose URB0 and URB1 belong to same endpoint, so their completion >> order is reversed. > > That can't happen. For ehci HCD example, ehci->need_rescan is set and returns when HCD is scanning, so the problem is avoided, but it is implemented in HCD code, and other HCDs may need to solve the same problem with similar trick too.(UHCI uses similar trick) I mean we can do it in usb core like giveback in tasklet patches, so HCDs won't worry about the problem any more, isn't it a simplification for HCDs? >> 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. OK. 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
