Messages in this thread | | | Date | Thu, 15 Feb 2024 12:57:05 +0100 | From | Lorenzo Bianconi <> | Subject | Re: [PATCH net-next] page_pool: disable direct recycling based on pool->cpuid on destroy |
| |
> Now that direct recycling is performed basing on pool->cpuid when set, > memory leaks are possible: > > 1. A pool is destroyed. > 2. Alloc cache is emptied (it's done only once). > 3. pool->cpuid is still set. > 4. napi_pp_put_page() does direct recycling basing on pool->cpuid. > 5. Now alloc cache is not empty, but it won't ever be freed. > > In order to avoid that, rewrite pool->cpuid to -1 when unlinking NAPI to > make sure no direct recycling will be possible after emptying the cache. > This involves a bit of overhead as pool->cpuid now must be accessed > via READ_ONCE() to avoid partial reads. > Rename page_pool_unlink_napi() -> page_pool_disable_direct_recycling() > to reflect what it actually does and unexport it.
Hi Alexander,
IIUC the reported issue, it requires the page_pool is destroyed (correct?), but system page_pool (the only one with cpuid not set to -1) will never be destroyed at runtime (or at we should avoid that). Am I missing something?
Rergards, Lorenzo
> > Fixes: 2b0cfa6e4956 ("net: add generic percpu page_pool allocator") > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> > --- > include/net/page_pool/types.h | 5 ----- > net/core/page_pool.c | 10 +++++++--- > net/core/skbuff.c | 2 +- > 3 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h > index 3828396ae60c..3590fbe6e3f1 100644 > --- a/include/net/page_pool/types.h > +++ b/include/net/page_pool/types.h > @@ -210,17 +210,12 @@ struct page_pool *page_pool_create_percpu(const struct page_pool_params *params, > struct xdp_mem_info; > > #ifdef CONFIG_PAGE_POOL > -void page_pool_unlink_napi(struct page_pool *pool); > void page_pool_destroy(struct page_pool *pool); > void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *), > struct xdp_mem_info *mem); > void page_pool_put_page_bulk(struct page_pool *pool, void **data, > int count); > #else > -static inline void page_pool_unlink_napi(struct page_pool *pool) > -{ > -} > - > static inline void page_pool_destroy(struct page_pool *pool) > { > } > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index 89c835fcf094..e8b9399d8e32 100644 > --- a/net/core/page_pool.c > +++ b/net/core/page_pool.c > @@ -949,8 +949,13 @@ void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *), > pool->xdp_mem_id = mem->id; > } > > -void page_pool_unlink_napi(struct page_pool *pool) > +static void page_pool_disable_direct_recycling(struct page_pool *pool) > { > + /* Disable direct recycling based on pool->cpuid. > + * Paired with READ_ONCE() in napi_pp_put_page(). > + */ > + WRITE_ONCE(pool->cpuid, -1); > + > if (!pool->p.napi) > return; > > @@ -962,7 +967,6 @@ void page_pool_unlink_napi(struct page_pool *pool) > > WRITE_ONCE(pool->p.napi, NULL); > } > -EXPORT_SYMBOL(page_pool_unlink_napi); > > void page_pool_destroy(struct page_pool *pool) > { > @@ -972,7 +976,7 @@ void page_pool_destroy(struct page_pool *pool) > if (!page_pool_put(pool)) > return; > > - page_pool_unlink_napi(pool); > + page_pool_disable_direct_recycling(pool); > page_pool_free_frag(pool); > > if (!page_pool_release(pool)) > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 0d9a489e6ae1..b41856585c24 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -1018,7 +1018,7 @@ bool napi_pp_put_page(struct page *page, bool napi_safe) > unsigned int cpuid = smp_processor_id(); > > allow_direct = napi && READ_ONCE(napi->list_owner) == cpuid; > - allow_direct |= (pp->cpuid == cpuid); > + allow_direct |= READ_ONCE(pp->cpuid) == cpuid; > } > > /* Driver set this to memory recycling info. Reset it on recycle. > -- > 2.43.0 > [unhandled content-type:application/pgp-signature] | |