| Message ID | 1370791112-18464-3-git-send-email-ming.lei@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 9 Jun 2013, Ming Lei wrote: > 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. Alan Stern
On Mon, Jun 10, 2013 at 12:06 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Sun, 9 Jun 2013, Ming Lei wrote: > >> 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. I did't mention this 3/4 patch can save much time. In fact, without this patch, handling URB giveback still can work. But releasing the private lock can make hard irq handler become random long, since reacquiring the lock may busy wait quite a while until other CPUs released the lock. > > 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. Generally the interrupt handler without URB giveback consumes not much time, and the time consumed is about 10~20us on fast machine, and <40us on slow machine averagely per my tests in 4/4 commit log. You may ague the situation would become worse if there are many URBs to be handled in ehci_irq() on CPU1, but I think it should be very very rare to happen attributed to this patchset: HCD hard interrupt handler takes much less time than general HCD irq interval(for example, in EHCI, generally there won't have many URBs to be completed in one uFrame, and the URBs completed in next uFrame will interrupt CPUs 125us later) Also we may introduce another lock/flags to let CPU1 handle CPU2's interrupts and let CPU2 return IRQ_HANDLED immediately, but I am not sure if it is necessary. Thanks, -- Ming Lei
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c index d34b399..0387a81 100644 --- a/drivers/usb/host/ehci-q.c +++ b/drivers/usb/host/ehci-q.c @@ -283,9 +283,11 @@ __acquires(ehci->lock) /* complete() can reenter this HCD */ usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb); - spin_unlock (&ehci->lock); + if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci))) + spin_unlock(&ehci->lock); usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); - spin_lock (&ehci->lock); + if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci))) + spin_lock(&ehci->lock); } static int qh_schedule (struct ehci_hcd *ehci, struct ehci_qh *qh);
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(). Cc: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- drivers/usb/host/ehci-q.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)