Re: [PATCH net-next v9 06/13] mm: page_frag: reuse existing space for 'size' and 'pfmemalloc'

[Alexander Duyck]

On Sat, Jul 13, 2024 at 9:52 PM Yunsheng Lin <yunshenglin0825@xxxxxxxxx> wrote:
>
> On 7/14/2024 12:55 AM, Alexander Duyck wrote:
>
> ...
>
> >>>>
> >>>> Perhaps the 'remaining' changing in this patch does seems to make things
> >>>> harder to discuss. Anyway, it would be more helpful if there is some pseudo
> >>>> code to show the steps of how the above can be done in your mind.
> >>>
> >>> Basically what you would really need do for all this is:
> >>>     remaining = __ALIGN_KERNEL_MASK(nc->remaining, ~align_mask);
> >>>     nc->remaining = remaining + fragsz;
> >>>     return encoded_page_address(nc->encoded_va) + size + remaining;
> >>
> >
> > I might have mixed my explanation up a bit. This is assuming remaining
> > is a negative value as I mentioned before.
>
> Let's be more specific about the options here, what you meant is below,
> right? Let's say it is option 1 as below:
> struct page_frag_cache {
>          /* encoded_va consists of the virtual address, pfmemalloc bit
> and order
>           * of a page.
>           */
>          unsigned long encoded_va;
>
> #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) && (BITS_PER_LONG <= 32)
>          __s16 remaining;
>          __u16 pagecnt_bias;
> #else
>          __s32 remaining;
>          __u32 pagecnt_bias;
> #endif
> };
>
> void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
>                                   unsigned int fragsz, gfp_t gfp_mask,
>                                   unsigned int align_mask)
> {
>          unsigned int size = page_frag_cache_page_size(nc->encoded_va);
>          int remaining;
>
>          remaining = __ALIGN_KERNEL_MASK(nc->remaining, ~align_mask);
>          if (unlikely(remaining + (int)fragsz > 0)) {
>                  if (!__page_frag_cache_refill(nc, gfp_mask))
>                          return NULL;
>
>                  size = page_frag_cache_page_size(nc->encoded_va);
>
>                  remaining = -size;
>                  if (unlikely(remaining + (int)fragsz > 0))
>                          return NULL;
>          }
>
>          nc->pagecnt_bias--;
>          nc->remaining = remaining + fragsz;
>
>          return encoded_page_address(nc->encoded_va) + size + remaining;
> }
>
>
> And let's say what I am proposing in v10 is option 2 as below:
> struct page_frag_cache {
>          /* encoded_va consists of the virtual address, pfmemalloc bit
> and order
>           * of a page.
>           */
>          unsigned long encoded_va;
>
> #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) && (BITS_PER_LONG <= 32)
>          __u16 remaining;
>          __u16 pagecnt_bias;
> #else
>          __u32 remaining;
>          __u32 pagecnt_bias;
> #endif
> };
>
> void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
>                                   unsigned int fragsz, gfp_t gfp_mask,
>                                   unsigned int align_mask)
> {
>          unsigned int size = page_frag_cache_page_size(nc->encoded_va);
>          int aligned_remaining = nc->remaining & align_mask;
>          int remaining = aligned_remaining - fragsz;
>
>          if (unlikely(remaining < 0)) {
>                  if (!__page_frag_cache_refill(nc, gfp_mask))
>                          return NULL;
>
>                  size = page_frag_cache_page_size(nc->encoded_va);
>
>                  aligned_remaining = size;
>                  remaining = aligned_remaining - fragsz;
>                  if (unlikely(remaining < 0))
>                          return NULL;
>          }
>
>          nc->pagecnt_bias--;
>          nc->remaining = remaining;
>
>          return encoded_page_address(nc->encoded_va) + (size -
> aligned_remaining);
> }
>
> If the option 1 is not what you have in mind, it would be better to be
> more specific about what you have in mind.

Option 1 was more or less what I had in mind.

> If the option 1 is what you have in mind, it seems both option 1 and
> option 2 have the same semantics as my understanding, right? The
> question here seems to be what is your perfer option and why?
>
> I implemented both of them, and the option 1 seems to have a
> bigger generated asm size as below:
> ./scripts/bloat-o-meter vmlinux_non_neg vmlinux
> add/remove: 0/0 grow/shrink: 1/0 up/down: 37/0 (37)
> Function                                     old     new   delta
> __page_frag_alloc_va_align                   414     451     +37

My big complaint is that it seems option 2 is harder for people to
understand and more likely to not be done correctly. In some cases if
the performance difference is negligible it is better to go with the
more maintainable solution.