USB: HCD/EHCI: giveback of URB in tasklet context

This patch implements the mechanism of giveback of URB in tasklet context, so that hardware interrupt handling time for usb host controller can be saved much, and HCD interrupt handling can be simplified.

Motivations:

1), on some arch(such as ARM), DMA mapping/unmapping is a bit time-consuming, for example: when accessing usb mass storage via EHCI on pandaboard, the common length of transfer buffer is 120KB, the time consumed on DMA unmapping may reach hundreds of microseconds; even on A15 based box, the time is still about scores of microseconds

2), on some arch, reading DMA coherent memoery is very time-consuming, the most common example is usb video class driver[1]

3), driver's complete() callback may do much things which is driver specific, so the time is consumed unnecessarily in hardware irq context.

4), running driver's complete() callback in hardware irq context causes that host controller driver has to release its lock in interrupt handler, so reacquiring the lock after return may busy wait a while and increase interrupt handling time. More seriously, releasing the HCD lock makes HCD becoming quite complicated to deal with introduced races.

So the patch proposes to run giveback of URB in tasklet context, then time consumed in HCD irq handling doesn't depend on drivers' complete and DMA mapping/unmapping any more, also we can simplify HCD since the HCD lock isn't needed to be released during irq handling.

The patch should be reasonable and doable:

1), for drivers, they don't care if the complete() is called in hard irq context or softirq context

2), the biggest change is the situation in which usb_submit_urb() is called in complete() callback, so the introduced tasklet schedule delay might be a con, but it shouldn't be a big deal:

- control/bulk asynchronous transfer isn't sensitive to schedule delay

- the patch schedules giveback of periodic URBs using tasklet_hi_schedule, so the introduced delay should be very small

- use percpu tasklset for each HCD to schedule giveback of URB, which are running as much as parallel

- for ISOC transfer, generally, drivers submit several URBs concurrently to avoid interrupt delay, so it is OK with the little schedule delay.

- for interrupt transfer, generally, drivers only submit one URB at the same time, but interrupt transfer is often used in event report, polling, ... situations, and a little delay should be OK.

Quoted 4 lines · Ming Lei

This patch implements the mechanism of giveback of URB in tasklet context, so that hardware interrupt handling time for usb host controller can be saved much, and HCD interrupt handling can be simplified.

Why do you want to minimize the hardware interrupt handling time? The total interrupt handling time will actually be increased by your change; the only advantage is that much of the work will not be in hardirq context.

Also, I suspect that this will make HCD interrupt handling _more_ complicated, not less.

Quoted 8 lines

Motivations:

1), on some arch(such as ARM), DMA mapping/unmapping is a bit time-consuming, for example: when accessing usb mass storage via EHCI on pandaboard, the common length of transfer buffer is 120KB, the time consumed on DMA unmapping may reach hundreds of microseconds; even on A15 based box, the time is still about scores of microseconds

DMA mapping/unmapping will remain just as time-consuming as before. This patch won't change that.

Quoted 3 lines

2), on some arch, reading DMA coherent memoery is very time-consuming, the most common example is usb video class driver[1]

Reading DMA-coherent memory will remain just as time-consuming as before.

Quoted 3 lines

3), driver's complete() callback may do much things which is driver specific, so the time is consumed unnecessarily in hardware irq context.

With this patch, the time is consumed in softirq context. What is the advantage?

Quoted 6 lines

4), running driver's complete() callback in hardware irq context causes that host controller driver has to release its lock in interrupt handler, so reacquiring the lock after return may busy wait a while and increase interrupt handling time. More seriously, releasing the HCD lock makes HCD becoming quite complicated to deal with introduced races.

There is no choice; the lock _has_ to be released before giving back completed URBs. Therefore the races are unavoidable, as is the busy-wait.

Quoted 5 lines

So the patch proposes to run giveback of URB in tasklet context, then time consumed in HCD irq handling doesn't depend on drivers' complete and DMA mapping/unmapping any more, also we can simplify HCD since the HCD lock isn't needed to be released during irq handling.

I'm not convinced. In particular, I'm not convinced that this is better than using a threaded interrupt handler.

Quoted 5 lines

The patch should be reasonable and doable:

1), for drivers, they don't care if the complete() is called in hard irq context or softirq context

True.

Quoted 6 lines

2), the biggest change is the situation in which usb_submit_urb() is called in complete() callback, so the introduced tasklet schedule delay might be a con, but it shouldn't be a big deal:

- control/bulk asynchronous transfer isn't sensitive to schedule delay

Mass-storage device access (which uses bulk async transfers) certainly _is_ sensitive to scheduling delays.

Quoted 6 lines

- the patch schedules giveback of periodic URBs using tasklet_hi_schedule, so the introduced delay should be very small

- use percpu tasklset for each HCD to schedule giveback of URB, which are running as much as parallel

That might be an advantage; it depends on the work load. But if the tasklet ends up running on a different CPU from the task that submitted the URB originally, it would be less of an advantage.

Quoted 3 lines

- for ISOC transfer, generally, drivers submit several URBs concurrently to avoid interrupt delay, so it is OK with the little schedule delay.

This varies. Some drivers use very low overheads for isochronous transfers. A delay of even one millisecond might be too long for them.

Quoted 3 lines

- for interrupt transfer, generally, drivers only submit one URB at the same time, but interrupt transfer is often used in event report, polling, ... situations, and a little delay should be OK.

Agreed.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

2), the biggest change is the situation in which usb_submit_urb() is called in complete() callback, so the introduced tasklet schedule delay might be a con, but it shouldn't be a big deal:

- control/bulk asynchronous transfer isn't sensitive to schedule delay

Quoted 6 lines · On Sunday 09 June 2013 23:18:28 Ming Lei wrote:

2), the biggest change is the situation in which usb_submit_urb() is called in complete() callback, so the introduced tasklet schedule delay might be a con, but it shouldn't be a big deal:

- control/bulk asynchronous transfer isn't sensitive to schedule delay

That is debatable.Missing a frame boundary is expensive because the increased latency then translates into lower throughput.

Quoted 5 lines · Oliver Neukum

Quoted 4 lines · On Sunday 09 June 2013 23:18:28 Ming Lei wrote:

2), the biggest change is the situation in which usb_submit_urb() is called in complete() callback, so the introduced tasklet schedule delay might be a con, but it shouldn't be a big deal:

Quoted 5 lines

Quoted 2 lines

- control/bulk asynchronous transfer isn't sensitive to schedule delay

That is debatable.Missing a frame boundary is expensive because the increased latency then translates into lower throughput.

Suppose so, considered that bulk transfer will do large data block transfer, and the extra frame or uFrame doesn't matter over the whole transfer time.

Quoted 12 lines · On Monday 10 June 2013 17:23:46 Ming Lei wrote:

Quoted 10 lines · Oliver Neukum

Quoted 6 lines · On Sunday 09 June 2013 23:18:28 Ming Lei wrote:

2), the biggest change is the situation in which usb_submit_urb() is called in complete() callback, so the introduced tasklet schedule delay might be a con, but it shouldn't be a big deal:

- control/bulk asynchronous transfer isn't sensitive to schedule delay

That is debatable.Missing a frame boundary is expensive because the increased latency then translates into lower throughput.

Quoted 2 lines

Suppose so, considered that bulk transfer will do large data block transfer, and the extra frame or uFrame doesn't matter over the whole transfer time.

That is not true for all use cases. Networking looks vulnerable.

That is debatable.Missing a frame boundary is expensive because the increased latency then translates into lower throughput.

If HCD_BH is set for HC driver's flags, URB giveback will be done in tasklet context instead of interrupt context, so the ehci->lock needn't to be released any more before calling usb_hcd_giveback_urb().

Quoted 4 lines · Ming Lei

If HCD_BH is set for HC driver's flags, URB giveback will be done in tasklet context instead of interrupt context, so the ehci->lock needn't to be released any more before calling usb_hcd_giveback_urb().

Ah, this is the reason why you don't need to release the private lock. I'm not sure that this will save much time overall.

With the existing code, the main reason for lock contention would be if (for example) CPU 2 submitted or cancelled an URB while the giveback was occurring on CPU 1. Then CPU 1 would be forced to wait while CPU 2 finished its submission or cancellation.

With this patch, it's the other way around. CPU 2 would be forced to wait while CPU 1 does all the rest of the work in its interrupt handler. The total time spent holding the lock will be the same; it will just be distributed differently among the CPUs. Hence it is not at all clear that this change will save any time.

From below test results on 3 machines(2 ARM and one x86), time

Hi,

The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb().

Quoted 2 lines · Ming Lei

