From: smoch@web.de (Soeren Moch)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] USB: EHCI: fix for leaking isochronous data
Date: Thu, 21 Mar 2013 18:04:59 +0100 [thread overview]
Message-ID: <514B3DBB.3060302@web.de> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1303171320560.26486-100000@netrider.rowland.org>
On 03/17/13 18:36, Alan Stern wrote:
> On Sun, 17 Mar 2013, Soeren Moch wrote:
>
>> For each device only one isochronous endpoint is used (EP IN4, 1x 940
>> Bytes, Interval 1).
>> When the ENOMEM error occurs, a huge number of iTDs is in the free_list
>> of one stream. This number is much higher than the 2*M entries, which
>> should be there according to your description.
>
> Okay, but how did they get there? With each URB requiring 9 iTDs, and
> about 5 URBs active at any time, there should be about 5*9 = 45 iTDs in
> use and 2*9 = 18 iTDs on the free list. By the time each URB
> completes, it should have released all 9 iTDs back to the free list,
> and each time an URB is submitted, it should be able to acquire all 9
> of the iTDs that it needs from the free list -- it shouldn't have to
> allocate any from the DMA pool.
>
> Looks like you'll have to investigate what's going on inside
> itd_urb_transaction(). Print out some useful information whenever the
> size of stream->free_list is above 50, such as the value of num_itds,
> how many of the loop iterations could get an iTD from the free list,
> and the value of itd->frame in the case where the "goto alloc_itd"
> statement is followed.
>
> It might be a good idea also to print out the size of the free list in
> itd_complete(), where it calls ehci_urb_done(), and include the value
> of ehci->now_frame.
>
Now I found out what is going on here:
In itd_urb_transaction() we allocate 9 iTDs for each URB with
number_of_packets == 64 in my case. The iTDs are added to
sched->td_list. For a frame-aligned scheduling we need 8 iTDs, the 9th
one is released back to the front of the streams free_list in
iso_sched_free(). This iTD was cleared after allocation and has a frame
number of 0 now. So for each allocation when now_frame == 0 we allocate
from the dma_pool, not from the free_list. The attached patch
invalidates the frame number in each iTD before it is sent to the
scheduler. This fixes the problem without the need to iterate over a iTD
list.
Signed-off-by: Soeren Moch <smoch@web.de>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehci.diff
Type: text/x-patch
Size: 603 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130321/5c488fe2/attachment.bin>
WARNING: multiple messages have this Message-ID (diff)
From: Soeren Moch <smoch@web.de>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Arnd Bergmann <arnd@arndb.de>,
USB list <linux-usb@vger.kernel.org>,
Jason Cooper <jason@lakedaemon.net>, Andrew Lunn <andrew@lunn.ch>,
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
linux-mm@kvack.org,
Kernel development list <linux-kernel@vger.kernel.org>,
linux-arm-kernel@lists.infradead.org,
michael@amarulasolutions.com
Subject: Re: [PATCH] USB: EHCI: fix for leaking isochronous data
Date: Thu, 21 Mar 2013 18:04:59 +0100 [thread overview]
Message-ID: <514B3DBB.3060302@web.de> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1303171320560.26486-100000@netrider.rowland.org>
[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]
On 03/17/13 18:36, Alan Stern wrote:
> On Sun, 17 Mar 2013, Soeren Moch wrote:
>
>> For each device only one isochronous endpoint is used (EP IN4, 1x 940
>> Bytes, Interval 1).
>> When the ENOMEM error occurs, a huge number of iTDs is in the free_list
>> of one stream. This number is much higher than the 2*M entries, which
>> should be there according to your description.
>
> Okay, but how did they get there? With each URB requiring 9 iTDs, and
> about 5 URBs active at any time, there should be about 5*9 = 45 iTDs in
> use and 2*9 = 18 iTDs on the free list. By the time each URB
> completes, it should have released all 9 iTDs back to the free list,
> and each time an URB is submitted, it should be able to acquire all 9
> of the iTDs that it needs from the free list -- it shouldn't have to
> allocate any from the DMA pool.
>
> Looks like you'll have to investigate what's going on inside
> itd_urb_transaction(). Print out some useful information whenever the
> size of stream->free_list is above 50, such as the value of num_itds,
> how many of the loop iterations could get an iTD from the free list,
> and the value of itd->frame in the case where the "goto alloc_itd"
> statement is followed.
>
> It might be a good idea also to print out the size of the free list in
> itd_complete(), where it calls ehci_urb_done(), and include the value
> of ehci->now_frame.
>
Now I found out what is going on here:
In itd_urb_transaction() we allocate 9 iTDs for each URB with
number_of_packets == 64 in my case. The iTDs are added to
sched->td_list. For a frame-aligned scheduling we need 8 iTDs, the 9th
one is released back to the front of the streams free_list in
iso_sched_free(). This iTD was cleared after allocation and has a frame
number of 0 now. So for each allocation when now_frame == 0 we allocate
from the dma_pool, not from the free_list. The attached patch
invalidates the frame number in each iTD before it is sent to the
scheduler. This fixes the problem without the need to iterate over a iTD
list.
Signed-off-by: Soeren Moch <smoch@web.de>
[-- Attachment #2: ehci.diff --]
[-- Type: text/x-patch, Size: 603 bytes --]
--- linux-3.9.0-rc3-guru/drivers/usb/host/ehci-sched.c.orig 2013-03-21 17:36:21.000000000 +0100
+++ linux-3.9.0-rc3-guru/drivers/usb/host/ehci-sched.c 2013-03-21 17:38:56.000000000 +0100
@@ -1214,6 +1214,7 @@ itd_urb_transaction (
memset (itd, 0, sizeof *itd);
itd->itd_dma = itd_dma;
+ itd->frame = -1;
list_add (&itd->itd_list, &sched->td_list);
}
spin_unlock_irqrestore (&ehci->lock, flags);
@@ -1915,6 +1916,7 @@ sitd_urb_transaction (
memset (sitd, 0, sizeof *sitd);
sitd->sitd_dma = sitd_dma;
+ sitd->frame = -1;
list_add (&sitd->sitd_list, &iso_sched->td_list);
}
next prev parent reply other threads:[~2013-03-21 17:04 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <Pine.LNX.4.44L0.1302211337580.1529-100000@iolanthe.rowland.org>
2013-03-10 18:45 ` [PATCH] USB: EHCI: fix for leaking isochronous data Soeren Moch
2013-03-10 18:45 ` Soeren Moch
2013-03-10 18:45 ` Soeren Moch
2013-03-10 20:59 ` Alan Stern
2013-03-10 20:59 ` Alan Stern
2013-03-10 20:59 ` Alan Stern
2013-03-14 18:48 ` Soeren Moch
2013-03-14 18:48 ` Soeren Moch
2013-03-14 18:48 ` Soeren Moch
2013-03-14 20:22 ` Soeren Moch
2013-03-14 20:22 ` Soeren Moch
2013-03-14 20:22 ` Soeren Moch
2013-03-14 20:32 ` Alan Stern
2013-03-14 20:32 ` Alan Stern
2013-03-14 20:32 ` Alan Stern
2013-03-14 20:51 ` Soeren Moch
2013-03-14 20:51 ` Soeren Moch
2013-03-14 21:33 ` Alan Stern
2013-03-14 21:33 ` Alan Stern
2013-03-14 21:33 ` Alan Stern
2013-03-15 0:00 ` Soeren Moch
2013-03-15 0:00 ` Soeren Moch
2013-03-15 0:00 ` Soeren Moch
2013-03-15 14:30 ` Alan Stern
2013-03-15 14:30 ` Alan Stern
2013-03-15 14:30 ` Alan Stern
2013-03-16 2:10 ` Soeren Moch
2013-03-16 2:10 ` Soeren Moch
2013-03-16 2:10 ` Soeren Moch
2013-03-16 17:39 ` Alan Stern
2013-03-16 17:39 ` Alan Stern
2013-03-16 17:39 ` Alan Stern
2013-03-17 16:56 ` Soeren Moch
2013-03-17 16:56 ` Soeren Moch
2013-03-17 16:56 ` Soeren Moch
2013-03-17 17:36 ` Alan Stern
2013-03-17 17:36 ` Alan Stern
2013-03-17 17:36 ` Alan Stern
2013-03-17 17:39 ` Alan Stern
2013-03-17 17:39 ` Alan Stern
2013-03-17 17:39 ` Alan Stern
2013-03-21 17:04 ` Soeren Moch [this message]
2013-03-21 17:04 ` Soeren Moch
2013-03-21 17:33 ` Jason Cooper
2013-03-21 17:33 ` Jason Cooper
2013-03-21 17:33 ` Jason Cooper
2013-03-21 19:10 ` Arnd Bergmann
2013-03-21 19:10 ` Arnd Bergmann
2013-03-21 19:10 ` Arnd Bergmann
2013-03-21 19:34 ` Michael Trimarchi
2013-03-21 19:34 ` Michael Trimarchi
2013-03-21 19:34 ` Michael Trimarchi
2013-03-21 21:52 ` Soeren Moch
2013-03-21 21:52 ` Soeren Moch
2013-03-21 21:52 ` Soeren Moch
2013-03-21 21:06 ` Alan Stern
2013-03-21 21:06 ` Alan Stern
2013-03-21 21:06 ` Alan Stern
2013-03-21 21:12 ` Alan Stern
2013-03-21 21:12 ` Alan Stern
2013-03-21 21:12 ` Alan Stern
2013-03-21 21:20 ` Andrew Lunn
2013-03-21 21:20 ` Andrew Lunn
2013-03-21 21:20 ` Andrew Lunn
2013-03-21 22:16 ` Soeren Moch
2013-03-21 22:16 ` Soeren Moch
2013-03-21 22:16 ` Soeren Moch
2013-03-22 14:24 ` Alan Stern
2013-03-22 14:24 ` Alan Stern
2013-03-22 14:24 ` Alan Stern
2013-03-21 21:45 ` Soeren Moch
2013-03-21 21:45 ` Soeren Moch
2013-03-21 21:45 ` Soeren Moch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=514B3DBB.3060302@web.de \
--to=smoch@web.de \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.