| Message ID | 20240625135216.47007-11-linyunsheng@huawei.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | First try to replace page_frag with page_frag_cache | expand |
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?
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? > > . >
On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > 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); > > I am pretty sure you just need to add: #include <asm/page.h>
On 6/30/2024 1:37 AM, Alexander Duyck wrote: > On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: ... >>> >>> 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); >> >> > > I am pretty sure you just need to add: > #include <asm/page.h> I am supposing you mean adding the above to page_frag_cache.h, right? It seems thing is more complicated for SPARSEMEM_VMEMMAP case, as it needs the declaration of 'vmemmap'(some arch defines it as a pointer variable while some arch defines it as a macro) and the definition of 'struct page' for '(vmemmap + (pfn))' operation. Adding below for 'vmemmap' and 'struct page' seems to have some compiler error caused by interdependence between linux/mm_types.h and asm/pgtable.h: #include <asm/pgtable.h> #include <linux/mm_types.h> As below, asm/pgtable.h obviously need the definition of 'struct mm_struct' from linux/mm_types.h, and linux/mm_types.h has some a long dependency of asm/pgtable.h starting from linux/uprobes.h if we add '#include <asm/pgtable.h>' in linux/page_frag_cache.h: In file included from ./include/linux/page_frag_cache.h:8, from ./include/linux/sched.h:49, from ./include/linux/percpu.h:13, from ./arch/x86/include/asm/msr.h:15, from ./arch/x86/include/asm/tsc.h:10, from ./arch/x86/include/asm/timex.h:6, from ./include/linux/timex.h:67, from ./include/linux/time32.h:13, from ./include/linux/time.h:60, from ./include/linux/jiffies.h:10, from ./include/linux/ktime.h:25, from ./include/linux/timer.h:6, from ./include/linux/workqueue.h:9, from ./include/linux/srcu.h:21, from ./include/linux/notifier.h:16, from ./arch/x86/include/asm/uprobes.h:13, from ./include/linux/uprobes.h:49, from ./include/linux/mm_types.h:16, from ./include/linux/mmzone.h:22, from ./include/linux/gfp.h:7, from ./include/linux/slab.h:16, from ./include/linux/crypto.h:17, from arch/x86/kernel/asm-offsets.c:9: ./arch/x86/include/asm/pgtable.h: In function ‘pte_accessible’: ./arch/x86/include/asm/pgtable.h:970:40: error: invalid use of undefined type ‘struct mm_struct’ 970 | atomic_read(&mm->tlb_flush_pending)) | ^~ ./arch/x86/include/asm/pgtable.h: In function ‘pmdp_establish’: ./arch/x86/include/asm/pgtable.h:1370:37: error: invalid use of undefined type ‘struct vm_area_struct’ 1370 | page_table_check_pmd_set(vma->vm_mm, pmdp, pmd); | ^~ ./arch/x86/include/asm/pgtable.h: At top level: ./arch/x86/include/asm/pgtable.h:1495:50: error: ‘struct vm_fault’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror] 1495 | static inline void update_mmu_cache_range(struct vm_fault *vmf, | ^~~~~~~~ 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_probe’: ./include/asm-generic/memory_model.h:37:42: error: invalid use of undefined type ‘struct page’ 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:225:16: note: in expansion of macro ‘virt_to_page’ 225 | return virt_to_page(encoded_va); | ^~~~~~~~~~~~ cc1: all warnings being treated as errors >
On Sun, Jun 30, 2024 at 7:05 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote: > > On 6/30/2024 1:37 AM, Alexander Duyck wrote: > > On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > ... > > >>> > >>> 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 am pretty sure you just need to add: > > #include <asm/page.h> > > I am supposing you mean adding the above to page_frag_cache.h, right? > > It seems thing is more complicated for SPARSEMEM_VMEMMAP case, as it > needs the declaration of 'vmemmap'(some arch defines it as a pointer > variable while some arch defines it as a macro) and the definition of > 'struct page' for '(vmemmap + (pfn))' operation. > > Adding below for 'vmemmap' and 'struct page' seems to have some compiler > error caused by interdependence between linux/mm_types.h and asm/pgtable.h: > #include <asm/pgtable.h> > #include <linux/mm_types.h> > Maybe you should just include linux/mm.h as that should have all the necessary includes to handle these cases. In any case though it doesn't make any sense to have a define in one include that expects the user to then figure out what other headers to include in order to make the define work they should be included in the header itself to avoid any sort of weird dependencies.
On 6/30/2024 10:35 PM, Alexander Duyck wrote: > On Sun, Jun 30, 2024 at 7:05 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote: >> >> On 6/30/2024 1:37 AM, Alexander Duyck wrote: >>> On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: >> >> ... >> >>>>> >>>>> 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 am pretty sure you just need to add: >>> #include <asm/page.h> >> >> I am supposing you mean adding the above to page_frag_cache.h, right? >> >> It seems thing is more complicated for SPARSEMEM_VMEMMAP case, as it >> needs the declaration of 'vmemmap'(some arch defines it as a pointer >> variable while some arch defines it as a macro) and the definition of >> 'struct page' for '(vmemmap + (pfn))' operation. >> >> Adding below for 'vmemmap' and 'struct page' seems to have some compiler >> error caused by interdependence between linux/mm_types.h and asm/pgtable.h: >> #include <asm/pgtable.h> >> #include <linux/mm_types.h> >> > > Maybe you should just include linux/mm.h as that should have all the > necessary includes to handle these cases. In any case though it Including linux/mm.h seems to have similar compiler error, just the interdependence is between linux/mm_types.h and linux/mm.h now. As below, linux/mmap_lock.h obviously need the definition of 'struct mm_struct' from linux/mm_types.h, and linux/mm_types.h has some a long dependency of linux/mm.h starting from linux/uprobes.h if we add '#include <linux/mm.h>' in linux/page_frag_cache.h: In file included from ./include/linux/mm.h:16, from ./include/linux/page_frag_cache.h:6, from ./include/linux/sched.h:49, from ./include/linux/percpu.h:13, from ./arch/x86/include/asm/msr.h:15, from ./arch/x86/include/asm/tsc.h:10, from ./arch/x86/include/asm/timex.h:6, from ./include/linux/timex.h:67, from ./include/linux/time32.h:13, from ./include/linux/time.h:60, from ./include/linux/jiffies.h:10, from ./include/linux/ktime.h:25, from ./include/linux/timer.h:6, from ./include/linux/workqueue.h:9, from ./include/linux/srcu.h:21, from ./include/linux/notifier.h:16, from ./arch/x86/include/asm/uprobes.h:13, from ./include/linux/uprobes.h:49, from ./include/linux/mm_types.h:16, from ./include/linux/mmzone.h:22, from ./include/linux/gfp.h:7, from ./include/linux/slab.h:16, from ./include/linux/crypto.h:17, from arch/x86/kernel/asm-offsets.c:9: ./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’: ./include/linux/mmap_lock.h:65:30: error: invalid use of undefined type ‘const struct mm_struct’ 65 | rwsem_assert_held(&mm->mmap_lock); | ^~ > doesn't make any sense to have a define in one include that expects > the user to then figure out what other headers to include in order to > make the define work they should be included in the header itself to > avoid any sort of weird dependencies. Perhaps there are some season why there are two headers for the mm subsystem, linux/mm_types.h and linux/mm.h? And .h file is supposed to include the linux/mm_types.h while .c file is supposed to include the linux/mm.h? If the above is correct, it seems the above rule is broked by including linux/mm.h in linux/page_frag_cache.h.
On 2024/6/30 23:05, Yunsheng Lin wrote: > On 6/30/2024 10:35 PM, Alexander Duyck wrote: >> On Sun, Jun 30, 2024 at 7:05 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote: >>> >>> On 6/30/2024 1:37 AM, Alexander Duyck wrote: >>>> On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: >>> >>> ... >>> >>>>>> >>>>>> 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 am pretty sure you just need to add: >>>> #include <asm/page.h> >>> >>> I am supposing you mean adding the above to page_frag_cache.h, right? >>> >>> It seems thing is more complicated for SPARSEMEM_VMEMMAP case, as it >>> needs the declaration of 'vmemmap'(some arch defines it as a pointer >>> variable while some arch defines it as a macro) and the definition of >>> 'struct page' for '(vmemmap + (pfn))' operation. >>> >>> Adding below for 'vmemmap' and 'struct page' seems to have some compiler >>> error caused by interdependence between linux/mm_types.h and asm/pgtable.h: >>> #include <asm/pgtable.h> >>> #include <linux/mm_types.h> >>> >> >> Maybe you should just include linux/mm.h as that should have all the >> necessary includes to handle these cases. In any case though it > > Including linux/mm.h seems to have similar compiler error, just the > interdependence is between linux/mm_types.h and linux/mm.h now. How about splitting page_frag_cache.h into page_frag_types.h and page_frag_cache.h mirroring the above linux/mm_types.h and linux/mm.h to fix the compiler error? > > As below, linux/mmap_lock.h obviously need the definition of > 'struct mm_struct' from linux/mm_types.h, and linux/mm_types.h > has some a long dependency of linux/mm.h starting from > linux/uprobes.h if we add '#include <linux/mm.h>' in linux/page_frag_cache.h: > > In file included from ./include/linux/mm.h:16, > from ./include/linux/page_frag_cache.h:6, > from ./include/linux/sched.h:49, > from ./include/linux/percpu.h:13, > from ./arch/x86/include/asm/msr.h:15, > from ./arch/x86/include/asm/tsc.h:10, > from ./arch/x86/include/asm/timex.h:6, > from ./include/linux/timex.h:67, > from ./include/linux/time32.h:13, > from ./include/linux/time.h:60, > from ./include/linux/jiffies.h:10, > from ./include/linux/ktime.h:25, > from ./include/linux/timer.h:6, > from ./include/linux/workqueue.h:9, > from ./include/linux/srcu.h:21, > from ./include/linux/notifier.h:16, > from ./arch/x86/include/asm/uprobes.h:13, > from ./include/linux/uprobes.h:49, > from ./include/linux/mm_types.h:16, > from ./include/linux/mmzone.h:22, > from ./include/linux/gfp.h:7, > from ./include/linux/slab.h:16, > from ./include/linux/crypto.h:17, > from arch/x86/kernel/asm-offsets.c:9: > ./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’: > ./include/linux/mmap_lock.h:65:30: error: invalid use of undefined type ‘const struct mm_struct’ > 65 | rwsem_assert_held(&mm->mmap_lock); > | ^~ > >> doesn't make any sense to have a define in one include that expects >> the user to then figure out what other headers to include in order to >> make the define work they should be included in the header itself to >> avoid any sort of weird dependencies. > > Perhaps there are some season why there are two headers for the mm subsystem, linux/mm_types.h and linux/mm.h? > And .h file is supposed to include the linux/mm_types.h while .c file > is supposed to include the linux/mm.h? > If the above is correct, it seems the above rule is broked by including linux/mm.h in linux/page_frag_cache.h. > . >
On Wed, Jul 3, 2024 at 5:40 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > On 2024/6/30 23:05, Yunsheng Lin wrote: > > On 6/30/2024 10:35 PM, Alexander Duyck wrote: > >> On Sun, Jun 30, 2024 at 7:05 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote: > >>> > >>> On 6/30/2024 1:37 AM, Alexander Duyck wrote: > >>>> On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > >>> > >>> ... > >>> > >>>>>> > >>>>>> 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 am pretty sure you just need to add: > >>>> #include <asm/page.h> > >>> > >>> I am supposing you mean adding the above to page_frag_cache.h, right? > >>> > >>> It seems thing is more complicated for SPARSEMEM_VMEMMAP case, as it > >>> needs the declaration of 'vmemmap'(some arch defines it as a pointer > >>> variable while some arch defines it as a macro) and the definition of > >>> 'struct page' for '(vmemmap + (pfn))' operation. > >>> > >>> Adding below for 'vmemmap' and 'struct page' seems to have some compiler > >>> error caused by interdependence between linux/mm_types.h and asm/pgtable.h: > >>> #include <asm/pgtable.h> > >>> #include <linux/mm_types.h> > >>> > >> > >> Maybe you should just include linux/mm.h as that should have all the > >> necessary includes to handle these cases. In any case though it > > > > Including linux/mm.h seems to have similar compiler error, just the > > interdependence is between linux/mm_types.h and linux/mm.h now. > > How about splitting page_frag_cache.h into page_frag_types.h and > page_frag_cache.h mirroring the above linux/mm_types.h and linux/mm.h > to fix the compiler error? > > > > > As below, linux/mmap_lock.h obviously need the definition of > > 'struct mm_struct' from linux/mm_types.h, and linux/mm_types.h > > has some a long dependency of linux/mm.h starting from > > linux/uprobes.h if we add '#include <linux/mm.h>' in linux/page_frag_cache.h: > > > > In file included from ./include/linux/mm.h:16, > > from ./include/linux/page_frag_cache.h:6, > > from ./include/linux/sched.h:49, > > from ./include/linux/percpu.h:13, > > from ./arch/x86/include/asm/msr.h:15, > > from ./arch/x86/include/asm/tsc.h:10, > > from ./arch/x86/include/asm/timex.h:6, > > from ./include/linux/timex.h:67, > > from ./include/linux/time32.h:13, > > from ./include/linux/time.h:60, > > from ./include/linux/jiffies.h:10, > > from ./include/linux/ktime.h:25, > > from ./include/linux/timer.h:6, > > from ./include/linux/workqueue.h:9, > > from ./include/linux/srcu.h:21, > > from ./include/linux/notifier.h:16, > > from ./arch/x86/include/asm/uprobes.h:13, > > from ./include/linux/uprobes.h:49, > > from ./include/linux/mm_types.h:16, > > from ./include/linux/mmzone.h:22, > > from ./include/linux/gfp.h:7, > > from ./include/linux/slab.h:16, > > from ./include/linux/crypto.h:17, > > from arch/x86/kernel/asm-offsets.c:9: > > ./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’: > > ./include/linux/mmap_lock.h:65:30: error: invalid use of undefined type ‘const struct mm_struct’ > > 65 | rwsem_assert_held(&mm->mmap_lock); > > | ^~ > > > >> doesn't make any sense to have a define in one include that expects > >> the user to then figure out what other headers to include in order to > >> make the define work they should be included in the header itself to > >> avoid any sort of weird dependencies. > > > > Perhaps there are some season why there are two headers for the mm subsystem, linux/mm_types.h and linux/mm.h? > > And .h file is supposed to include the linux/mm_types.h while .c file > > is supposed to include the linux/mm.h? > > If the above is correct, it seems the above rule is broked by including linux/mm.h in linux/page_frag_cache.h. > > . The issue is the dependency mess that has been created with patch 11 in the set. Again you are conflating patches which makes this really hard to debug or discuss as I make suggestions on one patch and you claim it breaks things that are really due to issues in another patch. So the issue is you included this header into include/linux/sched.h which is included in linux/mm_types.h. So what happens then is that you have to include page_frag_cache.h *before* you can include the bits from mm_types.h What might make more sense to solve this is to look at just moving the page_frag_cache into mm_types_task.h and then having it replace the page_frag struct there since mm_types.h will pull that in anyway. That way sched.h can avoid having to pull in page_frag_cache.h.
On 2024/7/8 1:12, Alexander Duyck wrote: ... > The issue is the dependency mess that has been created with patch 11 > in the set. Again you are conflating patches which makes this really > hard to debug or discuss as I make suggestions on one patch and you > claim it breaks things that are really due to issues in another patch. > So the issue is you included this header into include/linux/sched.h > which is included in linux/mm_types.h. So what happens then is that > you have to include page_frag_cache.h *before* you can include the > bits from mm_types.h > > What might make more sense to solve this is to look at just moving the > page_frag_cache into mm_types_task.h and then having it replace the > page_frag struct there since mm_types.h will pull that in anyway. That > way sched.h can avoid having to pull in page_frag_cache.h. It seems the above didn't work either, as asm-offsets.c does depend on mm_types_task.h too. In file included from ./include/linux/mm.h:16, from ./include/linux/page_frag_cache.h:10, from ./include/linux/mm_types_task.h:11, from ./include/linux/mm_types.h:5, from ./include/linux/mmzone.h:22, from ./include/linux/gfp.h:7, from ./include/linux/slab.h:16, from ./include/linux/resource_ext.h:11, from ./include/linux/acpi.h:13, from ./include/acpi/apei.h:9, from ./include/acpi/ghes.h:5, from ./include/linux/arm_sdei.h:8, from arch/arm64/kernel/asm-offsets.c:10: ./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’: ./include/linux/mmap_lock.h:65:23: error: invalid use of undefined type ‘const struct mm_struct’ 65 | rwsem_assert_held(&mm->mmap_lock); > . >
On Mon, Jul 8, 2024 at 3:58 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > On 2024/7/8 1:12, Alexander Duyck wrote: > > ... > > > The issue is the dependency mess that has been created with patch 11 > > in the set. Again you are conflating patches which makes this really > > hard to debug or discuss as I make suggestions on one patch and you > > claim it breaks things that are really due to issues in another patch. > > So the issue is you included this header into include/linux/sched.h > > which is included in linux/mm_types.h. So what happens then is that > > you have to include page_frag_cache.h *before* you can include the > > bits from mm_types.h > > > > What might make more sense to solve this is to look at just moving the > > page_frag_cache into mm_types_task.h and then having it replace the > > page_frag struct there since mm_types.h will pull that in anyway. That > > way sched.h can avoid having to pull in page_frag_cache.h. > > It seems the above didn't work either, as asm-offsets.c does depend on > mm_types_task.h too. > > In file included from ./include/linux/mm.h:16, > from ./include/linux/page_frag_cache.h:10, > from ./include/linux/mm_types_task.h:11, > from ./include/linux/mm_types.h:5, > from ./include/linux/mmzone.h:22, > from ./include/linux/gfp.h:7, > from ./include/linux/slab.h:16, > from ./include/linux/resource_ext.h:11, > from ./include/linux/acpi.h:13, > from ./include/acpi/apei.h:9, > from ./include/acpi/ghes.h:5, > from ./include/linux/arm_sdei.h:8, > from arch/arm64/kernel/asm-offsets.c:10: > ./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’: > ./include/linux/mmap_lock.h:65:23: error: invalid use of undefined type ‘const struct mm_struct’ > 65 | rwsem_assert_held(&mm->mmap_lock); Do not include page_frag_cache.h in mm_types_task.h. Just move the struct page_frag_cache there to replace struct page_frag.
On 2024/7/8 22:30, Alexander Duyck wrote: > On Mon, Jul 8, 2024 at 3:58 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: >> >> On 2024/7/8 1:12, Alexander Duyck wrote: >> >> ... >> >>> The issue is the dependency mess that has been created with patch 11 >>> in the set. Again you are conflating patches which makes this really >>> hard to debug or discuss as I make suggestions on one patch and you >>> claim it breaks things that are really due to issues in another patch. >>> So the issue is you included this header into include/linux/sched.h >>> which is included in linux/mm_types.h. So what happens then is that >>> you have to include page_frag_cache.h *before* you can include the >>> bits from mm_types.h >>> >>> What might make more sense to solve this is to look at just moving the >>> page_frag_cache into mm_types_task.h and then having it replace the >>> page_frag struct there since mm_types.h will pull that in anyway. That >>> way sched.h can avoid having to pull in page_frag_cache.h. >> >> It seems the above didn't work either, as asm-offsets.c does depend on >> mm_types_task.h too. >> >> In file included from ./include/linux/mm.h:16, >> from ./include/linux/page_frag_cache.h:10, >> from ./include/linux/mm_types_task.h:11, >> from ./include/linux/mm_types.h:5, >> from ./include/linux/mmzone.h:22, >> from ./include/linux/gfp.h:7, >> from ./include/linux/slab.h:16, >> from ./include/linux/resource_ext.h:11, >> from ./include/linux/acpi.h:13, >> from ./include/acpi/apei.h:9, >> from ./include/acpi/ghes.h:5, >> from ./include/linux/arm_sdei.h:8, >> from arch/arm64/kernel/asm-offsets.c:10: >> ./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’: >> ./include/linux/mmap_lock.h:65:23: error: invalid use of undefined type ‘const struct mm_struct’ >> 65 | rwsem_assert_held(&mm->mmap_lock); > > Do not include page_frag_cache.h in mm_types_task.h. Just move the > struct page_frag_cache there to replace struct page_frag. The above does seem a clever idea, but doesn't doing above also seem to defeat some purpose of patch 1? Anyway, it seems workable for trying to avoid adding a new header for a single struct. About the 'replace' part, as mentioned in [1], the 'struct page_frag' is still needed as this patchset is large enough that replacing is only done for sk_page_frag(), there are still other places using page_frag that can be replaced by page_frag_cache in the following patchset. 1. https://lore.kernel.org/all/b200a609-2f30-ec37-39b6-f37ed8092f41@huawei.com/ > . >
On Mon, Jul 8, 2024 at 11:58 PM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > On 2024/7/8 22:30, Alexander Duyck wrote: > > On Mon, Jul 8, 2024 at 3:58 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > >> > >> On 2024/7/8 1:12, Alexander Duyck wrote: > >> > >> ... > >> > >>> The issue is the dependency mess that has been created with patch 11 > >>> in the set. Again you are conflating patches which makes this really > >>> hard to debug or discuss as I make suggestions on one patch and you > >>> claim it breaks things that are really due to issues in another patch. > >>> So the issue is you included this header into include/linux/sched.h > >>> which is included in linux/mm_types.h. So what happens then is that > >>> you have to include page_frag_cache.h *before* you can include the > >>> bits from mm_types.h > >>> > >>> What might make more sense to solve this is to look at just moving the > >>> page_frag_cache into mm_types_task.h and then having it replace the > >>> page_frag struct there since mm_types.h will pull that in anyway. That > >>> way sched.h can avoid having to pull in page_frag_cache.h. > >> > >> It seems the above didn't work either, as asm-offsets.c does depend on > >> mm_types_task.h too. > >> > >> In file included from ./include/linux/mm.h:16, > >> from ./include/linux/page_frag_cache.h:10, > >> from ./include/linux/mm_types_task.h:11, > >> from ./include/linux/mm_types.h:5, > >> from ./include/linux/mmzone.h:22, > >> from ./include/linux/gfp.h:7, > >> from ./include/linux/slab.h:16, > >> from ./include/linux/resource_ext.h:11, > >> from ./include/linux/acpi.h:13, > >> from ./include/acpi/apei.h:9, > >> from ./include/acpi/ghes.h:5, > >> from ./include/linux/arm_sdei.h:8, > >> from arch/arm64/kernel/asm-offsets.c:10: > >> ./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’: > >> ./include/linux/mmap_lock.h:65:23: error: invalid use of undefined type ‘const struct mm_struct’ > >> 65 | rwsem_assert_held(&mm->mmap_lock); > > > > Do not include page_frag_cache.h in mm_types_task.h. Just move the > > struct page_frag_cache there to replace struct page_frag. > > The above does seem a clever idea, but doesn't doing above also seem to > defeat some purpose of patch 1? Anyway, it seems workable for trying > to avoid adding a new header for a single struct. > > About the 'replace' part, as mentioned in [1], the 'struct page_frag' > is still needed as this patchset is large enough that replacing is only > done for sk_page_frag(), there are still other places using page_frag > that can be replaced by page_frag_cache in the following patchset. > > 1. https://lore.kernel.org/all/b200a609-2f30-ec37-39b6-f37ed8092f41@huawei.com/ The point is you need to avoid pulling mm.h into sched.h. To do that you have to pull the data structure out and place it in a different header file. So maybe instead of creating yet another header file you can just place the structure in mm_types_task.h and once you have dealt with all of the other users you can finally drop the page_frag structure.
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; \ +}) + +static inline void page_frag_alloc_commit(struct page_frag_cache *nc, + unsigned int fragsz) +{ + VM_BUG_ON(fragsz > nc->remaining || !nc->pagecnt_bias); + nc->pagecnt_bias--; + nc->remaining -= fragsz; +} + +static inline void page_frag_alloc_commit_noref(struct page_frag_cache *nc, + unsigned int fragsz) +{ + VM_BUG_ON(fragsz > nc->remaining); + nc->remaining -= fragsz; +} + +static inline void page_frag_alloc_abort(struct page_frag_cache *nc, + unsigned int fragsz) +{ + nc->pagecnt_bias++; + nc->remaining += fragsz; +} + void page_frag_free_va(void *addr); #endif diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c index 58facd2b59f7..a6eb0ab2e7f9 100644 --- a/mm/page_frag_cache.c +++ b/mm/page_frag_cache.c @@ -91,6 +91,120 @@ static struct page *__page_frag_cache_refill(struct page_frag_cache *nc, return page; } +void *page_frag_alloc_va_prepare(struct page_frag_cache *nc, + unsigned int *fragsz, gfp_t gfp) +{ + struct encoded_va *encoded_va; + unsigned int remaining; + + remaining = nc->remaining; + if (unlikely(*fragsz > remaining)) { + if (unlikely(!__page_frag_cache_refill(nc, gfp) || + *fragsz > PAGE_SIZE)) + return NULL; + + remaining = nc->remaining; + } + + encoded_va = nc->encoded_va; + *fragsz = remaining; + return encoded_page_address(encoded_va) + + page_frag_cache_page_size(encoded_va) - remaining; +} +EXPORT_SYMBOL(page_frag_alloc_va_prepare); + +struct page *page_frag_alloc_pg_prepare(struct page_frag_cache *nc, + unsigned int *offset, + unsigned int *fragsz, gfp_t gfp) +{ + struct encoded_va *encoded_va; + unsigned int remaining; + struct page *page; + + remaining = nc->remaining; + if (unlikely(*fragsz > remaining)) { + if (unlikely(*fragsz > PAGE_SIZE)) { + *fragsz = 0; + return NULL; + } + + page = __page_frag_cache_refill(nc, gfp); + remaining = nc->remaining; + encoded_va = nc->encoded_va; + } else { + encoded_va = nc->encoded_va; + page = virt_to_page(encoded_va); + } + + *offset = page_frag_cache_page_size(encoded_va) - remaining; + *fragsz = remaining; + + return page; +} +EXPORT_SYMBOL(page_frag_alloc_pg_prepare); + +struct page *page_frag_alloc_prepare(struct page_frag_cache *nc, + unsigned int *offset, + unsigned int *fragsz, + void **va, gfp_t gfp) +{ + struct encoded_va *encoded_va; + unsigned int remaining; + struct page *page; + + remaining = nc->remaining; + if (unlikely(*fragsz > remaining)) { + if (unlikely(*fragsz > PAGE_SIZE)) { + *fragsz = 0; + return NULL; + } + + page = __page_frag_cache_refill(nc, gfp); + remaining = nc->remaining; + encoded_va = nc->encoded_va; + } else { + encoded_va = nc->encoded_va; + page = virt_to_page(encoded_va); + } + + *offset = page_frag_cache_page_size(encoded_va) - remaining; + *fragsz = remaining; + *va = encoded_page_address(encoded_va) + *offset; + + return page; +} +EXPORT_SYMBOL(page_frag_alloc_prepare); + +struct page *page_frag_alloc_pg(struct page_frag_cache *nc, + unsigned int *offset, unsigned int fragsz, + gfp_t gfp) +{ + struct page *page; + + if (unlikely(fragsz > nc->remaining)) { + if (unlikely(fragsz > PAGE_SIZE)) + return NULL; + + page = __page_frag_cache_refill(nc, gfp); + if (unlikely(!page)) + return NULL; + + *offset = 0; + } else { + struct encoded_va *encoded_va = nc->encoded_va; + + page = virt_to_page(encoded_va); + *offset = page_frag_cache_page_size(encoded_va) - + nc->remaining; + } + + nc->remaining -= fragsz; + nc->pagecnt_bias--; + + return page; +} +EXPORT_SYMBOL(page_frag_alloc_pg); + void page_frag_cache_drain(struct page_frag_cache *nc) { if (!nc->encoded_va)
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(+)