| Message ID | 20241028115850.3409893-5-linyunsheng@huawei.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Replace page_frag with page_frag_cache (Part-2) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On Mon, Oct 28, 2024 at 5:05 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > For some case as tun_build_skb() without the needing of > using complicated prepare & commit API, add the abort API to > abort the operation of page_frag_alloc_*() related API for > error handling knowing that no one else is taking extra > reference to the just allocated fragment, and add abort_ref > API to only abort the reference counting of the allocated > fragment if it is already referenced by someone else. > > CC: Alexander Duyck <alexander.duyck@gmail.com> > CC: Andrew Morton <akpm@linux-foundation.org> > CC: Linux-MM <linux-mm@kvack.org> > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > --- > Documentation/mm/page_frags.rst | 7 +++++-- > include/linux/page_frag_cache.h | 20 ++++++++++++++++++++ > mm/page_frag_cache.c | 21 +++++++++++++++++++++ > 3 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/Documentation/mm/page_frags.rst b/Documentation/mm/page_frags.rst > index 34e654c2956e..339e641beb53 100644 > --- a/Documentation/mm/page_frags.rst > +++ b/Documentation/mm/page_frags.rst > @@ -114,9 +114,10 @@ fragsz if there is an alignment requirement for the size of the fragment. > .. kernel-doc:: include/linux/page_frag_cache.h > :identifiers: page_frag_cache_init page_frag_cache_is_pfmemalloc > __page_frag_alloc_align page_frag_alloc_align page_frag_alloc > + page_frag_alloc_abort > > .. kernel-doc:: mm/page_frag_cache.c > - :identifiers: page_frag_cache_drain page_frag_free > + :identifiers: page_frag_cache_drain page_frag_free page_frag_alloc_abort_ref > > Coding examples > =============== > @@ -143,8 +144,10 @@ Allocation & freeing API > goto do_error; > > err = do_something(va, size); > - if (err) > + if (err) { > + page_frag_alloc_abort(nc, va, size); > goto do_error; > + } > > ... > > diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h > index a2b1127e8ac8..c3347c97522c 100644 > --- a/include/linux/page_frag_cache.h > +++ b/include/linux/page_frag_cache.h > @@ -141,5 +141,25 @@ static inline void *page_frag_alloc(struct page_frag_cache *nc, > } > > void page_frag_free(void *addr); > +void page_frag_alloc_abort_ref(struct page_frag_cache *nc, void *va, > + unsigned int fragsz); > + > +/** > + * page_frag_alloc_abort - Abort the page fragment allocation. > + * @nc: page_frag cache to which the page fragment is aborted back > + * @va: virtual address of page fragment to be aborted > + * @fragsz: size of the page fragment to be aborted > + * > + * It is expected to be called from the same context as the allocation API. > + * Mostly used for error handling cases to abort the fragment allocation knowing > + * that no one else is taking extra reference to the just aborted fragment, so > + * that the aborted fragment can be reused. > + */ > +static inline void page_frag_alloc_abort(struct page_frag_cache *nc, void *va, > + unsigned int fragsz) > +{ > + page_frag_alloc_abort_ref(nc, va, fragsz); > + nc->offset -= fragsz; > +} > > #endif > diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c > index d014130fb893..4d5626da42ed 100644 > --- a/mm/page_frag_cache.c > +++ b/mm/page_frag_cache.c > @@ -201,3 +201,24 @@ void page_frag_free(void *addr) > free_unref_page(page, compound_order(page)); > } > EXPORT_SYMBOL(page_frag_free); > + > +/** > + * page_frag_alloc_abort_ref - Abort the reference of allocated fragment. > + * @nc: page_frag cache to which the page fragment is aborted back > + * @va: virtual address of page fragment to be aborted > + * @fragsz: size of the page fragment to be aborted > + * > + * It is expected to be called from the same context as the allocation API. > + * Mostly used for error handling cases to abort the reference of allocated > + * fragment if the fragment has been referenced for other usages, to aovid the > + * atomic operation of page_frag_free() API. > + */ > +void page_frag_alloc_abort_ref(struct page_frag_cache *nc, void *va, > + unsigned int fragsz) > +{ > + VM_BUG_ON(va + fragsz != > + encoded_page_decode_virt(nc->encoded_page) + nc->offset); > + > + nc->pagecnt_bias++; > +} > +EXPORT_SYMBOL(page_frag_alloc_abort_ref); It isn't clear to me why you split this over two functions. It seems like you could just update the offset in this lower function rather than do it in the upper one since you are passing all the arguments here anyway.
On 2024/10/29 1:53, Alexander Duyck wrote: > On Mon, Oct 28, 2024 at 5:05 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: >> >> For some case as tun_build_skb() without the needing of >> using complicated prepare & commit API, add the abort API to >> abort the operation of page_frag_alloc_*() related API for >> error handling knowing that no one else is taking extra >> reference to the just allocated fragment, and add abort_ref >> API to only abort the reference counting of the allocated >> fragment if it is already referenced by someone else. >> ... >> + >> +/** >> + * page_frag_alloc_abort_ref - Abort the reference of allocated fragment. >> + * @nc: page_frag cache to which the page fragment is aborted back >> + * @va: virtual address of page fragment to be aborted >> + * @fragsz: size of the page fragment to be aborted >> + * >> + * It is expected to be called from the same context as the allocation API. >> + * Mostly used for error handling cases to abort the reference of allocated >> + * fragment if the fragment has been referenced for other usages, to aovid the >> + * atomic operation of page_frag_free() API. >> + */ >> +void page_frag_alloc_abort_ref(struct page_frag_cache *nc, void *va, >> + unsigned int fragsz) >> +{ >> + VM_BUG_ON(va + fragsz != >> + encoded_page_decode_virt(nc->encoded_page) + nc->offset); >> + >> + nc->pagecnt_bias++; >> +} >> +EXPORT_SYMBOL(page_frag_alloc_abort_ref); > > It isn't clear to me why you split this over two functions. It seems > like you could just update the offset in this lower function rather > than do it in the upper one since you are passing all the arguments > here anyway. For the usecase in tun_build_skb(), There seems to be XDP_REDIRECT and XDP_TX case that the allocated fragment has been referenced for other usages even when xdp_do_redirect() or tun_xdp_tx() return error, so that caller can call page_frag_alloc_abort_ref() to abort its reference of the allocated fragment, but not abort the whole fragment for later reuse. As the doc mentioned above page_frag_alloc_abort_ref(), it is mainly to avoid the atomic operation of page_frag_free() API when the caller has the allocation context and has the all the arguments page_frag_alloc_abort_ref() needs even though it might be a unlikely case if page_frag_alloc_abort() API is already provided.
diff --git a/Documentation/mm/page_frags.rst b/Documentation/mm/page_frags.rst index 34e654c2956e..339e641beb53 100644 --- a/Documentation/mm/page_frags.rst +++ b/Documentation/mm/page_frags.rst @@ -114,9 +114,10 @@ fragsz if there is an alignment requirement for the size of the fragment. .. kernel-doc:: include/linux/page_frag_cache.h :identifiers: page_frag_cache_init page_frag_cache_is_pfmemalloc __page_frag_alloc_align page_frag_alloc_align page_frag_alloc + page_frag_alloc_abort .. kernel-doc:: mm/page_frag_cache.c - :identifiers: page_frag_cache_drain page_frag_free + :identifiers: page_frag_cache_drain page_frag_free page_frag_alloc_abort_ref Coding examples =============== @@ -143,8 +144,10 @@ Allocation & freeing API goto do_error; err = do_something(va, size); - if (err) + if (err) { + page_frag_alloc_abort(nc, va, size); goto do_error; + } ... diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h index a2b1127e8ac8..c3347c97522c 100644 --- a/include/linux/page_frag_cache.h +++ b/include/linux/page_frag_cache.h @@ -141,5 +141,25 @@ static inline void *page_frag_alloc(struct page_frag_cache *nc, } void page_frag_free(void *addr); +void page_frag_alloc_abort_ref(struct page_frag_cache *nc, void *va, + unsigned int fragsz); + +/** + * page_frag_alloc_abort - Abort the page fragment allocation. + * @nc: page_frag cache to which the page fragment is aborted back + * @va: virtual address of page fragment to be aborted + * @fragsz: size of the page fragment to be aborted + * + * It is expected to be called from the same context as the allocation API. + * Mostly used for error handling cases to abort the fragment allocation knowing + * that no one else is taking extra reference to the just aborted fragment, so + * that the aborted fragment can be reused. + */ +static inline void page_frag_alloc_abort(struct page_frag_cache *nc, void *va, + unsigned int fragsz) +{ + page_frag_alloc_abort_ref(nc, va, fragsz); + nc->offset -= fragsz; +} #endif diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c index d014130fb893..4d5626da42ed 100644 --- a/mm/page_frag_cache.c +++ b/mm/page_frag_cache.c @@ -201,3 +201,24 @@ void page_frag_free(void *addr) free_unref_page(page, compound_order(page)); } EXPORT_SYMBOL(page_frag_free); + +/** + * page_frag_alloc_abort_ref - Abort the reference of allocated fragment. + * @nc: page_frag cache to which the page fragment is aborted back + * @va: virtual address of page fragment to be aborted + * @fragsz: size of the page fragment to be aborted + * + * It is expected to be called from the same context as the allocation API. + * Mostly used for error handling cases to abort the reference of allocated + * fragment if the fragment has been referenced for other usages, to aovid the + * atomic operation of page_frag_free() API. + */ +void page_frag_alloc_abort_ref(struct page_frag_cache *nc, void *va, + unsigned int fragsz) +{ + VM_BUG_ON(va + fragsz != + encoded_page_decode_virt(nc->encoded_page) + nc->offset); + + nc->pagecnt_bias++; +} +EXPORT_SYMBOL(page_frag_alloc_abort_ref);
For some case as tun_build_skb() without the needing of using complicated prepare & commit API, add the abort API to abort the operation of page_frag_alloc_*() related API for error handling knowing that no one else is taking extra reference to the just allocated fragment, and add abort_ref API to only abort the reference counting of the allocated fragment if it is already referenced by someone else. CC: Alexander Duyck <alexander.duyck@gmail.com> CC: Andrew Morton <akpm@linux-foundation.org> CC: Linux-MM <linux-mm@kvack.org> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> --- Documentation/mm/page_frags.rst | 7 +++++-- include/linux/page_frag_cache.h | 20 ++++++++++++++++++++ mm/page_frag_cache.c | 21 +++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-)