Hi,

Quoted 5 lines

The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb().

That's a lot of material. However, you haven't specifically said _what_ you want to accomplish. It sounds like you're trying to

minimize the amount of time spent in hardirq context, but I can't be sure that is really what you want.

If it is, this is not a good way to do it. A better way to minimize the time spent in hardirq context is to use a threaded interrupt handler. But why do you want to minimize this time in the first place?

The CPU will be still have to use at least as much time as before; the only difference is that it will be in softirq or in a high-priority thread instead of in hardirq.

Also, I don't see how you can avoid releasing the HCD's private lock before calling usb_hcd_giveback_urb(). When the completion handler

tries to resubmit the URB, the HCD's enqueue routine will try to reacquire the private lock. If it didn't release the lock earlier, it

Quoted 10 lines · Alan Stern

Quoted 7 lines · Ming Lei

Hi,

The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb().

Quoted 2 lines

That's a lot of material. However, you haven't specifically said _what_ you want to accomplish. It sounds like you're trying to

All IRQs are disabled(locally) in hard interrupt context, but they are enabled in softirq context, this patchset can decrease the time a lot.

Quoted 6 lines

minimize the amount of time spent in hardirq context, but I can't be sure that is really what you want.

If it is, this is not a good way to do it. A better way to minimize the time spent in hardirq context is to use a threaded interrupt handler. But why do you want to minimize this time in the first place?

Process context switch may have more cost than switching to softirq, then USB performance may be effected, that is why I choose to use tasklet. Also now there is no any sleep in URB giveback, so it isn't necessary to introduce process to handle the giveback or completion.

Quoted 3 lines

The CPU will be still have to use at least as much time as before; the only difference is that it will be in softirq or in a high-priority thread instead of in hardirq.

Yes, as I said above, but we can allow other IRQs to be served in handling URB giveback in softirq context, that is the value of softirq/tasklet. Of course, system may have more important things to do during the time. Generally speaking, for one driver, hard interrupt handler should always consume less time as possible.

[Thomas and Steve, please see the question below.]

On Mon, 10 Jun 2013, Ming Lei wrote:

That isn't clear. It is documented that USB completion handlers are called with local interrupts disabled. An IRQ arriving before the tasklet starts up might therefore be serviced during the short interval before the tasklet disables local interrupts, but if that happens it would mean that the completion handler would be delayed.

In effect, you are prioritizing other IRQs higher than USB completion handlers. Nevertheless, the total time spent with interrupts disabled will remain the same.

(There's one other side effect that perhaps you haven't considered. When multiple URBs are addressed to the same endpoint, their completion handlers are called in order of URB completion, which is the same as the order of URB submission unless an URB is cancelled. By delegating the completion handlers to tasklets, and particularly by using per-CPU tasklets, you may violate this behavior.)

Quoted 4 lines · On Monday 10 June 2013 10:03:00 Alan Stern wrote:

[Thomas and Steve, please see the question below.]

On Mon, 10 Jun 2013, Ming Lei wrote:

Quoted 5 lines

That isn't clear. It is documented that USB completion handlers are called with local interrupts disabled. An IRQ arriving before the tasklet starts up might therefore be serviced during the short interval before the tasklet disables local interrupts, but if that happens it would mean that the completion handler would be delayed.

That is what tasklets do by definition, isn't it?

Quoted 3 lines

In effect, you are prioritizing other IRQs higher than USB completion handlers. Nevertheless, the total time spent with interrupts disabled will remain the same.

It pobably slightly increases. You have colder caches twice. And potentially you swich CPUs.

[Thomas and Steve, please see the question below.]

Quoted 11 lines · Ming Lei

Quoted 10 lines · Alan Stern

Quoted 7 lines · Ming Lei

Hi,

The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb().

Quoted 5 lines

Quoted 2 lines

That's a lot of material. However, you haven't specifically said _what_ you want to accomplish. It sounds like you're trying to

All IRQs are disabled(locally) in hard interrupt context, but they are enabled in softirq context, this patchset can decrease the time a lot.

That isn't clear. It is documented that USB completion handlers are called with local interrupts disabled. An IRQ arriving before the

tasklet starts up might therefore be serviced during the short interval before the tasklet disables local interrupts, but if that happens it

would mean that the completion handler would be delayed.

In effect, you are prioritizing other IRQs higher than USB completion handlers.

Nevertheless, the total time spent with interrupts disabled will remain the same.

(There's one other side effect that perhaps you haven't considered. When multiple URBs are addressed to the same endpoint, their completion handlers are called in order of URB completion, which is the same as the order of URB submission unless an URB is cancelled. By delegating the completion handlers to tasklets, and particularly by using per-CPU tasklets, you may violate this behavior.)

Quoted 4 lines

Quoted 4 lines

minimize the amount of time spent in hardirq context, but I can't be sure that is really what you want.

Quoted 8 lines

Quoted 3 lines

If it is, this is not a good way to do it. A better way to minimize the time spent in hardirq context is to use a threaded interrupt handler. But why do you want to minimize this time in the first place?

Process context switch may have more cost than switching to softirq, then USB performance may be effected, that is why I choose to use tasklet. Also now there is no any sleep in URB giveback, so it isn't necessary to introduce process to handle the giveback or completion.

Thomas and Steve, can you comment on this? I do not have a good understanding of the relative benefits/drawbacks of tasklets vs. threaded interrupt handlers.

Quoted 4 lines

Quoted 3 lines

The CPU will be still have to use at least as much time as before; the only difference is that it will be in softirq or in a high-priority thread instead of in hardirq.

Quoted 5 lines

Yes, as I said above, but we can allow other IRQs to be served in handling URB giveback in softirq context, that is the value of softirq/tasklet. Of course, system may have more important things to do during the time. Generally speaking, for one driver, hard interrupt handler should always consume less time as possible.

As I understand it, the tradeoff between hardirq and softirq is generally unimportant. It does matter a lot in realtime settings --

but the RT kernel effectively makes _every_ interrupt handler threaded, so it won't benefit from this change.

Quoted 2 lines

That isn't clear. It is documented that USB completion handlers are called with local interrupts disabled. An IRQ arriving before the

Is there any good reason to do URB giveback with interrupt disabled?

IMO, disabling IRQs isn't necessary for completing URB since the complete() of URBs for same endpoint is reentrant.

Quoted 2 lines

tasklet starts up might therefore be serviced during the short interval before the tasklet disables local interrupts, but if that happens it

Tasklet doesn't disable local interrupts.

Quoted 1 line

would mean that the completion handler would be delayed.

Yes, the tasklet schedule delay is introduced to URB completion, but the delay doesn't have much side effect about completing URB.

For async transfer, generally, it should have no effect since there should have URBs queued on the qh queue before submitting URB.

Quoted 2 lines

In effect, you are prioritizing other IRQs higher than USB completion handlers.

Both other IRQs and HCD's IRQ are prioritizing the URB completion.

Quoted 2 lines

Nevertheless, the total time spent with interrupts disabled will remain the same.

No, the total time spent with interrupts disabled does not remain same, since no local IRQs are disabled during URB giveback anymore.

Quoted 6 lines

(There's one other side effect that perhaps you haven't considered. When multiple URBs are addressed to the same endpoint, their completion handlers are called in order of URB completion, which is the same as the order of URB submission unless an URB is cancelled. By delegating the completion handlers to tasklets, and particularly by using per-CPU tasklets, you may violate this behavior.)

Even though URB giveback is handled by hard interrupt handler, it still can't guarantee that 100%, please see below:

- interrupt for URB A comes in CPU 0, and handled, then HCD private lock is released, and usb_hcd_giveback_urb() is called to complete URB A

- interrupt for URB B comes in CPU 1 just before CPU0 releases HCD private lock, so CPU1 holds the private lock when CPU 0 releases the lock and handles the HCD interrupt.

So now just like two CPUs are racing, if CPU0 wins, URB A is completed first, otherwise URB B is completed first.

With introducing completing URB in percpu tasklet, the situation is just very similar.

On ehci hcd, it can make sure that by always letting CPU0 complete all URBs (use ehci->scanning/ehci->need_rescan flag), but the patch can change percpu tasklet to tasklet to meet the demand.

Quoted 2 lines

As I understand it, the tradeoff between hardirq and softirq is generally unimportant. It does matter a lot in realtime settings --

I don't think so, and obviously softirq allows IRQs to be served quickly, otherwise why is softirq and tasklet introduced? and why so many drivers are using tasklet/softirq?

Quoted 2 lines

I don't think so, and obviously softirq allows IRQs to be served quickly, otherwise why is softirq and tasklet introduced?

