[PATCH 2/2] drivers: usb: core: Minimize irq disabling in usb_sg_cancel()
From: David Mosberger
Restructure usb_sg_cancel() so we don't have to disable interrupts
while cancelling the URBs.
Suggested-by: Alan Stern
Signed-off-by: David Mosberger
---
drivers/usb/core/message.c | 37 +
1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index e13a2e5..ea681f1 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -577,31 +577,28 @@ EXPORT_SYMBOL_GPL(usb_sg_wait);
void usb_sg_cancel(struct usb_sg_request *io)
{
unsigned long flags;
+ int i, retval;
spin_lock_irqsave(&io->lock, flags);
+ if (io->status) {
+ spin_unlock_irqrestore(&io->lock, flags);
+ return;
+ }
+ /* shut everything down */
+ io->status = -ECONNRESET;
+ spin_unlock_irqrestore(&io->lock, flags);
- /* shut everything down, if it didn't already */
- if (!io->status) {
- int i;
+ for (i = io->entries - 1; i >= 0; --i) {
+ usb_block_urb(io->urbs[i]);
- io->status = -ECONNRESET;
- spin_unlock(&io->lock);
- for (i = 0; i < io->entries; i++) {
- int retval;
-
- usb_block_urb(io->urbs[i]);
-
- retval = usb_unlink_urb(io->urbs[i]);
- if (retval != -EINPROGRESS
- && retval != -ENODEV
- && retval != -EBUSY
- && retval != -EIDRM)
- dev_warn(&io->dev->dev, "%s, unlink --> %d\n",
- __func__, retval);
- }
- spin_lock(&io->lock);
+ retval = usb_unlink_urb(io->urbs[i]);
+ if (retval != -EINPROGRESS
+ && retval != -ENODEV
+ && retval != -EBUSY
+ && retval != -EIDRM)
+ dev_warn(&io->dev->dev, "%s, unlink --> %d\n",
+__func__, retval);
}
- spin_unlock_irqrestore(&io->lock, flags);
}
EXPORT_SYMBOL_GPL(usb_sg_cancel);
--
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
[PATCH 1/2] 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 | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 8e641b5..e13a2e5 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;
- 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);
+
+ retval = usb_submit_urb(io->urbs[i], GFP_NOIO);
+
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
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
high interrupt latency due to usb_sg_wait()
[Second transmission; hopefully this one will go through...] Alan, How about the attached patch? Works for me but definitely needs more review and testing. --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
[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
