firewire: fw-ohci: plug dma memory leak in AR handler
PATCH 1/1 firewire: fw-ohci: plug dma memory leak in AR handler
There's a nasty memory leak in firewire-ohci's ar_context_tasklet(), in that we're not freeing up some of the memory we use for each ar_buffer, due to a moving pointer. The problem has been there for a while, but didn't start to be noticed until we were doing a coherent allocation for the ar_buffer -- meaning we have a smaller pool of memory to work with now, so the problem crops up sooner. The manifestation of this comes after doing a bunch of I/O to a firewire disk, which eventually stalls, and this starts spewing to the console:
PCI-DMA: Out of IOMMU space for 53248 bytes at device 0000:04:09.0
The device there is one of my FireWire controllers trying to do I/O. The host is a fairly new rev. opteron.
Just need to make sure we're freeing the correct memory range is pass through ar_context_tasklet to fix it. Probably something we ought to sneak into 2.6.25 if its still doable...
---
drivers/firewire/fw-ohci.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
drivers/firewire/fw-ohci.c +5 -4
@@ -579,7 +579,8 @@ static void ar_context_tasklet(unsigned long data)if (d->res_count ==0 ) {size_t size, rest, offset; -dma_addr_t buffer_bus; +dma_addr_t start_bus; +void *start;/* * This descriptor is finished and we may have a
@@ -588,9 +589,9 @@ static void ar_context_tasklet(unsigned long data)*/ offset = offsetof(struct ar_buffer , data); - buffer_bus =le32_to_cpu (ab->descriptor.data_address) - offset; + start = buffer = ab; + start_bus =le32_to_cpu (ab->descriptor.data_address) - offset; - buffer = ab; ab = ab->next; d = &ab->descriptor; size = buffer +PAGE_SIZE - ctx->pointer;
@@ -605,7 +606,7 @@ static void ar_context_tasklet(unsigned long data) buffer =handle_ar_packet (ctx, buffer);dma_free_coherent (ohci->card.device,PAGE_SIZE , - buffer, buffer_bus); + start, start_bus);ar_context_add_page (ctx); }else { buffer = ctx->pointer;
Signature · 2 lines
Jarod Wilson [email protected]
Quoted 1 line · Jarod Wilson
Just need to make sure we're freeing the correct memory
That would be a plus. :-)
Quoted 1 line
Probably something we ought to sneak into 2.6.25 if its still doable...
Looks good and initial testing here is fine. I don't have a board with IOMMU though. Will look over it once more tomorrow, then submit it.
Signature · 3 lines
Stefan Richter -=====-==--- --== ==--= http://arcgraph.de/sr/
drivers/firewire/fw-ohci.c static void ar_context_tasklet(unsigned long data) · line 610
Quoted 7 lines · Jarod Wilson
drivers/firewire/fw-ohci.c +1
-1@@ -605,7 +606,7 @@ static void ar_context_tasklet(unsigned long data) buffer =handle_ar_packet (ctx, buffer);dma_free_coherent (ohci->card.device,PAGE_SIZE , - buffer, buffer_bus); + start, start_bus);ar_context_add_page (ctx);
On the other hand, why do we free a page + allocate a page? Why don't we re-initialize and re-add the old page?
Signature · 3 lines
Stefan Richter -=====-==--- --== ==-=- http://arcgraph.de/sr/
Quoted 11 lines · Stefan Richter
drivers/firewire/fw-ohci.c static void ar_context_tasklet(unsigned long data) · line 610
Quoted 7 lines · Jarod Wilson
drivers/firewire/fw-ohci.c +1
-1@@ -605,7 +606,7 @@ static void ar_context_tasklet(unsigned long data) buffer =handle_ar_packet (ctx, buffer);dma_free_coherent (ohci->card.device,PAGE_SIZE , - buffer, buffer_bus); + start, start_bus);ar_context_add_page (ctx);On the other hand, why do we free a page + allocate a page? Why don't we re-initialize and re-add the old page?
Oh good, I'm not crazy (outside of having firewire on the brain way too much right now). I had that same thought tossing and turning in bed late last night. :)
Signature · 2 lines
Jarod Wilson [email protected]
Quoted 11 lines · I wrote:
Review static void ar_context_tasklet(unsigned long data) · line 610
Quoted 7 lines · Jarod Wilson
(quoted diff) +1
-1@@ -605,7 +606,7 @@ static void ar_context_tasklet(unsigned long data) buffer = handle_ar_packet(ctx, buffer); dma_free_coherent(ohci->card.device, PAGE_SIZE, - buffer, buffer_bus); + start, start_bus); ar_context_add_page(ctx);On the other hand, why do we free a page + allocate a page? Why don't we re-initialize and re-add the old page?
Meanwhile I tried a simple modification to ar_context_add_page and its callers which results in _add_page simply re-adding the old page. I must do something fundamentally wrong though.
After plugging in a FW disk and starting hdparm -tT, I get the modified _add_page called for the ar_request_ctx, then for the ar_response_ctx, then for the ar_request_ctx again, then everything stalls in one of these modes:
- No status write request reception is logged anymore, or
- status write request reception with evt_no_status is logged.
The number of _add_page calls for ar_request_ctx until failure corresponds to the number of pages added in ar_context_init. (Normally two, I also tried three and four.)
Just FYI, here is basically what I tested, with a debug printk in it.
--- drivers/firewire/fw-ohci.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-)
drivers/firewire/fw-ohci.c +15 -19
@@ -451,14 +451,19 @@ ohci_update_phy_reg(struct fw_card *cardreturn 0 ; } -static int ar_context_add_page (struct ar_context *ctx) +static int ar_context_add_page (struct ar_context *ctx,struct ar_buffer *ab) {struct device *dev = ctx->ohci->card.device; -struct ar_buffer *ab;dma_addr_t uninitialized_var (ab_bus ); -size_t offset; +size_t offset = offsetof(struct ar_buffer , data); - ab =dma_alloc_coherent (dev,PAGE_SIZE , &ab_bus,GFP_ATOMIC ); +if (ab ==NULL ) + ab =dma_alloc_coherent (dev,PAGE_SIZE , &ab_bus,GFP_KERNEL ); +else { + ab_bus =le32_to_cpu (ab->descriptor.data_address) - offset; +fw_notify ("=== %s ===\n" , + ctx == &ctx->ohci->ar_request_ctx ?"Req " :"Resp" ); + }if (ab ==NULL )return -ENOMEM ;
@@ -466,7 +471,6 @@ static int ar_context_add_page(struct ar ab->descriptor.control =cpu_to_le16 (DESCRIPTOR_INPUT_MORE |DESCRIPTOR_STATUS |DESCRIPTOR_BRANCH_ALWAYS ); - offset = offsetof(struct ar_buffer , data); ab->descriptor.req_count =cpu_to_le16 (PAGE_SIZE - offset); ab->descriptor.data_address =cpu_to_le32 (ab_bus + offset); ab->descriptor.res_count =cpu_to_le16 (PAGE_SIZE - offset);
@@ -569,8 +573,7 @@ static __le32 *handle_ar_packet(struct astatic void ar_context_tasklet (unsigned long data) {struct ar_context *ctx = (struct ar_context *)data; -struct fw_ohci *ohci = ctx->ohci; -struct ar_buffer *ab; +struct ar_buffer *ab, *old_ab;struct descriptor *d;void *buffer, *end;
@@ -578,9 +581,7 @@ static void ar_context_tasklet(unsigned d = &ab->descriptor;if (d->res_count ==0 ) { -size_t size, rest, offset; -dma_addr_t start_bus; -void *start; +size_t size, rest;/* * This descriptor is finished and we may have a
@@ -588,10 +589,7 @@ static void ar_context_tasklet(unsigned* reuse the page for reassembling the split packet. */ - offset = offsetof(struct ar_buffer , data); - start = buffer = ab; - start_bus =le32_to_cpu (ab->descriptor.data_address) - offset; - + buffer = old_ab = ab; ab = ab->next; d = &ab->descriptor; size = buffer +PAGE_SIZE - ctx->pointer;
@@ -605,9 +603,7 @@ static void ar_context_tasklet(unsignedwhile (buffer < end) buffer =handle_ar_packet (ctx, buffer); -dma_free_coherent (ohci->card.device,PAGE_SIZE , - start, start_bus); -ar_context_add_page (ctx); +ar_context_add_page (ctx, old_ab); }else { buffer = ctx->pointer; ctx->pointer = end =
@@ -628,8 +624,8 @@ ar_context_init(struct ar_context *ctx, ctx->last_buffer = &ab;tasklet_init (&ctx->tasklet, ar_context_tasklet, (unsigned long )ctx); -ar_context_add_page (ctx); -ar_context_add_page (ctx); +ar_context_add_page (ctx,NULL ); +ar_context_add_page (ctx,NULL ); ctx->current_buffer = ab.next; ctx->pointer = ctx->current_buffer->data;
Signature · 3 lines
Stefan Richter -=====-==--- --== ==-== http://arcgraph.de/sr/
Quoted 7 lines · I wrote:
Quoted 2 lines · I wrote:
On the other hand, why do we free a page + allocate a page? Why don't we re-initialize and re-add the old page?
Meanwhile I tried a simple modification to
ar_context_add_pageand its callers which results in_add_pagesimply re-adding the old page. I must do something fundamentally wrong though.
Besides, the current code which reassembles packets that reach into the next buffer is broken for packets whose total size approaches PAGE_SIZE. (Remember, async packets can be sized 4kB + 1394 headers + OHCI trailer.) Reminds me of ohci1394 somehow. :-(
I will attempt to fix this for post 2.6.25, unless you aspire to do so.
Signature · 3 lines
Stefan Richter -=====-==--- --== ==-== http://arcgraph.de/sr/
Quoted 9 lines · Jarod Wilson
There's a nasty memory leak in firewire-ohci's
ar_context_tasklet(), in that we're not freeing up some of the memory we use for eachar_buffer, due to a moving pointer. The problem has been there for a while, but didn't start to be noticed until we were doing a coherent allocation for thear_buffer-- meaning we have a smaller pool of memory to work with now, so the problem crops up sooner. The manifestation of this comes after doing a bunch of I/O to a firewire disk, which eventually stalls, and this starts spewing to the console:
Quoted 8 lines
PCI-DMA: Out of IOMMU space for 53248 bytes at device 0000:04:09.0
The device there is one of my FireWire controllers trying to do I/O. The host is a fairly new rev. opteron.
Just need to make sure we're freeing the correct memory range is pass through
ar_context_taskletto fix it. Probably something we ought to sneak into 2.6.25 if its still doable...
So as it turns out, while this is indeed a leak that needs to be plugged, it does NOT remedy the 'out of iommu space' issue, it just delays it a while longer. Still working on tracing the root cause of the memory exhaustion.
Signature · 2 lines
Jarod Wilson [email protected]
Quoted 1 line · Jarod Wilson
So as it turns out, while this is indeed a leak that needs to be plugged, it does NOT remedy the 'out of iommu space' issue, it just delays it a while longer. Still working on tracing the root cause of the memory exhaustion.
Do you want to change the wording of the patch description before I submit it upstream?
Signature · 3 lines
Stefan Richter -=====-==--- --== ==-== http://arcgraph.de/sr/
Quoted 8 lines · Stefan Richter
Quoted 4 lines · Jarod Wilson
So as it turns out, while this is indeed a leak that needs to be plugged, it does NOT remedy the 'out of iommu space' issue, it just delays it a while longer. Still working on tracing the root cause of the memory exhaustion.
Do you want to change the wording of the patch description before I submit it upstream?
Yeah, I'll whip something up in just a sec and get it out the door...
Signature · 2 lines
Jarod Wilson [email protected]