[PATCHv2,09/11] dmapool: simplify freeing
Commit Message
From: Keith Busch <kbusch@kernel.org>
The actions for busy and not busy are mostly the same, so combine these
and remove the unnecessary function. Also, the pool is about to be freed
so there's no need to poison the page data since we only check for
poison on alloc, which can't be done on a freed pool.
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
mm/dmapool.c | 29 ++++++-----------------------
1 file changed, 6 insertions(+), 23 deletions(-)
Comments
> @@ -280,14 +268,14 @@ void dma_pool_destroy(struct dma_pool *pool)
> mutex_unlock(&pools_reg_lock);
>
> list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
> + if (!is_page_busy(page))
> + dma_free_coherent(pool->dev, pool->allocation,
> + page->vaddr, page->dma);
> + else
> dev_err(pool->dev, "%s %s, %p busy\n", __func__,
> pool->name, page->vaddr);
> + list_del(&page->page_list);
> + kfree(page);
Hmm. The is_page_busy case is really a should not happen case.
What is the benefit of skipping the dma_free_coherent and leaking
memory here, vs letting KASAN and friends see the free and possibly
help with debugging? In other words, why is this not:
WARN_ON_ONCE(is_page_busy(page));
dma_free_coherent(pool->dev, pool->allocation, page->vaddr,
page->dma);
...
> page->in_use--;
> *(int *)vaddr = page->offset;
> page->offset = offset;
> - /*
> - * Resist a temptation to do
> - * if (!is_page_busy(page)) pool_free_page(pool, page);
> - * Better have a few empty pages hang around.
> - */
This doesn't look related to the rest, or am I missing something?
On Fri, Dec 23, 2022 at 08:38:55AM -0800, Christoph Hellwig wrote:
> > @@ -280,14 +268,14 @@ void dma_pool_destroy(struct dma_pool *pool)
> > mutex_unlock(&pools_reg_lock);
> >
> > list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
> > + if (!is_page_busy(page))
> > + dma_free_coherent(pool->dev, pool->allocation,
> > + page->vaddr, page->dma);
> > + else
> > dev_err(pool->dev, "%s %s, %p busy\n", __func__,
> > pool->name, page->vaddr);
> > + list_del(&page->page_list);
> > + kfree(page);
>
> Hmm. The is_page_busy case is really a should not happen case.
> What is the benefit of skipping the dma_free_coherent and leaking
> memory here, vs letting KASAN and friends see the free and possibly
> help with debugging? In other words, why is this not:
>
> WARN_ON_ONCE(is_page_busy(page));
> dma_free_coherent(pool->dev, pool->allocation, page->vaddr,
> page->dma);
> ...
The memory is presumed to still be owned by the device in this case, so
the kernel shouldn't free it. I don't think KASAN will be very helpful
if the device corrupts memory.
> > page->in_use--;
> > *(int *)vaddr = page->offset;
> > page->offset = offset;
> > - /*
> > - * Resist a temptation to do
> > - * if (!is_page_busy(page)) pool_free_page(pool, page);
> > - * Better have a few empty pages hang around.
> > - */
>
> This doesn't look related to the rest, or am I missing something?
Oops, this was supposed to go with a later patch in this series that
removed "is_page_busy()".
@@ -241,18 +241,6 @@ static inline bool is_page_busy(struct dma_page *page)
return page->in_use != 0;
}
-static void pool_free_page(struct dma_pool *pool, struct dma_page *page)
-{
- dma_addr_t dma = page->dma;
-
-#ifdef DMAPOOL_DEBUG
- memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
-#endif
- dma_free_coherent(pool->dev, pool->allocation, page->vaddr, dma);
- list_del(&page->page_list);
- kfree(page);
-}
-
/**
* dma_pool_destroy - destroys a pool of dma memory blocks.
* @pool: dma pool that will be destroyed
@@ -280,14 +268,14 @@ void dma_pool_destroy(struct dma_pool *pool)
mutex_unlock(&pools_reg_lock);
list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
- if (is_page_busy(page)) {
+ if (!is_page_busy(page))
+ dma_free_coherent(pool->dev, pool->allocation,
+ page->vaddr, page->dma);
+ else
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);
- } else
- pool_free_page(pool, page);
+ list_del(&page->page_list);
+ kfree(page);
}
kfree(pool);
@@ -445,11 +433,6 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
page->in_use--;
*(int *)vaddr = page->offset;
page->offset = offset;
- /*
- * Resist a temptation to do
- * if (!is_page_busy(page)) pool_free_page(pool, page);
- * Better have a few empty pages hang around.
- */
spin_unlock_irqrestore(&pool->lock, flags);
}
EXPORT_SYMBOL(dma_pool_free);