[RFC PATCH 0/4] USB: HCD/EHCI: giveback of URB in tasklet context

[Ming Lei]

On Fri, Jun 14, 2013 at 5:09 AM, Alan Stern <stern at rowland.harvard.edu> wrote:
> On Thu, 13 Jun 2013, Steven Rostedt wrote:
>
>> On Thu, 2013-06-13 at 15:41 -0400, Alan Stern wrote:
>>
>> > The test results above show a 2.4% degradation for threaded interrupts
>> > as compared to tasklets.  That's in addition to the bottlenecks caused
>> > by the device; no doubt it would be worse for a faster device.  This
>> > result calls into question the benefits of threaded interrupts.
>>
>> That's because it was written like a top half and not a full blown
>> interrupt. I just looked at the patch, and saw this:
>>
>> +#ifndef USB_HCD_THREADED_IRQ
>>         if (sched) {
>>                 if (async)
>>                         tasklet_schedule(&bh->bh);
>>                 else
>>                         tasklet_hi_schedule(&bh->bh);
>>         }
>> +#else
>> +       if (sched)
>> +               schedule_work(&hcd->periodic_bh->work);
>> +#endif
>>
>> What is this? The work isn't done by an interrupt thread, but by work queues!
>
> You don't understand the patch.  Most of the time, sched will be 0
> here and hence the work queue won't be involved.
>
> Yes, part of the work is done by a work queue rather than the interrupt
> thread.  But it is an unimportant part, the part that involves
> transfers to root hubs or transfers that were cancelled.  These things
> can complete without any interrupt occurring, so they can't be handled
> by the interrupt thread.  However, they are the abnormal case; the
> transfers we care about are not to root hubs and they do complete
> normally.
>
>> The point of the interrupt thread is that you do *all* the work that
>> needs to be done when an interrupt comes in. You don't need to delay the
>> work.
>
> You've got it backward.  The patch doesn't leave part of the work
> undone when an interrupt occurs.  Rather it's the other way around --
> sometimes work needs to be done when there isn't any interrupt.  This
> could happen in a timer callback, or it could happen as a direct result
> of a function call.
>
> Since there doesn't seem to be any way to invoke the interrupt thread
> in the absence of an interrupt, Ming pushed the job off to a work
> queue.
>
>> If you just treat a threaded interrupt like a real interrupt and push
>> off work to something else, then yes, it will degrade performance.
>>
>> If you go the threaded interrupt route, you need to rethink the
>> paradigm. There's no reason that the interrupt handler needs to be fast
>> like it needs to be in true interrupt context. The handler can now use
>> mutexes, and other full features that currently only threads benefit
>> from. It should improve locking issues, and can serialize things if
>> needed.
>>
>> All this patch did was to switch the main irq to a thread and make a
>> bottom half into a work queue.
>
> In case it's not clear, the code you quoted above is part of the
> interrupt handler, not part of the thread.
>
>> Why couldn't you just do:
>>
>>       if (sched)
>>               usb_giveback_urb_bh(bh);
>>
>> ?
>
> Because usb_giveback_urb_bh() is supposed to run in the context of the
> tasklet or interrupt thread or work queue, not in the context of the
> interrupt handler.

Exactly, the code should have been the below shape:

        #ifndef USB_HCD_THREADED_IRQ
              ......
        #else
               if (!in_irq()) {
                      bh = hcd->periodic_bh;
                      sched = 1;
              }
        #endif

Then it will be quite clear.

Thanks,
--
Ming Lei