* [PATCH iwl-net 0/2] i40e: disable XDP Tx queues on ifdown @ 2024-02-01 15:42 Maciej Fijalkowski 2024-02-01 15:42 ` [PATCH iwl-net 1/2] i40e: avoid double calling i40e_pf_rxq_wait() Maciej Fijalkowski 2024-02-01 15:42 ` [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings Maciej Fijalkowski 0 siblings, 2 replies; 7+ messages in thread From: Maciej Fijalkowski @ 2024-02-01 15:42 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, anthony.l.nguyen, magnus.karlsson, Maciej Fijalkowski Seth reported in [0] that he couldn't get traffic flowing again after a round of down/up of interface that i40e driver manages. While looking into fixing Tx disable timeout issue I also noticed that there is a doubled function call on Rx side which is fixed in patch 1. Thanks, Maciej [0]: https://lore.kernel.org/netdev/ZbkE7Ep1N1Ou17sA@do-x1extreme/ Maciej Fijalkowski (2): i40e: avoid double calling i40e_pf_rxq_wait() i40e: take into account XDP Tx queues when stopping rings drivers/net/ethernet/intel/i40e/i40e_main.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH iwl-net 1/2] i40e: avoid double calling i40e_pf_rxq_wait() 2024-02-01 15:42 [PATCH iwl-net 0/2] i40e: disable XDP Tx queues on ifdown Maciej Fijalkowski @ 2024-02-01 15:42 ` Maciej Fijalkowski 2024-02-05 11:13 ` Simon Horman 2024-02-01 15:42 ` [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings Maciej Fijalkowski 1 sibling, 1 reply; 7+ messages in thread From: Maciej Fijalkowski @ 2024-02-01 15:42 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, anthony.l.nguyen, magnus.karlsson, Maciej Fijalkowski Currently, when interface is being brought down and i40e_vsi_stop_rings() is called, i40e_pf_rxq_wait() is called two times, which is wrong. To showcase this scenario, simplified call stack looks as follows: i40e_vsi_stop_rings() i40e_control wait rx_q() i40e_control_rx_q() i40e_pf_rxq_wait() i40e_vsi_wait_queues_disabled() i40e_pf_rxq_wait() // redundant call To fix this, let us s/i40e_control_wait_rx_q/i40e_control_rx_q within i40e_vsi_stop_rings(). Fixes: 65662a8dcdd0 ("i40e: Fix logic of disabling queues") Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- drivers/net/ethernet/intel/i40e/i40e_main.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 6e7fd473abfd..2c46a5e7d222 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -4926,7 +4926,7 @@ int i40e_vsi_start_rings(struct i40e_vsi *vsi) void i40e_vsi_stop_rings(struct i40e_vsi *vsi) { struct i40e_pf *pf = vsi->back; - int pf_q, err, q_end; + int pf_q, q_end; /* When port TX is suspended, don't wait */ if (test_bit(__I40E_PORT_SUSPENDED, vsi->back->state)) @@ -4936,16 +4936,10 @@ void i40e_vsi_stop_rings(struct i40e_vsi *vsi) for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) i40e_pre_tx_queue_cfg(&pf->hw, (u32)pf_q, false); - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) { - err = i40e_control_wait_rx_q(pf, pf_q, false); - if (err) - dev_info(&pf->pdev->dev, - "VSI seid %d Rx ring %d disable timeout\n", - vsi->seid, pf_q); - } + for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) + i40e_control_rx_q(pf, pf_q, false); msleep(I40E_DISABLE_TX_GAP_MSEC); - pf_q = vsi->base_queue; for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) wr32(&pf->hw, I40E_QTX_ENA(pf_q), 0); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH iwl-net 1/2] i40e: avoid double calling i40e_pf_rxq_wait() 2024-02-01 15:42 ` [PATCH iwl-net 1/2] i40e: avoid double calling i40e_pf_rxq_wait() Maciej Fijalkowski @ 2024-02-05 11:13 ` Simon Horman 0 siblings, 0 replies; 7+ messages in thread From: Simon Horman @ 2024-02-05 11:13 UTC (permalink / raw) To: Maciej Fijalkowski Cc: intel-wired-lan, netdev, anthony.l.nguyen, magnus.karlsson On Thu, Feb 01, 2024 at 04:42:18PM +0100, Maciej Fijalkowski wrote: > Currently, when interface is being brought down and > i40e_vsi_stop_rings() is called, i40e_pf_rxq_wait() is called two times, > which is wrong. To showcase this scenario, simplified call stack looks > as follows: > > i40e_vsi_stop_rings() > i40e_control wait rx_q() > i40e_control_rx_q() > i40e_pf_rxq_wait() > i40e_vsi_wait_queues_disabled() > i40e_pf_rxq_wait() // redundant call > > To fix this, let us s/i40e_control_wait_rx_q/i40e_control_rx_q within > i40e_vsi_stop_rings(). > > Fixes: 65662a8dcdd0 ("i40e: Fix logic of disabling queues") > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Reviewed-by: Simon Horman <horms@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings 2024-02-01 15:42 [PATCH iwl-net 0/2] i40e: disable XDP Tx queues on ifdown Maciej Fijalkowski 2024-02-01 15:42 ` [PATCH iwl-net 1/2] i40e: avoid double calling i40e_pf_rxq_wait() Maciej Fijalkowski @ 2024-02-01 15:42 ` Maciej Fijalkowski 2024-02-01 18:40 ` Seth Forshee 2024-02-05 11:13 ` Simon Horman 1 sibling, 2 replies; 7+ messages in thread From: Maciej Fijalkowski @ 2024-02-01 15:42 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, anthony.l.nguyen, magnus.karlsson, Maciej Fijalkowski, Seth Forshee Seth reported that on his side XDP traffic can not survive a round of down/up against i40e interface. Dmesg output was telling us that we were not able to disable the very first XDP ring. That was due to the fact that in i40e_vsi_stop_rings() in a pre-work that is done before calling i40e_vsi_wait_queues_disabled(), XDP Tx queues were not taken into the account. To fix this, let us distinguish between Rx and Tx queue boundaries and take into the account XDP queues for Tx side. Reported-by: Seth Forshee <sforshee@kernel.org> Closes: https://lore.kernel.org/netdev/ZbkE7Ep1N1Ou17sA@do-x1extreme/ Fixes: 65662a8dcdd0 ("i40e: Fix logic of disabling queues") Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- drivers/net/ethernet/intel/i40e/i40e_main.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 2c46a5e7d222..907be56965f5 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -4926,21 +4926,22 @@ int i40e_vsi_start_rings(struct i40e_vsi *vsi) void i40e_vsi_stop_rings(struct i40e_vsi *vsi) { struct i40e_pf *pf = vsi->back; - int pf_q, q_end; + u32 pf_q, tx_q_end, rx_q_end; /* When port TX is suspended, don't wait */ if (test_bit(__I40E_PORT_SUSPENDED, vsi->back->state)) return i40e_vsi_stop_rings_no_wait(vsi); - q_end = vsi->base_queue + vsi->num_queue_pairs; - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) - i40e_pre_tx_queue_cfg(&pf->hw, (u32)pf_q, false); + tx_q_end = vsi->alloc_queue_pairs * (i40e_enabled_xdp_vsi(vsi) ? 2 : 1); + for (pf_q = vsi->base_queue; pf_q < tx_q_end; pf_q++) + i40e_pre_tx_queue_cfg(&pf->hw, pf_q, false); - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) + rx_q_end = vsi->base_queue + vsi->num_queue_pairs; + for (pf_q = vsi->base_queue; pf_q < rx_q_end; pf_q++) i40e_control_rx_q(pf, pf_q, false); msleep(I40E_DISABLE_TX_GAP_MSEC); - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) + for (pf_q = vsi->base_queue; pf_q < tx_q_end; pf_q++) wr32(&pf->hw, I40E_QTX_ENA(pf_q), 0); i40e_vsi_wait_queues_disabled(vsi); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings 2024-02-01 15:42 ` [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings Maciej Fijalkowski @ 2024-02-01 18:40 ` Seth Forshee 2024-02-06 12:08 ` Maciej Fijalkowski 2024-02-05 11:13 ` Simon Horman 1 sibling, 1 reply; 7+ messages in thread From: Seth Forshee @ 2024-02-01 18:40 UTC (permalink / raw) To: Maciej Fijalkowski Cc: intel-wired-lan, netdev, anthony.l.nguyen, magnus.karlsson On Thu, Feb 01, 2024 at 04:42:19PM +0100, Maciej Fijalkowski wrote: > Seth reported that on his side XDP traffic can not survive a round of > down/up against i40e interface. Dmesg output was telling us that we were > not able to disable the very first XDP ring. That was due to the fact > that in i40e_vsi_stop_rings() in a pre-work that is done before calling > i40e_vsi_wait_queues_disabled(), XDP Tx queues were not taken into the > account. > > To fix this, let us distinguish between Rx and Tx queue boundaries and > take into the account XDP queues for Tx side. > > Reported-by: Seth Forshee <sforshee@kernel.org> > Closes: https://lore.kernel.org/netdev/ZbkE7Ep1N1Ou17sA@do-x1extreme/ > Fixes: 65662a8dcdd0 ("i40e: Fix logic of disabling queues") > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> This fixes the issue we're seeing. Thanks! Tested-by: Seth Forshee <sforshee@kernel.org> > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index 2c46a5e7d222..907be56965f5 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -4926,21 +4926,22 @@ int i40e_vsi_start_rings(struct i40e_vsi *vsi) > void i40e_vsi_stop_rings(struct i40e_vsi *vsi) > { > struct i40e_pf *pf = vsi->back; > - int pf_q, q_end; > + u32 pf_q, tx_q_end, rx_q_end; > > /* When port TX is suspended, don't wait */ > if (test_bit(__I40E_PORT_SUSPENDED, vsi->back->state)) > return i40e_vsi_stop_rings_no_wait(vsi); > > - q_end = vsi->base_queue + vsi->num_queue_pairs; > - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) > - i40e_pre_tx_queue_cfg(&pf->hw, (u32)pf_q, false); > + tx_q_end = vsi->alloc_queue_pairs * (i40e_enabled_xdp_vsi(vsi) ? 2 : 1); > + for (pf_q = vsi->base_queue; pf_q < tx_q_end; pf_q++) > + i40e_pre_tx_queue_cfg(&pf->hw, pf_q, false); > > - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) > + rx_q_end = vsi->base_queue + vsi->num_queue_pairs; > + for (pf_q = vsi->base_queue; pf_q < rx_q_end; pf_q++) > i40e_control_rx_q(pf, pf_q, false); > > msleep(I40E_DISABLE_TX_GAP_MSEC); > - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) > + for (pf_q = vsi->base_queue; pf_q < tx_q_end; pf_q++) > wr32(&pf->hw, I40E_QTX_ENA(pf_q), 0); > > i40e_vsi_wait_queues_disabled(vsi); > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings 2024-02-01 18:40 ` Seth Forshee @ 2024-02-06 12:08 ` Maciej Fijalkowski 0 siblings, 0 replies; 7+ messages in thread From: Maciej Fijalkowski @ 2024-02-06 12:08 UTC (permalink / raw) To: Seth Forshee; +Cc: intel-wired-lan, netdev, anthony.l.nguyen, magnus.karlsson On Thu, Feb 01, 2024 at 12:40:22PM -0600, Seth Forshee wrote: > On Thu, Feb 01, 2024 at 04:42:19PM +0100, Maciej Fijalkowski wrote: > > Seth reported that on his side XDP traffic can not survive a round of > > down/up against i40e interface. Dmesg output was telling us that we were > > not able to disable the very first XDP ring. That was due to the fact > > that in i40e_vsi_stop_rings() in a pre-work that is done before calling > > i40e_vsi_wait_queues_disabled(), XDP Tx queues were not taken into the > > account. > > > > To fix this, let us distinguish between Rx and Tx queue boundaries and > > take into the account XDP queues for Tx side. > > > > Reported-by: Seth Forshee <sforshee@kernel.org> > > Closes: https://lore.kernel.org/netdev/ZbkE7Ep1N1Ou17sA@do-x1extreme/ > > Fixes: 65662a8dcdd0 ("i40e: Fix logic of disabling queues") > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > This fixes the issue we're seeing. Thanks! > > Tested-by: Seth Forshee <sforshee@kernel.org> > > > --- > > drivers/net/ethernet/intel/i40e/i40e_main.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > > index 2c46a5e7d222..907be56965f5 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > > @@ -4926,21 +4926,22 @@ int i40e_vsi_start_rings(struct i40e_vsi *vsi) > > void i40e_vsi_stop_rings(struct i40e_vsi *vsi) > > { > > struct i40e_pf *pf = vsi->back; > > - int pf_q, q_end; > > + u32 pf_q, tx_q_end, rx_q_end; > > > > /* When port TX is suspended, don't wait */ > > if (test_bit(__I40E_PORT_SUSPENDED, vsi->back->state)) > > return i40e_vsi_stop_rings_no_wait(vsi); > > > > - q_end = vsi->base_queue + vsi->num_queue_pairs; > > - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) > > - i40e_pre_tx_queue_cfg(&pf->hw, (u32)pf_q, false); > > + tx_q_end = vsi->alloc_queue_pairs * (i40e_enabled_xdp_vsi(vsi) ? 2 : 1); While this fixes one thing, it breaks another ;) vsi->base_queue needs to be involved into tx_q_end, otherwise anything besides PF0 would not go through Tx loops. I noticed that FDIR VSI started to get Tx disable timeouts with this patch. Will send a v2. > > + for (pf_q = vsi->base_queue; pf_q < tx_q_end; pf_q++) > > + i40e_pre_tx_queue_cfg(&pf->hw, pf_q, false); > > > > - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) > > + rx_q_end = vsi->base_queue + vsi->num_queue_pairs; > > + for (pf_q = vsi->base_queue; pf_q < rx_q_end; pf_q++) > > i40e_control_rx_q(pf, pf_q, false); > > > > msleep(I40E_DISABLE_TX_GAP_MSEC); > > - for (pf_q = vsi->base_queue; pf_q < q_end; pf_q++) > > + for (pf_q = vsi->base_queue; pf_q < tx_q_end; pf_q++) > > wr32(&pf->hw, I40E_QTX_ENA(pf_q), 0); > > > > i40e_vsi_wait_queues_disabled(vsi); > > -- > > 2.34.1 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings 2024-02-01 15:42 ` [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings Maciej Fijalkowski 2024-02-01 18:40 ` Seth Forshee @ 2024-02-05 11:13 ` Simon Horman 1 sibling, 0 replies; 7+ messages in thread From: Simon Horman @ 2024-02-05 11:13 UTC (permalink / raw) To: Maciej Fijalkowski Cc: intel-wired-lan, netdev, anthony.l.nguyen, magnus.karlsson, Seth Forshee On Thu, Feb 01, 2024 at 04:42:19PM +0100, Maciej Fijalkowski wrote: > Seth reported that on his side XDP traffic can not survive a round of > down/up against i40e interface. Dmesg output was telling us that we were > not able to disable the very first XDP ring. That was due to the fact > that in i40e_vsi_stop_rings() in a pre-work that is done before calling > i40e_vsi_wait_queues_disabled(), XDP Tx queues were not taken into the > account. > > To fix this, let us distinguish between Rx and Tx queue boundaries and > take into the account XDP queues for Tx side. > > Reported-by: Seth Forshee <sforshee@kernel.org> > Closes: https://lore.kernel.org/netdev/ZbkE7Ep1N1Ou17sA@do-x1extreme/ > Fixes: 65662a8dcdd0 ("i40e: Fix logic of disabling queues") > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Reviewed-by: Simon Horman <horms@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-02-06 12:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-01 15:42 [PATCH iwl-net 0/2] i40e: disable XDP Tx queues on ifdown Maciej Fijalkowski 2024-02-01 15:42 ` [PATCH iwl-net 1/2] i40e: avoid double calling i40e_pf_rxq_wait() Maciej Fijalkowski 2024-02-05 11:13 ` Simon Horman 2024-02-01 15:42 ` [PATCH iwl-net 2/2] i40e: take into account XDP Tx queues when stopping rings Maciej Fijalkowski 2024-02-01 18:40 ` Seth Forshee 2024-02-06 12:08 ` Maciej Fijalkowski 2024-02-05 11:13 ` Simon Horman
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).