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