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

[Yunsheng Lin]

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.

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

> Basically the issue is your current code is using nc->remaining to
> generate the current address and that is bad as it isn't aligned to
> anything as fragsz was added to it and no alignment check had been
> done on that value.