On Thu, Aug 14, 2025 at 01:26:37PM +0200, Jesper Dangaard Brouer wrote: > > > 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. > Nice! Thanks for looking at this! Will you send the patch separately? > > > 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. > Will send a patch when I find some time. > > > 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). > So you mean to drop the napi_direct flag in __xdp_return and let page_pool_put_unrefed_netmem() decide if direct should be used by page_pool_napi_local()? Thanks, Dragos
