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

[Steven Rostedt]

On Thu, 2013-06-13 at 17:09 -0400, Alan Stern 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.

Hey, I'll admit that I don't understand how USB works ;-)

I also only looked at the second patch without applying it. Thus, a lot
of the changes were out of context for me.


>   Most of the time, sched will be 0 
> here and hence the work queue won't be involved.

OK, I only compared that current tasklets were being commented out, and
that's pretty much all I had to go on.

> 
> 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.

Got it.

> 
> > 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.

I only took a quick look at the second patch. I'm now looking at both
patches applied to the code. I didn't realize this was called from the
top half.

Usually the top half for threaded interrupts is used just to quite the
interrupt line. Either by acknowledging the interrupt or by disabling
the device from sending more interrupts till the bottom half (thread)
can run. This looks to be doing a bit more than that.

I'll look a bit deeper at the patch, but this still doesn't look like a
typical threaded interrupt usage.

-- Steve