| Message ID | 20240826124021.2635705-8-linyunsheng@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Replace page_frag with page_frag_cache for sk_page_frag() | expand |
On Mon, Aug 26, 2024 at 5:46 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > Refactor common codes from __page_frag_alloc_va_align() to > __page_frag_cache_prepare() and __page_frag_cache_commit(), > so that the new API can make use of them. > > CC: Alexander Duyck <alexander.duyck@gmail.com> > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > --- > include/linux/page_frag_cache.h | 51 +++++++++++++++++++++++++++++++-- > mm/page_frag_cache.c | 20 ++++++------- > 2 files changed, 59 insertions(+), 12 deletions(-) > > diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h > index 372d6ed7e20a..2cc18a525936 100644 > --- a/include/linux/page_frag_cache.h > +++ b/include/linux/page_frag_cache.h > @@ -7,6 +7,7 @@ > #include <linux/build_bug.h> > #include <linux/log2.h> > #include <linux/mm.h> > +#include <linux/mmdebug.h> > #include <linux/mm_types_task.h> > #include <linux/types.h> > > @@ -75,8 +76,54 @@ static inline unsigned int page_frag_cache_page_size(unsigned long encoded_page) > > void page_frag_cache_drain(struct page_frag_cache *nc); > void __page_frag_cache_drain(struct page *page, unsigned int count); > -void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz, > - gfp_t gfp_mask, unsigned int align_mask); > +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int fragsz, > + struct page_frag *pfrag, gfp_t gfp_mask, > + unsigned int align_mask); > + > +static inline void __page_frag_cache_commit(struct page_frag_cache *nc, > + struct page_frag *pfrag, bool referenced, > + unsigned int used_sz) > +{ > + if (referenced) { > + VM_BUG_ON(!nc->pagecnt_bias); > + nc->pagecnt_bias--; > + } > + > + VM_BUG_ON(used_sz > pfrag->size); > + VM_BUG_ON(pfrag->page != page_frag_encoded_page_ptr(nc->encoded_page)); > + > + /* nc->offset is not reset when reusing an old page, so do not check for the > + * first fragment. > + * Committed offset might be bigger than the current offset due to alignment > + */ nc->offset should be reset when you are allocating a new page. I would suggest making that change as you should be able to verify that the fragment you are working with contains the frag you are working with. The page and offset should essentially be equal. > + VM_BUG_ON(pfrag->offset && nc->offset > pfrag->offset); > + VM_BUG_ON(pfrag->offset && > + pfrag->offset + pfrag->size > page_frag_cache_page_size(nc->encoded_page)); > + > + pfrag->size = used_sz; > + > + /* Calculate true size for the fragment due to alignment, nc->offset is not > + * reset for the first fragment when reusing an old page. > + */ > + pfrag->size += pfrag->offset ? (pfrag->offset - nc->offset) : 0; The pfrag->size should be the truesize already. You should have stored it as fragsz so that all you really need to do is push the offset forward by pfrag->size. > + > + nc->offset = pfrag->offset + used_sz; > +} > + I think this function might be better to keep in the .c file versus having it in the header file. ... > diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c > index 228cff9a4cdb..bba59c87d478 100644 > --- a/mm/page_frag_cache.c > +++ b/mm/page_frag_cache.c > @@ -67,16 +67,14 @@ void __page_frag_cache_drain(struct page *page, unsigned int count) > } > EXPORT_SYMBOL(__page_frag_cache_drain); > > -void *__page_frag_alloc_align(struct page_frag_cache *nc, > - unsigned int fragsz, gfp_t gfp_mask, > - unsigned int align_mask) > +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int fragsz, > + struct page_frag *pfrag, gfp_t gfp_mask, > + unsigned int align_mask) > { > unsigned long encoded_page = nc->encoded_page; > unsigned int size, offset; The 3 changes below can all be dropped. They are unnecessary optimizations of the unlikely path. > struct page *page; > > - size = page_frag_cache_page_size(encoded_page); > - > if (unlikely(!encoded_page)) { > refill: > page = __page_frag_cache_refill(nc, gfp_mask); > @@ -94,6 +92,9 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, > /* reset page count bias and offset to start of new frag */ > nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; > nc->offset = 0; Your code above said that offset wasn't reset. But it looks like it is reset here isn't it? > + } else { > + size = page_frag_cache_page_size(encoded_page); > + page = page_frag_encoded_page_ptr(encoded_page); > } > > offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask); > @@ -111,8 +112,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, > return NULL; > } > > - page = page_frag_encoded_page_ptr(encoded_page); > - > if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) > goto refill; > These 3 changes to move the size and page are unnecessary optimization. I would recommend just dropping them and leave the code as is as you are just optimizing for unlikely paths. > @@ -130,12 +129,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, > offset = 0; > } > > - nc->pagecnt_bias--; > - nc->offset = offset + fragsz; > + pfrag->page = page; > + pfrag->offset = offset; > + pfrag->size = size - offset; Why are you subtracting the offset from the size? Shouldn't this just be fragsz? > > return page_frag_encoded_page_address(encoded_page) + offset; > } > -EXPORT_SYMBOL(__page_frag_alloc_align); > +EXPORT_SYMBOL(__page_frag_cache_prepare); > > /* > * Frees a page fragment allocated out of either a compound or order 0 page. > -- > 2.33.0 >
On 2024/8/28 0:00, Alexander Duyck wrote: > On Mon, Aug 26, 2024 at 5:46 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: >> >> Refactor common codes from __page_frag_alloc_va_align() to >> __page_frag_cache_prepare() and __page_frag_cache_commit(), >> so that the new API can make use of them. >> >> CC: Alexander Duyck <alexander.duyck@gmail.com> >> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> >> --- >> include/linux/page_frag_cache.h | 51 +++++++++++++++++++++++++++++++-- >> mm/page_frag_cache.c | 20 ++++++------- >> 2 files changed, 59 insertions(+), 12 deletions(-) >> >> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h >> index 372d6ed7e20a..2cc18a525936 100644 >> --- a/include/linux/page_frag_cache.h >> +++ b/include/linux/page_frag_cache.h >> @@ -7,6 +7,7 @@ >> #include <linux/build_bug.h> >> #include <linux/log2.h> >> #include <linux/mm.h> >> +#include <linux/mmdebug.h> >> #include <linux/mm_types_task.h> >> #include <linux/types.h> >> >> @@ -75,8 +76,54 @@ static inline unsigned int page_frag_cache_page_size(unsigned long encoded_page) >> >> void page_frag_cache_drain(struct page_frag_cache *nc); >> void __page_frag_cache_drain(struct page *page, unsigned int count); >> -void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz, >> - gfp_t gfp_mask, unsigned int align_mask); >> +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int fragsz, >> + struct page_frag *pfrag, gfp_t gfp_mask, >> + unsigned int align_mask); >> + >> +static inline void __page_frag_cache_commit(struct page_frag_cache *nc, >> + struct page_frag *pfrag, bool referenced, >> + unsigned int used_sz) >> +{ >> + if (referenced) { >> + VM_BUG_ON(!nc->pagecnt_bias); >> + nc->pagecnt_bias--; >> + } >> + >> + VM_BUG_ON(used_sz > pfrag->size); >> + VM_BUG_ON(pfrag->page != page_frag_encoded_page_ptr(nc->encoded_page)); >> + >> + /* nc->offset is not reset when reusing an old page, so do not check for the >> + * first fragment. >> + * Committed offset might be bigger than the current offset due to alignment >> + */ > > nc->offset should be reset when you are allocating a new page. I would It is reset when a new page is allocated, but not when reusing a old page. > suggest making that change as you should be able to verify that the > fragment you are working with contains the frag you are working with. > The page and offset should essentially be equal. For the part why offset is not equal, currently we do the alignment for local variable 'offset' and set it to pfrag->offset instead of nc->offset in below __page_frag_cache_prepare(). > >> + VM_BUG_ON(pfrag->offset && nc->offset > pfrag->offset); >> + VM_BUG_ON(pfrag->offset && >> + pfrag->offset + pfrag->size > page_frag_cache_page_size(nc->encoded_page)); >> + >> + pfrag->size = used_sz; >> + >> + /* Calculate true size for the fragment due to alignment, nc->offset is not >> + * reset for the first fragment when reusing an old page. >> + */ >> + pfrag->size += pfrag->offset ? (pfrag->offset - nc->offset) : 0; > > The pfrag->size should be the truesize already. You should have stored > it as fragsz so that all you really need to do is push the offset > forward by pfrag->size. The general idea is below: The pfrag->size is set to indicate the max available cache of page_frag_cache when caller calls __page_frag_cache_prepare(), after deciding how much cache to use, the caller calls __page_frag_cache_commit() with 'used_sz', and __page_frag_cache_commit() returns the true size back to the caller by considering the offset alignment. And the above is mainly used to support the prepare API in patch 10. > >> + >> + nc->offset = pfrag->offset + used_sz; >> +} >> + > > I think this function might be better to keep in the .c file versus > having it in the header file. > > ... > >> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c >> index 228cff9a4cdb..bba59c87d478 100644 >> --- a/mm/page_frag_cache.c >> +++ b/mm/page_frag_cache.c >> @@ -67,16 +67,14 @@ void __page_frag_cache_drain(struct page *page, unsigned int count) >> } >> EXPORT_SYMBOL(__page_frag_cache_drain); >> >> -void *__page_frag_alloc_align(struct page_frag_cache *nc, >> - unsigned int fragsz, gfp_t gfp_mask, >> - unsigned int align_mask) >> +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int fragsz, >> + struct page_frag *pfrag, gfp_t gfp_mask, >> + unsigned int align_mask) >> { >> unsigned long encoded_page = nc->encoded_page; >> unsigned int size, offset; > > The 3 changes below can all be dropped. They are unnecessary > optimizations of the unlikely path. > >> struct page *page; >> >> - size = page_frag_cache_page_size(encoded_page); >> - >> if (unlikely(!encoded_page)) { >> refill: >> page = __page_frag_cache_refill(nc, gfp_mask); >> @@ -94,6 +92,9 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, >> /* reset page count bias and offset to start of new frag */ >> nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; >> nc->offset = 0; > > Your code above said that offset wasn't reset. But it looks like it is > reset here isn't it? This is refilling a new page for page_frag_cache, not reusing a old page for page_frag_cache as mentioned above, nc->offset not being reset is in below: https://elixir.free-electrons.com/linux/v6.11-rc5/source/mm/page_alloc.c#L4902 > >> + } else { >> + size = page_frag_cache_page_size(encoded_page); >> + page = page_frag_encoded_page_ptr(encoded_page); >> } >> >> offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask); >> @@ -111,8 +112,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, >> return NULL; >> } >> >> - page = page_frag_encoded_page_ptr(encoded_page); >> - >> if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) >> goto refill; >> > > These 3 changes to move the size and page are unnecessary > optimization. I would recommend just dropping them and leave the code > as is as you are just optimizing for unlikely paths. It is not an optimization, the page need to be set to pfrag->page in this patch, and page_frag_encoded_page_ptr() is only called when refill a new page and reusing a old page before this patch, the above changes enable the calling of page_frag_encoded_page_ptr() for all cases in this patch. > >> @@ -130,12 +129,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, >> offset = 0; >> } >> >> - nc->pagecnt_bias--; >> - nc->offset = offset + fragsz; >> + pfrag->page = page; >> + pfrag->offset = offset; >> + pfrag->size = size - offset; > > Why are you subtracting the offset from the size? Shouldn't this just be fragsz? As above, it is about supporting the prepare API in patch 10. > >> >> return page_frag_encoded_page_address(encoded_page) + offset; >> } >> -EXPORT_SYMBOL(__page_frag_alloc_align); >> +EXPORT_SYMBOL(__page_frag_cache_prepare); >> >> /* >> * Frees a page fragment allocated out of either a compound or order 0 page. >> -- >> 2.33.0 >>
diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h index 372d6ed7e20a..2cc18a525936 100644 --- a/include/linux/page_frag_cache.h +++ b/include/linux/page_frag_cache.h @@ -7,6 +7,7 @@ #include <linux/build_bug.h> #include <linux/log2.h> #include <linux/mm.h> +#include <linux/mmdebug.h> #include <linux/mm_types_task.h> #include <linux/types.h> @@ -75,8 +76,54 @@ static inline unsigned int page_frag_cache_page_size(unsigned long encoded_page) void page_frag_cache_drain(struct page_frag_cache *nc); void __page_frag_cache_drain(struct page *page, unsigned int count); -void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz, - gfp_t gfp_mask, unsigned int align_mask); +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int fragsz, + struct page_frag *pfrag, gfp_t gfp_mask, + unsigned int align_mask); + +static inline void __page_frag_cache_commit(struct page_frag_cache *nc, + struct page_frag *pfrag, bool referenced, + unsigned int used_sz) +{ + if (referenced) { + VM_BUG_ON(!nc->pagecnt_bias); + nc->pagecnt_bias--; + } + + VM_BUG_ON(used_sz > pfrag->size); + VM_BUG_ON(pfrag->page != page_frag_encoded_page_ptr(nc->encoded_page)); + + /* nc->offset is not reset when reusing an old page, so do not check for the + * first fragment. + * Committed offset might be bigger than the current offset due to alignment + */ + VM_BUG_ON(pfrag->offset && nc->offset > pfrag->offset); + VM_BUG_ON(pfrag->offset && + pfrag->offset + pfrag->size > page_frag_cache_page_size(nc->encoded_page)); + + pfrag->size = used_sz; + + /* Calculate true size for the fragment due to alignment, nc->offset is not + * reset for the first fragment when reusing an old page. + */ + pfrag->size += pfrag->offset ? (pfrag->offset - nc->offset) : 0; + + nc->offset = pfrag->offset + used_sz; +} + +static inline void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz, + gfp_t gfp_mask, unsigned int align_mask) +{ + struct page_frag page_frag; + void *va; + + va = __page_frag_cache_prepare(nc, fragsz, &page_frag, gfp_mask, align_mask); + if (unlikely(!va)) + return NULL; + + __page_frag_cache_commit(nc, &page_frag, true, fragsz); + + return va; +} static inline void *page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz, gfp_t gfp_mask, diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c index 228cff9a4cdb..bba59c87d478 100644 --- a/mm/page_frag_cache.c +++ b/mm/page_frag_cache.c @@ -67,16 +67,14 @@ void __page_frag_cache_drain(struct page *page, unsigned int count) } EXPORT_SYMBOL(__page_frag_cache_drain); -void *__page_frag_alloc_align(struct page_frag_cache *nc, - unsigned int fragsz, gfp_t gfp_mask, - unsigned int align_mask) +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int fragsz, + struct page_frag *pfrag, gfp_t gfp_mask, + unsigned int align_mask) { unsigned long encoded_page = nc->encoded_page; unsigned int size, offset; struct page *page; - size = page_frag_cache_page_size(encoded_page); - if (unlikely(!encoded_page)) { refill: page = __page_frag_cache_refill(nc, gfp_mask); @@ -94,6 +92,9 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, /* reset page count bias and offset to start of new frag */ nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; nc->offset = 0; + } else { + size = page_frag_cache_page_size(encoded_page); + page = page_frag_encoded_page_ptr(encoded_page); } offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask); @@ -111,8 +112,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, return NULL; } - page = page_frag_encoded_page_ptr(encoded_page); - if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) goto refill; @@ -130,12 +129,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc, offset = 0; } - nc->pagecnt_bias--; - nc->offset = offset + fragsz; + pfrag->page = page; + pfrag->offset = offset; + pfrag->size = size - offset; return page_frag_encoded_page_address(encoded_page) + offset; } -EXPORT_SYMBOL(__page_frag_alloc_align); +EXPORT_SYMBOL(__page_frag_cache_prepare); /* * Frees a page fragment allocated out of either a compound or order 0 page.
Refactor common codes from __page_frag_alloc_va_align() to __page_frag_cache_prepare() and __page_frag_cache_commit(), so that the new API can make use of them. CC: Alexander Duyck <alexander.duyck@gmail.com> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> --- include/linux/page_frag_cache.h | 51 +++++++++++++++++++++++++++++++-- mm/page_frag_cache.c | 20 ++++++------- 2 files changed, 59 insertions(+), 12 deletions(-)