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

[Yunsheng Lin <linyunsheng@huawei.com>]

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.

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

> 
>