[PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Dragos Tatulea]

Direct page releases to cache must be done on the same CPU as where NAPI
is running. Not doing so results in races that are quite difficult to
debug.

This change adds a debug configuration which issues a warning when
such buggy behaviour is encountered.

Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
 net/Kconfig.debug    | 10 +++++++
 net/core/page_pool.c | 66 ++++++++++++++++++++++++++------------------
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/net/Kconfig.debug b/net/Kconfig.debug
index 277fab8c4d77..7cd417fabbdc 100644
--- a/net/Kconfig.debug
+++ b/net/Kconfig.debug
@@ -39,3 +39,13 @@ config DEBUG_NET_SMALL_RTNL
 
 	  Once the conversion completes, rtnl_lock() will be removed
 	  and rtnetlink will gain per-netns scalability.
+
+config DEBUG_PAGE_POOL_CACHE_RELEASE
+	bool "Debug page releases into page_pool cache"
+	depends on DEBUG_KERNEL && NET && PAGE_POOL
+	help
+	  Enable debugging feature to track page releases to the
+	  page_pool cache from incorrect CPUs.
+
+	  This makes it easier to track races related to this incorrect
+	  usage of the page_pool API.
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index ba70569bd4b0..404064d893d6 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -755,6 +755,33 @@ static bool page_pool_recycle_in_ring(struct page_pool *pool, netmem_ref netmem)
 	return ret;
 }
 
+static bool page_pool_napi_local(const struct page_pool *pool)
+{
+	const struct napi_struct *napi;
+	u32 cpuid;
+
+	/* On PREEMPT_RT the softirq can be preempted by the consumer */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		return false;
+
+	if (unlikely(!in_softirq()))
+		return false;
+
+	/* Allow direct recycle if we have reasons to believe that we are
+	 * in the same context as the consumer would run, so there's
+	 * no possible race.
+	 * __page_pool_put_page() makes sure we're not in hardirq context
+	 * and interrupts are enabled prior to accessing the cache.
+	 */
+	cpuid = smp_processor_id();
+	if (READ_ONCE(pool->cpuid) == cpuid)
+		return true;
+
+	napi = READ_ONCE(pool->p.napi);
+
+	return napi && READ_ONCE(napi->list_owner) == cpuid;
+}
+
 /* Only allow direct recycling in special circumstances, into the
  * alloc side cache.  E.g. during RX-NAPI processing for XDP_DROP use-case.
  *
@@ -768,6 +795,18 @@ static bool page_pool_recycle_in_cache(netmem_ref netmem,
 		return false;
 	}
 
+#ifdef CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE
+	if (unlikely(!page_pool_napi_local(pool))) {
+		u32 pp_cpuid = READ_ONCE(pool->cpuid);
+		u32 cpuid = smp_processor_id();
+
+		WARN_RATELIMIT(1, "page_pool %d: direct page release from wrong CPU %d, expected CPU %d",
+			       pool->user.id, cpuid, pp_cpuid);
+
+		return false;
+	}
+#endif
+
 	/* Caller MUST have verified/know (page_ref_count(page) == 1) */
 	pool->alloc.cache[pool->alloc.count++] = netmem;
 	recycle_stat_inc(pool, cached);
@@ -833,33 +872,6 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem,
 	return 0;
 }
 
-static bool page_pool_napi_local(const struct page_pool *pool)
-{
-	const struct napi_struct *napi;
-	u32 cpuid;
-
-	/* On PREEMPT_RT the softirq can be preempted by the consumer */
-	if (IS_ENABLED(CONFIG_PREEMPT_RT))
-		return false;
-
-	if (unlikely(!in_softirq()))
-		return false;
-
-	/* Allow direct recycle if we have reasons to believe that we are
-	 * in the same context as the consumer would run, so there's
-	 * no possible race.
-	 * __page_pool_put_page() makes sure we're not in hardirq context
-	 * and interrupts are enabled prior to accessing the cache.
-	 */
-	cpuid = smp_processor_id();
-	if (READ_ONCE(pool->cpuid) == cpuid)
-		return true;
-
-	napi = READ_ONCE(pool->p.napi);
-
-	return napi && READ_ONCE(napi->list_owner) == cpuid;
-}
-
 void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,
 				  unsigned int dma_sync_size, bool allow_direct)
 {
-- 
2.48.1

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Sebastian Andrzej Siewior]

