Re: [RFC net-next 4/7] net/mlx5e: Drain old TX SQs before activating new channels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net/mlx5e: mlx5e_poll_tx_cqes() queues recover_work on
  deactivated SQs
--

commit fd69ab25361dddbeaf22971fdc4198ef4326e153
Author: Tariq Toukan <tariqt@nvidia.com>

net/mlx5e: Drain old TX SQs before activating new channels

This commit drains pending WQEs from deactivated TX SQs before activating
new channels when tis_tir_td_order is supported, falling back to closing
the old channels first if the drain times out.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> index bee6f0bf1eff5..eb20e3d14d461 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> @@ -780,10 +780,11 @@ void mlx5e_txqsq_wake(struct mlx5e_txqsq *sq)
>  	}
>  }
>  
> -bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
> +/* Returns the number of polled CQEs. */
> +static int mlx5e_poll_tx_cqes(struct mlx5e_txqsq *sq, int napi_budget)
>  {
> +	struct mlx5e_cq *cq = &sq->cq;
>  	struct mlx5e_sq_stats *stats;
> -	struct mlx5e_txqsq *sq;
>  	struct mlx5_cqe64 *cqe;
>  	u32 dma_fifo_cc;
>  	u32 nbytes;
> @@ -791,14 +792,9 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>  	u16 sqcc;
>  	int i;
>  
> -	sq = container_of(cq, struct mlx5e_txqsq, cq);
> -
> -	if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> -		return false;
> -
>  	cqe = mlx5_cqwq_get_cqe(&cq->wq);
>  	if (!cqe)
> -		return false;
> +		return 0;
>  
>  	stats = sq->stats;
>  
> @@ -891,11 +887,40 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>  
>  	netdev_tx_completed_queue(sq->txq, npkts, nbytes);
>  
> +	return i;
> +}
> +
> +bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
> +{
> +	struct mlx5e_txqsq *sq = container_of(cq, struct mlx5e_txqsq, cq);
> +	int i;
> +
> +	if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> +		return false;
> +
> +	i = mlx5e_poll_tx_cqes(sq, napi_budget);
> +	if (!i)
> +		return false;
> +
>  	mlx5e_txqsq_wake(sq);
>  
>  	return (i == MLX5E_TX_CQ_POLL_BUDGET);
>  }
>  
> +/* Reap the completions of a deactivated SQ, whose NAPI is disabled, without
> + * waking its TX queue. Returns true once all its WQEs have completed.
> + */
> +bool mlx5e_txqsq_drain(struct mlx5e_txqsq *sq)
> +{
> +	int polled;
> +
> +	do {
> +		polled = mlx5e_poll_tx_cqes(sq, 0);
> +	} while (polled == MLX5E_TX_CQ_POLL_BUDGET);
> +
> +	return sq->cc == sq->pc;
> +}

[Severity: High]
Can mlx5e_txqsq_drain() -> mlx5e_poll_tx_cqes() queue sq->recover_work on
a deactivated SQ when it polls an MLX5_CQE_REQ_ERR completion?

Before this patch, mlx5e_poll_tx_cq() returned early when
MLX5E_SQ_STATE_ENABLED was clear.  Now mlx5e_poll_tx_cqes() runs on the
deactivated SQs in old_chs during mlx5e_switch_priv_channels(), after
mlx5e_open_channels() -> mlx5e_health_channels_update() has already reset
priv->tx_reporter->health_state to DEVLINK_HEALTH_REPORTER_STATE_HEALTHY.
If an old SQ completes a WQE or the deactivation NOP with
MLX5_CQE_REQ_ERR, mlx5e_poll_tx_cqes() queues sq->recover_work on
cq->workqueue:

drivers/net/ethernet/mellanox/mlx5/core/en_tx.c:mlx5e_poll_tx_cqes() {
	...
		if (unlikely(err_cqe)) {
			if (!test_and_set_bit(MLX5E_SQ_STATE_RECOVERING,
					      &sq->state)) {
				mlx5e_dump_error_cqe(&sq->cq, sq->sqn,
						     (struct mlx5_err_cqe *)cqe);
				mlx5_wq_cyc_wqe_dump(&sq->wq, ci, wi->num_wqebbs);
				queue_work(cq->workqueue, &sq->recover_work);
			}
			stats->cqe_err++;
		}
	...
}

When mlx5e_safe_reopen_channels() is invoked from devlink health recovery
under devl_lock(devlink), this causes an ABBA deadlock with
cancel_work_sync(&sq->recover_work) when old_chs is closed:

CPU0                              CPU1 (kworker)
----                              --------------
devl_lock(devlink)
  ...
  mlx5e_switch_priv_channels()
    mlx5e_drain_channels_txqsqs()
      mlx5e_poll_tx_cqes()
        queue_work(..., &sq->recover_work)
                                  mlx5e_tx_err_cqe_work()
                                    mlx5e_reporter_tx_err_cqe()
                                      devlink_health_report()
                                        devl_lock(devlink)
    mlx5e_close_channels(old_chs)
      ...
      mlx5e_close_txqsq()
        cancel_work_sync(&sq->recover_work)

 *** DEADLOCK ***

Even when devl_lock(devlink) is not held by the caller (such as during an
ethtool channel change), queuing sq->recover_work for an SQ in old_chs
runs devlink_health_report() after mlx5e_open_channels() marked
tx_reporter healthy, which either leaves tx_reporter in an error state for
an already-closed SQ or starts a recovery graceful period that suppresses
real error recovery on new_chs.

Should mlx5e_poll_tx_cqes() only queue sq->recover_work when
MLX5E_SQ_STATE_ENABLED is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=4