Re: [PATCH net-next v3] net/mlx5e: link NAPI instances to queues and IRQs

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Thu, 08 Feb, 2024 21:13:25 +0200 Tariq Toukan <ttoukan.linux@gmail.com> wrote:
> On 08/02/2024 5:07, Joe Damato wrote:
>> Make mlx5 compatible with the newly added netlink queue GET APIs.
>> Signed-off-by: Joe Damato <jdamato@fastly.com>
>> ---

Just came back from testing this code. Let me make one cosmetic point
here. I noticed this patch has a line that is past 90 characters. Would
be nice to get it wrapped in the next version. We use 90 instead of 80
characters for the line wrap in the mlx5 driver because of firmware
command interface related code would lead to very hard to read lines if
wrapped at 80.

  https://patchwork.kernel.org/project/netdevbpf/patch/20240208030702.27296-1-jdamato@fastly.com/

>> v2 -> v3:
>>    - Fix commit message subject
>>    - call netif_queue_set_napi in mlx5e_ptp_activate_channel and
>>      mlx5e_ptp_deactivate_channel to enable/disable NETDEV_QUEUE_TYPE_RX for
>>      the PTP channel.
>>    - Modify mlx5e_activate_txqsq and mlx5e_deactivate_txqsq to set
>>      NETDEV_QUEUE_TYPE_TX which should take care of all TX queues including
>>      QoS/HTB and PTP.
>>    - Rearrange mlx5e_activate_channel and mlx5e_deactivate_channel for
>>      better ordering when setting and unsetting NETDEV_QUEUE_TYPE_RX NAPI
>>      structs
>> v1 -> v2:
>>    - Move netlink NULL code to mlx5e_deactivate_channel
>>    - Move netif_napi_set_irq to mlx5e_open_channel and avoid storing the
>>      irq, after netif_napi_add which itself sets the IRQ to -1
>>   drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c  | 3 +++
>>   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++++
>>   2 files changed, 10 insertions(+)
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
>> index 078f56a3cbb2..fbbc287d924d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
>> @@ -927,6 +927,8 @@ void mlx5e_ptp_activate_channel(struct mlx5e_ptp *c)
>>   	int tc;
>>     	napi_enable(&c->napi);
>> +	netif_queue_set_napi(c->netdev, c->rq.ix, NETDEV_QUEUE_TYPE_RX,
>> +			     &c->napi);

This should only be set if MLX5E_PTP_STATE_RX is set. Otherwise, the rq
is not initialized. The following callgraph should help illustrate this.

   mlx5e_ptp_open
    |_ mlx5e_ptp_open_queues
       |_ mlx5e_ptp_open_rq
          |_ mlx5e_open_rq

>>     	if (test_bit(MLX5E_PTP_STATE_TX, c->state)) {
>>   		for (tc = 0; tc < c->num_tc; tc++)
>> @@ -951,6 +953,7 @@ void mlx5e_ptp_deactivate_channel(struct mlx5e_ptp *c)
>>   			mlx5e_deactivate_txqsq(&c->ptpsq[tc].txqsq);
>>   	}
>>   +	netif_queue_set_napi(c->netdev, c->rq.ix, NETDEV_QUEUE_TYPE_RX, NULL);

I believe it would be best to tie this to whether MLX5E_PTP_STATE_RX is
set or not.

>>   	napi_disable(&c->napi);
>>   }
>>   diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index c8e8f512803e..2f1792854dd5 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -1806,6 +1806,7 @@ void mlx5e_activate_txqsq(struct mlx5e_txqsq *sq)
>>   	set_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
>>   	netdev_tx_reset_queue(sq->txq);
>>   	netif_tx_start_queue(sq->txq);
>> +	netif_queue_set_napi(sq->channel->netdev, sq->txq_ix, NETDEV_QUEUE_TYPE_TX, &sq->channel->napi);
>
> Might be called with channel==NULL.
> For example for PTP.
>
> Prefer sq->netdev and sq->cq.napi.
>
>>   }
>>     void mlx5e_tx_disable_queue(struct netdev_queue *txq)
>> @@ -1819,6 +1820,7 @@ void mlx5e_deactivate_txqsq(struct mlx5e_txqsq *sq)
>>   {
>>   	struct mlx5_wq_cyc *wq = &sq->wq;
>>   +	netif_queue_set_napi(sq->channel->netdev, sq->txq_ix,
>> NETDEV_QUEUE_TYPE_TX, NULL);
>
> Same here.
>
>>   	clear_bit(MLX5E_SQ_STATE_ENABLED, &sq->state);
>>   	synchronize_net(); /* Sync with NAPI to prevent netif_tx_wake_queue. */
>>   @@ -2560,6 +2562,7 @@ static int mlx5e_open_channel(struct mlx5e_priv *priv,
>> int ix,
>>   	c->lag_port = mlx5e_enumerate_lag_port(priv->mdev, ix);
>>     	netif_napi_add(netdev, &c->napi, mlx5e_napi_poll);
>> +	netif_napi_set_irq(&c->napi, irq);
>>     	err = mlx5e_open_queues(c, params, cparam);
>>   	if (unlikely(err))
>> @@ -2602,12 +2605,16 @@ static void mlx5e_activate_channel(struct mlx5e_channel *c)
>>   		mlx5e_activate_xsk(c);
>>   	else
>>   		mlx5e_activate_rq(&c->rq);
>> +
>> +	netif_queue_set_napi(c->netdev, c->ix, NETDEV_QUEUE_TYPE_RX, &c->napi);
>>   }
>>     static void mlx5e_deactivate_channel(struct mlx5e_channel *c)
>>   {
>>   	int tc;
>>   +	netif_queue_set_napi(c->netdev, c->ix, NETDEV_QUEUE_TYPE_RX, NULL);
>> +
>>   	if (test_bit(MLX5E_CHANNEL_STATE_XSK, c->state))
>>   		mlx5e_deactivate_xsk(c);
>>   	else

--
Thanks,

Rahul Rameshbabu