On Thu, 2013-06-13 at 17:09 -0400, Alan Stern wrote:

On Thu, 13 Jun 2013, Steven Rostedt wrote:

quoted

On Thu, 2013-06-13 at 15:41 -0400, Alan Stern wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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