and why so many drivers

Quoted 1 line

are using tasklet/softirq?

Quoted 2 lines

and why so many drivers

Quoted 1 line

are using tasklet/softirq?

Because it's easy to set up and device driver authors don't know any better ;-). Note, a lot of drivers are now using work queues today, which run as a task.

Yes, there's a little more overhead with a task than for a softirq, but the problem with softirqs and tasklets is that they can't be preempted, and they are more important than all tasks on the system. If you have a task that is critical, it must yield for your softirq. Almost!

That is, even if you have a softirq, it may not run in irq context at all. If you get too many softirqs at a time (one comes while another one is running), it will defer the processing to the ksoftirq thread. This not only runs as a task, but also runs as SCHED_OTHER, and must yield to other tasks like everyone else too.

Thus, adding it as a softirq does not guarantee that it will be processed quickly. It just means that most of the time it will prevent anything else from happening while your "most important handler in the world" is running.

From this, it sounds like you generally advise using threaded interrupt

Quoted 18 lines · Steven Rostedt

Quoted 2 lines

and why so many drivers

Quoted 1 line

are using tasklet/softirq?

Because it's easy to set up and device driver authors don't know any better ;-). Note, a lot of drivers are now using work queues today, which run as a task.

Yes, there's a little more overhead with a task than for a softirq, but the problem with softirqs and tasklets is that they can't be preempted, and they are more important than all tasks on the system. If you have a task that is critical, it must yield for your softirq. Almost!

That is, even if you have a softirq, it may not run in irq context at all. If you get too many softirqs at a time (one comes while another one is running), it will defer the processing to the ksoftirq thread. This not only runs as a task, but also runs as SCHED_OTHER, and must yield to other tasks like everyone else too.

Quoted 6 lines

Thus, adding it as a softirq does not guarantee that it will be processed quickly. It just means that most of the time it will prevent anything else from happening while your "most important handler in the world" is running.

From this, it sounds like you generally advise using threaded interrupt

handlers rather than tasklets/softirqs.

Quoted 25 lines · Alan Stern

Quoted 22 lines · Steven Rostedt

Quoted 2 lines

and why so many drivers

Quoted 1 line

are using tasklet/softirq?

Because it's easy to set up and device driver authors don't know any better ;-). Note, a lot of drivers are now using work queues today, which run as a task.

Yes, there's a little more overhead with a task than for a softirq, but the problem with softirqs and tasklets is that they can't be preempted, and they are more important than all tasks on the system. If you have a task that is critical, it must yield for your softirq. Almost!

That is, even if you have a softirq, it may not run in irq context at all. If you get too many softirqs at a time (one comes while another one is running), it will defer the processing to the ksoftirq thread. This not only runs as a task, but also runs as SCHED_OTHER, and must yield to other tasks like everyone else too.

Thus, adding it as a softirq does not guarantee that it will be processed quickly. It just means that most of the time it will prevent anything else from happening while your "most important handler in the world" is running.

Quoted 2 lines

Quoted 1 line

From this, it sounds like you generally advise using threaded interrupt

handlers rather than tasklets/softirqs.

Yes, there's plenty of benefits for them, and I highly doubt that any USB device would even notice the difference between a softirq and a thread for response time latencies.

Quoted 1 line

Tasklet doesn't disable local interrupts.

It is documented that interrupts will be disabled while the completion handler runs. Therefore the tasklet _must_ disable local interrupts.

Quoted 4 lines · Alan Stern

Quoted 1 line

Tasklet doesn't disable local interrupts.

It is documented that interrupts will be disabled while the completion handler runs. Therefore the tasklet _must_ disable local interrupts.

You know, it may be that you can get most of the advantages you want by enabling local interrupts around the call to unmap_urb_for_dma() in usb_hcd_giveback_urb().

This may be a little dangerous, though, because it is possible for an URB to be given back at the time it is submitted. Drivers may not expect interrupts to get enabled temporarily when they call usb_submit_urb().

Quoted 4 lines · Ming Lei

Quoted 2 lines

That isn't clear. It is documented that USB completion handlers are called with local interrupts disabled. An IRQ arriving before the

Is there any good reason to do URB giveback with interrupt disabled?

That's a good question. Offhand I can't think of any drivers that rely on this -- although there certainly are places where callback routines use spin_lock() rather than spin_lock_irq() or spin_lock_irqsave(), because they know that interrupts are already disabled.

However, changing a documented API is not something to be done lightly. You would have to audit _every_ USB driver to make sure no trouble would arise.

Quoted 3 lines

IMO, disabling IRQs isn't necessary for completing URB since the complete() of URBs for same endpoint is reentrant.

Whether it is _necessary_ isn't the point. It is _documented_, and therefore it cannot be changed easily.

Quoted 5 lines

Quoted 3 lines

tasklet starts up might therefore be serviced during the short interval before the tasklet disables local interrupts, but if that happens it

Tasklet doesn't disable local interrupts.

It is documented that interrupts will be disabled while the completion handler runs. Therefore the tasklet _must_ disable local interrupts.

Quoted 4 lines

Quoted 1 line

would mean that the completion handler would be delayed.

Yes, the tasklet schedule delay is introduced to URB completion, but the delay doesn't have much side effect about completing URB.

What about delays that occur because a separate interrupt is serviced before the URB's completion handler is called? With the current code that can't happen, but with your scheme it can.

Quoted 2 lines

For async transfer, generally, it should have no effect since there should have URBs queued on the qh queue before submitting URB.

That's not how the Bulk-Only mass-storage protocol works. There are times when the protocol _requires_ bulk packets not to be submitted until a particular URB has completed.

Since mass-storage is an important use case for USB, its requirements should not be ignored.

Quoted 5 lines

Quoted 3 lines

In effect, you are prioritizing other IRQs higher than USB completion handlers.

Both other IRQs and HCD's IRQ are prioritizing the URB completion.

I don't understand that sentence. "Prioritizing" means "assigning a priority to". IRQs and HCDs don't assign priorities to anything; priorities are assigned by human programmers.

Quoted 5 lines

Quoted 2 lines

Nevertheless, the total time spent with interrupts disabled will remain the same.

No, the total time spent with interrupts disabled does not remain same, since no local IRQs are disabled during URB giveback anymore.

That is a bug. Local IRQs must be disabled during URB giveback.

Quoted 17 lines

Quoted 6 lines

(There's one other side effect that perhaps you haven't considered. When multiple URBs are addressed to the same endpoint, their completion handlers are called in order of URB completion, which is the same as the order of URB submission unless an URB is cancelled. By delegating the completion handlers to tasklets, and particularly by using per-CPU tasklets, you may violate this behavior.)

Even though URB giveback is handled by hard interrupt handler, it still can't guarantee that 100%, please see below:

- interrupt for URB A comes in CPU 0, and handled, then HCD private lock is released, and usb_hcd_giveback_urb() is called to complete URB A

- interrupt for URB B comes in CPU 1 just before CPU0 releases HCD private lock, so CPU1 holds the private lock when CPU 0 releases the lock and handles the HCD interrupt.

Quoted 2 lines

So now just like two CPUs are racing, if CPU0 wins, URB A is completed first, otherwise URB B is completed first.

Although you didn't say so, I assume you mean that A and B are URBs for the same endpoint. This means they use the same host controller and

hence they use the same IRQ line.

When an interrupt is handled, it is not possible for a second interrupt to arrive on the same IRQ line before the handler for the first interrupt returns. The kernel disables the IRQ line when the first interrupt occurs, and keeps it disabled until all the handlers are finished. Therefore the scenario you described cannot occur.

Quoted 2 lines

With introducing completing URB in percpu tasklet, the situation is just very similar.

No, it isn't, for the reason given above.

Quoted 4 lines

On ehci hcd, it can make sure that by always letting CPU0 complete all URBs (use ehci->scanning/ehci->need_rescan flag), but the patch can change percpu tasklet to tasklet to meet the demand.

Or perhaps you could assign each endpoint to a particular tasklet.

Quoted 6 lines

Quoted 2 lines

As I understand it, the tradeoff between hardirq and softirq is generally unimportant. It does matter a lot in realtime settings --

I don't think so, and obviously softirq allows IRQs to be served quickly, otherwise why is softirq and tasklet introduced? and why so many drivers are using tasklet/softirq?

This is part of what I would like to hear Thomas and Steve comment on.

Quoted 1 line · Alan Stern

On Mon, 10 Jun 2013, Ming Lei wrote:

If complete() callback runs in one tasklet context, spin_lock() inside complete() is enough, just like hard irq, the tasklet itself is disabled during complete(), if the percpu tasklet is converted to single tasklet. So no problem when the lock is to protect shared resources between complete().

