From: Keith Busch <[email protected]>
Time spent in dma_pool alloc/free increases linearly with the number of
pages backing the pool. We can reduce this to constant time with minor
changes to how free pages are tracked.
Changes since v2:
Added received reviews
Applied suggestions from Christoph (removed inlines, use preferred
conditional compiling style, minor changes in patch sequence, use
kzalloc)
Fixed printf formats caught by kernel test robot
Added one extra cleanup patch at the end
Keith Busch (8):
dmapool: add alloc/free performance test
dmapool: move debug code to own functions
dmapool: rearrange page alloc failure handling
dmapool: consolidate page initialization
dmapool: simplify freeing
dmapool: don't memset on free twice
dmapool: link blocks across pages
dmapool: create/destroy cleanup
Tony Battersby (4):
dmapool: remove checks for dev == NULL
dmapool: use sysfs_emit() instead of scnprintf()
dmapool: cleanup integer types
dmapool: speedup DMAPOOL_DEBUG with init_on_alloc
mm/Kconfig | 9 ++
mm/Makefile | 1 +
mm/dmapool.c | 371 ++++++++++++++++++++++------------------------
mm/dmapool_test.c | 147 ++++++++++++++++++
4 files changed, 331 insertions(+), 197 deletions(-)
create mode 100644 mm/dmapool_test.c
--
2.30.2
From: Tony Battersby <[email protected]>
Avoid double-memset of the same allocated memory in dma_pool_alloc()
when both DMAPOOL_DEBUG is enabled and init_on_alloc=1.
Signed-off-by: Tony Battersby <[email protected]>
Signed-off-by: Keith Busch <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
mm/dmapool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index ee993bb59fc27..eaed3ffb42aa8 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -356,7 +356,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
break;
}
}
- if (!(mem_flags & __GFP_ZERO))
+ if (!want_init_on_alloc(mem_flags))
memset(retval, POOL_POISON_ALLOCATED, pool->size);
#endif
spin_unlock_irqrestore(&pool->lock, flags);
--
2.30.2
From: Tony Battersby <[email protected]>
dmapool originally tried to support pools without a device because
dma_alloc_coherent() supports allocations without a device. But nobody
ended up using dma pools without a device, and trying to do so will
result in an oops. So remove the checks for pool->dev == NULL since they
are unneeded bloat.
Signed-off-by: Tony Battersby <[email protected]>
[added check for null dev on create]
Signed-off-by: Keith Busch <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
mm/dmapool.c | 45 ++++++++++++++-------------------------------
1 file changed, 14 insertions(+), 31 deletions(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index a7eb5d0eb2da7..559207e1c3339 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -134,6 +134,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
size_t allocation;
bool empty = false;
+ if (!dev)
+ return NULL;
+
if (align == 0)
align = 1;
else if (align & (align - 1))
@@ -275,7 +278,7 @@ void dma_pool_destroy(struct dma_pool *pool)
mutex_lock(&pools_reg_lock);
mutex_lock(&pools_lock);
list_del(&pool->pools);
- if (pool->dev && list_empty(&pool->dev->dma_pools))
+ if (list_empty(&pool->dev->dma_pools))
empty = true;
mutex_unlock(&pools_lock);
if (empty)
@@ -284,12 +287,8 @@ void dma_pool_destroy(struct dma_pool *pool)
list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
if (is_page_busy(page)) {
- if (pool->dev)
- dev_err(pool->dev, "%s %s, %p busy\n", __func__,
- pool->name, page->vaddr);
- else
- pr_err("%s %s, %p busy\n", __func__,
- pool->name, page->vaddr);
+ dev_err(pool->dev, "%s %s, %p busy\n", __func__,
+ pool->name, page->vaddr);
/* leak the still-in-use consistent memory */
list_del(&page->page_list);
kfree(page);
@@ -351,12 +350,8 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
for (i = sizeof(page->offset); i < pool->size; i++) {
if (data[i] == POOL_POISON_FREED)
continue;
- if (pool->dev)
- dev_err(pool->dev, "%s %s, %p (corrupted)\n",
- __func__, pool->name, retval);
- else
- pr_err("%s %s, %p (corrupted)\n",
- __func__, pool->name, retval);
+ dev_err(pool->dev, "%s %s, %p (corrupted)\n",
+ __func__, pool->name, retval);
/*
* Dump the first 4 bytes even if they are not
@@ -411,12 +406,8 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
page = pool_find_page(pool, dma);
if (!page) {
spin_unlock_irqrestore(&pool->lock, flags);
- if (pool->dev)
- dev_err(pool->dev, "%s %s, %p/%pad (bad dma)\n",
- __func__, pool->name, vaddr, &dma);
- else
- pr_err("%s %s, %p/%pad (bad dma)\n",
- __func__, pool->name, vaddr, &dma);
+ dev_err(pool->dev, "%s %s, %p/%pad (bad dma)\n",
+ __func__, pool->name, vaddr, &dma);
return;
}
@@ -426,12 +417,8 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
#ifdef DMAPOOL_DEBUG
if ((dma - page->dma) != offset) {
spin_unlock_irqrestore(&pool->lock, flags);
- if (pool->dev)
- dev_err(pool->dev, "%s %s, %p (bad vaddr)/%pad\n",
- __func__, pool->name, vaddr, &dma);
- else
- pr_err("%s %s, %p (bad vaddr)/%pad\n",
- __func__, pool->name, vaddr, &dma);
+ dev_err(pool->dev, "%s %s, %p (bad vaddr)/%pad\n",
+ __func__, pool->name, vaddr, &dma);
return;
}
{
@@ -442,12 +429,8 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
continue;
}
spin_unlock_irqrestore(&pool->lock, flags);
- if (pool->dev)
- dev_err(pool->dev, "%s %s, dma %pad already free\n",
- __func__, pool->name, &dma);
- else
- pr_err("%s %s, dma %pad already free\n",
- __func__, pool->name, &dma);
+ dev_err(pool->dev, "%s %s, dma %pad already free\n",
+ __func__, pool->name, &dma);
return;
}
}
--
2.30.2
From: Keith Busch <[email protected]>
Clean up the normal path by moving the debug code outside it.
Signed-off-by: Keith Busch <[email protected]>
---
mm/dmapool.c | 113 +++++++++++++++++++++++++++++++--------------------
1 file changed, 68 insertions(+), 45 deletions(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index eaed3ffb42aa8..7bd8990e1913d 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -294,6 +294,38 @@ void dma_pool_destroy(struct dma_pool *pool)
}
EXPORT_SYMBOL(dma_pool_destroy);
+#ifdef DMAPOOL_DEBUG
+static void pool_check_block(struct dma_pool *pool, void *retval,
+ unsigned int offset, gfp_t mem_flags)
+{
+ int i;
+ u8 *data = retval;
+ /* page->offset is stored in first 4 bytes */
+ for (i = sizeof(offset); i < pool->size; i++) {
+ if (data[i] == POOL_POISON_FREED)
+ continue;
+ dev_err(pool->dev, "%s %s, %p (corrupted)\n",
+ __func__, pool->name, retval);
+
+ /*
+ * Dump the first 4 bytes even if they are not
+ * POOL_POISON_FREED
+ */
+ print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 16, 1,
+ data, pool->size, 1);
+ break;
+ }
+ if (!want_init_on_alloc(mem_flags))
+ memset(retval, POOL_POISON_ALLOCATED, pool->size);
+}
+#else
+static void pool_check_block(struct dma_pool *pool, void *retval,
+ unsigned int offset, gfp_t mem_flags)
+
+{
+}
+#endif
+
/**
* dma_pool_alloc - get a block of consistent memory
* @pool: dma pool that will produce the block
@@ -336,29 +368,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
page->offset = *(int *)(page->vaddr + offset);
retval = offset + page->vaddr;
*handle = offset + page->dma;
-#ifdef DMAPOOL_DEBUG
- {
- int i;
- u8 *data = retval;
- /* page->offset is stored in first 4 bytes */
- for (i = sizeof(page->offset); i < pool->size; i++) {
- if (data[i] == POOL_POISON_FREED)
- continue;
- dev_err(pool->dev, "%s %s, %p (corrupted)\n",
- __func__, pool->name, retval);
-
- /*
- * Dump the first 4 bytes even if they are not
- * POOL_POISON_FREED
- */
- print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 16, 1,
- data, pool->size, 1);
- break;
- }
- }
- if (!want_init_on_alloc(mem_flags))
- memset(retval, POOL_POISON_ALLOCATED, pool->size);
-#endif
+ pool_check_block(pool, retval, offset, mem_flags);
spin_unlock_irqrestore(&pool->lock, flags);
if (want_init_on_alloc(mem_flags))
@@ -381,6 +391,39 @@ static struct dma_page *pool_find_page(struct dma_pool *pool, dma_addr_t dma)
return NULL;
}
+#ifdef DMAPOOL_DEBUG
+static bool pool_page_err(struct dma_pool *pool, struct dma_page *page,
+ void *vaddr, dma_addr_t dma)
+{
+ unsigned int offset = vaddr - page->vaddr;
+ unsigned int chain = page->offset;
+
+ if ((dma - page->dma) != offset) {
+ dev_err(pool->dev, "%s %s, %p (bad vaddr)/%pad\n",
+ __func__, pool->name, vaddr, &dma);
+ return true;
+ }
+
+ while (chain < pool->allocation) {
+ if (chain != offset) {
+ chain = *(int *)(page->vaddr + chain);
+ continue;
+ }
+ dev_err(pool->dev, "%s %s, dma %pad already free\n",
+ __func__, pool->name, &dma);
+ return true;
+ }
+ memset(vaddr, POOL_POISON_FREED, pool->size);
+ return false;
+}
+#else
+static bool pool_page_err(struct dma_pool *pool, struct dma_page *page,
+ void *vaddr, dma_addr_t dma)
+{
+ return false;
+}
+#endif
+
/**
* dma_pool_free - put block back into dma pool
* @pool: the dma pool holding the block
@@ -394,7 +437,6 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
{
struct dma_page *page;
unsigned long flags;
- unsigned int offset;
spin_lock_irqsave(&pool->lock, flags);
page = pool_find_page(pool, dma);
@@ -405,35 +447,16 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
return;
}
- offset = vaddr - page->vaddr;
if (want_init_on_free())
memset(vaddr, 0, pool->size);
-#ifdef DMAPOOL_DEBUG
- if ((dma - page->dma) != offset) {
+ if (pool_page_err(pool, page, vaddr, dma)) {
spin_unlock_irqrestore(&pool->lock, flags);
- dev_err(pool->dev, "%s %s, %p (bad vaddr)/%pad\n",
- __func__, pool->name, vaddr, &dma);
return;
}
- {
- unsigned int chain = page->offset;
- while (chain < pool->allocation) {
- if (chain != offset) {
- chain = *(int *)(page->vaddr + chain);
- continue;
- }
- spin_unlock_irqrestore(&pool->lock, flags);
- dev_err(pool->dev, "%s %s, dma %pad already free\n",
- __func__, pool->name, &dma);
- return;
- }
- }
- memset(vaddr, POOL_POISON_FREED, pool->size);
-#endif
page->in_use--;
*(int *)vaddr = page->offset;
- page->offset = offset;
+ page->offset = vaddr - page->vaddr;
/*
* Resist a temptation to do
* if (!is_page_busy(page)) pool_free_page(pool, page);
--
2.30.2
From: Tony Battersby <[email protected]>
Use sysfs_emit instead of scnprintf, snprintf or sprintf.
Signed-off-by: Tony Battersby <[email protected]>
Signed-off-by: Keith Busch <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
mm/dmapool.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 559207e1c3339..20616b760bb9c 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -64,18 +64,11 @@ static DEFINE_MUTEX(pools_reg_lock);
static ssize_t pools_show(struct device *dev, struct device_attribute *attr, char *buf)
{
- unsigned temp;
- unsigned size;
- char *next;
+ int size;
struct dma_page *page;
struct dma_pool *pool;
- next = buf;
- size = PAGE_SIZE;
-
- temp = scnprintf(next, size, "poolinfo - 0.1\n");
- size -= temp;
- next += temp;
+ size = sysfs_emit(buf, "poolinfo - 0.1\n");
mutex_lock(&pools_lock);
list_for_each_entry(pool, &dev->dma_pools, pools) {
@@ -90,16 +83,14 @@ static ssize_t pools_show(struct device *dev, struct device_attribute *attr, cha
spin_unlock_irq(&pool->lock);
/* per-pool info, no real statistics yet */
- temp = scnprintf(next, size, "%-16s %4u %4zu %4zu %2u\n",
- pool->name, blocks,
- pages * (pool->allocation / pool->size),
- pool->size, pages);
- size -= temp;
- next += temp;
+ size += sysfs_emit_at(buf, size, "%-16s %4u %4zu %4zu %2u\n",
+ pool->name, blocks,
+ pages * (pool->allocation / pool->size),
+ pool->size, pages);
}
mutex_unlock(&pools_lock);
- return PAGE_SIZE - size;
+ return size;
}
static DEVICE_ATTR_RO(pools);
--
2.30.2
> +#ifdef DMAPOOL_DEBUG
I'd drop the weird tab indent carrier over from the original code here.
Also any reason to not use a single big ifdef blocked instead of
multiple ones?
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>