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

[Yunsheng Lin <yunshenglin0825@gmail.com>]

On 7/22/2024 2:34 AM, Alexander Duyck wrote:
> On Fri, Jul 19, 2024 at 2:37 AM Yunsheng Lin <linyunsheng@huawei.com> 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);
>