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