Re: [PATCH net-next v9 10/13] mm: page_frag: introduce prepare/probe/commit API

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/6/29 6:35, Alexander H Duyck wrote:
> On Tue, 2024-06-25 at 21:52 +0800, Yunsheng Lin wrote:
>> There are many use cases that need minimum memory in order
>> for forward progress, but more performant if more memory is
>> available or need to probe the cache info to use any memory
>> available for frag caoleasing reason.
>>
>> Currently skb_page_frag_refill() API is used to solve the
>> above use cases, but caller needs to know about the internal
>> detail and access the data field of 'struct page_frag' to
>> meet the requirement of the above use cases and its
>> implementation is similar to the one in mm subsystem.
>>
>> To unify those two page_frag implementations, introduce a
>> prepare API to ensure minimum memory is satisfied and return
>> how much the actual memory is available to the caller and a
>> probe API to report the current available memory to caller
>> without doing cache refilling. The caller needs to either call
>> the commit API to report how much memory it actually uses, or
>> not do so if deciding to not use any memory.
>>
>> As next patch is about to replace 'struct page_frag' with
>> 'struct page_frag_cache' in linux/sched.h, which is included
>> by the asm-offsets.s, using the virt_to_page() in the inline
>> helper of page_frag_cache.h cause a "'vmemmap' undeclared"
>> compiling error for asm-offsets.s, use a macro for probe API
>> to avoid that compiling error.
>>
>> CC: Alexander Duyck <alexander.duyck@gmail.com>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> ---
>>  include/linux/page_frag_cache.h |  82 +++++++++++++++++++++++
>>  mm/page_frag_cache.c            | 114 ++++++++++++++++++++++++++++++++
>>  2 files changed, 196 insertions(+)
>>
>> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
>> index b33904d4494f..e95d44a36ec9 100644
>> --- a/include/linux/page_frag_cache.h
>> +++ b/include/linux/page_frag_cache.h
>> @@ -4,6 +4,7 @@
>>  #define _LINUX_PAGE_FRAG_CACHE_H
>>  
>>  #include <linux/gfp_types.h>
>> +#include <linux/mmdebug.h>
>>  
>>  #define PAGE_FRAG_CACHE_MAX_SIZE	__ALIGN_MASK(32768, ~PAGE_MASK)
>>  #define PAGE_FRAG_CACHE_MAX_ORDER	get_order(PAGE_FRAG_CACHE_MAX_SIZE)
>> @@ -87,6 +88,9 @@ static inline unsigned int page_frag_cache_page_size(struct encoded_va *encoded_
>>  
>>  void page_frag_cache_drain(struct page_frag_cache *nc);
>>  void __page_frag_cache_drain(struct page *page, unsigned int count);
>> +struct page *page_frag_alloc_pg(struct page_frag_cache *nc,
>> +				unsigned int *offset, unsigned int fragsz,
>> +				gfp_t gfp);
>>  void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
>>  				 unsigned int fragsz, gfp_t gfp_mask,
>>  				 unsigned int align_mask);
>> @@ -99,12 +103,90 @@ static inline void *page_frag_alloc_va_align(struct page_frag_cache *nc,
>>  	return __page_frag_alloc_va_align(nc, fragsz, gfp_mask, -align);
>>  }
>>  
>> +static inline unsigned int page_frag_cache_page_offset(const struct page_frag_cache *nc)
>> +{
>> +	return page_frag_cache_page_size(nc->encoded_va) - nc->remaining;
>> +}
>> +
>>  static inline void *page_frag_alloc_va(struct page_frag_cache *nc,
>>  				       unsigned int fragsz, gfp_t gfp_mask)
>>  {
>>  	return __page_frag_alloc_va_align(nc, fragsz, gfp_mask, ~0u);
>>  }
>>  
>> +void *page_frag_alloc_va_prepare(struct page_frag_cache *nc, unsigned int *fragsz,
>> +				 gfp_t gfp);
>> +
>> +static inline void *page_frag_alloc_va_prepare_align(struct page_frag_cache *nc,
>> +						     unsigned int *fragsz,
>> +						     gfp_t gfp,
>> +						     unsigned int align)
>> +{
>> +	WARN_ON_ONCE(!is_power_of_2(align) || align > PAGE_SIZE);
>> +	nc->remaining = nc->remaining & -align;
>> +	return page_frag_alloc_va_prepare(nc, fragsz, gfp);
>> +}
>> +
>> +struct page *page_frag_alloc_pg_prepare(struct page_frag_cache *nc,
>> +					unsigned int *offset,
>> +					unsigned int *fragsz, gfp_t gfp);
>> +
>> +struct page *page_frag_alloc_prepare(struct page_frag_cache *nc,
>> +				     unsigned int *offset,
>> +				     unsigned int *fragsz,
>> +				     void **va, gfp_t gfp);
>> +
>> +static inline struct encoded_va *__page_frag_alloc_probe(struct page_frag_cache *nc,
>> +							 unsigned int *offset,
>> +							 unsigned int *fragsz,
>> +							 void **va)
>> +{
>> +	struct encoded_va *encoded_va;
>> +
>> +	*fragsz = nc->remaining;
>> +	encoded_va = nc->encoded_va;
>> +	*offset = page_frag_cache_page_size(encoded_va) - *fragsz;
>> +	*va = encoded_page_address(encoded_va) + *offset;
>> +
>> +	return encoded_va;
>> +}
>> +
>> +#define page_frag_alloc_probe(nc, offset, fragsz, va)			\
>> +({									\
>> +	struct page *__page = NULL;					\
>> +									\
>> +	VM_BUG_ON(!*(fragsz));						\
>> +	if (likely((nc)->remaining >= *(fragsz)))			\
>> +		__page = virt_to_page(__page_frag_alloc_probe(nc,	\
>> +							      offset,	\
>> +							      fragsz,	\
>> +							      va));	\
>> +									\
>> +	__page;								\
>> +})
>> +
> 
> Why is this a macro instead of just being an inline? Are you trying to
> avoid having to include a header due to the virt_to_page?

