[PATCH net] ice: don't stop netdev tx queues when setting up XSK socket

[PATCH net] ice: don't stop netdev tx queues when setting up XSK socket

[Tony Nguyen]

From: Kamil Maziarz <kamil.maziarz@intel.com>

Avoid stopping netdev tx queues during XSK setup by removing
netif_tx_stop_queue() and netif_tx_start_queue().
These changes prevent unnecessary stopping and starting of netdev
transmit queues during the setup of XDP socket. Without this change,
after stopping the XDP traffic flow tracker and then stopping
the XDP prog - NETDEV WATCHDOG transmit queue timed out appears.

Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 2a3f0834e139..cec492b827d4 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -179,7 +179,6 @@ static int ice_qp_dis(struct ice_vsi *vsi, u16 q_idx)
 			return -EBUSY;
 		usleep_range(1000, 2000);
 	}
-	netif_tx_stop_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
 
 	ice_fill_txq_meta(vsi, tx_ring, &txq_meta);
 	err = ice_vsi_stop_tx_ring(vsi, ICE_NO_RESET, 0, tx_ring, &txq_meta);
@@ -268,7 +267,6 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
 	ice_qvec_toggle_napi(vsi, q_vector, true);
 	ice_qvec_ena_irq(vsi, q_vector);
 
-	netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
 free_buf:
 	kfree(qg_buf);
 	return err;
-- 
2.38.1

Re: [PATCH net] ice: don't stop netdev tx queues when setting up XSK socket

[Jacob Keller]

On 9/25/2023 10:19 AM, Tony Nguyen wrote:
> From: Kamil Maziarz <kamil.maziarz@intel.com>
> 
> Avoid stopping netdev tx queues during XSK setup by removing
> netif_tx_stop_queue() and netif_tx_start_queue().
> These changes prevent unnecessary stopping and starting of netdev
> transmit queues during the setup of XDP socket. Without this change,
> after stopping the XDP traffic flow tracker and then stopping
> the XDP prog - NETDEV WATCHDOG transmit queue timed out appears.
> 
> Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  drivers/net/ethernet/intel/ice/ice_xsk.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 2a3f0834e139..cec492b827d4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -179,7 +179,6 @@ static int ice_qp_dis(struct ice_vsi *vsi, u16 q_idx)
>  			return -EBUSY;
>  		usleep_range(1000, 2000);
>  	}
> -	netif_tx_stop_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
>  

These were introduced by the original implementation in commit
2d4238f55697 ("ice: Add support for AF_XDP"), without explanation.

Looking at some of the other implementations I don't see calls to
netif_tx_stop_queue or netif_tx_start_queue, so at least its not common.
In fact the only caller in an _xsk.c file appears to be ice.

Thanks,
Jake

>  	ice_fill_txq_meta(vsi, tx_ring, &txq_meta);
>  	err = ice_vsi_stop_tx_ring(vsi, ICE_NO_RESET, 0, tx_ring, &txq_meta);
> @@ -268,7 +267,6 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
>  	ice_qvec_toggle_napi(vsi, q_vector, true);
>  	ice_qvec_ena_irq(vsi, q_vector);
>  
> -	netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
>  free_buf:
>  	kfree(qg_buf);
>  	return err;

Re: [PATCH net] ice: don't stop netdev tx queues when setting up XSK socket

[Jakub Kicinski]

On Mon, 25 Sep 2023 10:19:57 -0700 Tony Nguyen wrote:
> Avoid stopping netdev tx queues during XSK setup by removing
> netif_tx_stop_queue() and netif_tx_start_queue().
> These changes prevent unnecessary stopping and starting of netdev
> transmit queues during the setup of XDP socket. Without this change,
> after stopping the XDP traffic flow tracker and then stopping
> the XDP prog - NETDEV WATCHDOG transmit queue timed out appears.

I think we need more info about what happens here.

Maybe ice_qp_ena() fails before it gets to the start?
If we don't understand what happens, exactly, we may be papering
over other bugs.
-- 
pw-bot: cr

Re: [PATCH net] ice: don't stop netdev tx queues when setting up XSK socket

[Maciej Fijalkowski]

On Tue, Oct 03, 2023 at 03:49:20PM -0700, Jakub Kicinski wrote:
> On Mon, 25 Sep 2023 10:19:57 -0700 Tony Nguyen wrote:
> > Avoid stopping netdev tx queues during XSK setup by removing
> > netif_tx_stop_queue() and netif_tx_start_queue().
> > These changes prevent unnecessary stopping and starting of netdev
> > transmit queues during the setup of XDP socket. Without this change,
> > after stopping the XDP traffic flow tracker and then stopping
> > the XDP prog - NETDEV WATCHDOG transmit queue timed out appears.

what is xdp traffic flow tracker? what do you mean by stopping xdp prog -
or you removing it while keeping xsk pool present on interface? or are you
just downing interface?

can you provide steps to reproduce it?

> 
> I think we need more info about what happens here.
> 
> Maybe ice_qp_ena() fails before it gets to the start?
> If we don't understand what happens, exactly, we may be papering
> over other bugs.

+1

> -- 
> pw-bot: cr
>