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

On 2024/10/10 7:50, Alexander Duyck wrote:

...

quoted

+
+#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.

quoted

 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()