Yes, you are right.
I tried including different headers for virt_to_page(), and it did not
work for arch/x86/kernel/asm-offsets.s, which has included linux/sched.h,
and linux/sched.h need 'struct page_frag_cache' for 'struct task_struct'
after this patchset, including page_frag_cache.h for sched.h causes the
below compiler error:

  CC      arch/x86/kernel/asm-offsets.s
In file included from ./arch/x86/include/asm/page.h:89,
                 from ./arch/x86/include/asm/thread_info.h:12,
                 from ./include/linux/thread_info.h:60,
                 from ./include/linux/spinlock.h:60,
                 from ./include/linux/swait.h:7,
                 from ./include/linux/completion.h:12,
                 from ./include/linux/crypto.h:15,
                 from arch/x86/kernel/asm-offsets.c:9:
./include/linux/page_frag_cache.h: In function ‘page_frag_alloc_align’:
./include/asm-generic/memory_model.h:37:34: error: ‘vmemmap’ undeclared (first use in this function); did you mean ‘mem_map’?
   37 | #define __pfn_to_page(pfn)      (vmemmap + (pfn))
      |                                  ^~~~~~~
./include/asm-generic/memory_model.h:65:21: note: in expansion of macro ‘__pfn_to_page’
   65 | #define pfn_to_page __pfn_to_page
      |                     ^~~~~~~~~~~~~
./arch/x86/include/asm/page.h:68:33: note: in expansion of macro ‘pfn_to_page’
   68 | #define virt_to_page(kaddr)     pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
      |                                 ^~~~~~~~~~~
./include/linux/page_frag_cache.h:151:16: note: in expansion of macro ‘virt_to_page’
  151 |         return virt_to_page(va);
      |                ^~~~~~~~~~~~
./include/asm-generic/memory_model.h:37:34: note: each undeclared identifier is reported only once for each function it appears in
   37 | #define __pfn_to_page(pfn)      (vmemmap + (pfn))
      |                                  ^~~~~~~
./include/asm-generic/memory_model.h:65:21: note: in expansion of macro ‘__pfn_to_page’
   65 | #define pfn_to_page __pfn_to_page
      |                     ^~~~~~~~~~~~~
./arch/x86/include/asm/page.h:68:33: note: in expansion of macro ‘pfn_to_page’
   68 | #define virt_to_page(kaddr)     pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
      |                                 ^~~~~~~~~~~
./include/linux/page_frag_cache.h:151:16: note: in expansion of macro ‘virt_to_page’
  151 |         return virt_to_page(va);


Another possible way I can think of to aovid the above problem is to
split the page_frag_cache.h to something like page_frag_cache/types.h
and page_frag_cache/helpers.h as page_pool does, so that sched.h only
need to include page_frag_cache/types.h.
But I am not sure it is the correct way or it is worth the effort, what
do you think about this?

> 
> .
>