When the lock is to protect shared resources between complete() and non-IRQ context, currently spin_lock_irqsave() is used in non-IRQ context, which is enough to prevent tasklet from being run on the CPU, so no problem for this situation too.

When all HCDs support to run URB giveback in tasklet context, we can change all spin_lock_irq*() to spin_lock() in drivers URB->complete(), and in other places, the spin_lock_irq*() can be changed to spin_lock_bh().

Quoted 8 lines · On Tuesday 11 June 2013 13:40:20 Ming Lei wrote:

Quoted 1 line · Alan Stern

On Mon, 10 Jun 2013, Ming Lei wrote:

If complete() callback runs in one tasklet context, spin_lock() inside complete() is enough, just like hard irq, the tasklet itself is disabled during complete(), if the percpu tasklet is converted to single tasklet. So no problem when the lock is to protect shared resources between complete().

We also have exclusion between complete() and other contexts, i.e. timers.

Quoted 6 lines

When the lock is to protect shared resources between complete() and non-IRQ context, currently spin_lock_irqsave() is used in non-IRQ context, which is enough to prevent tasklet from being run on the CPU, so no problem for this situation too.

Quoted 3 lines

When all HCDs support to run URB giveback in tasklet context, we can change all spin_lock_irq*() to spin_lock() in drivers URB->complete(), and in other places, the spin_lock_irq*() can be changed to spin_lock_bh().

Even now we cannot guarantee that all calls to complete() are in irq. There is the case of HCD hotunplug and other cases, like timeouts. They will have to be verified.

Quoted 10 lines · Oliver Neukum

Quoted 8 lines · On Tuesday 11 June 2013 13:40:20 Ming Lei wrote:

Quoted 1 line · Alan Stern

On Mon, 10 Jun 2013, Ming Lei wrote:

If complete() callback runs in one tasklet context, spin_lock() inside complete() is enough, just like hard irq, the tasklet itself is disabled during complete(), if the percpu tasklet is converted to single tasklet. So no problem when the lock is to protect shared resources between complete().

Quoted 1 line

We also have exclusion between complete() and other contexts, i.e. timers.

The patch also converts the complete() called in timers to tasklet context too.

So could you let me know if that eases your concern? If not, could you explain your concern about other contexts in a bit detail?

Quoted 3 lines

Even now we cannot guarantee that all calls to complete() are in irq. There is the case of HCD hotunplug and other cases, like timeouts. They will have to be verified.

All URBs are completed via usb_hcd_giveback_urb(), so there should be no differences between these cases and the common one about introducing the patchset.

Quoted 13 lines · On Tuesday 11 June 2013 16:14:25 Ming Lei wrote:

Quoted 11 lines · Oliver Neukum

Quoted 8 lines · On Tuesday 11 June 2013 13:40:20 Ming Lei wrote:

Quoted 1 line · Alan Stern

On Mon, 10 Jun 2013, Ming Lei wrote:

If complete() callback runs in one tasklet context, spin_lock() inside complete() is enough, just like hard irq, the tasklet itself is disabled during complete(), if the percpu tasklet is converted to single tasklet. So no problem when the lock is to protect shared resources between complete().

We also have exclusion between complete() and other contexts, i.e. timers.

Quoted 4 lines

The patch also converts the complete() called in timers to tasklet context too.

So could you let me know if that eases your concern? If not, could you explain your concern about other contexts in a bit detail?

The driver itself may have submitted a timer and race against it. What locking do you need in complete() and a timer to lock against each other?

Quoted 8 lines

Quoted 4 lines

Even now we cannot guarantee that all calls to complete() are in irq. There is the case of HCD hotunplug and other cases, like timeouts. They will have to be verified.

All URBs are completed via usb_hcd_giveback_urb(), so there should be no differences between these cases and the common one about introducing the patchset.

But it makes no sense to go to a tasklet when you are already in task context. In those cases you need to do something, essentially blocking the tasklet.

Quoted 5 lines · Oliver Neukum

On Tuesday 11 June 2013 16:14:25 Ming Lei wrote:

The driver itself may have submitted a timer and race against it. What locking do you need in complete() and a timer to lock against each other?

Good catch.

The problem will come if only spin_lock() is called inside complete(), I will check main USB drivers in tree to see if there is such use case.

Quoted 2 lines

But it makes no sense to go to a tasklet when you are already in task context. In those cases you need to do something, essentially blocking the tasklet.

At least now, always doing complete() in tasklet handler can simplify implementation since these cases aren't in hot path.

Quoted 7 lines · On Tuesday 11 June 2013 17:27:28 Ming Lei wrote:

Quoted 5 lines · Oliver Neukum

On Tuesday 11 June 2013 16:14:25 Ming Lei wrote:

The driver itself may have submitted a timer and race against it. What locking do you need in complete() and a timer to lock against each other?

Quoted 4 lines

Good catch.

The problem will come if only spin_lock() is called inside complete(), I will check main USB drivers in tree to see if there is such use case.

All network drivers race against timeout. I think they just unlink the URB, but there's a lot of them.

Quoted 6 lines

Quoted 3 lines

But it makes no sense to go to a tasklet when you are already in task context. In those cases you need to do something, essentially blocking the tasklet.

At least now, always doing complete() in tasklet handler can simplify implementation since these cases aren't in hot path.

Well, I am afraid this is not simply the case. These cases are partially synchronous. For example you need to make sure all calls to complete() are finished before you disconnect a HCD itself. The same applies to a device being disconnected.

It the same area, what happens if an URB is unlinked between the irq handler and the tasklet?

Quoted 6 lines

Quoted 1 line

Is there any good reason to do URB giveback with interrupt disabled?

That's a good question. Offhand I can't think of any drivers that rely on this -- although there certainly are places where callback routines use spin_lock() rather than spin_lock_irq() or spin_lock_irqsave(), because they know that interrupts are already disabled.

Complete() may take long time, for example in UVC's driver, URB's complete() may take more than 3 ms, as reported in below link:

      http://marc.info/?t=136438111600010&r=1&w=2

Also complete() is provided by interface driver, and it does many driver specific works, so it isn't good to disable interrupt only for one driver.

If complete() callback runs in one tasklet context, spin_lock() inside complete() is enough, just like hard irq, the tasklet itself is disabled during complete(), if the percpu tasklet is converted to single tasklet. So no problem when the lock is to protect shared resources between complete().

When the lock is to protect shared resources between complete() and non-IRQ context, currently spin_lock_irqsave() is used in non-IRQ context, which is enough to prevent tasklet from being run on the CPU, so no problem for this situation too.

When all HCDs support to run URB giveback in tasklet context, we can change all spin_lock_irq*() to spin_lock() in drivers URB->complete(), and in other places, the spin_lock_irq*() can be changed to spin_lock_bh().

Quoted 3 lines

However, changing a documented API is not something to be done lightly. You would have to audit _every_ USB driver to make sure no trouble would arise.

OK, I will write patch to request for changing the API if the improvement on the situation isn't objected.

In fact, at least now, the change on API is only about document change, and no drivers' change is required, isn't it? We can describe that URB->complete() might run in hard irq or softirq context, depends on HCD_BH flag of hcd->driver->flags.

Even with current code, one HCD's interrupt handling still may delay the handling for another HCD interrupt handling for quite long, that is the motivation to decrease the interrupt handling time of USB HCD, ;-) And finally, USB HCDs themselves will benefit from the change, won't they?

Quoted 6 lines

Quoted 2 lines

For async transfer, generally, it should have no effect since there should have URBs queued on the qh queue before submitting URB.

That's not how the Bulk-Only mass-storage protocol works. There are times when the protocol _requires_ bulk packets not to be submitted until a particular URB has completed.

Yes, I agree. But mass-storage driver itself is very fragile, it will perform badly if CPU has a higher load.

And it is better to submit DATA/CSW
transfer in previous transfer's complete(), IMO.

Compared with "usb-storage" task's schedule latency, the tasklet schedule delay should be lower at most of situations since the tasklet is scheduled inside irq handler.

So I decided to replace the percpu tasklet with one single tasklet, which can avoid the problem.

Also the tasklet running in CPU0 can handle the work which should have been done by the same tasket scheduled in CPU1, so we can avoid busy-waitting in CPU1 which may be introduced by tasklet_schedule() in CPU1.

Quoted 7 lines · Ming Lei

Quoted 6 lines

Quoted 1 line

Is there any good reason to do URB giveback with interrupt disabled?

