* [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
@ 2024-07-21 5:35 Taehee Yoo
2024-07-21 16:57 ` David Wei
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Taehee Yoo @ 2024-07-21 5:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, michael.chan, netdev
Cc: ap420073, somnath.kotur, dw, horms
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@gmail.com>
---
v2:
- Do not use memcpy in the bnxt_queue_start
- Call xdp_rxq_info_unreg() before page_pool_destroy() in the
bnxt_queue_mem_free().
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index bb3be33c1bbd..ffa74c26ee53 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4052,6 +4052,7 @@ static void bnxt_reset_rx_ring_struct(struct bnxt *bp,
rxr->page_pool->p.napi = NULL;
rxr->page_pool = NULL;
+ memset(&rxr->xdp_rxq, 0, sizeof(struct xdp_rxq_info));
ring = &rxr->rx_ring_struct;
rmem = &ring->ring_mem;
@@ -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)
@@ -15047,6 +15058,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
bnxt_free_ring(bp, &clone->rx_agg_ring_struct.ring_mem);
err_free_rx_ring:
bnxt_free_ring(bp, &clone->rx_ring_struct.ring_mem);
+err_rxq_info_unreg:
+ xdp_rxq_info_unreg(&clone->xdp_rxq);
+err_page_pool_destroy:
clone->page_pool->p.napi = NULL;
page_pool_destroy(clone->page_pool);
clone->page_pool = NULL;
@@ -15062,6 +15076,8 @@ static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
bnxt_free_one_rx_ring(bp, rxr);
bnxt_free_one_rx_agg_ring(bp, rxr);
+ xdp_rxq_info_unreg(&rxr->xdp_rxq);
+
page_pool_destroy(rxr->page_pool);
rxr->page_pool = NULL;
@@ -15145,6 +15161,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
rxr->rx_sw_agg_prod = clone->rx_sw_agg_prod;
rxr->rx_next_cons = clone->rx_next_cons;
rxr->page_pool = clone->page_pool;
+ rxr->xdp_rxq = clone->xdp_rxq;
bnxt_copy_rx_ring(bp, rxr, clone);
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-21 5:35 [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic Taehee Yoo
@ 2024-07-21 16:57 ` David Wei
2024-07-23 10:59 ` Paolo Abeni
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: David Wei @ 2024-07-21 16:57 UTC (permalink / raw)
To: Taehee Yoo, davem, kuba, pabeni, edumazet, michael.chan, netdev
Cc: somnath.kotur, horms
On 2024-07-20 22: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@gmail.com>
> ---
>
> v2:
> - Do not use memcpy in the bnxt_queue_start
> - Call xdp_rxq_info_unreg() before page_pool_destroy() in the
> bnxt_queue_mem_free().
>
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index bb3be33c1bbd..ffa74c26ee53 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -4052,6 +4052,7 @@ static void bnxt_reset_rx_ring_struct(struct bnxt *bp,
>
> rxr->page_pool->p.napi = NULL;
> rxr->page_pool = NULL;
> + memset(&rxr->xdp_rxq, 0, sizeof(struct xdp_rxq_info));
>
> ring = &rxr->rx_ring_struct;
> rmem = &ring->ring_mem;
> @@ -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)
> @@ -15047,6 +15058,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
> bnxt_free_ring(bp, &clone->rx_agg_ring_struct.ring_mem);
> err_free_rx_ring:
> bnxt_free_ring(bp, &clone->rx_ring_struct.ring_mem);
> +err_rxq_info_unreg:
> + xdp_rxq_info_unreg(&clone->xdp_rxq);
> +err_page_pool_destroy:
> clone->page_pool->p.napi = NULL;
> page_pool_destroy(clone->page_pool);
> clone->page_pool = NULL;
> @@ -15062,6 +15076,8 @@ static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
> bnxt_free_one_rx_ring(bp, rxr);
> bnxt_free_one_rx_agg_ring(bp, rxr);
>
> + xdp_rxq_info_unreg(&rxr->xdp_rxq);
> +
> page_pool_destroy(rxr->page_pool);
> rxr->page_pool = NULL;
>
> @@ -15145,6 +15161,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
> rxr->rx_sw_agg_prod = clone->rx_sw_agg_prod;
> rxr->rx_next_cons = clone->rx_next_cons;
> rxr->page_pool = clone->page_pool;
> + rxr->xdp_rxq = clone->xdp_rxq;
>
> bnxt_copy_rx_ring(bp, rxr, clone);
>
Thanks for addressing my comments. Checkpatch and make W=1 C=1 also
showed no errors.
Reviewed-by: David Wei <dw@davidwei.uk>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-21 5:35 [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic Taehee Yoo
2024-07-21 16:57 ` David Wei
@ 2024-07-23 10:59 ` Paolo Abeni
2024-07-23 17:37 ` Taehee Yoo
2024-07-23 15:42 ` Brett Creeley
` (3 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2024-07-23 10:59 UTC (permalink / raw)
To: Taehee Yoo, michael.chan
Cc: somnath.kotur, kuba, dw, netdev, horms, edumazet, davem
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@gmail.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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-21 5:35 [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic Taehee Yoo
2024-07-21 16:57 ` David Wei
2024-07-23 10:59 ` Paolo Abeni
@ 2024-07-23 15:42 ` Brett Creeley
2024-07-23 17:58 ` Taehee Yoo
2024-07-25 5:00 ` Taehee Yoo
` (2 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Brett Creeley @ 2024-07-23 15:42 UTC (permalink / raw)
To: Taehee Yoo, davem, kuba, pabeni, edumazet, michael.chan, netdev
Cc: somnath.kotur, dw, horms
On 7/20/2024 10:35 PM, Taehee Yoo wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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@gmail.com>
> ---
>
> v2:
> - Do not use memcpy in the bnxt_queue_start
> - Call xdp_rxq_info_unreg() before page_pool_destroy() in the
> bnxt_queue_mem_free().
>
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index bb3be33c1bbd..ffa74c26ee53 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -4052,6 +4052,7 @@ static void bnxt_reset_rx_ring_struct(struct bnxt *bp,
>
> rxr->page_pool->p.napi = NULL;
> rxr->page_pool = NULL;
> + memset(&rxr->xdp_rxq, 0, sizeof(struct xdp_rxq_info));
>
> ring = &rxr->rx_ring_struct;
> rmem = &ring->ring_mem;
> @@ -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)
> @@ -15047,6 +15058,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
> bnxt_free_ring(bp, &clone->rx_agg_ring_struct.ring_mem);
> err_free_rx_ring:
> bnxt_free_ring(bp, &clone->rx_ring_struct.ring_mem);
> +err_rxq_info_unreg:
> + xdp_rxq_info_unreg(&clone->xdp_rxq);
I think care needs to be taken calling xdp_rxq_info_unreg() here and
then page_pool_destroy() below due to the xdp_rxq_info_unreg() call flow
eventually calling page_pool_destroy(). Similar comment below.
> +err_page_pool_destroy:
> clone->page_pool->p.napi = NULL;
> page_pool_destroy(clone->page_pool);
> clone->page_pool = NULL;
> @@ -15062,6 +15076,8 @@ static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
> bnxt_free_one_rx_ring(bp, rxr);
> bnxt_free_one_rx_agg_ring(bp, rxr);
>
> + xdp_rxq_info_unreg(&rxr->xdp_rxq);
> +
If the memory type is MEM_TYPE_PAGE_POOL, xdp_rxq_info_unreg() will
eventually call page_pool_destroy(). Unless I am missing something I
think you want to remove the call below to page_pool_destroy()?
Thanks,
Brett
> page_pool_destroy(rxr->page_pool);
> rxr->page_pool = NULL;
>
> @@ -15145,6 +15161,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
> rxr->rx_sw_agg_prod = clone->rx_sw_agg_prod;
> rxr->rx_next_cons = clone->rx_next_cons;
> rxr->page_pool = clone->page_pool;
> + rxr->xdp_rxq = clone->xdp_rxq;
>
> bnxt_copy_rx_ring(bp, rxr, clone);
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-23 10:59 ` Paolo Abeni
@ 2024-07-23 17:37 ` Taehee Yoo
0 siblings, 0 replies; 10+ messages in thread
From: Taehee Yoo @ 2024-07-23 17:37 UTC (permalink / raw)
To: Paolo Abeni
Cc: michael.chan, somnath.kotur, kuba, dw, netdev, horms, edumazet,
davem
On Tue, Jul 23, 2024 at 7:59 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
Hi Paolo,
Thanks a lot for the review!
> 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@gmail.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.
>
Yes, I agree with you.
I will try to refactor it after the merge window.
Thanks a lot!
Taehee Yoo
> Thanks,
>
> Paolo
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-23 15:42 ` Brett Creeley
@ 2024-07-23 17:58 ` Taehee Yoo
2024-07-23 19:47 ` Brett Creeley
0 siblings, 1 reply; 10+ messages in thread
From: Taehee Yoo @ 2024-07-23 17:58 UTC (permalink / raw)
To: Brett Creeley
Cc: davem, kuba, pabeni, edumazet, michael.chan, netdev,
somnath.kotur, dw, horms
On Wed, Jul 24, 2024 at 12:42 AM Brett Creeley <bcreeley@amd.com> wrote:
>
Hi Brett,
Thanks a lot for the review!
>
>
> On 7/20/2024 10:35 PM, Taehee Yoo wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> >
> >
> > 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@gmail.com>
> > ---
> >
> > v2:
> > - Do not use memcpy in the bnxt_queue_start
> > - Call xdp_rxq_info_unreg() before page_pool_destroy() in the
> > bnxt_queue_mem_free().
> >
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index bb3be33c1bbd..ffa74c26ee53 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -4052,6 +4052,7 @@ static void bnxt_reset_rx_ring_struct(struct bnxt *bp,
> >
> > rxr->page_pool->p.napi = NULL;
> > rxr->page_pool = NULL;
> > + memset(&rxr->xdp_rxq, 0, sizeof(struct xdp_rxq_info));
> >
> > ring = &rxr->rx_ring_struct;
> > rmem = &ring->ring_mem;
> > @@ -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)
> > @@ -15047,6 +15058,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
> > bnxt_free_ring(bp, &clone->rx_agg_ring_struct.ring_mem);
> > err_free_rx_ring:
> > bnxt_free_ring(bp, &clone->rx_ring_struct.ring_mem);
> > +err_rxq_info_unreg:
> > + xdp_rxq_info_unreg(&clone->xdp_rxq);
>
> I think care needs to be taken calling xdp_rxq_info_unreg() here and
> then page_pool_destroy() below due to the xdp_rxq_info_unreg() call flow
> eventually calling page_pool_destroy(). Similar comment below.
>
> > +err_page_pool_destroy:
> > clone->page_pool->p.napi = NULL;
> > page_pool_destroy(clone->page_pool);
> > clone->page_pool = NULL;
> > @@ -15062,6 +15076,8 @@ static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
> > bnxt_free_one_rx_ring(bp, rxr);
> > bnxt_free_one_rx_agg_ring(bp, rxr);
> >
> > + xdp_rxq_info_unreg(&rxr->xdp_rxq);
> > +
>
> If the memory type is MEM_TYPE_PAGE_POOL, xdp_rxq_info_unreg() will
> eventually call page_pool_destroy(). Unless I am missing something I
> think you want to remove the call below to page_pool_destroy()?
>
I think both page_pool_destroy() and xdp_rxq_info_unreg() are needed here.
Because the page_pools are managed by reference count.
When a page_pool is created by page_pool_create(), its count is 1 and it
will be destroyed if the count reaches 0. The page_pool_destroy()
decreases a reference count so that page_pool will be destroyed.
The xdp_rxq_info_reg() also holds a reference count for page_pool if
the memory type is page_pool.
As you mentioned xdp_rxq_info_unreg() internally calls page_pool_destroy()
if the memory type is page pool.
So, to destroy page_pool if xdp_rxq_info was registered,
both xdp_rxq_info_unreg() and page_pool_destroy() should be called.
Thanks a lot!
Tahee Yoo
> Thanks,
>
> Brett
>
> > page_pool_destroy(rxr->page_pool);
> > rxr->page_pool = NULL;
> >
> > @@ -15145,6 +15161,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
> > rxr->rx_sw_agg_prod = clone->rx_sw_agg_prod;
> > rxr->rx_next_cons = clone->rx_next_cons;
> > rxr->page_pool = clone->page_pool;
> > + rxr->xdp_rxq = clone->xdp_rxq;
> >
> > bnxt_copy_rx_ring(bp, rxr, clone);
> >
> > --
> > 2.34.1
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-23 17:58 ` Taehee Yoo
@ 2024-07-23 19:47 ` Brett Creeley
0 siblings, 0 replies; 10+ messages in thread
From: Brett Creeley @ 2024-07-23 19:47 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, kuba, pabeni, edumazet, michael.chan, netdev,
somnath.kotur, dw, horms
On 7/23/2024 10:58 AM, Taehee Yoo wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, Jul 24, 2024 at 12:42 AM Brett Creeley <bcreeley@amd.com> wrote:
>>
>
> Hi Brett,
> Thanks a lot for the review!
>
>>
>>
>> On 7/20/2024 10:35 PM, Taehee Yoo wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> 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@gmail.com>
>>> ---
>>>
>>> v2:
>>> - Do not use memcpy in the bnxt_queue_start
>>> - Call xdp_rxq_info_unreg() before page_pool_destroy() in the
>>> bnxt_queue_mem_free().
>>>
>>> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> index bb3be33c1bbd..ffa74c26ee53 100644
>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> @@ -4052,6 +4052,7 @@ static void bnxt_reset_rx_ring_struct(struct bnxt *bp,
>>>
>>> rxr->page_pool->p.napi = NULL;
>>> rxr->page_pool = NULL;
>>> + memset(&rxr->xdp_rxq, 0, sizeof(struct xdp_rxq_info));
>>>
>>> ring = &rxr->rx_ring_struct;
>>> rmem = &ring->ring_mem;
>>> @@ -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)
>>> @@ -15047,6 +15058,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
>>> bnxt_free_ring(bp, &clone->rx_agg_ring_struct.ring_mem);
>>> err_free_rx_ring:
>>> bnxt_free_ring(bp, &clone->rx_ring_struct.ring_mem);
>>> +err_rxq_info_unreg:
>>> + xdp_rxq_info_unreg(&clone->xdp_rxq);
>>
>> I think care needs to be taken calling xdp_rxq_info_unreg() here and
>> then page_pool_destroy() below due to the xdp_rxq_info_unreg() call flow
>> eventually calling page_pool_destroy(). Similar comment below.
>>
>>> +err_page_pool_destroy:
>>> clone->page_pool->p.napi = NULL;
>>> page_pool_destroy(clone->page_pool);
>>> clone->page_pool = NULL;
>>> @@ -15062,6 +15076,8 @@ static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
>>> bnxt_free_one_rx_ring(bp, rxr);
>>> bnxt_free_one_rx_agg_ring(bp, rxr);
>>>
>>> + xdp_rxq_info_unreg(&rxr->xdp_rxq);
>>> +
>>
>> If the memory type is MEM_TYPE_PAGE_POOL, xdp_rxq_info_unreg() will
>> eventually call page_pool_destroy(). Unless I am missing something I
>> think you want to remove the call below to page_pool_destroy()?
>>
>
> I think both page_pool_destroy() and xdp_rxq_info_unreg() are needed here.
> Because the page_pools are managed by reference count.
>
> When a page_pool is created by page_pool_create(), its count is 1 and it
> will be destroyed if the count reaches 0. The page_pool_destroy()
> decreases a reference count so that page_pool will be destroyed.
> The xdp_rxq_info_reg() also holds a reference count for page_pool if
> the memory type is page_pool.
>
> As you mentioned xdp_rxq_info_unreg() internally calls page_pool_destroy()
> if the memory type is page pool.
> So, to destroy page_pool if xdp_rxq_info was registered,
> both xdp_rxq_info_unreg() and page_pool_destroy() should be called.
>
> Thanks a lot!
> Tahee Yoo
Ahh, yeah thanks for pointing that out. I missed the call to
page_pool_use_xdp_mem() in __xdp_reg_mem_model(). It also makes much
more sense this way as it makes the enable/disable flow symmetrical. Thanks!
Brett
>
>> Thanks,
>>
>> Brett
>>
>>> page_pool_destroy(rxr->page_pool);
>>> rxr->page_pool = NULL;
>>>
>>> @@ -15145,6 +15161,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
>>> rxr->rx_sw_agg_prod = clone->rx_sw_agg_prod;
>>> rxr->rx_next_cons = clone->rx_next_cons;
>>> rxr->page_pool = clone->page_pool;
>>> + rxr->xdp_rxq = clone->xdp_rxq;
>>>
>>> bnxt_copy_rx_ring(bp, rxr, clone);
>>>
>>> --
>>> 2.34.1
>>>
>>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-21 5:35 [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic Taehee Yoo
` (2 preceding siblings ...)
2024-07-23 15:42 ` Brett Creeley
@ 2024-07-25 5:00 ` Taehee Yoo
2024-07-25 5:45 ` Somnath Kotur
2024-07-25 14:50 ` patchwork-bot+netdevbpf
5 siblings, 0 replies; 10+ messages in thread
From: Taehee Yoo @ 2024-07-25 5:00 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, michael.chan, netdev
Cc: somnath.kotur, dw, horms
On Sun, Jul 21, 2024 at 2:36 PM Taehee Yoo <ap420073@gmail.com> 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@gmail.com>
> ---
>
> v2:
> - Do not use memcpy in the bnxt_queue_start
> - Call xdp_rxq_info_unreg() before page_pool_destroy() in the
> bnxt_queue_mem_free().
>
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index bb3be33c1bbd..ffa74c26ee53 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -4052,6 +4052,7 @@ static void bnxt_reset_rx_ring_struct(struct bnxt *bp,
>
> rxr->page_pool->p.napi = NULL;
> rxr->page_pool = NULL;
> + memset(&rxr->xdp_rxq, 0, sizeof(struct xdp_rxq_info));
>
> ring = &rxr->rx_ring_struct;
> rmem = &ring->ring_mem;
> @@ -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)
> @@ -15047,6 +15058,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
> bnxt_free_ring(bp, &clone->rx_agg_ring_struct.ring_mem);
> err_free_rx_ring:
> bnxt_free_ring(bp, &clone->rx_ring_struct.ring_mem);
> +err_rxq_info_unreg:
> + xdp_rxq_info_unreg(&clone->xdp_rxq);
> +err_page_pool_destroy:
> clone->page_pool->p.napi = NULL;
> page_pool_destroy(clone->page_pool);
> clone->page_pool = NULL;
> @@ -15062,6 +15076,8 @@ static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
> bnxt_free_one_rx_ring(bp, rxr);
> bnxt_free_one_rx_agg_ring(bp, rxr);
>
> + xdp_rxq_info_unreg(&rxr->xdp_rxq);
> +
> page_pool_destroy(rxr->page_pool);
> rxr->page_pool = NULL;
>
> @@ -15145,6 +15161,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
> rxr->rx_sw_agg_prod = clone->rx_sw_agg_prod;
> rxr->rx_next_cons = clone->rx_next_cons;
> rxr->page_pool = clone->page_pool;
> + rxr->xdp_rxq = clone->xdp_rxq;
>
> bnxt_copy_rx_ring(bp, rxr, clone);
>
> --
> 2.34.1
>
Hi Netdev maintainers,
I found the state of this patch is "change requested" from the patchwork.
I think there's no need for additional changes.
Could you please let me know what I'm missing?
Thanks a lot!
Taehee Yoo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-21 5:35 [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic Taehee Yoo
` (3 preceding siblings ...)
2024-07-25 5:00 ` Taehee Yoo
@ 2024-07-25 5:45 ` Somnath Kotur
2024-07-25 14:50 ` patchwork-bot+netdevbpf
5 siblings, 0 replies; 10+ messages in thread
From: Somnath Kotur @ 2024-07-25 5:45 UTC (permalink / raw)
To: Taehee Yoo; +Cc: davem, kuba, pabeni, edumazet, michael.chan, netdev, dw, horms
[-- Attachment #1: Type: text/plain, Size: 4355 bytes --]
On Sun, Jul 21, 2024 at 11:06 AM Taehee Yoo <ap420073@gmail.com> 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@gmail.com>
> ---
>
> v2:
> - Do not use memcpy in the bnxt_queue_start
> - Call xdp_rxq_info_unreg() before page_pool_destroy() in the
> bnxt_queue_mem_free().
>
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index bb3be33c1bbd..ffa74c26ee53 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -4052,6 +4052,7 @@ static void bnxt_reset_rx_ring_struct(struct bnxt *bp,
>
> rxr->page_pool->p.napi = NULL;
> rxr->page_pool = NULL;
> + memset(&rxr->xdp_rxq, 0, sizeof(struct xdp_rxq_info));
>
> ring = &rxr->rx_ring_struct;
> rmem = &ring->ring_mem;
> @@ -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)
> @@ -15047,6 +15058,9 @@ static int bnxt_queue_mem_alloc(struct net_device *dev, void *qmem, int idx)
> bnxt_free_ring(bp, &clone->rx_agg_ring_struct.ring_mem);
> err_free_rx_ring:
> bnxt_free_ring(bp, &clone->rx_ring_struct.ring_mem);
> +err_rxq_info_unreg:
> + xdp_rxq_info_unreg(&clone->xdp_rxq);
> +err_page_pool_destroy:
> clone->page_pool->p.napi = NULL;
> page_pool_destroy(clone->page_pool);
> clone->page_pool = NULL;
> @@ -15062,6 +15076,8 @@ static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
> bnxt_free_one_rx_ring(bp, rxr);
> bnxt_free_one_rx_agg_ring(bp, rxr);
>
> + xdp_rxq_info_unreg(&rxr->xdp_rxq);
> +
> page_pool_destroy(rxr->page_pool);
> rxr->page_pool = NULL;
>
> @@ -15145,6 +15161,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
> rxr->rx_sw_agg_prod = clone->rx_sw_agg_prod;
> rxr->rx_next_cons = clone->rx_next_cons;
> rxr->page_pool = clone->page_pool;
> + rxr->xdp_rxq = clone->xdp_rxq;
>
> bnxt_copy_rx_ring(bp, rxr, clone);
>
> --
> 2.34.1
>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic
2024-07-21 5:35 [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic Taehee Yoo
` (4 preceding siblings ...)
2024-07-25 5:45 ` Somnath Kotur
@ 2024-07-25 14:50 ` patchwork-bot+netdevbpf
5 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-07-25 14:50 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, kuba, pabeni, edumazet, michael.chan, netdev,
somnath.kotur, dw, horms
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Sun, 21 Jul 2024 05:35:54 +0000 you 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.
>
> [...]
Here is the summary with links:
- [net,v2] bnxt_en: update xdp_rxq_info in queue restart logic
https://git.kernel.org/netdev/net/c/b537633ce57b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-07-25 14:50 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-21 5:35 [PATCH net v2] bnxt_en: update xdp_rxq_info in queue restart logic Taehee Yoo
2024-07-21 16:57 ` David Wei
2024-07-23 10:59 ` Paolo Abeni
2024-07-23 17:37 ` Taehee Yoo
2024-07-23 15:42 ` Brett Creeley
2024-07-23 17:58 ` Taehee Yoo
2024-07-23 19:47 ` Brett Creeley
2024-07-25 5:00 ` Taehee Yoo
2024-07-25 5:45 ` Somnath Kotur
2024-07-25 14:50 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).