Re: [RFC v11 08/14] mm: page_frag: some minor refactoring before adding new API

[Yunsheng Lin]

On 2024/7/23 21:19, Yunsheng Lin wrote:

...

>>>>>  static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
>>>>>                                           gfp_t gfp_mask)
>>>>>  {
>>>>> @@ -26,6 +48,14 @@ static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
>>>>>      struct page *page = NULL;
>>>>>      gfp_t gfp = gfp_mask;
>>>>>
>>>>> +    if (likely(nc->encoded_va)) {
>>>>> +            page = __page_frag_cache_recharge(nc);
>>>>> +            if (page) {
>>>>> +                    order = encoded_page_order(nc->encoded_va);
>>>>> +                    goto out;
>>>>> +            }
>>>>> +    }
>>>>> +
>>>>
>>>> This code has no business here. This is refill, you just dropped
>>>> recharge in here which will make a complete mess of the ordering and be
>>>> confusing to say the least.
>>>>
>>>> The expectation was that if we are calling this function it is going to
>>>> overwrite the virtual address to NULL on failure so we discard the old
>>>> page if there is one present. This changes that behaviour. What you
>>>> effectively did is made __page_frag_cache_refill into the recharge
>>>> function.
>>>
>>> The idea is to reuse the below for both __page_frag_cache_refill() and
>>> __page_frag_cache_recharge(), which seems to be about maintainability
>>> to not having duplicated code. If there is a better idea to avoid that
>>> duplicated code while keeping the old behaviour, I am happy to change
>>> it.
>>>
>>>         /* reset page count bias and remaining to start of new frag */
>>>         nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
>>>         nc->remaining = PAGE_SIZE << order;
>>>
>>
>> The only piece that is really reused here is the pagecnt_bias
>> assignment. What is obfuscated away is that the order is gotten
>> through one of two paths. Really order isn't order here it is size.
>> Which should have been fetched already. What you end up doing with
>> this change is duplicating a bunch of code throughout the function.
>> You end up having to fetch size multiple times multiple ways. here you
>> are generating it with order. Then you have to turn around and get it
>> again at the start of the function, and again after calling this
>> function in order to pull it back out.
> 
> I am assuming you would like to reserve old behavior as below?
> 
> 	if(!encoded_va) {
> refill:
> 		__page_frag_cache_refill()
> 	}
> 
> 
> 	if(remaining < fragsz) {
> 		if(!__page_frag_cache_recharge())
> 			goto refill;
> 	}
> 
> As we are adding new APIs, are we expecting new APIs also duplicate
> the above pattern?
> 
>>

How about something like below? __page_frag_cache_refill() and
__page_frag_cache_reuse() does what their function name suggests
as much as possible, __page_frag_cache_reload() is added to avoid
new APIs duplicating similar pattern as much as possible, also
avoid fetching size multiple times multiple ways as much as possible.

static struct page *__page_frag_cache_reuse(unsigned long encoded_va,
                                            unsigned int pagecnt_bias)
{
        struct page *page;

        page = virt_to_page((void *)encoded_va);
        if (!page_ref_sub_and_test(page, pagecnt_bias))
                return NULL;

        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));
                return NULL;
        }

        /* OK, page count is 0, we can safely set it */
        set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);
        return page;
}

static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
                                             gfp_t gfp_mask)
{
        unsigned long order = PAGE_FRAG_CACHE_MAX_ORDER;
        struct page *page = NULL;
        gfp_t gfp = gfp_mask;

#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
        gfp_mask = (gfp_mask & ~__GFP_DIRECT_RECLAIM) |  __GFP_COMP |
                   __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC;
        page = alloc_pages_node(NUMA_NO_NODE, gfp_mask,
                                PAGE_FRAG_CACHE_MAX_ORDER);
#endif
        if (unlikely(!page)) {
                page = alloc_pages_node(NUMA_NO_NODE, gfp, 0);
                if (unlikely(!page)) {
                        memset(nc, 0, sizeof(*nc));
                        return NULL;
                }

                order = 0;
        }

        nc->encoded_va = encode_aligned_va(page_address(page), order,
                                           page_is_pfmemalloc(page));

        /* Even if we own the page, we do not use atomic_set().
         * This would break get_page_unless_zero() users.
         */
        page_ref_add(page, PAGE_FRAG_CACHE_MAX_SIZE);

        return page;
}

static struct page *__page_frag_cache_reload(struct page_frag_cache *nc,
                                             gfp_t gfp_mask)
{
        struct page *page;

        if (likely(nc->encoded_va)) {
                page = __page_frag_cache_reuse(nc->encoded_va, nc->pagecnt_bias);
                if (page)
                        goto out;
        }

        page = __page_frag_cache_refill(nc, gfp_mask);
        if (unlikely(!page))
                return NULL;

out:
        nc->remaining = page_frag_cache_page_size(nc->encoded_va);
        nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
        return page;
}

void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
                                 unsigned int fragsz, gfp_t gfp_mask,
                                 unsigned int align_mask)
{
        unsigned long encoded_va = nc->encoded_va;
        unsigned int remaining;

        remaining = nc->remaining & align_mask;
        if (unlikely(remaining < fragsz)) {
                if (unlikely(fragsz > PAGE_SIZE)) {
                        /*
                         * The caller is trying to allocate a fragment
                         * with fragsz > PAGE_SIZE but the cache isn't big
                         * enough to satisfy the request, this may
                         * happen in low memory conditions.
                         * We don't release the cache page because
                         * it could make memory pressure worse
                         * so we simply return NULL here.
                         */
                        return NULL;
                }

                if (unlikely(!__page_frag_cache_reload(nc, gfp_mask)))
                        return NULL;

                nc->pagecnt_bias--;
                nc->remaining -= fragsz;

                return encoded_page_address(nc->encoded_va);
        }

        nc->pagecnt_bias--;
        nc->remaining = remaining - fragsz;

        return encoded_page_address(encoded_va) +
                (page_frag_cache_page_size(encoded_va) - remaining);
}