[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7657b8ca-ce93-46a5-93bf-f5572ba7806b@redhat.com>
Date: Tue, 23 Jul 2024 12:59:32 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Taehee Yoo <ap420073@...il.com>, michael.chan@...adcom.com
Cc: somnath.kotur@...adcom.com, kuba@...nel.org, dw@...idwei.uk,
netdev@...r.kernel.org, horms@...nel.org, edumazet@...gle.com,
davem@...emloft.net
Subject: Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart
logic
On 7/21/24 07:35, Taehee Yoo wrote:
> When the netdev_rx_queue_restart() restarts queues, the bnxt_en driver
> updates(creates and deletes) a page_pool.
> But it doesn't update xdp_rxq_info, so the xdp_rxq_info is still
> connected to an old page_pool.
> So, bnxt_rx_ring_info->page_pool indicates a new page_pool, but
> bnxt_rx_ring_info->xdp_rxq is still connected to an old page_pool.
>
> An old page_pool is no longer used so it is supposed to be
> deleted by page_pool_destroy() but it isn't.
> Because the xdp_rxq_info is holding the reference count for it and the
> xdp_rxq_info is not updated, an old page_pool will not be deleted in
> the queue restart logic.
>
> Before restarting 1 queue:
> ./tools/net/ynl/samples/page-pool
> enp10s0f1np1[6] page pools: 4 (zombies: 0)
> refs: 8192 bytes: 33554432 (refs: 0 bytes: 0)
> recycling: 0.0% (alloc: 128:8048 recycle: 0:0)
>
> After restarting 1 queue:
> ./tools/net/ynl/samples/page-pool
> enp10s0f1np1[6] page pools: 5 (zombies: 0)
> refs: 10240 bytes: 41943040 (refs: 0 bytes: 0)
> recycling: 20.0% (alloc: 160:10080 recycle: 1920:128)
>
> Before restarting queues, an interface has 4 page_pools.
> After restarting one queue, an interface has 5 page_pools, but it
> should be 4, not 5.
> The reason is that queue restarting logic creates a new page_pool and
> an old page_pool is not deleted due to the absence of an update of
> xdp_rxq_info logic.
>
> Fixes: 2d694c27d32e ("bnxt_en: implement netdev_queue_mgmt_ops")
> Signed-off-by: Taehee Yoo <ap420073@...il.com>
@Michael: looks good?
> @@ -15018,6 +15019,16 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
> if (rc)
> return rc;
>
> + rc = xdp_rxq_info_reg(&clone->xdp_rxq, bp->dev, idx, 0);
> + if (rc < 0)
> + goto err_page_pool_destroy;
> +
> + rc = xdp_rxq_info_reg_mem_model(&clone->xdp_rxq,
> + MEM_TYPE_PAGE_POOL,
> + clone->page_pool);
> + if (rc)
> + goto err_rxq_info_unreg;
> +
> ring = &clone->rx_ring_struct;
> rc = bnxt_alloc_ring(bp, &ring->ring_mem);
> if (rc)
Side note for a possible 'net-next' follow-up: there is quite a bit of
duplicated code shared by both bnxt_queue_mem_alloc() and
bnxt_alloc_rx_rings(), that is likely worth a common helper.
Thanks,
Paolo
Powered by blists - more mailing lists