Re: [RFC v11 03/14] mm: page_frag: use initial zero offset for page_frag_alloc_align()

[Yunsheng Lin]

On 7/22/2024 2:34 AM, Alexander Duyck wrote:
> On Fri, Jul 19, 2024 at 2:37 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:

...

> > diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> > index 609a485cd02a..2958fe006fe7 100644
> > --- a/mm/page_frag_cache.c
> > +++ b/mm/page_frag_cache.c
> > @@ -22,6 +22,7 @@
> >   static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
> >                                               gfp_t gfp_mask)
> >   {
> > +       unsigned int page_size = PAGE_FRAG_CACHE_MAX_SIZE;
> >          struct page *page = NULL;
> >          gfp_t gfp = gfp_mask;
> >
> > @@ -30,12 +31,21 @@ static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
> >                     __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC;
> >          page = alloc_pages_node(NUMA_NO_NODE, gfp_mask,
> >                                  PAGE_FRAG_CACHE_MAX_ORDER);
> > -       nc->size = page ? PAGE_FRAG_CACHE_MAX_SIZE : PAGE_SIZE;
> >   #endif
> > -       if (unlikely(!page))
> > +       if (unlikely(!page)) {
> >                  page = alloc_pages_node(NUMA_NO_NODE, gfp, 0);
> > +               if (unlikely(!page)) {
> > +                       nc->va = NULL;
> > +                       return NULL;
> > +               }
> >
> > -       nc->va = page ? page_address(page) : NULL;
> > +               page_size = PAGE_SIZE;
> > +       }
> > +
> > +#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
> > +       nc->size = page_size;
> > +#endif
> > +       nc->va = page_address(page);
> >
> >          return page;
> >   }
>
> Not a huge fan of the changes here. If we are changing the direction
> then just do that. I don't see the point of these changes. As far as I
> can tell it is just adding noise to the diff and has no effect on the
> final code as the outcome is mostly the same except for you don't
> update size in the event that you overwrite nc->va to NULL.

While I am agreed the above changing is not really related to this
patch, but it does have some effect on the final code, as it seems
to avoid one extra '!page' checking:

 ./scripts/bloat-o-meter vmlinux.org vmlinux
add/remove: 0/0 grow/shrink: 1/0 up/down: 11/0 (11)
Function                                     old     new   delta
__page_frag_alloc_align                      594     605     +11
Total: Before=22083357, After=22083368, chg +0.00%

Let me see if I can move it to more related patch when refactoring.

> > @@ -64,8 +74,8 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> >                                unsigned int align_mask)
> >   {
> >          unsigned int size = PAGE_SIZE;
> > +       unsigned int remaining;
> >          struct page *page;
> > -       int offset;
> >
> >          if (unlikely(!nc->va)) {
> >   refill:
> > @@ -82,35 +92,20 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> >                   */
> >                  page_ref_add(page, PAGE_FRAG_CACHE_MAX_SIZE);
> >
> > -               /* reset page count bias and offset to start of new frag */
> > +               /* reset page count bias and remaining to start of new frag */
> >                  nc->pfmemalloc = page_is_pfmemalloc(page);
> >                  nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> > -               nc->offset = size;
> > +               nc->remaining = size;
> >          }
> >
> > -       offset = nc->offset - fragsz;
> > -       if (unlikely(offset < 0)) {
> > -               page = virt_to_page(nc->va);
> > -
> > -               if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
> > -                       goto refill;
> > -
> > -               if (unlikely(nc->pfmemalloc)) {
> > -                       free_unref_page(page, compound_order(page));
> > -                       goto refill;
> > -               }
> > -
> >   #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
> > -               /* if size can vary use size else just use PAGE_SIZE */
> > -               size = nc->size;
> > +       /* if size can vary use size else just use PAGE_SIZE */
> > +       size = nc->size;
> >   #endif
>
> Rather than pulling this out and placing it here it might make more
> sense at the start of the function. Basically just overwrite size w/
> either PAGE_SIZE or nc->size right at the start. Then if we have to
> reallocate we overwrite it. That way we can avoid some redundancy and
> this will be easier to read.

You meant something like below at the start of the function, it does
make more sense.
#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
	unsigned int size = nc->size;
#else
	unsigned int size = PAGE_SIZE;
#endif

> > -               /* OK, page count is 0, we can safely set it */
> > -               set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);
> >
> > -               /* reset page count bias and offset to start of new frag */
> > -               nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> > -               offset = size - fragsz;
> > -               if (unlikely(offset < 0)) {
> > +       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
> > @@ -122,13 +117,31 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> >                           */
> >                          return NULL;
> >                  }
> > +
> > +               page = virt_to_page(nc->va);
> > +
> > +               if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
> > +                       goto refill;
> > +
> > +               if (unlikely(nc->pfmemalloc)) {
> > +                       free_unref_page(page, compound_order(page));
> > +                       goto refill;
> > +               }
> > +
> > +               /* OK, page count is 0, we can safely set it */
> > +               set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);
> > +
> > +               /* reset page count bias and remaining to start of new frag */
> > +               nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> > +               nc->remaining = size;
>
> Why are you setting nc->remaining here? You set it a few lines below.
> This is redundant.

Yes, it is not needed after '(fragsz > PAGE_SIZE)' after checking is
moved upward.

> > +
> > +               remaining = size;
> >          }
> >
> >          nc->pagecnt_bias--;
> > -       offset &= align_mask;
> > -       nc->offset = offset;
> > +       nc->remaining = remaining - fragsz;
> >
> > -       return nc->va + offset;
> > +       return nc->va + (size - remaining);
> >   }
> >   EXPORT_SYMBOL(__page_frag_alloc_align);