Re: [PATCH] drivers: usb: core: Don't disable irqs in usb_sg_wait() during URB submit.
Alan, Thanks for your feedback! > This should now be GFP_NOIO. OK, I updated the patch to fix that. Good catch. > Strictly speaking, this loop should run backward. Then the > spin_unlock() could be replaced with spin_unlock_irqrestore(). Good idea. A separate patch for this is included. --david -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers: usb: core: Don't disable irqs in usb_sg_wait() during URB submit.
On Tue, 8 Mar 2016, David Mosberger-Tang wrote: > From: David Mosberger > > usb_submit_urb() may take quite long to execute. For example, a > single sg list may have 30 or more entries, possibly leading to that > many calls to DMA-map pages. This can cause interrupt latency of > several hundred micro-seconds. > > Avoid the problem by releasing the io->lock spinlock and re-enabling > interrupts before calling usb_submit_urb(). This opens races with > usb_sg_cancel() and sg_complete(). Handle those races by using > usb_block_urb() to stop URBs from being submitted after > usb_sg_cancel() or sg_complete() with error. > > Note that usb_unlink_urb() is guaranteed to return -ENODEV if > !io->urbs[i]->dev and since the -ENODEV case is already handled, > we don't have to check for !io->urbs[i]->dev explicitly. > > Before this change, reading 512MB from an ext3 filesystem on a USB > memory stick showed a throughput of 12 MB/s with about 500 missed > deadlines. > > With this change, reading the same file gave the same throughput but > only one or two missed deadlines. > > Signed-off-by: David Mosberger Pretty good. Only one change is really needed. > @@ -515,12 +516,10 @@ void usb_sg_wait(struct usb_sg_request *io) > int retval; > > io->urbs[i]->dev = io->dev; > + spin_unlock_irq(&io->lock); > + > retval = usb_submit_urb(io->urbs[i], GFP_ATOMIC); This should now be GFP_NOIO. > - /* after we submit, let completions or cancellations fire; > - * we handshake using io->status. > - */ > - spin_unlock_irq(&io->lock); > switch (retval) { > /* maybe we retrying will recover */ > case -ENXIO:/* hc didn't queue this one */ > @@ -590,8 +589,8 @@ void usb_sg_cancel(struct usb_sg_request *io) > for (i = 0; i < io->entries; i++) { > int retval; > > - if (!io->urbs[i]->dev) > - continue; > + usb_block_urb(io->urbs[i]); > + > retval = usb_unlink_urb(io->urbs[i]); > if (retval != -EINPROGRESS > && retval != -ENODEV Strictly speaking, this loop should run backward. Then the spin_unlock() could be replaced with spin_unlock_irqrestore(). In fact, the whole routine could be restructured like this: spin_lock_irqsave(&io->lock, flags); /* shut everything down, if it isn't already */ if (io->status) { spin_unlock_irqrestore(&io->lock, flags); return; } io->status = -ECONNRESET; spin_unlock_irqrestore(&io->lock, flags); for (i = io->entries - 1; i >= 0; --i) { ... But that should be done in a separate patch. It's not critical anyway; cancelling I/O is relatively rare and unimportant. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] drivers: usb: core: Don't disable irqs in usb_sg_wait() during URB submit.
From: David Mosberger usb_submit_urb() may take quite long to execute. For example, a single sg list may have 30 or more entries, possibly leading to that many calls to DMA-map pages. This can cause interrupt latency of several hundred micro-seconds. Avoid the problem by releasing the io->lock spinlock and re-enabling interrupts before calling usb_submit_urb(). This opens races with usb_sg_cancel() and sg_complete(). Handle those races by using usb_block_urb() to stop URBs from being submitted after usb_sg_cancel() or sg_complete() with error. Note that usb_unlink_urb() is guaranteed to return -ENODEV if !io->urbs[i]->dev and since the -ENODEV case is already handled, we don't have to check for !io->urbs[i]->dev explicitly. Before this change, reading 512MB from an ext3 filesystem on a USB memory stick showed a throughput of 12 MB/s with about 500 missed deadlines. With this change, reading the same file gave the same throughput but only one or two missed deadlines. Signed-off-by: David Mosberger --- drivers/usb/core/message.c | 13 ++--- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 8e641b5..8ead329 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -302,9 +302,10 @@ static void sg_complete(struct urb *urb) */ spin_unlock(&io->lock); for (i = 0, found = 0; i < io->entries; i++) { - if (!io->urbs[i] || !io->urbs[i]->dev) + if (!io->urbs[i]) continue; if (found) { + usb_block_urb(io->urbs[i]); retval = usb_unlink_urb(io->urbs[i]); if (retval != -EINPROGRESS && retval != -ENODEV && @@ -515,12 +516,10 @@ void usb_sg_wait(struct usb_sg_request *io) int retval; io->urbs[i]->dev = io->dev; + spin_unlock_irq(&io->lock); + retval = usb_submit_urb(io->urbs[i], GFP_ATOMIC); - /* after we submit, let completions or cancellations fire; -* we handshake using io->status. -*/ - spin_unlock_irq(&io->lock); switch (retval) { /* maybe we retrying will recover */ case -ENXIO:/* hc didn't queue this one */ @@ -590,8 +589,8 @@ void usb_sg_cancel(struct usb_sg_request *io) for (i = 0; i < io->entries; i++) { int retval; - if (!io->urbs[i]->dev) - continue; + usb_block_urb(io->urbs[i]); + retval = usb_unlink_urb(io->urbs[i]); if (retval != -EINPROGRESS && retval != -ENODEV -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html