netdev - Re: [PATCH net-next] net: Don't return pfmemalloc pages to the page pool.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20181220140327.4e186cdb@redhat.com>
Date:   Thu, 20 Dec 2018 14:03:27 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Jonathan Lemon <jonathan.lemon@...il.com>
Cc:     <netdev@...r.kernel.org>, brouer@...hat.com
Subject: Re: [PATCH net-next] net: Don't return pfmemalloc pages to the page
 pool.

On Wed, 19 Dec 2018 12:06:51 -0800
Jonathan Lemon <jonathan.lemon@...il.com> wrote:

> Return pfmemalloc pages back to the page allocator, instead of
> holding them in the page pool.

Have you experience this issue in practice or is it theory?

>  While here, also use the __page_pool_return_page() API.

Don't combine several unrelated changed in one patch.
 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
> ---
>  net/core/page_pool.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 43a932cb609b..091007ff14a3 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -233,7 +233,7 @@ void __page_pool_put_page(struct page_pool *pool,
>  	 *
>  	 * refcnt == 1 means page_pool owns page, and can recycle it.
>  	 */
> -	if (likely(page_ref_count(page) == 1)) {
> +	if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {

I don't like adding this in the hot-path.  Instead we could move this
to the page alloc slow-path, and reject allocating pages with
pgmemalloc in the first place.


>  		/* Read barrier done in page_ref_count / READ_ONCE */
>  
>  		if (allow_direct && in_serving_softirq())
> @@ -259,8 +259,7 @@ void __page_pool_put_page(struct page_pool *pool,
>  	 * doing refcnt based recycle tricks, meaning another process
>  	 * will be invoking put_page.
>  	 */
> -	__page_pool_clean_page(pool, page);
> -	put_page(page);
> +	__page_pool_return_page(pool, page);
>  }
>  EXPORT_SYMBOL(__page_pool_put_page);
>  



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer