Re: [BUG] mlx5_core memory management issue

[Jesper Dangaard Brouer <hawk@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4328 bytes --]



On 13/08/2025 22.24, Dragos Tatulea wrote:
> On Wed, Aug 13, 2025 at 07:26:49PM +0000, Dragos Tatulea wrote:
>> On Wed, Aug 13, 2025 at 01:53:48PM -0500, Chris Arges wrote:
>>> On 2025-08-12 16:25:58, Chris Arges wrote:
>>>> On 2025-08-12 20:19:30, Dragos Tatulea wrote:
>>>>> On Tue, Aug 12, 2025 at 11:55:39AM -0700, Jesse Brandeburg wrote:
>>>>>> On 8/12/25 8:44 AM, 'Dragos Tatulea' via kernel-team wrote:
>>>>>>
>>>>>>> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
>>>>>>> index 482d284a1553..484216c7454d 100644
>>>>>>> --- a/kernel/bpf/devmap.c
>>>>>>> +++ b/kernel/bpf/devmap.c
>>>>>>> @@ -408,8 +408,10 @@ static void bq_xmit_all(struct xdp_dev_bulk_queue *bq, u32 flags)
>>>>>>>           /* If not all frames have been transmitted, it is our
>>>>>>>            * responsibility to free them
>>>>>>>            */
>>>>>>> +       xdp_set_return_frame_no_direct();
>>>>>>>           for (i = sent; unlikely(i < to_send); i++)
>>>>>>>                   xdp_return_frame_rx_napi(bq->q[i]);
>>>>>>> +       xdp_clear_return_frame_no_direct();
>>>>>>
>>>>>> Why can't this instead just be xdp_return_frame(bq->q[i]); with no
>>>>>> "no_direct" fussing?
>>>>>>
>>>>>> Wouldn't this be the safest way for this function to call frame completion?
>>>>>> It seems like presuming the calling context is napi is wrong?
>>>>>>
>>>>> It would be better indeed. Thanks for removing my horse glasses!
>>>>>
>>>>> Once Chris verifies that this works for him I can prepare a fix patch.
>>>>>
>>>> Working on that now, I'm testing a kernel with the following change:
>>>>
>>>> ---
>>>>
>>>> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
>>>> index 3aa002a47..ef86d9e06 100644
>>>> --- a/kernel/bpf/devmap.c
>>>> +++ b/kernel/bpf/devmap.c
>>>> @@ -409,7 +409,7 @@ static void bq_xmit_all(struct xdp_dev_bulk_queue *bq, u32 flags)
>>>>           * responsibility to free them
>>>>           */
>>>>          for (i = sent; unlikely(i < to_send); i++)
>>>> -               xdp_return_frame_rx_napi(bq->q[i]);
>>>> +               xdp_return_frame(bq->q[i]);
>>>>   
>>>>   out:
>>>>          bq->count = 0;
>>>
>>> This patch resolves the issue I was seeing and I am no longer able to
>>> reproduce the issue. I tested for about 2 hours, when the reproducer usually
>>> takes about 1-2 minutes.
>>>
>> Thanks! Will send a patch tomorrow and also add you in the Tested-by tag.
>>

Looking at code ... there are more cases we need to deal with.
If simply replacing xdp_return_frame_rx_napi() with xdp_return_frame.

The normal way to fix this is to use the helpers:
  - xdp_set_return_frame_no_direct();
  - xdp_clear_return_frame_no_direct()

Because __xdp_return() code[1] via xdp_return_frame_no_direct() will
disable those napi_direct requests.

  [1] https://elixir.bootlin.com/linux/v6.16/source/net/core/xdp.c#L439

Something doesn't add-up, because the remote CPUMAP bpf-prog that 
redirects to veth is running in cpu_map_bpf_prog_run_xdp()[2] and that 
function already uses the xdp_set_return_frame_no_direct() helper.

  [2] https://elixir.bootlin.com/linux/v6.16/source/kernel/bpf/cpumap.c#L189

I see the bug now... attached a patch with the fix.
The scope for the "no_direct" forgot to wrap the xdp_do_flush() call.

Looks like bug was introduced in 11941f8a8536 ("bpf: cpumap: Implement 
generic cpumap") v5.15.

>> As follow up work it would be good to have a way to catch this family of
>> issues. Something in the lines of the patch below.
>>

Yes, please, we want something that can catch these kind of hard to find 
bugs.

>> Thanks,
>> Dragos
>>
>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>> index f1373756cd0f..0c498fbd8df6 100644
>> --- a/net/core/page_pool.c
>> +++ b/net/core/page_pool.c
>> @@ -794,6 +794,10 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem,
>>   {
>>          lockdep_assert_no_hardirq();
>>   
>> +#ifdef CONFIG_PAGE_POOL_CACHEDEBUG
>> +       WARN(page_pool_napi_local(pool), "Page pool cache access from non-direct napi context");
> I meant to negate the condition here.
> 

The XDP code have evolved since the xdp_set_return_frame_no_direct()
calls were added.  Now page_pool keeps track of pp->napi and
pool-> cpuid.  Maybe the __xdp_return [1] checks should be updated?
(and maybe it allows us to remove the no_direct helpers).

--Jesper

[-- Attachment #2: 01-cpumap-disable-pp-direct.patch --]
[-- Type: text/x-patch, Size: 2101 bytes --]

cpumap: disable page_pool direct xdp_return need larger scope

From: Jesper Dangaard Brouer <hawk@kernel.org>

When running an XDP bpf_prog on the remote CPU in cpumap code
then we must disable the direct return optimization that
xdp_return can perform for mem_type page_pool.  This optimization
assumes code is still executing under RX-NAPI of the original
receiving CPU, which isn't true on this remote CPU.

The cpumap code already disabled this via helpers
xdp_set_return_frame_no_direct() and xdp_clear_return_frame_no_direct(),
but the scope didn't include xdp_do_flush().

When doing XDP_REDIRECT towards e.g devmap this causes the
function bq_xmit_all() to run with direct return optimization
enabled. This can lead to hard to find bugs.

Fix by expanding scope to include xdp_do_flush().

Fixes: 11941f8a8536 ("bpf: cpumap: Implement generic cpumap")
Signed-off-by: Jesper Dangaard Brouer <hawk@kernel.org>
---
 kernel/bpf/cpumap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index b2b7b8ec2c2a..c46360b27871 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -186,7 +186,6 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
 	struct xdp_buff xdp;
 	int i, nframes = 0;
 
-	xdp_set_return_frame_no_direct();
 	xdp.rxq = &rxq;
 
 	for (i = 0; i < n; i++) {
@@ -231,7 +230,6 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
 		}
 	}
 
-	xdp_clear_return_frame_no_direct();
 	stats->pass += nframes;
 
 	return nframes;
@@ -255,6 +253,7 @@ static void cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames,
 
 	rcu_read_lock();
 	bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
+	xdp_set_return_frame_no_direct();
 
 	ret->xdp_n = cpu_map_bpf_prog_run_xdp(rcpu, frames, ret->xdp_n, stats);
 	if (unlikely(ret->skb_n))
@@ -264,6 +263,7 @@ static void cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames,
 	if (stats->redirect)
 		xdp_do_flush();
 
+	xdp_clear_return_frame_no_direct();
 	bpf_net_ctx_clear(bpf_net_ctx);
 	rcu_read_unlock();