From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 256BDC433F5 for ; Tue, 31 May 2022 19:33:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8CCE86B0071; Tue, 31 May 2022 15:33:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 851746B0073; Tue, 31 May 2022 15:33:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6CC056B0074; Tue, 31 May 2022 15:33:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 5A3636B0071 for ; Tue, 31 May 2022 15:33:31 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 23D3034454 for ; Tue, 31 May 2022 19:33:31 +0000 (UTC) X-FDA: 79527037422.11.5FBD08C Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf12.hostedemail.com (Postfix) with ESMTP id 7C8E840003 for ; Tue, 31 May 2022 19:32:49 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 72B3723A; Tue, 31 May 2022 12:33:29 -0700 (PDT) Received: from [10.57.81.38] (unknown [10.57.81.38]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EDC683F766; Tue, 31 May 2022 12:33:27 -0700 (PDT) Message-ID: Date: Tue, 31 May 2022 20:33:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH 08/10] dmapool: cleanup dma_pool_destroy Content-Language: en-GB To: Tony Battersby , linux-mm@kvack.org, linux-kernel@vger.kernel.org Cc: iommu@lists.linux-foundation.org, kernel-team@fb.com, Matthew Wilcox , Keith Busch , Andy Shevchenko , Tony Lindgren References: <9b08ab7c-b80b-527d-9adf-7716b0868fbc@cybernetics.com> <30fd23ae-7035-5ce3-5643-89a5956f1e79@cybernetics.com> From: Robin Murphy In-Reply-To: <30fd23ae-7035-5ce3-5643-89a5956f1e79@cybernetics.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Stat-Signature: g51fapdahypizmzhbtmnh1ksa7cm19u3 Authentication-Results: imf12.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf12.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com X-Rspam-User: X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: 7C8E840003 X-HE-Tag: 1654025569-603963 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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 > --- > 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);