On 2024/8/28 0:00, Alexander Duyck wrote:
> On Mon, Aug 26, 2024 at 5:46 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>>
>> Refactor common codes from __page_frag_alloc_va_align() to
>> __page_frag_cache_prepare() and __page_frag_cache_commit(),
>> so that the new API can make use of them.
>>
>> CC: Alexander Duyck <alexander.duyck@xxxxxxxxx>
>> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
>> ---
>> include/linux/page_frag_cache.h | 51 +++++++++++++++++++++++++++++++--
>> mm/page_frag_cache.c | 20 ++++++-------
>> 2 files changed, 59 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
>> index 372d6ed7e20a..2cc18a525936 100644
>> --- a/include/linux/page_frag_cache.h
>> +++ b/include/linux/page_frag_cache.h
>> @@ -7,6 +7,7 @@
>> #include <linux/build_bug.h>
>> #include <linux/log2.h>
>> #include <linux/mm.h>
>> +#include <linux/mmdebug.h>
>> #include <linux/mm_types_task.h>
>> #include <linux/types.h>
>>
>> @@ -75,8 +76,54 @@ static inline unsigned int page_frag_cache_page_size(unsigned long encoded_page)
>>
>> void page_frag_cache_drain(struct page_frag_cache *nc);
>> void __page_frag_cache_drain(struct page *page, unsigned int count);
>> -void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz,
>> - gfp_t gfp_mask, unsigned int align_mask);
>> +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int fragsz,
>> + struct page_frag *pfrag, gfp_t gfp_mask,
>> + unsigned int align_mask);
>> +
>> +static inline void __page_frag_cache_commit(struct page_frag_cache *nc,
>> + struct page_frag *pfrag, bool referenced,
>> + unsigned int used_sz)
>> +{
>> + if (referenced) {
>> + VM_BUG_ON(!nc->pagecnt_bias);
>> + nc->pagecnt_bias--;
>> + }
>> +
>> + VM_BUG_ON(used_sz > pfrag->size);
>> + VM_BUG_ON(pfrag->page != page_frag_encoded_page_ptr(nc->encoded_page));
>> +
>> + /* nc->offset is not reset when reusing an old page, so do not check for the
>> + * first fragment.
>> + * Committed offset might be bigger than the current offset due to alignment
>> + */
>
> nc->offset should be reset when you are allocating a new page. I would
It is reset when a new page is allocated, but not when reusing a old page.
> suggest making that change as you should be able to verify that the
> fragment you are working with contains the frag you are working with.
> The page and offset should essentially be equal.
For the part why offset is not equal, currently we do the alignment for
local variable 'offset' and set it to pfrag->offset instead of nc->offset
in below __page_frag_cache_prepare().
>
>> + VM_BUG_ON(pfrag->offset && nc->offset > pfrag->offset);
>> + VM_BUG_ON(pfrag->offset &&
>> + pfrag->offset + pfrag->size > page_frag_cache_page_size(nc->encoded_page));
>> +
>> + pfrag->size = used_sz;
>> +
>> + /* Calculate true size for the fragment due to alignment, nc->offset is not
>> + * reset for the first fragment when reusing an old page.
>> + */
>> + pfrag->size += pfrag->offset ? (pfrag->offset - nc->offset) : 0;
>
> The pfrag->size should be the truesize already. You should have stored
> it as fragsz so that all you really need to do is push the offset
> forward by pfrag->size.
The general idea is below:
The pfrag->size is set to indicate the max available cache of page_frag_cache
when caller calls __page_frag_cache_prepare(), after deciding how much cache
to use, the caller calls __page_frag_cache_commit() with 'used_sz', and
__page_frag_cache_commit() returns the true size back to the caller by
considering the offset alignment.
And the above is mainly used to support the prepare API in patch 10.
>
>> +
>> + nc->offset = pfrag->offset + used_sz;
>> +}
>> +
>
> I think this function might be better to keep in the .c file versus
> having it in the header file.
>
> ...
>
>> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
>> index 228cff9a4cdb..bba59c87d478 100644
>> --- a/mm/page_frag_cache.c
>> +++ b/mm/page_frag_cache.c
>> @@ -67,16 +67,14 @@ void __page_frag_cache_drain(struct page *page, unsigned int count)
>> }
>> EXPORT_SYMBOL(__page_frag_cache_drain);
>>
>> -void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> - unsigned int fragsz, gfp_t gfp_mask,
>> - unsigned int align_mask)
>> +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int fragsz,
>> + struct page_frag *pfrag, gfp_t gfp_mask,
>> + unsigned int align_mask)
>> {
>> unsigned long encoded_page = nc->encoded_page;
>> unsigned int size, offset;
>
> The 3 changes below can all be dropped. They are unnecessary
> optimizations of the unlikely path.
>
>> struct page *page;
>>
>> - size = page_frag_cache_page_size(encoded_page);
>> -
>> if (unlikely(!encoded_page)) {
>> refill:
>> page = __page_frag_cache_refill(nc, gfp_mask);
>> @@ -94,6 +92,9 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> /* reset page count bias and offset to start of new frag */
>> nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
>> nc->offset = 0;
>
> Your code above said that offset wasn't reset. But it looks like it is
> reset here isn't it?
This is refilling a new page for page_frag_cache, not reusing a old page for
page_frag_cache as mentioned above, nc->offset not being reset is in below:
https://elixir.free-electrons.com/linux/v6.11-rc5/source/mm/page_alloc.c#L4902
>
>> + } else {
>> + size = page_frag_cache_page_size(encoded_page);
>> + page = page_frag_encoded_page_ptr(encoded_page);
>> }
>>
>> offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
>> @@ -111,8 +112,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> return NULL;
>> }
>>
>> - page = page_frag_encoded_page_ptr(encoded_page);
>> -
>> if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>> goto refill;
>>
>
> These 3 changes to move the size and page are unnecessary
> optimization. I would recommend just dropping them and leave the code
> as is as you are just optimizing for unlikely paths.
It is not an optimization, the page need to be set to pfrag->page in this
patch, and page_frag_encoded_page_ptr() is only called when refill a new
page and reusing a old page before this patch, the above changes enable the
calling of page_frag_encoded_page_ptr() for all cases in this patch.
>
>> @@ -130,12 +129,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> offset = 0;
>> }
>>
>> - nc->pagecnt_bias--;
>> - nc->offset = offset + fragsz;
>> + pfrag->page = page;
>> + pfrag->offset = offset;
>> + pfrag->size = size - offset;
>
> Why are you subtracting the offset from the size? Shouldn't this just be fragsz?
As above, it is about supporting the prepare API in patch 10.
>
>>
>> return page_frag_encoded_page_address(encoded_page) + offset;
>> }
>> -EXPORT_SYMBOL(__page_frag_alloc_align);
>> +EXPORT_SYMBOL(__page_frag_cache_prepare);
>>
>> /*
>> * Frees a page fragment allocated out of either a compound or order 0 page.
>> --
>> 2.33.0
>>
