Re: [PATCH 1/1] mm: avoid re-using pfmemalloc page in page_frag_alloc()

[Matthew Wilcox]

On Tue, Nov 03, 2020 at 11:32:39AM -0800, Dongli Zhang wrote:
> The ethernet driver may allocates skb (and skb->data) via napi_alloc_skb().
> This ends up to page_frag_alloc() to allocate skb->data from
> page_frag_cache->va.
> 
> During the memory pressure, page_frag_cache->va may be allocated as
> pfmemalloc page. As a result, the skb->pfmemalloc is always true as
> skb->data is from page_frag_cache->va. The skb will be dropped if the
> sock (receiver) does not have SOCK_MEMALLOC. This is expected behaviour
> under memory pressure.
> 
> However, once kernel is not under memory pressure any longer (suppose large
> amount of memory pages are just reclaimed), the page_frag_alloc() may still
> re-use the prior pfmemalloc page_frag_cache->va to allocate skb->data. As a
> result, the skb->pfmemalloc is always true unless page_frag_cache->va is
> re-allocated, even the kernel is not under memory pressure any longer.
> 
> Here is how kernel runs into issue.
> 
> 1. The kernel is under memory pressure and allocation of
> PAGE_FRAG_CACHE_MAX_ORDER in __page_frag_cache_refill() will fail. Instead,
> the pfmemalloc page is allocated for page_frag_cache->va.
> 
> 2: All skb->data from page_frag_cache->va (pfmemalloc) will have
> skb->pfmemalloc=true. The skb will always be dropped by sock without
> SOCK_MEMALLOC. This is an expected behaviour.
> 
> 3. Suppose a large amount of pages are reclaimed and kernel is not under
> memory pressure any longer. We expect skb->pfmemalloc drop will not happen.
> 
> 4. Unfortunately, page_frag_alloc() does not proactively re-allocate
> page_frag_alloc->va and will always re-use the prior pfmemalloc page. The
> skb->pfmemalloc is always true even kernel is not under memory pressure any
> longer.
> 
> Therefore, this patch always checks and tries to avoid re-using the
> pfmemalloc page for page_frag_alloc->va.
> 
> Cc: Aruna Ramakrishna <aruna.ramakrishna@xxxxxxxxxx>
> Cc: Bert Barbe <bert.barbe@xxxxxxxxxx>
> Cc: Rama Nichanamatlu <rama.nichanamatlu@xxxxxxxxxx>
> Cc: Venkat Venkatsubra <venkat.x.venkatsubra@xxxxxxxxxx>
> Cc: Manjunath Patil <manjunath.b.patil@xxxxxxxxxx>
> Cc: Joe Jin <joe.jin@xxxxxxxxxx>
> Cc: SRINIVAS <srinivas.eeda@xxxxxxxxxx>
> Signed-off-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
> ---
>  mm/page_alloc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 23f5066bd4a5..291df2f9f8f3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5075,6 +5075,16 @@ void *page_frag_alloc(struct page_frag_cache *nc,
>  	struct page *page;
>  	int offset;
>  
> +	/*
> +	 * Try to avoid re-using pfmemalloc page because kernel may already
> +	 * run out of the memory pressure situation at any time.
> +	 */
> +	if (unlikely(nc->va && nc->pfmemalloc)) {
> +		page = virt_to_page(nc->va);
> +		__page_frag_cache_drain(page, nc->pagecnt_bias);
> +		nc->va = NULL;
> +	}

I think this is the wrong way to solve this problem.  Instead, we should
use up this page, but refuse to recycle it.  How about something like this (not even compile tested):

+++ b/mm/page_alloc.c
@@ -5139,6 +5139,10 @@ void *page_frag_alloc(struct page_frag_cache *nc,
 
                if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
                        goto refill;
+               if (nc->pfmemalloc) {
+                       free_the_page(page);
+                       goto refill;
+               }
 
 #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
                /* if size can vary use size else just use PAGE_SIZE */