That's a good question. Offhand I can't think of any drivers that rely on this -- although there certainly are places where callback routines use spin_lock() rather than spin_lock_irq() or spin_lock_irqsave(), because they know that interrupts are already disabled.

Quoted 7 lines

Complete() may take long time, for example in UVC's driver, URB's complete() may take more than 3 ms, as reported in below link:

      http://marc.info/?t=136438111600010&r=1&w=2

Also complete() is provided by interface driver, and it does many driver specific works, so it isn't good to disable interrupt only for one driver.

You probably mean "it isn't good to disable interrupts for the sake of only one driver". As things stand now, we disable interrupts for all callbacks in all drivers.

Quoted 6 lines

If complete() callback runs in one tasklet context, spin_lock() inside complete() is enough, just like hard irq, the tasklet itself is disabled during complete(), if the percpu tasklet is converted to single tasklet. So no problem when the lock is to protect shared resources between complete().

Quoted 8 lines

When the lock is to protect shared resources between complete() and non-IRQ context, currently spin_lock_irqsave() is used in non-IRQ context, which is enough to prevent tasklet from being run on the CPU, so no problem for this situation too.

When all HCDs support to run URB giveback in tasklet context, we can change all spin_lock_irq*() to spin_lock() in drivers URB->complete(), and in other places, the spin_lock_irq*() can be changed to spin_lock_bh().

I don't follow your reasoning. Consider the following situation, where the same spinlock is used in both a URB completion handler and an interrupt handler:

URB completes
A tasklet calls the completion handler, with interrupts enabled The completion handler does
spin_lock(&lock);
An interrupt occurs
The interrupt handler does
spin_lock(&lock); // Deadlock!

In order to prevent this from happening, you would have to change the spin_lock() call in the completion handler to spin_lock_irqsave(). Furthermore, you will have to audit every USB driver to make sure that all the completion handlers get fixed.

Quoted 7 lines

Quoted 4 lines

However, changing a documented API is not something to be done lightly. You would have to audit _every_ USB driver to make sure no trouble would arise.

OK, I will write patch to request for changing the API if the improvement on the situation isn't objected.

I don't mind. But don't be surprised if you end up breaking some drivers.

Quoted 5 lines

In fact, at least now, the change on API is only about document change, and no drivers' change is required, isn't it? We can describe that URB->complete() might run in hard irq or softirq context, depends on HCD_BH flag of hcd->driver->flags.

The drivers _do_ need to be changed, as explained above. And that explanation was only one failure scenario. There may be others.

Quoted 5 lines

Even with current code, one HCD's interrupt handling still may delay the handling for another HCD interrupt handling for quite long, that is the motivation to decrease the interrupt handling time of USB HCD, ;-) And finally, USB HCDs themselves will benefit from the change, won't they?

How will they benefit? Merely from not releasing their private locks?

That involves a fairly small amount of code.

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

For async transfer, generally, it should have no effect since there should have URBs queued on the qh queue before submitting URB.

That's not how the Bulk-Only mass-storage protocol works. There are times when the protocol _requires_ bulk packets not to be submitted until a particular URB has completed.

Quoted 2 lines

Yes, I agree. But mass-storage driver itself is very fragile, it will perform badly if CPU has a higher load.

Why does that make it fragile? _Every_ driver and program will behave worse when the system is under a heavy load.

Quoted 2 lines

And it is better to submit DATA/CSW
transfer in previous transfer's complete(), IMO.

Actually, it would be even better to submit DATA _before_ the CBW completes, and to submit the CSW before submitting DATA-OUT. Unfortunately, the design of the SG library makes it impossible to submit the CSW during DATA-IN completion.

Quoted 3 lines

Compared with "usb-storage" task's schedule latency, the tasklet schedule delay should be lower at most of situations since the tasklet is scheduled inside irq handler.

True. But the two latencies add.

Quoted 3 lines

So I decided to replace the percpu tasklet with one single tasklet, which can avoid the problem.

Hmmm. What happens when there is more than one host controller? The tasklet will give back only one URB at a time, and it won't run on more than one CPU at a time. The existing drivers allow URBs to be given back on multiple CPUs simultaneously.

Quoted 5 lines

Also the tasklet running in CPU0 can handle the work which should have been done by the same tasket scheduled in CPU1, so we can avoid busy-waitting in CPU1 which may be introduced by tasklet_schedule() in CPU1.

Or you could have a separate tasklet for each host controller.

Quoted 5 lines · Alan Stern

I don't follow your reasoning. Consider the following situation, where the same spinlock is used in both a URB completion handler and an interrupt handler:

Quoted 7 lines

URB completes
A tasklet calls the completion handler, with interrupts enabled The completion handler does
spin_lock(&lock);
An interrupt occurs
The interrupt handler does
spin_lock(&lock); // Deadlock!

If you mean the interrupt handler is HCD irq handler of the device, no such problem since all complete() will run in tasklet context.

If not, I am wondering why one USB driver need register another hard interrupt handler? Could you share such examples? I understand the case only exists with timer handler as discussed with Oliver, don't I?

Quoted 6 lines · Alan Stern

Quoted 4 lines

Also the tasklet running in CPU0 can handle the work which should have been done by the same tasket scheduled in CPU1, so we can avoid busy-waitting in CPU1 which may be introduced by tasklet_schedule() in CPU1.

Or you could have a separate tasklet for each host controller.

Yes, but I will compare tasklet with interrupt threaded handler first.

On Wed, 12 Jun 2013, Ming Lei wrote:

Yes, please. I read through the thread and I really recommend that you get rid of the tasklet. tasklets are a complete disaster by design and we really should make efforts to get rid of the last users instead of trying to work around their semantical shortcomings.

- using interrupt threaded handler(default)

        33.440 MB/sec

