Re: [PATCH net] net: page_pool: don't try to stash the napi id

On Fri, Jan 24, 2025 at 2:18 PM Toke Høiland-Jørgensen [off-list ref] wrote:

Mina Almasry [off-list ref] writes:

quoted

On Thu, Jan 23, 2025 at 3:16 PM Jakub Kicinski [off-list ref] wrote:

quoted

Page ppol tried to cache the NAPI ID in page pool info to avoid

Page pool

quoted

having a dependency on the life cycle of the NAPI instance.
Since commit under Fixes the NAPI ID is not populated until
napi_enable() and there's a good chance that page pool is
created before NAPI gets enabled.

Protect the NAPI pointer with the existing page pool mutex,
the reading path already holds it. napi_id itself we need

The reading paths in page_pool.c don't hold the lock, no? Only the
reading paths in page_pool_user.c seem to do.

I could not immediately wrap my head around why pool->p.napi can be
accessed in page_pool_napi_local with no lock, but needs to be
protected in the code in page_pool_user.c. It seems
READ_ONCE/WRITE_ONCE protection is good enough to make sure
page_pool_napi_local doesn't race with
page_pool_disable_direct_recycling in a way that can crash (the
reading code either sees a valid pointer or NULL). Why is that not
good enough to also synchronize the accesses between
page_pool_disable_direct_recycling and page_pool_nl_fill? I.e., drop
the locking?

It actually seems that this is *not* currently the case. See the
discussion here:

https://lore.kernel.org/all/8734h8qgmz.fsf@toke.dk/ (local)

IMO (as indicated in the message linked above), we should require users
to destroy the page pool before freeing the NAPI memory, rather than add
additional synchronisation.

Ah, I see. I wonder if we should make this part of the API via comment
and/or add DEBUG_NET_WARN_ON to catch misuse, something like:

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index ed4cd114180a..3919ca302e95 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h

@@ -257,6 +257,10 @@ struct xdp_mem_info;

 #ifdef CONFIG_PAGE_POOL
 void page_pool_disable_direct_recycling(struct page_pool *pool);
+
+/* page_pool_destroy or page_pool_disable_direct_recycling must be

called before
+ * netif_napi_del if pool->p.napi is set.
+ */
 void page_pool_destroy(struct page_pool *pool);
 void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *),
                           const struct xdp_mem_info *mem);

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 5c4b788b811b..dc82767b2516 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -1161,6 +1161,8 @@ void page_pool_destroy(struct page_pool *pool)
        if (!page_pool_put(pool))
                return;

+       DEBUG_NET_WARN_ON(pool->p.napi && !napi_is_valid(pool->p.napi));
+
        page_pool_disable_direct_recycling(pool);
        page_pool_free_frag(pool);

I also took a quick spot check - which could be wrong - but it seems
to me both gve and bnxt free the napi before destroying the pool :(

But I think this entire discussion is unrelated to this patch, so and
the mutex sync in this patch seems necessary for the page_pool_user.c
code which runs outside of softirq context:

Reviewed-by: Mina Almasry <redacted>


-- 
Thanks,
Mina