Re: [PATCH net-next v9 07/13] mm: page_frag: some minor refactoring before adding new API

[Alexander H Duyck <alexander.duyck@gmail.com>]

On Tue, 2024-06-25 at 21:52 +0800, Yunsheng Lin wrote:
> Refactor common codes from __page_frag_alloc_va_align()
> to __page_frag_cache_refill(), so that the new API can
> make use of them.
> 
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>

I am generally not a fan of the concept behind this patch. I really
think we should keep the page_frag_cache_refill function to just
allocating the page, or in this case the encoded_va and populating only
that portion of the struct.

> ---
>  mm/page_frag_cache.c | 61 ++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 30 deletions(-)
> 
> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> index a3316dd50eff..4fd421d4f22c 100644
> --- a/mm/page_frag_cache.c
> +++ b/mm/page_frag_cache.c
> @@ -29,10 +29,36 @@ static void *page_frag_cache_current_va(struct page_frag_cache *nc)
>  static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
>  					     gfp_t gfp_mask)
>  {
> -	struct page *page = NULL;
> +	struct encoded_va *encoded_va = nc->encoded_va;
>  	gfp_t gfp = gfp_mask;
>  	unsigned int order;
> +	struct page *page;
> +
> +	if (unlikely(!encoded_va))
> +		goto alloc;
> +
> +	page = virt_to_page(encoded_va);
> +	if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
> +		goto alloc;
> +
> +	if (unlikely(encoded_page_pfmemalloc(encoded_va))) {
> +		VM_BUG_ON(compound_order(page) !=
> +			  encoded_page_order(encoded_va));
> +		free_unref_page(page, encoded_page_order(encoded_va));
> +		goto alloc;
> +	}
> +
> +	/* OK, page count is 0, we can safely set it */
> +	set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);

Why not just make this block of code a function onto itself? You put an
if statement at the top that essentially is just merging two functions
into one. Perhaps this logic could be __page_frag_cache_recharge which
would return an error if the page is busy or the wrong type. Then
acting on that you could switch to the refill attempt.

Also thinking about it more the set_page_count in this function and
page_ref_add in the other can probably be merged into the recharge and
refill functions since they are acting directly on the encoded page and
not interacting with the other parts of the page_frag_cache.

> +
> +	/* reset page count bias and remaining of new frag */
> +	nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> +	nc->remaining = page_frag_cache_page_size(encoded_va);

These two parts are more or less agnostic to the setup and could be
applied to refill or recharge. Also one thought occurs to me. You were
encoding "order" into the encoded VA. Why use that when your choices
are either PAGE_FRAG_CACHE_MAX_SIZE or PAGE_SIZE. It should be a single
bit and doesn't need to be a fully byte to store that. That would allow
you to reduce this down to just 2 bits, one for pfmemalloc and one for
max order vs order 0.

> +
> +	return page;
>  
> +alloc:
> +	page = NULL;
>  #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
>  	gfp_mask = (gfp_mask & ~__GFP_DIRECT_RECLAIM) |  __GFP_COMP |
>  		   __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC;
> @@ -89,40 +115,15 @@ void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
>  				 unsigned int fragsz, gfp_t gfp_mask,
>  				 unsigned int align_mask)
>  {
> -	struct encoded_va *encoded_va = nc->encoded_va;
> -	struct page *page;
> -	int remaining;
> +	int remaining = nc->remaining & align_mask;
>  	void *va;
>  
> -	if (unlikely(!encoded_va)) {
> -refill:
> -		if (unlikely(!__page_frag_cache_refill(nc, gfp_mask)))
> -			return NULL;
> -
> -		encoded_va = nc->encoded_va;
> -	}
> -
> -	remaining = nc->remaining & align_mask;
>  	remaining -= fragsz;
>  	if (unlikely(remaining < 0)) {

I see, so this is why you were using the memset calls everywhere.

> -		page = virt_to_page(encoded_va);
> -		if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
> -			goto refill;
> -
> -		if (unlikely(encoded_page_pfmemalloc(encoded_va))) {
> -			VM_BUG_ON(compound_order(page) !=
> -				  encoded_page_order(encoded_va));
> -			free_unref_page(page, encoded_page_order(encoded_va));
> -			goto refill;
> -		}
> -
> -		/* OK, page count is 0, we can safely set it */
> -		set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);
> +		if (unlikely(!__page_frag_cache_refill(nc, gfp_mask)))
> +			return NULL;
>  
> -		/* reset page count bias and remaining of new frag */
> -		nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> -		nc->remaining = remaining = page_frag_cache_page_size(encoded_va);
> -		remaining -= fragsz;
> +		remaining = nc->remaining - fragsz;
>  		if (unlikely(remaining < 0)) {
>  			/*
>  			 * The caller is trying to allocate a fragment