| Message ID | 20240721053554.1233549-1-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b537633ce57b29f4c9687fea6f2d92e1ca7853fc |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] bnxt_en: update xdp_rxq_info in queue restart logic | expand |
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>
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
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 > >
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 >
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 > > > >
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 >>> >>>
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
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>
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!
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);
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(+)