On Fri, Oct 18, 2024 at 4:00 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 | 36 +++++++++++++++++++++++++++--
> mm/page_frag_cache.c | 40 ++++++++++++++++++++++++++-------
> 2 files changed, 66 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
> index 41a91df82631..feed99d0cddb 100644
> --- a/include/linux/page_frag_cache.h
> +++ b/include/linux/page_frag_cache.h
> @@ -5,6 +5,7 @@
>
> #include <linux/bits.h>
> #include <linux/log2.h>
> +#include <linux/mmdebug.h>
> #include <linux/mm_types_task.h>
> #include <linux/types.h>
>
> @@ -39,8 +40,39 @@ static inline bool page_frag_cache_is_pfmemalloc(struct page_frag_cache *nc)
>
> 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);
> +unsigned int __page_frag_cache_commit_noref(struct page_frag_cache *nc,
> + struct page_frag *pfrag,
> + unsigned int used_sz);
> +
> +static inline unsigned int __page_frag_cache_commit(struct page_frag_cache *nc,
> + struct page_frag *pfrag,
> + unsigned int used_sz)
> +{
> + VM_BUG_ON(!nc->pagecnt_bias);
> + nc->pagecnt_bias--;
> +
> + return __page_frag_cache_commit_noref(nc, pfrag, used_sz);
> +}
> +
> +static inline void *__page_frag_alloc_align(struct page_frag_cache *nc,
> + unsigned int fragsz, gfp_t gfp_mask,
> + unsigned int align_mask)
> +{
> + struct page_frag page_frag;
> + void *va;
> +
> + va = __page_frag_cache_prepare(nc, fragsz, &page_frag, gfp_mask,
> + align_mask);
> + if (unlikely(!va))
> + return NULL;
> +
> + __page_frag_cache_commit(nc, &page_frag, fragsz);
Minor nit here. Rather than if (!va) return I think it might be better
to just go with if (likely(va)) __page_frag_cache_commit.
> +
> + return va;
> +}
>
> static inline void *page_frag_alloc_align(struct page_frag_cache *nc,
> unsigned int fragsz, gfp_t gfp_mask,
> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> index a36fd09bf275..a852523bc8ca 100644
> --- a/mm/page_frag_cache.c
> +++ b/mm/page_frag_cache.c
> @@ -90,9 +90,31 @@ 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)
> +unsigned int __page_frag_cache_commit_noref(struct page_frag_cache *nc,
> + struct page_frag *pfrag,
> + unsigned int used_sz)
> +{
> + unsigned int orig_offset;
> +
> + VM_BUG_ON(used_sz > pfrag->size);
> + VM_BUG_ON(pfrag->page != encoded_page_decode_page(nc->encoded_page));
> + VM_BUG_ON(pfrag->offset + pfrag->size >
> + (PAGE_SIZE << encoded_page_decode_order(nc->encoded_page)));
> +
> + /* pfrag->offset might be bigger than the nc->offset due to alignment */
> + VM_BUG_ON(nc->offset > pfrag->offset);
> +
> + orig_offset = nc->offset;
> + nc->offset = pfrag->offset + used_sz;
> +
> + /* Return true size back to caller considering the offset alignment */
> + return nc->offset - orig_offset;
> +}
> +EXPORT_SYMBOL(__page_frag_cache_commit_noref);
> +
I have a question. How often is it that we are committing versus just
dropping the fragment? It seems like this approach is designed around
optimizing for not commiting the page as we are having to take an
extra function call to commit the change every time. Would it make
more sense to have an abort versus a commit?
> +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;
> @@ -114,6 +136,8 @@ 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;
> + } else {
> + page = encoded_page_decode_page(encoded_page);
> }
>
> size = PAGE_SIZE << encoded_page_decode_order(encoded_page);
This makes no sense to me. Seems like there are scenarios where you
are grabbing the page even if you aren't going to use it? Why?
I think you would be better off just waiting to the end and then
fetching it instead of trying to grab it and potentially throw it away
if there is no space left in the page. Otherwise what you might do is
something along the lines of:
pfrag->page = page ? : encoded_page_decode_page(encoded_page);
> @@ -132,8 +156,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> return NULL;
> }
>
> - page = encoded_page_decode_page(encoded_page);
> -
> if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
> goto refill;
>
> @@ -148,15 +170,17 @@ 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;
> offset = 0;
> }
>
> - nc->pagecnt_bias--;
> - nc->offset = offset + fragsz;
> + pfrag->page = page;
> + pfrag->offset = offset;
> + pfrag->size = size - offset;
I really think we should still be moving the nc->offset forward at
least with each allocation. It seems like you end up doing two flavors
of commit, one with and one without the decrement of the bias. So I
would be okay with that being pulled out into some separate logic to
avoid the extra increment in the case of merging the pages. However in
both cases you need to move the offset, so I would recommend keeping
that bit there as it would allow us to essentially call this multiple
times without having to do a commit in between to keep the offset
correct. With that your commit logic only has to verify nothing
changes out from underneath us and then update the pagecnt_bias if
needed.
>
> return encoded_page_decode_virt(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
>
