On Sat, Jul 13, 2024 at 9:52 PM Yunsheng Lin [off-list ref] wrote:

On 7/14/2024 12:55 AM, Alexander Duyck wrote:

...

quoted

quoted

quoted

quoted

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.