| Message ID | 20241204172204.4180482-6-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring zero copy rx | expand |
On Wed, Dec 4, 2024 at 9:22 AM David Wei <dw@davidwei.uk> wrote: > > From: Pavel Begunkov <asml.silence@gmail.com> > > Some page pool memory providers like io_uring need to catch the point > when the page pool is asked to be destroyed. ->destroy is not enough > because it relies on the page pool to wait for its buffers first, but > for that to happen a provider might need to react, e.g. to collect all > buffers that are currently given to the user space. > > Add a new provider's scrub callback serving the purpose and called off > the pp's generic (cold) scrubbing path, i.e. page_pool_scrub(). > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > Signed-off-by: David Wei <dw@davidwei.uk> I think after numerous previous discussions on this op, I guess I finally see the point. AFAIU on destruction tho io_uring instance will destroy the page_pool, but we need to drop the user reference in the memory region. So the io_uring instance will destroy the pool, then the scrub callback tells io_uring that the pool is being destroyed, which drops the user references. I would have preferred if io_uring drops the user references before destroying the pool, which I think would have accomplished the same thing without adding a memory provider callback that is a bit specific to this use case, but I guess it's all the same. Reviewed-by: Mina Almasry <almasrymina@google.com>
On 12/9/24 17:08, Mina Almasry wrote: > On Wed, Dec 4, 2024 at 9:22 AM David Wei <dw@davidwei.uk> wrote: >> >> From: Pavel Begunkov <asml.silence@gmail.com> >> >> Some page pool memory providers like io_uring need to catch the point >> when the page pool is asked to be destroyed. ->destroy is not enough >> because it relies on the page pool to wait for its buffers first, but >> for that to happen a provider might need to react, e.g. to collect all >> buffers that are currently given to the user space. >> >> Add a new provider's scrub callback serving the purpose and called off >> the pp's generic (cold) scrubbing path, i.e. page_pool_scrub(). >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> Signed-off-by: David Wei <dw@davidwei.uk> > > I think after numerous previous discussions on this op, I guess I > finally see the point. > > AFAIU on destruction tho io_uring instance will destroy the page_pool, > but we need to drop the user reference in the memory region. So the > io_uring instance will destroy the pool, then the scrub callback tells > io_uring that the pool is being destroyed, which drops the user > references. > > I would have preferred if io_uring drops the user references before > destroying the pool, which I think would have accomplished the same > thing without adding a memory provider callback that is a bit specific > to this use case, but I guess it's all the same. For unrelated reasons I moved it to a later stage to io_uring code, so pool->mp_ops->scrub is no more. v8 is just weird, I think David sent an old branch because Jakub asked or so. > Reviewed-by: Mina Almasry <almasrymina@google.com>
diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h index 8a35fe474adb..fd0376ad0d26 100644 --- a/include/net/page_pool/types.h +++ b/include/net/page_pool/types.h @@ -157,6 +157,7 @@ struct memory_provider_ops { bool (*release_netmem)(struct page_pool *pool, netmem_ref netmem); int (*init)(struct page_pool *pool); void (*destroy)(struct page_pool *pool); + void (*scrub)(struct page_pool *pool); }; struct pp_memory_provider_params { diff --git a/net/core/page_pool.c b/net/core/page_pool.c index 36f61a1e4ffe..13f1a4a63760 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -1038,6 +1038,9 @@ static void page_pool_empty_alloc_cache_once(struct page_pool *pool) static void page_pool_scrub(struct page_pool *pool) { + if (pool->mp_ops && pool->mp_ops->scrub) + pool->mp_ops->scrub(pool); + page_pool_empty_alloc_cache_once(pool); pool->destroy_cnt++;