On 2025-09-18 11:48:21 [+0300], Dragos Tatulea wrote:
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index ba70569bd4b0..404064d893d6 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -768,6 +795,18 @@ static bool page_pool_recycle_in_cache(netmem_ref netmem,
>  		return false;
>  	}
>  
> +#ifdef CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE
> +	if (unlikely(!page_pool_napi_local(pool))) {

if you do IS_ENABLED(CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE) you could
avoid the ifdef.

A quick question, where is this allow_direct argument supposed to come
from? I just noticed that mlx5 does
   page_pool_put_unrefed_netmem(, true);

which then does not consider page_pool_napi_local(). But your proposed
change here will complain as it should.

> +		u32 pp_cpuid = READ_ONCE(pool->cpuid);
> +		u32 cpuid = smp_processor_id();
> +
> +		WARN_RATELIMIT(1, "page_pool %d: direct page release from wrong CPU %d, expected CPU %d",
> +			       pool->user.id, cpuid, pp_cpuid);
> +
> +		return false;
> +	}
> +#endif
> +
>  	/* Caller MUST have verified/know (page_ref_count(page) == 1) */
>  	pool->alloc.cache[pool->alloc.count++] = netmem;
>  	recycle_stat_inc(pool, cached);

Sebastian

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Dragos Tatulea]

On 18.09.25 12:13, Sebastian Andrzej Siewior wrote:
> On 2025-09-18 11:48:21 [+0300], Dragos Tatulea wrote:
>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>> index ba70569bd4b0..404064d893d6 100644
>> --- a/net/core/page_pool.c
>> +++ b/net/core/page_pool.c
>> @@ -768,6 +795,18 @@ static bool page_pool_recycle_in_cache(netmem_ref netmem,
>>  		return false;
>>  	}
>>  
>> +#ifdef CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE
>> +	if (unlikely(!page_pool_napi_local(pool))) {
> 
> if you do IS_ENABLED(CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE) you could
> avoid the ifdef.
>
Ack. Makes sense. 
> A quick question, where is this allow_direct argument supposed to come
> from? I just noticed that mlx5 does
>    page_pool_put_unrefed_netmem(, true);
> 
> which then does not consider page_pool_napi_local(). But your proposed
> change here will complain as it should.
>

Good point and an oversight on my behalf. It will indeed complain during
rq teardown. If there is agreement on the approach proposed here then the
mlx5 driver will be changed to set the flag to false during teardown.
We used to do that but removed it for simplicity.

Thanks,
Dragos

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Jakub Kicinski]

On Thu, 18 Sep 2025 11:48:21 +0300 Dragos Tatulea wrote:
> Direct page releases to cache must be done on the same CPU as where NAPI
> is running.

You talk about NAPI..

