On 2022-05-31 19:22, Tony Battersby wrote:
Remove a small amount of code duplication between dma_pool_destroy() and
pool_free_page() in preparation for adding more code without having to
duplicate it. No functional changes.
Signed-off-by: Tony Battersby <tonyb@xxxxxxxxxxxxxxx>
---
mm/dmapool.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 8749a9d7927e..58c11dcaa4e4 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -250,14 +250,25 @@ 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)
+static void pool_free_page(struct dma_pool *pool,
+ struct dma_page *page,
+ bool destroying_pool)
{
+ void *vaddr = page->vaddr;
dma_addr_t dma = page->dma;
+ if (destroying_pool && is_page_busy(page)) {
+ dev_err(pool->dev,
+ "dma_pool_destroy %s, %p busy\n",
+ pool->name, vaddr);
+ /* leak the still-in-use consistent memory */
+ } else {
#ifdef DMAPOOL_DEBUG
- memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
+ memset(vaddr, POOL_POISON_FREED, pool->allocation);
#endif
- dma_free_coherent(pool->dev, pool->allocation, page->vaddr, dma);
+ dma_free_coherent(pool->dev, pool->allocation, vaddr, dma);
+ }
+
list_del(&page->page_list);
If we're tearing down the whole pool, surely we can skip this as well?
(Same for the second list in patch #9)
In fact I think it might make more sense to refactor in the opposite
direction and just streamline this directly into dma_pool_destroy(),
more like:
list_for_each_entry_safe() {
if (is_page_busy()) {
dev_err();
} else {
dma_free_coherent();
}
kfree(page);
}
kfree(page);
}
@@ -272,7 +283,7 @@ static void pool_free_page(struct dma_pool *pool, struct dma_page *page)
*/
void dma_pool_destroy(struct dma_pool *pool)
{
- struct dma_page *page, *tmp;
+ struct dma_page *page;
Nit: you bring this back again in patch #10, so we may as well leave the
list_for_each_entry_safe() iterator in place until then as well, and
save a bit of churn in this patch.
bool empty = false;
if (unlikely(!pool))
@@ -288,15 +299,10 @@ void dma_pool_destroy(struct dma_pool *pool)
device_remove_file(pool->dev, &dev_attr_pools);
mutex_unlock(&pools_reg_lock);
- list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
- if (is_page_busy(page)) {
- 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);
+ while ((page = list_first_entry_or_null(&pool->page_list,
+ struct dma_page,
+ page_list))) {
+ pool_free_page(pool, page, true);
}
kfree(pool);
@@ -469,7 +475,7 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
page->offset = offset;
/*
* Resist a temptation to do
- * if (!is_page_busy(page)) pool_free_page(pool, page);
+ * if (!is_page_busy(page)) pool_free_page(pool, page, false);
Further to the above, even if we did retain a separate function, if an
argument is hard-coded at the one single callsite, and the only
reference to passing any other value is in a comment effectively saying
"don't do this", do we really need to pretend it's an argument at all? ;)
FWIW I'd just reword the comment in more general terms, e.g. "Resist the
temptation to free unused pages immediately..."
Thanks,
Robin.
* Better have a few empty pages hang around.
*/
spin_unlock_irqrestore(&pool->lock, flags);
