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
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.
> + 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.
> +
> + 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?
> + } 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.
> @@ -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?
>
> 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
>
