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

[Alexander Duyck]

On Thu, Oct 10, 2024 at 4:32 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2024/10/10 7:50, Alexander Duyck wrote:
>
> ...
>
> >> +
> >> +#define PAGE_FRAG_CACHE_PFMEMALLOC_BIT         (PAGE_FRAG_CACHE_ORDER_MASK + 1)
> >> +
> >> +static inline bool page_frag_encoded_page_pfmemalloc(unsigned long encoded_page)
> >> +{
> >> +       return !!(encoded_page & PAGE_FRAG_CACHE_PFMEMALLOC_BIT);
> >> +}
> >> +
> >
> > Rather than calling this encoded_page_pfmemalloc you might just go
> > with decode_pfmemalloc. Also rather than passing the unsigned long we
> > might just want to pass the page_frag_cache pointer.
> As the page_frag_encoded_page_pfmemalloc() is also called in
> __page_frag_alloc_align(), and __page_frag_alloc_align() uses a
> local variable for 'nc->encoded_page' to avoid fetching from
> page_frag_cache pointer multi-times, so passing an 'unsigned long'
> is perferred here?
>
> I am not sure if decode_pfmemalloc() is simple enough that it
> might be conflicted with naming from other subsystem in the
> future. I thought about adding a '__' prefix to it, but the naming
> seems long enough that some inline helper' naming is over 80 characters.

What you might do is look at having a page_frag version of the
function and a encoded_page version as I called out below with the
naming. It would make sense to call the two out separately as this is
operating on an encoded page, not a page frag. With that we can avoid
any sort of naming confusion.

> >
> >>  static inline void page_frag_cache_init(struct page_frag_cache *nc)
> >>  {
> >> -       nc->va = NULL;
> >> +       nc->encoded_page = 0;
> >>  }
> >>
> >>  static inline bool page_frag_cache_is_pfmemalloc(struct page_frag_cache *nc)
> >>  {
> >> -       return !!nc->pfmemalloc;
> >> +       return page_frag_encoded_page_pfmemalloc(nc->encoded_page);
> >>  }
> >>
> >>  void page_frag_cache_drain(struct page_frag_cache *nc);
> >> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> >> index 4c8e04379cb3..4bff4de58808 100644
> >> --- a/mm/page_frag_cache.c
> >> +++ b/mm/page_frag_cache.c
> >> @@ -12,6 +12,7 @@
> >>   * be used in the "frags" portion of skb_shared_info.
> >>   */
> >>
> >> +#include <linux/build_bug.h>
> >>  #include <linux/export.h>
> >>  #include <linux/gfp_types.h>
> >>  #include <linux/init.h>
> >> @@ -19,9 +20,41 @@
> >>  #include <linux/page_frag_cache.h>
> >>  #include "internal.h"
> >>
> >> +static unsigned long page_frag_encode_page(struct page *page, unsigned int order,
> >> +                                          bool pfmemalloc)
> >> +{
> >> +       BUILD_BUG_ON(PAGE_FRAG_CACHE_MAX_ORDER > PAGE_FRAG_CACHE_ORDER_MASK);
> >> +       BUILD_BUG_ON(PAGE_FRAG_CACHE_PFMEMALLOC_BIT >= PAGE_SIZE);
> >> +
> >> +       return (unsigned long)page_address(page) |
> >> +               (order & PAGE_FRAG_CACHE_ORDER_MASK) |
> >> +               ((unsigned long)pfmemalloc * PAGE_FRAG_CACHE_PFMEMALLOC_BIT);
> >> +}
> >> +
> >> +static unsigned long page_frag_encoded_page_order(unsigned long encoded_page)
> >> +{
> >> +       return encoded_page & PAGE_FRAG_CACHE_ORDER_MASK;
> >> +}
> >> +
> >> +static void *page_frag_encoded_page_address(unsigned long encoded_page)
> >> +{
> >> +       return (void *)(encoded_page & PAGE_MASK);
> >> +}
> >> +
> >> +static struct page *page_frag_encoded_page_ptr(unsigned long encoded_page)
> >> +{
> >> +       return virt_to_page((void *)encoded_page);
> >> +}
> >> +
> >
> > Same with these. Instead of calling it encoded_page_XXX we could
> > probably just go with decode_page, decode_order, and decode_address.
> > Also instead of passing an unsigned long it would make more sense to
> > be passing the page_frag_cache pointer, especially once you start
> > pulling these out of this block.
>
> For the not passing the page_frag_cache pointer part, it is the same
> as above, it is mainly to avoid fetching from pointer multi-times.
>
> >
> > If you are wanting to just work with the raw unsigned long value in
> > the file it might make more sense to drop the "page_frag_" prefix from
> > it and just have functions for handling your "encoded_page_" value. In
> > that case you might rename page_frag_encode_page to
> > "encoded_page_encode" or something like that.
>
> It am supposing you meant 'encoded_page_decode' here instead of
> "encoded_page_encode"?
> Something like below?
> encoded_page_decode_pfmemalloc()
> encoded_page_decode_order()
> encoded_page_decode_page()
> encoded_page_decode_virt()

For the decodes yes. I was referring to page_frag_encode_page.
Basically the output from that isn't anything page frag, it is your
encoded page type so you could probably just call it
encoded_page_encode, or encoded_page_create or something like that.