netdev - Re: [PATCH net-next 4/4] net: page_pool: merge page_pool_release_page() with page_pool_return

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZL5UkY+4wq4raIlv@hera>
Date: Mon, 24 Jul 2023 13:38:09 +0300
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, hawk@...nel.org
Subject: Re: [PATCH net-next 4/4] net: page_pool: merge
 page_pool_release_page() with page_pool_return_page()

On Wed, Jul 19, 2023 at 06:04:09PM -0700, Jakub Kicinski wrote:
> Now that page_pool_release_page() is not exported we can
> merge it with page_pool_return_page(). I believe that
> the "Do not replace this with page_pool_return_page()"
> comment was there in case page_pool_return_page() was
> not inlined, to avoid two function calls.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: hawk@...nel.org
> CC: ilias.apalodimas@...aro.org
> ---
>  net/core/page_pool.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 2c7cf5f2bcb8..7ca456bfab71 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -492,7 +492,7 @@ static s32 page_pool_inflight(struct page_pool *pool)
>   * a regular page (that will eventually be returned to the normal
>   * page-allocator via put_page).
>   */
> -static void page_pool_release_page(struct page_pool *pool, struct page *page)
> +static void page_pool_return_page(struct page_pool *pool, struct page *page)
>  {
>  	dma_addr_t dma;
>  	int count;
> @@ -518,12 +518,6 @@ static void page_pool_release_page(struct page_pool *pool, struct page *page)
>  	 */
>  	count = atomic_inc_return_relaxed(&pool->pages_state_release_cnt);
>  	trace_page_pool_state_release(pool, page, count);
> -}
> -
> -/* Return a page to the page allocator, cleaning up our state */
> -static void page_pool_return_page(struct page_pool *pool, struct page *page)
> -{
> -	page_pool_release_page(pool, page);
>
>  	put_page(page);
>  	/* An optimization would be to call __free_pages(page, pool->p.order)
> @@ -615,9 +609,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
>  	 * will be invoking put_page.
>  	 */
>  	recycle_stat_inc(pool, released_refcnt);
> -	/* Do not replace this with page_pool_return_page() */
> -	page_pool_release_page(pool, page);
> -	put_page(page);
> +	page_pool_return_page(pool, page);

That comment is my fault.  In hindsight, it wasn't very helpful ...
IIRC Jesper wanted to keep the calls discrete because we could optimize the
page pool internal and eventually not call put_page().  But I am fine with
the change regardless, it makes the code easier to read.

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@...aro.org>

>
>  	return NULL;
>  }
> --
> 2.41.0
>