- using tasklet(#undef USB_HCD_THREADED_IRQ)

        34.29 MB/sec

- using hard interrupt handler(by removing HCD_BH in ehci-hcd.c )

        34.260 MB/s

So looks usb mass storage performance loss can be observed with interrupt threaded handler because one mass storage read/write sectors requires at least 3 interrupts which wake up usb-storage thread 3 times (each interrupt wakeup the usb-storage each time), introducing irq threaded handler will make 2 threads to be waken up about 6 times for one read/write.

I think usb mass storage transfer handler need to be rewritten, otherwise it may become worsen after using irq threaded handler in USB 3.0.(the above device can reach >120MB/sec with hardware handler or tasklet handler, which means about ~3K interrupts/sec, so ~6K contexts switch in case of using irq threaded handler)

So how about supporting tasklet first, then convert to interrupt threaded handler
after usb mass storage transfer is rewritten without performance loss? (rewriting
usb mass storage transfer handler may need some time and work since storage stability/correctness is extremely important, :-)

Also another problem with irq threaded handler is that there is no sort of tasklet_schedule() interface to wakeup the thread handler manually, so I have to use work to schedule some URB giveback from drivers(root hub transfer, unlink), even though that isn't a big deal but will cause code a bit much/complicated, :-)

Quoted 22 lines · Ming Lei

- using interrupt threaded handler(default)

        33.440 MB/sec

- using tasklet(#undef USB_HCD_THREADED_IRQ)

        34.29 MB/sec

- using hard interrupt handler(by removing HCD_BH in ehci-hcd.c )

        34.260 MB/s

So looks usb mass storage performance loss can be observed with interrupt threaded handler because one mass storage read/write sectors requires at least 3 interrupts which wake up usb-storage thread 3 times (each interrupt wakeup the usb-storage each time), introducing irq threaded handler will make 2 threads to be waken up about 6 times for one read/write.

I think usb mass storage transfer handler need to be rewritten, otherwise it may become worsen after using irq threaded handler in USB 3.0.(the above device can reach >120MB/sec with hardware handler or tasklet handler, which means about ~3K interrupts/sec, so ~6K contexts switch in case of using irq threaded handler)

Quoted 6 lines

So how about supporting tasklet first, then convert to interrupt threaded handler
after usb mass storage transfer is rewritten without performance loss? (rewriting
usb mass storage transfer handler may need some time and work since storage stability/correctness is extremely important, :-)

Maybe we should simply copy what the networking people do. They are very concerned about performance and latency; whatever technique they use should be good for USB too.

Quoted 31 lines · Alan Stern

Quoted 28 lines · Ming Lei

- using interrupt threaded handler(default)

        33.440 MB/sec

- using tasklet(#undef USB_HCD_THREADED_IRQ)

        34.29 MB/sec

- using hard interrupt handler(by removing HCD_BH in ehci-hcd.c )

        34.260 MB/s

So looks usb mass storage performance loss can be observed with interrupt threaded handler because one mass storage read/write sectors requires at least 3 interrupts which wake up usb-storage thread 3 times (each interrupt wakeup the usb-storage each time), introducing irq threaded handler will make 2 threads to be waken up about 6 times for one read/write.

I think usb mass storage transfer handler need to be rewritten, otherwise it may become worsen after using irq threaded handler in USB 3.0.(the above device can reach >120MB/sec with hardware handler or tasklet handler, which means about ~3K interrupts/sec, so ~6K contexts switch in case of using irq threaded handler)

So how about supporting tasklet first, then convert to interrupt threaded handler
after usb mass storage transfer is rewritten without performance loss? (rewriting
usb mass storage transfer handler may need some time and work since storage stability/correctness is extremely important, :-)

Quoted 3 lines

Maybe we should simply copy what the networking people do. They are very concerned about performance and latency; whatever technique they use should be good for USB too.

Yes, but for "old-style" usb-storage, is this really a big deal? We are still easily hitting the "line-speed" of USB for usb-storage with simple machines, the bottlenecks that I'm seeing are in the devices themselves, and then in the USB wire speed.

Once hardware comes out that uses USB streams, and we get device support for the UAS protocol, then we might have a need to change things, but at this point in time, for the "old" driver, I think we are fine.

Unless someone has a workload / benchmark that shows otherwise?

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.

Quoted 4 lines · Alan Stern

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:

(quoted diff) +5 -0

+#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!

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.

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.

Why couldn't you just do:

	if (sched)
		usb_giveback_urb_bh(bh);

?

Quoted 22 lines · Steven Rostedt

Quoted 4 lines · Alan Stern

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:

(quoted diff) +5 -0

+#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

Quoted 1 line

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.

Quoted 3 lines

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 10 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 22 lines · Steven Rostedt

Quoted 4 lines · Alan Stern

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:

(quoted diff) +5 -0

+#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

Quoted 1 line

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.

Quoted 3 lines

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 10 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

Why couldn't you just do:

Quoted 4 lines

      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.

Quoted 36 lines · Greg Kroah-Hartman

Quoted 34 lines · Alan Stern

Quoted 22 lines · Ming Lei

- using interrupt threaded handler(default)

        33.440 MB/sec

- using tasklet(#undef USB_HCD_THREADED_IRQ)

        34.29 MB/sec

- using hard interrupt handler(by removing HCD_BH in ehci-hcd.c )

        34.260 MB/s

So looks usb mass storage performance loss can be observed with interrupt threaded handler because one mass storage read/write sectors requires at least 3 interrupts which wake up usb-storage thread 3 times (each interrupt wakeup the usb-storage each time), introducing irq threaded handler will make 2 threads to be waken up about 6 times for one read/write.

I think usb mass storage transfer handler need to be rewritten, otherwise it may become worsen after using irq threaded handler in USB 3.0.(the above device can reach >120MB/sec with hardware handler or tasklet handler, which means about ~3K interrupts/sec, so ~6K contexts switch in case of using irq threaded handler)

Quoted 6 lines

So how about supporting tasklet first, then convert to interrupt threaded handler
after usb mass storage transfer is rewritten without performance loss? (rewriting
usb mass storage transfer handler may need some time and work since storage stability/correctness is extremely important, :-)

Maybe we should simply copy what the networking people do. They are very concerned about performance and latency; whatever technique they use should be good for USB too.

Quoted 4 lines

Yes, but for "old-style" usb-storage, is this really a big deal? We are still easily hitting the "line-speed" of USB for usb-storage with simple machines, the bottlenecks that I'm seeing are in the devices themselves, and then in the USB wire speed.

What about with USB-3 storage devices? Many of them still use the bulk-only transport instead of UAS. They may push the limits up.

Quoted 5 lines

Once hardware comes out that uses USB streams, and we get device support for the UAS protocol, then we might have a need to change things, but at this point in time, for the "old" driver, I think we are fine.

Unless someone has a workload / benchmark that shows otherwise?

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.

The main reason for moving away from the current scheme is to reduce latency for other interrupt handlers. Ming gave two examples of slow USB code that runs in hardirq context now; with his change they would run in softirq context and therefore wouldn't delay other interrupts so much. (Interrupt latency is hard to measure, however.)

Quoted 43 lines · Alan Stern

Quoted 40 lines · Greg Kroah-Hartman

Quoted 31 lines · Alan Stern

Quoted 22 lines · Ming Lei

- using interrupt threaded handler(default)

        33.440 MB/sec

- using tasklet(#undef USB_HCD_THREADED_IRQ)

        34.29 MB/sec

- using hard interrupt handler(by removing HCD_BH in ehci-hcd.c )

        34.260 MB/s

So looks usb mass storage performance loss can be observed with interrupt threaded handler because one mass storage read/write sectors requires at least 3 interrupts which wake up usb-storage thread 3 times (each interrupt wakeup the usb-storage each time), introducing irq threaded handler will make 2 threads to be waken up about 6 times for one read/write.

I think usb mass storage transfer handler need to be rewritten, otherwise it may become worsen after using irq threaded handler in USB 3.0.(the above device can reach >120MB/sec with hardware handler or tasklet handler, which means about ~3K interrupts/sec, so ~6K contexts switch in case of using irq threaded handler)

Quoted 6 lines

So how about supporting tasklet first, then convert to interrupt threaded handler
after usb mass storage transfer is rewritten without performance loss? (rewriting
usb mass storage transfer handler may need some time and work since storage stability/correctness is extremely important, :-)

Quoted 3 lines

Maybe we should simply copy what the networking people do. They are very concerned about performance and latency; whatever technique they use should be good for USB too.

Yes, but for "old-style" usb-storage, is this really a big deal? We are still easily hitting the "line-speed" of USB for usb-storage with simple machines, the bottlenecks that I'm seeing are in the devices themselves, and then in the USB wire speed.

Quoted 2 lines

What about with USB-3 storage devices? Many of them still use the bulk-only transport instead of UAS. They may push the limits up.

Are they really? Have we seen that happen yet? With the number's I've

seen published, we are easily serving up enough data to keep the pipe full, but that all depends on your CPU / host controller.

Quoted 11 lines

Quoted 5 lines

Once hardware comes out that uses USB streams, and we get device support for the UAS protocol, then we might have a need to change things, but at this point in time, for the "old" driver, I think we are fine.

Unless someone has a workload / benchmark that shows otherwise?

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.

Quoted 5 lines

The main reason for moving away from the current scheme is to reduce latency for other interrupt handlers. Ming gave two examples of slow USB code that runs in hardirq context now; with his change they would run in softirq context and therefore wouldn't delay other interrupts so much. (Interrupt latency is hard to measure, however.)

Yes, I know that people keep wanting to worry about latency issues, and the best answer for them has always been, "don't use USB." :)

You suffer throughput issues with predicitable latency dependancies, so

we need to be careful we don't slow down the 99% of the systems out there that do not care about this at all.

Quoted 44 lines · Greg Kroah-Hartman

Quoted 43 lines · Alan Stern

Quoted 40 lines · Greg Kroah-Hartman

Quoted 31 lines · Alan Stern

Quoted 22 lines · Ming Lei

- using interrupt threaded handler(default)

        33.440 MB/sec

- using tasklet(#undef USB_HCD_THREADED_IRQ)

        34.29 MB/sec

- using hard interrupt handler(by removing HCD_BH in ehci-hcd.c )

        34.260 MB/s

So looks usb mass storage performance loss can be observed with interrupt threaded handler because one mass storage read/write sectors requires at least 3 interrupts which wake up usb-storage thread 3 times (each interrupt wakeup the usb-storage each time), introducing irq threaded handler will make 2 threads to be waken up about 6 times for one read/write.

I think usb mass storage transfer handler need to be rewritten, otherwise it may become worsen after using irq threaded handler in USB 3.0.(the above device can reach >120MB/sec with hardware handler or tasklet handler, which means about ~3K interrupts/sec, so ~6K contexts switch in case of using irq threaded handler)

Quoted 6 lines

So how about supporting tasklet first, then convert to interrupt threaded handler
after usb mass storage transfer is rewritten without performance loss? (rewriting
usb mass storage transfer handler may need some time and work since storage stability/correctness is extremely important, :-)

Quoted 3 lines

Maybe we should simply copy what the networking people do. They are very concerned about performance and latency; whatever technique they use should be good for USB too.

Yes, but for "old-style" usb-storage, is this really a big deal? We are still easily hitting the "line-speed" of USB for usb-storage with simple machines, the bottlenecks that I'm seeing are in the devices themselves, and then in the USB wire speed.

Quoted 4 lines

Quoted 2 lines

What about with USB-3 storage devices? Many of them still use the bulk-only transport instead of UAS. They may push the limits up.

Are they really? Have we seen that happen yet? With the number's I've

Yes, the device I am testing is bulk-only, no uas support , and it is very popular in market.

Quoted 20 lines

seen published, we are easily serving up enough data to keep the pipe full, but that all depends on your CPU / host controller.

Quoted 16 lines

Quoted 5 lines

Once hardware comes out that uses USB streams, and we get device support for the UAS protocol, then we might have a need to change things, but at this point in time, for the "old" driver, I think we are fine.

Unless someone has a workload / benchmark that shows otherwise?

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.

The main reason for moving away from the current scheme is to reduce latency for other interrupt handlers. Ming gave two examples of slow USB code that runs in hardirq context now; with his change they would run in softirq context and therefore wouldn't delay other interrupts so much. (Interrupt latency is hard to measure, however.)

Quoted 2 lines

Yes, I know that people keep wanting to worry about latency issues, and the best answer for them has always been, "don't use USB." :)

I think we can do it better, why don't do it? :-)

Quoted 1 line

You suffer throughput issues with predicitable latency dependancies, so

This patchset don't cause throughout degradation but decrease latency much, also has other advantages.

Quoted 2 lines

we need to be careful we don't slow down the 99% of the systems out there that do not care about this at all.

Considered great amount of ARM devices in market, I think we need to consider the problem on these devices, :-)

Quoted 74 lines · Ming Lei

Quoted 44 lines · Greg Kroah-Hartman

Quoted 43 lines · Alan Stern

Quoted 40 lines · Greg Kroah-Hartman

Quoted 31 lines · Alan Stern

Quoted 22 lines · Ming Lei

- using interrupt threaded handler(default)

        33.440 MB/sec

- using tasklet(#undef USB_HCD_THREADED_IRQ)

        34.29 MB/sec

- using hard interrupt handler(by removing HCD_BH in ehci-hcd.c )

        34.260 MB/s

So looks usb mass storage performance loss can be observed with interrupt threaded handler because one mass storage read/write sectors requires at least 3 interrupts which wake up usb-storage thread 3 times (each interrupt wakeup the usb-storage each time), introducing irq threaded handler will make 2 threads to be waken up about 6 times for one read/write.

I think usb mass storage transfer handler need to be rewritten, otherwise it may become worsen after using irq threaded handler in USB 3.0.(the above device can reach >120MB/sec with hardware handler or tasklet handler, which means about ~3K interrupts/sec, so ~6K contexts switch in case of using irq threaded handler)

Quoted 6 lines

So how about supporting tasklet first, then convert to interrupt threaded handler
after usb mass storage transfer is rewritten without performance loss? (rewriting
usb mass storage transfer handler may need some time and work since storage stability/correctness is extremely important, :-)

Quoted 3 lines

Maybe we should simply copy what the networking people do. They are very concerned about performance and latency; whatever technique they use should be good for USB too.

Yes, but for "old-style" usb-storage, is this really a big deal? We are still easily hitting the "line-speed" of USB for usb-storage with simple machines, the bottlenecks that I'm seeing are in the devices themselves, and then in the USB wire speed.

Quoted 4 lines

Quoted 2 lines

What about with USB-3 storage devices? Many of them still use the bulk-only transport instead of UAS. They may push the limits up.

Are they really? Have we seen that happen yet? With the number's I've

Yes, the device I am testing is bulk-only, no uas support , and it is very popular in market.

Quoted 20 lines

seen published, we are easily serving up enough data to keep the pipe full, but that all depends on your CPU / host controller.

Quoted 16 lines

Quoted 5 lines

Once hardware comes out that uses USB streams, and we get device support for the UAS protocol, then we might have a need to change things, but at this point in time, for the "old" driver, I think we are fine.

Unless someone has a workload / benchmark that shows otherwise?

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.

The main reason for moving away from the current scheme is to reduce latency for other interrupt handlers. Ming gave two examples of slow USB code that runs in hardirq context now; with his change they would run in softirq context and therefore wouldn't delay other interrupts so much. (Interrupt latency is hard to measure, however.)

Quoted 4 lines

Quoted 2 lines

Yes, I know that people keep wanting to worry about latency issues, and the best answer for them has always been, "don't use USB." :)

I think we can do it better, why don't do it? :-)

Because of other issues, that have been brought up here already.

But if you can do it without affecting others, that's fine.

Quoted 5 lines

Quoted 2 lines

You suffer throughput issues with predicitable latency dependancies, so

This patchset don't cause throughout degradation but decrease latency much, also has other advantages.

Like what?

Quoted 6 lines

Quoted 3 lines

we need to be careful we don't slow down the 99% of the systems out there that do not care about this at all.

Considered great amount of ARM devices in market, I think we need to consider the problem on these devices, :-)

Is it a problem on those devices? I think they have host controller issues that are way bigger problems than this device driver, right?

Quoted 36 lines · Greg Kroah-Hartman

Quoted 34 lines · Alan Stern

Quoted 22 lines · Ming Lei

- using interrupt threaded handler(default)

        33.440 MB/sec

- using tasklet(#undef USB_HCD_THREADED_IRQ)

        34.29 MB/sec

- using hard interrupt handler(by removing HCD_BH in ehci-hcd.c )

        34.260 MB/s

So looks usb mass storage performance loss can be observed with interrupt threaded handler because one mass storage read/write sectors requires at least 3 interrupts which wake up usb-storage thread 3 times (each interrupt wakeup the usb-storage each time), introducing irq threaded handler will make 2 threads to be waken up about 6 times for one read/write.

I think usb mass storage transfer handler need to be rewritten, otherwise it may become worsen after using irq threaded handler in USB 3.0.(the above device can reach >120MB/sec with hardware handler or tasklet handler, which means about ~3K interrupts/sec, so ~6K contexts switch in case of using irq threaded handler)

Quoted 6 lines

So how about supporting tasklet first, then convert to interrupt threaded handler
after usb mass storage transfer is rewritten without performance loss? (rewriting
usb mass storage transfer handler may need some time and work since storage stability/correctness is extremely important, :-)

Maybe we should simply copy what the networking people do. They are very concerned about performance and latency; whatever technique they use should be good for USB too.

Quoted 4 lines

Yes, but for "old-style" usb-storage, is this really a big deal? We are still easily hitting the "line-speed" of USB for usb-storage with simple machines, the bottlenecks that I'm seeing are in the devices themselves, and then in the USB wire speed.

What about with USB-3 storage devices? Many of them still use the bulk-only transport instead of UAS. They may push the limits up.

Quoted 6 lines

Once hardware comes out that uses USB streams, and we get device support for the UAS protocol, then we might have a need to change things, but at this point in time, for the "old" driver, I think we are fine.

Unless someone has a workload / benchmark that shows otherwise?

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.

The main reason for moving away from the current scheme is to reduce latency for other interrupt handlers. Ming gave two examples of slow USB code that runs in hardirq context now; with his change they would run in softirq context and therefore wouldn't delay other interrupts so much. (Interrupt latency is hard to measure, however.)

Quoted 6 lines · Alan Stern

The main reason for moving away from the current scheme is to reduce latency for other interrupt handlers. Ming gave two examples of slow USB code that runs in hardirq context now; with his change they would run in softirq context and therefore wouldn't delay other interrupts so much. (Interrupt latency is hard to measure, however.)

With the two trace points of irq_handler_entry and irq_handler_exit, the interrupt latency(or the time taken by hard irq handler) isn't hard to measure. One simple script can figure out the average/maximum latency for one irq handler, like I did in 4/4.

Quoted 8 lines · Ming Lei

Quoted 6 lines · Alan Stern

The main reason for moving away from the current scheme is to reduce latency for other interrupt handlers. Ming gave two examples of slow USB code that runs in hardirq context now; with his change they would run in softirq context and therefore wouldn't delay other interrupts so much. (Interrupt latency is hard to measure, however.)

Quoted 4 lines

With the two trace points of irq_handler_entry and irq_handler_exit, the interrupt latency(or the time taken by hard irq handler) isn't hard to measure. One simple script can figure out the average/maximum latency for one irq handler, like I did in 4/4.

But that doesn't measure the time between when the IRQ request is issued and when irq_handler_entry runs.

Quoted 7 lines

Quoted 4 lines

With the two trace points of irq_handler_entry and irq_handler_exit, the interrupt latency(or the time taken by hard irq handler) isn't hard to measure. One simple script can figure out the average/maximum latency for one irq handler, like I did in 4/4.

But that doesn't measure the time between when the IRQ request is issued and when irq_handler_entry runs.

That might be hard to measure, also I am wondering if the time can be measured only by software, but these patches only focus on the time between HCD irq entry and irq exit.

Quoted 11 lines · Ming Lei

Quoted 7 lines

Quoted 4 lines

With the two trace points of irq_handler_entry and irq_handler_exit, the interrupt latency(or the time taken by hard irq handler) isn't hard to measure. One simple script can figure out the average/maximum latency for one irq handler, like I did in 4/4.

But that doesn't measure the time between when the IRQ request is issued and when irq_handler_entry runs.

That might be hard to measure, also I am wondering if the time can be measured only by software, but these patches only focus on the time between HCD irq entry and irq exit.

Not entirely. On a UP system, leaving interrupts disabled for a long time (which is what we do now) increases the delay between when the IRQ is raised and when it is serviced. On an SMP system, a long-running interrupt handler will delay servicing a different device that shares the same IRQ line.

Not entirely. On a UP system, leaving interrupts disabled for a long time (which is what we do now) increases the delay between when the IRQ is raised and when it is serviced. On an SMP system, a long-running

interrupt handler will delay servicing a different device that shares the same IRQ line.

And on UP it delays ALL other interrupts. I've seen 500us+ caused by the USB interrupt handlers...

Maybe we should simply copy what the networking people do. They are very concerned about performance and latency; whatever technique they use should be good for USB too.

If not, I am wondering why one USB driver need register another hard interrupt handler? Could you share such examples? I understand the case only exists with timer handler as discussed with Oliver, don't I?

Quoted 2 lines

The drivers _do_ need to be changed, as explained above. And that explanation was only one failure scenario. There may be others.

OK, I'd like to know what the others are? :-)

Quoted 3 lines

Quoted 1 line

And finally, USB HCDs themselves will benefit from the change, won't they?

How will they benefit? Merely from not releasing their private locks?

The interrupt of one HCD will be responded lately since another HCD's interrupt handler takes very long time. With the change, the problem can be avoided.

Quoted 5 lines

Quoted 3 lines

Compared with "usb-storage" task's schedule latency, the tasklet schedule delay should be lower at most of situations since the tasklet is scheduled inside irq handler.

True. But the two latencies add.

I think it should be one tasklet latency added, but what is the other one?

Quoted 1 line

Or you could have a separate tasklet for each host controller.

Yes, but I will compare tasklet with interrupt threaded handler first.

Quoted 3 lines · Ming Lei

If not, I am wondering why one USB driver need register another hard interrupt handler? Could you share such examples? I understand the case only exists with timer handler as discussed with Oliver, don't I?

Here's another possibility. A USB driver has a private lock, and it acquires this lock in its completion handler. But it also acquires this lock in other routines, and those other routines may be called by other drivers or subsystems in interrupt context.

Quoted 3 lines · Ming Lei

If not, I am wondering why one USB driver need register another hard interrupt handler? Could you share such examples? I understand the case only exists with timer handler as discussed with Oliver, don't I?

Here's another possibility. A USB driver has a private lock, and it acquires this lock in its completion handler. But it also acquires this lock in other routines, and those other routines may be called by other drivers or subsystems in interrupt context.

Quoted 5 lines

Quoted 3 lines

The drivers _do_ need to be changed, as explained above. And that explanation was only one failure scenario. There may be others.

OK, I'd like to know what the others are? :-)

Me too. :-)

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

And finally, USB HCDs themselves will benefit from the change, won't they?

Quoted 5 lines

Quoted 1 line

How will they benefit? Merely from not releasing their private locks?

The interrupt of one HCD will be responded lately since another HCD's interrupt handler takes very long time. With the change, the problem can be avoided.

For reasonably sophisticated host controllers like EHCI, the delay in responding to an interrupt doesn't matter much. But for simpler host

controllers (for example, those using PIO instead of DMA) it matters more, so those other HCDs will benefit more from the conversion.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Compared with "usb-storage" task's schedule latency, the tasklet schedule delay should be lower at most of situations since the tasklet is scheduled inside irq handler.

True. But the two latencies add.

I think it should be one tasklet latency added, but what is the other one?

I meant that the latency in tasklet scheduling has to be added to the latency in scheduling the usb-storage thread.

Quoted 3 lines

Quoted 1 line

Or you could have a separate tasklet for each host controller.

Yes, but I will compare tasklet with interrupt threaded handler first.

Switching to a threaded interrupt handler offers more possibilities. Instead of moving just the givebacks to the thread, we could put almost all the processing there. (At least, we can for ehci-hcd. Other HCDs may not be able to do this.)

At this point we are talking about multiple changes:

Moving the givebacks to a tasklet or threaded handler.

Changing the USB completion handlers so that they can be called with interrupts enabled.

Allowing the tasklet/thread to run with interrupts enabled.

Moving more of the HCD processing into the tasklet/thread.

Maybe other things too. In principle, the first and second items can be done simultaneously.

Quoted 2 lines

For reasonably sophisticated host controllers like EHCI, the delay in responding to an interrupt doesn't matter much. But for simpler host

EHCI might still benefit from the change: suppose uvc video is playing on one EHCI bus, and mass storage's performance over another EHCI may decrease because of the IRQ latency introduced.

Quoted 7 lines

At this point we are talking about multiple changes:

Moving the givebacks to a tasklet or threaded handler.

Changing the USB completion handlers so that they can be called with interrupts enabled.

Quoted 6 lines

Allowing the tasklet/thread to run with interrupts enabled.

Moving more of the HCD processing into the tasklet/thread.

Maybe other things too. In principle, the first and second items can be done simultaneously.

Very good summery, we can push the 1st change with disabling local IRQ when calling complete(), so that at least DMA unmapping can be done with IRQ enabled, and at the same time do the API change, which should be a bit slow but the mechanical way proposed by Oliver may be OK.

The 3rd item is easy once the 1st two items are completed.

For the 4th one, it might be a long term thing, since refactoring HCD interrupt handler is a bit complicated. Also, when the 1st three items are completed, hard interrupt handler takes less time, often less than 20us at average about EHCI, so I am wondering if the 4th is worthy.

In order to prevent this from happening, you would have to change the spin_lock() call in the completion handler to spin_lock_irqsave(). Furthermore, you will have to audit every USB driver to make sure that all the completion handlers get fixed.

Quoted 4 lines · On Tuesday 11 June 2013 15:10:03 Alan Stern wrote:

In order to prevent this from happening, you would have to change the spin_lock() call in the completion handler to spin_lock_irqsave(). Furthermore, you will have to audit every USB driver to make sure that all the completion handlers get fixed.

Yes. However, it can be done mechanically. And we know only the handlers for complete need to be fixed.

Quoted 8 lines · Oliver Neukum

Quoted 4 lines · On Tuesday 11 June 2013 15:10:03 Alan Stern wrote:

In order to prevent this from happening, you would have to change the spin_lock() call in the completion handler to spin_lock_irqsave(). Furthermore, you will have to audit every USB driver to make sure that all the completion handlers get fixed.

Yes. However, it can be done mechanically. And we know only the handlers for complete need to be fixed.

I am wondering if the change is needed since timer function is still run in softirq context instead of hard irq.

Looks Alan concerned that one USB interface driver may have another hard interrupt handler involved. Is there such kind of USB driver/device in tree?

Quoted 13 lines · On Wednesday 12 June 2013 18:11:34 Ming Lei wrote:

Quoted 8 lines · Oliver Neukum

Quoted 4 lines · On Tuesday 11 June 2013 15:10:03 Alan Stern wrote:

In order to prevent this from happening, you would have to change the spin_lock() call in the completion handler to spin_lock_irqsave(). Furthermore, you will have to audit every USB driver to make sure that all the completion handlers get fixed.

Yes. However, it can be done mechanically. And we know only the handlers for complete need to be fixed.

I am wondering if the change is needed since timer function is still run in softirq context instead of hard irq.

Quoted 3 lines

Looks Alan concerned that one USB interface driver may have another hard interrupt handler involved. Is there such kind of USB driver/device in tree?

No. Suppose a USB network driver.
The complete() handler is written on the assumption that interrupts are off. So it takes a spinlock from the network subsystem. It does so with spin_lock()

Other network drivers also take the lock. And they may take it from an IRQ handler. If such an IRQ interrupts the tasklet complete() is running in, the CPU will deadlock.

The danger is not interrupt handlers in the same driver but IRQ handlers of _other_ drivers (PCI, ...) a lock is shared with.

You need to go through all USB drivers and change every spin_lock() that goes for a lock that is exported to a spin_lock_irqsave()