>  /* Only allow direct recycling in special circumstances, into the
>   * alloc side cache.  E.g. during RX-NAPI processing for XDP_DROP use-case.
>   *
> @@ -768,6 +795,18 @@ static bool page_pool_recycle_in_cache(netmem_ref netmem,
>  		return false;
>  	}
>  
> +#ifdef CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE
> +	if (unlikely(!page_pool_napi_local(pool))) {
> +		u32 pp_cpuid = READ_ONCE(pool->cpuid);

but then you print pp->cpuid?

The patch seems half-baked. If the NAPI local recycling is incorrect
the pp will leak a reference and live forever. Which hopefully people
would notice. Are you adding this check just to double confirm that
any leaks you're chasing are in the driver, and not in the core?

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Dragos Tatulea]

On Fri, Sep 19, 2025 at 04:57:46PM -0700, Jakub Kicinski wrote:
> On Thu, 18 Sep 2025 11:48:21 +0300 Dragos Tatulea wrote:
> > Direct page releases to cache must be done on the same CPU as where NAPI
> > is running.
> 
> You talk about NAPI..
> 
> >  /* Only allow direct recycling in special circumstances, into the
> >   * alloc side cache.  E.g. during RX-NAPI processing for XDP_DROP use-case.
> >   *
> > @@ -768,6 +795,18 @@ static bool page_pool_recycle_in_cache(netmem_ref netmem,
> >  		return false;
> >  	}
> >  
> > +#ifdef CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE
> > +	if (unlikely(!page_pool_napi_local(pool))) {
> > +		u32 pp_cpuid = READ_ONCE(pool->cpuid);
> 
> but then you print pp->cpuid?
>
Point taken. I didn't want to replicate half of page_pool_napi_local()
in the error path. Printing information about the CPU id is also not
really important. The value comes from the stack trace which points to
the code that recycles to the cache from the wrong CPU.

> The patch seems half-baked. If the NAPI local recycling is incorrect
> the pp will leak a reference and live forever. Which hopefully people
> would notice. Are you adding this check just to double confirm that
> any leaks you're chasing are in the driver, and not in the core?
The point is not to chase leaks but races from doing a recycle to cache
from the wrong CPU. This is how XDP issue was caught where
xdp_set_return_frame_no_direct() was not set appropriately for cpumap [1].

My first approach was to __page_pool_put_page() but then I figured that
the warning should live closer to where the actual assignment happens.

[1] https://lore.kernel.org/all/e60404e2-4782-409f-8596-ae21ce7272c4@kernel.org/

Thanks,
Dragos

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Jesper Dangaard Brouer]

On 18/09/2025 10.48, Dragos Tatulea wrote:
> Direct page releases to cache must be done on the same CPU as where NAPI
> is running. Not doing so results in races that are quite difficult to
> debug.
> 
> This change adds a debug configuration which issues a warning when
> such buggy behaviour is encountered.
> 
> Signed-off-by: Dragos Tatulea<dtatulea@nvidia.com>
> Reviewed-by: Tariq Toukan<tariqt@nvidia.com>
> ---
>   net/Kconfig.debug    | 10 +++++++
>   net/core/page_pool.c | 66 ++++++++++++++++++++++++++------------------
>   2 files changed, 49 insertions(+), 27 deletions(-)
> 
[...]

> @@ -768,6 +795,18 @@ static bool page_pool_recycle_in_cache(netmem_ref netmem,
>   		return false;
>   	}
>   
> +#ifdef CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE
> +	if (unlikely(!page_pool_napi_local(pool))) {
> +		u32 pp_cpuid = READ_ONCE(pool->cpuid);
> +		u32 cpuid = smp_processor_id();
> +
> +		WARN_RATELIMIT(1, "page_pool %d: direct page release from wrong CPU %d, expected CPU %d",
> +			       pool->user.id, cpuid, pp_cpuid);
> +
> +		return false;
> +	}
> +#endif

The page_pool_recycle_in_cache() is an extreme fast-path for page_pool.
I know this is a debugging patch, but I would like to know the overhead
this adds (when enabled, compared to not enabled).

We (Mina) recently added a benchmark module for page_pool
under tools/testing/selftests/net/bench/page_pool/ that you can use.

Adding a WARN in fast-path code need extra careful review (maybe is it
okay here), this is because it adds an asm instruction (on Intel CPUs
ud2) what influence instruction cache prefetching.  Looks like this only
gets inlined two places (page_pool_put_unrefed_netmem and
page_pool_put_page_bulk), and it might be okay... I think it is.
See how I worked around this in commit 34cc0b338a61 ("xdp: Xdp_frame add
member frame_sz and handle in convert_to_xdp_frame").

--Jesper

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Dragos Tatulea]

On Sat, Sep 20, 2025 at 07:36:49PM +0200, Jesper Dangaard Brouer wrote:
> 
> 
> On 18/09/2025 10.48, Dragos Tatulea wrote:
> > Direct page releases to cache must be done on the same CPU as where NAPI
> > is running. Not doing so results in races that are quite difficult to
> > debug.
> > 
> > This change adds a debug configuration which issues a warning when
> > such buggy behaviour is encountered.
> > 
> > Signed-off-by: Dragos Tatulea<dtatulea@nvidia.com>
> > Reviewed-by: Tariq Toukan<tariqt@nvidia.com>
> > ---
> >   net/Kconfig.debug    | 10 +++++++
> >   net/core/page_pool.c | 66 ++++++++++++++++++++++++++------------------
> >   2 files changed, 49 insertions(+), 27 deletions(-)
> > 
> [...]
> 
> > @@ -768,6 +795,18 @@ static bool page_pool_recycle_in_cache(netmem_ref netmem,
> >   		return false;
> >   	}
> > +#ifdef CONFIG_DEBUG_PAGE_POOL_CACHE_RELEASE
> > +	if (unlikely(!page_pool_napi_local(pool))) {
> > +		u32 pp_cpuid = READ_ONCE(pool->cpuid);
> > +		u32 cpuid = smp_processor_id();
> > +
> > +		WARN_RATELIMIT(1, "page_pool %d: direct page release from wrong CPU %d, expected CPU %d",
> > +			       pool->user.id, cpuid, pp_cpuid);
> > +
> > +		return false;
> > +	}
> > +#endif
> 
> The page_pool_recycle_in_cache() is an extreme fast-path for page_pool.
> I know this is a debugging patch, but I would like to know the overhead
> this adds (when enabled, compared to not enabled).
> 
> We (Mina) recently added a benchmark module for page_pool
> under tools/testing/selftests/net/bench/page_pool/ that you can use.
>
Is there a way to trigger the fast-path? It doesn't seem to run
in_softirq() ...

I will also have to do some additional configuration so that
page_pool_napi_local() doesn't return false. I assume that we want to
test the perf of the non-erroneous case. Right?

> Adding a WARN in fast-path code need extra careful review (maybe is it
> okay here), this is because it adds an asm instruction (on Intel CPUs
> ud2) what influence instruction cache prefetching.  Looks like this only
> gets inlined two places (page_pool_put_unrefed_netmem and
> page_pool_put_page_bulk), and it might be okay... I think it is.
> See how I worked around this in commit 34cc0b338a61 ("xdp: Xdp_frame add
> member frame_sz and handle in convert_to_xdp_frame").
>
Thanks for the explanation and the pointer. Will do this in the next
version.

Thanks,
Dragos

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Jakub Kicinski]

On Sat, 20 Sep 2025 09:25:31 +0000 Dragos Tatulea wrote:
> > The patch seems half-baked. If the NAPI local recycling is incorrect
> > the pp will leak a reference and live forever. Which hopefully people
> > would notice. Are you adding this check just to double confirm that
> > any leaks you're chasing are in the driver, and not in the core?  
>
> The point is not to chase leaks but races from doing a recycle to cache
> from the wrong CPU. This is how XDP issue was caught where
> xdp_set_return_frame_no_direct() was not set appropriately for cpumap [1].
> 
> My first approach was to __page_pool_put_page() but then I figured that
> the warning should live closer to where the actual assignment happens.
> 
> [1] https://lore.kernel.org/all/e60404e2-4782-409f-8596-ae21ce7272c4@kernel.org/

Ah, that thing. I wonder whether the complexity in the driver-facing 
xdp_return API is really worth the gain here. IIUC we want to extract
the cases where we're doing local recycling and let those cases use
the lockless cache. But all those cases should be caught by automatic
local recycling detection, so caller can just pass false..

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Dragos Tatulea]

On Mon, Sep 22, 2025 at 04:18:27PM -0700, Jakub Kicinski wrote:
> On Sat, 20 Sep 2025 09:25:31 +0000 Dragos Tatulea wrote:
> > > The patch seems half-baked. If the NAPI local recycling is incorrect
> > > the pp will leak a reference and live forever. Which hopefully people
> > > would notice. Are you adding this check just to double confirm that
> > > any leaks you're chasing are in the driver, and not in the core?  
> >
> > The point is not to chase leaks but races from doing a recycle to cache
> > from the wrong CPU. This is how XDP issue was caught where
> > xdp_set_return_frame_no_direct() was not set appropriately for cpumap [1].
> > 
> > My first approach was to __page_pool_put_page() but then I figured that
> > the warning should live closer to where the actual assignment happens.
> > 
> > [1] https://lore.kernel.org/all/e60404e2-4782-409f-8596-ae21ce7272c4@kernel.org/
> 
> Ah, that thing. I wonder whether the complexity in the driver-facing 
> xdp_return API is really worth the gain here. IIUC we want to extract
> the cases where we're doing local recycling and let those cases use
> the lockless cache. But all those cases should be caught by automatic
> local recycling detection, so caller can just pass false..
>
This patch was simply adding the debugging code to catch the potential
misuse from any callers.

I was planning to send another patch for the xdp_return() API part
once/if this one got accepted. If it makes more sense I can bundle them
together in a RFC (as merge window is coming).

Thanks,
Dragos

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Jakub Kicinski]

On Tue, 23 Sep 2025 15:23:02 +0000 Dragos Tatulea wrote:
> On Mon, Sep 22, 2025 at 04:18:27PM -0700, Jakub Kicinski wrote:
> > On Sat, 20 Sep 2025 09:25:31 +0000 Dragos Tatulea wrote:  
> > > The point is not to chase leaks but races from doing a recycle to cache
> > > from the wrong CPU. This is how XDP issue was caught where
> > > xdp_set_return_frame_no_direct() was not set appropriately for cpumap [1].
> > > 
> > > My first approach was to __page_pool_put_page() but then I figured that
> > > the warning should live closer to where the actual assignment happens.
> > > 
> > > [1] https://lore.kernel.org/all/e60404e2-4782-409f-8596-ae21ce7272c4@kernel.org/  
> > 
> > Ah, that thing. I wonder whether the complexity in the driver-facing 
> > xdp_return API is really worth the gain here. IIUC we want to extract
> > the cases where we're doing local recycling and let those cases use
> > the lockless cache. But all those cases should be caught by automatic
> > local recycling detection, so caller can just pass false..
> >  
> This patch was simply adding the debugging code to catch the potential
> misuse from any callers.
> 
> I was planning to send another patch for the xdp_return() API part
> once/if this one got accepted. If it makes more sense I can bundle them
> together in a RFC (as merge window is coming).

Combined RFC would make sense, yes.

But you get what I'm saying right? I'm questioning whether _rx_napi()
flavor of calls even make sense these days. If they don't I'd think
the drivers can't be wrong and the need for the debug check is
diminished?

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Dragos Tatulea]

On Tue, Sep 23, 2025 at 08:34:39AM -0700, Jakub Kicinski wrote:
> On Tue, 23 Sep 2025 15:23:02 +0000 Dragos Tatulea wrote:
> > On Mon, Sep 22, 2025 at 04:18:27PM -0700, Jakub Kicinski wrote:
> > > On Sat, 20 Sep 2025 09:25:31 +0000 Dragos Tatulea wrote:  
> > > > The point is not to chase leaks but races from doing a recycle to cache
> > > > from the wrong CPU. This is how XDP issue was caught where
> > > > xdp_set_return_frame_no_direct() was not set appropriately for cpumap [1].
> > > > 
> > > > My first approach was to __page_pool_put_page() but then I figured that
> > > > the warning should live closer to where the actual assignment happens.
> > > > 
> > > > [1] https://lore.kernel.org/all/e60404e2-4782-409f-8596-ae21ce7272c4@kernel.org/  
> > > 
> > > Ah, that thing. I wonder whether the complexity in the driver-facing 
> > > xdp_return API is really worth the gain here. IIUC we want to extract
> > > the cases where we're doing local recycling and let those cases use
> > > the lockless cache. But all those cases should be caught by automatic
> > > local recycling detection, so caller can just pass false..
> > >  
> > This patch was simply adding the debugging code to catch the potential
> > misuse from any callers.
> > 
> > I was planning to send another patch for the xdp_return() API part
> > once/if this one got accepted. If it makes more sense I can bundle them
> > together in a RFC (as merge window is coming).
> 
> Combined RFC would make sense, yes.
>
Ack.

> But you get what I'm saying right? I'm questioning whether _rx_napi()
> flavor of calls even make sense these days. If they don't I'd think
> the drivers can't be wrong and the need for the debug check is
> diminished?
I got the point for XDP. I am not sure if you are arguing the same thing
for the other users though. For example. there are many drivers
releasing netmems with direct=true.

Thanks,
Dragos

Re: [PATCH net-next] page_pool: add debug for release to cache from wrong CPU

[Jakub Kicinski]

On Tue, 23 Sep 2025 16:00:27 +0000 Dragos Tatulea wrote:
> > But you get what I'm saying right? I'm questioning whether _rx_napi()
> > flavor of calls even make sense these days. If they don't I'd think
> > the drivers can't be wrong and the need for the debug check is
> > diminished?  
> I got the point for XDP. I am not sure if you are arguing the same thing
> for the other users though. For example. there are many drivers
> releasing netmems with direct=true.

Right, I was thinking that XDP is the only complex case.
The other direct=true cases generally happen on the Rx path
when we are processing the frame. So chances that we get the 
context wrong are much lower. XDP is using the recycling from
Tx completions paths IIUC. So we need far more care in checking
that the frame actually came from the local NAPI.