* Take lock only once in dma_pool_free()
@ 2010-12-20 17:03 Rolf Eike Beer
2011-01-04 20:37 ` Andrew Morton
0 siblings, 1 reply; 2+ messages in thread
From: Rolf Eike Beer @ 2010-12-20 17:03 UTC (permalink / raw)
To: linux-mm, linux-kernel
>From 0db01c2ea9476609c399de3e9fdf7861df07d2f1 Mon Sep 17 00:00:00 2001
From: Rolf Eike Beer <eike-kernel@sf-tec.de>
Date: Mon, 20 Dec 2010 17:29:33 +0100
Subject: [PATCH] Speed up dma_pool_free()
dma_pool_free() scans for the page to free in the pool list holding the pool
lock. Then it releases the lock basically to acquire it immediately again.
Modify the code to only take the lock once.
This will do some additional loops and computations with the lock held in if
memory debugging is activated. If it is not activated the only new operations
with this lock is one if and one substraction.
Signed-off-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
---
mm/dmapool.c | 14 ++++++--------
1 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 4df2de7..a2f6295 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -355,20 +355,15 @@ EXPORT_SYMBOL(dma_pool_alloc);
static struct dma_page *pool_find_page(struct dma_pool *pool, dma_addr_t dma)
{
- unsigned long flags;
struct dma_page *page;
- spin_lock_irqsave(&pool->lock, flags);
list_for_each_entry(page, &pool->page_list, page_list) {
if (dma < page->dma)
continue;
if (dma < (page->dma + pool->allocation))
- goto done;
+ return page;
}
- page = NULL;
- done:
- spin_unlock_irqrestore(&pool->lock, flags);
- return page;
+ return NULL;
}
/**
@@ -386,8 +381,10 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr,
dma_addr_t dma)
unsigned long flags;
unsigned int offset;
+ spin_lock_irqsave(&pool->lock, flags);
page = pool_find_page(pool, dma);
if (!page) {
+ spin_unlock_irqrestore(&pool->lock, flags);
if (pool->dev)
dev_err(pool->dev,
"dma_pool_free %s, %p/%lx (bad dma)\n",
@@ -401,6 +398,7 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr,
dma_addr_t dma)
offset = vaddr - page->vaddr;
#ifdef DMAPOOL_DEBUG
if ((dma - page->dma) != offset) {
+ spin_unlock_irqrestore(&pool->lock, flags);
if (pool->dev)
dev_err(pool->dev,
"dma_pool_free %s, %p (bad vaddr)/%Lx\n",
@@ -418,6 +416,7 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr,
dma_addr_t dma)
chain = *(int *)(page->vaddr + chain);
continue;
}
+ spin_unlock_irqrestore(&pool->lock, flags);
if (pool->dev)
dev_err(pool->dev, "dma_pool_free %s, dma %Lx "
"already free\n", pool->name,
@@ -432,7 +431,6 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr,
dma_addr_t dma)
memset(vaddr, POOL_POISON_FREED, pool->size);
#endif
- spin_lock_irqsave(&pool->lock, flags);
page->in_use--;
*(int *)vaddr = page->offset;
page->offset = offset;
--
1.6.4.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: Take lock only once in dma_pool_free()
2010-12-20 17:03 Take lock only once in dma_pool_free() Rolf Eike Beer
@ 2011-01-04 20:37 ` Andrew Morton
0 siblings, 0 replies; 2+ messages in thread
From: Andrew Morton @ 2011-01-04 20:37 UTC (permalink / raw)
To: Rolf Eike Beer; +Cc: linux-mm, linux-kernel
On Mon, 20 Dec 2010 18:03:06 +0100
Rolf Eike Beer <eike-kernel@sf-tec.de> wrote:
> >From 0db01c2ea9476609c399de3e9fdf7861df07d2f1 Mon Sep 17 00:00:00 2001
> From: Rolf Eike Beer <eike-kernel@sf-tec.de>
> Date: Mon, 20 Dec 2010 17:29:33 +0100
> Subject: [PATCH] Speed up dma_pool_free()
>
> dma_pool_free() scans for the page to free in the pool list holding the pool
> lock. Then it releases the lock basically to acquire it immediately again.
> Modify the code to only take the lock once.
>
> This will do some additional loops and computations with the lock held in if
> memory debugging is activated. If it is not activated the only new operations
> with this lock is one if and one substraction.
>
Fair enough, I guess.
>
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index 4df2de7..a2f6295 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -355,20 +355,15 @@ EXPORT_SYMBOL(dma_pool_alloc);
>
> static struct dma_page *pool_find_page(struct dma_pool *pool, dma_addr_t dma)
> {
> - unsigned long flags;
> struct dma_page *page;
>
> - spin_lock_irqsave(&pool->lock, flags);
> list_for_each_entry(page, &pool->page_list, page_list) {
> if (dma < page->dma)
> continue;
> if (dma < (page->dma + pool->allocation))
> - goto done;
> + return page;
> }
> - page = NULL;
> - done:
> - spin_unlock_irqrestore(&pool->lock, flags);
> - return page;
> + return NULL;
> }
>
> /**
> @@ -386,8 +381,10 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr,
> dma_addr_t dma)
You have some wordwrapping there.
> unsigned long flags;
> unsigned int offset;
>
> + spin_lock_irqsave(&pool->lock, flags);
> page = pool_find_page(pool, dma);
> if (!page) {
> + spin_unlock_irqrestore(&pool->lock, flags);
> if (pool->dev)
> dev_err(pool->dev,
> "dma_pool_free %s, %p/%lx (bad dma)\n",
> @@ -401,6 +398,7 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr,
> dma_addr_t dma)
> offset = vaddr - page->vaddr;
> #ifdef DMAPOOL_DEBUG
> if ((dma - page->dma) != offset) {
> + spin_unlock_irqrestore(&pool->lock, flags);
> if (pool->dev)
> dev_err(pool->dev,
> "dma_pool_free %s, %p (bad vaddr)/%Lx\n",
> @@ -418,6 +416,7 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr,
> dma_addr_t dma)
> chain = *(int *)(page->vaddr + chain);
> continue;
> }
> + spin_unlock_irqrestore(&pool->lock, flags);
> if (pool->dev)
> dev_err(pool->dev, "dma_pool_free %s, dma %Lx "
> "already free\n", pool->name,
> @@ -432,7 +431,6 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr,
> dma_addr_t dma)
> memset(vaddr, POOL_POISON_FREED, pool->size);
> #endif
>
> - spin_lock_irqsave(&pool->lock, flags);
> page->in_use--;
> *(int *)vaddr = page->offset;
> page->offset = offset;
It's a bit scary that the code is playing with the dma_page outside the
lock, but I guess the refcounting takes care of that. As does the
apparently-intentional leakiness of leaving a cache of pages around.
The use of TASK_INTERRUPTIBLE in dma_pool_alloc() looks like a bug -
the code will busywait if signal_pending().
--- a/mm/dmapool.c~a
+++ a/mm/dmapool.c
@@ -324,7 +324,7 @@ void *dma_pool_alloc(struct dma_pool *po
if (mem_flags & __GFP_WAIT) {
DECLARE_WAITQUEUE(wait, current);
- __set_current_state(TASK_INTERRUPTIBLE);
+ __set_current_state(TASK_UNINTERRUPTIBLE);
__add_wait_queue(&pool->waitq, &wait);
spin_unlock_irqrestore(&pool->lock, flags);
_
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2011-01-04 20:37 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-20 17:03 Take lock only once in dma_pool_free() Rolf Eike Beer
2011-01-04 20:37 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).