diff mbox series

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

Message ID 20240208030702.27296-1-jdamato@fastly.com
State New
Headers
Series [net-next,v3] net/mlx5e: link NAPI instances to queues and IRQs |

Commit Message

Joe Damato Feb. 8, 2024, 3:07 a.m. UTC 
  Make mlx5 compatible with the newly added netlink queue GET APIs.

Signed-off-by: Joe Damato <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(+)

Comments

Jakub Kicinski Feb. 8, 2024, 5:41 p.m. UTC | #1 
On Thu,  8 Feb 2024 03:07:00 +0000 Joe Damato wrote:
> Make mlx5 compatible with the newly added netlink queue GET APIs.
> 
> Signed-off-by: Joe Damato <jdamato@fastly.com>

nVidia folks, I'm also waiting to use this, so I'd like to apply
this directly. Please review.
Tariq Toukan Feb. 8, 2024, 7:13 p.m. UTC | #2 
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>
> ---
> 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);
>   
>   	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);
>   	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
Joe Damato Feb. 8, 2024, 7:41 p.m. UTC | #3 
On Thu, Feb 08, 2024 at 09:13:25PM +0200, Tariq Toukan 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>
> >---
> >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);
> >  	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);
> >  	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.

OK, I'll make that change for the v4 which I'll send sometime tomorrow.

Thanks.

> >  }
> >  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
Rahul Rameshbabu Feb. 9, 2024, 4:43 a.m. UTC | #4 
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
Joe Damato Feb. 9, 2024, 7:24 p.m. UTC | #5 
On Thu, Feb 08, 2024 at 08:43:57PM -0800, Rahul Rameshbabu wrote:
> 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/

OK, I had wrapped them in the next version I was going to send to 80, but
I'll adjust that to 90.

> >> 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

I had made the change that Tariq had suggested in the previous message of
using sq->netdev and sq->cq.napi, but I can also tie that into
MLX5E_PTP_STATE_RX.

I'll add this change as well, test locally again and resend shortly.

> >>     	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
Rahul Rameshbabu Feb. 9, 2024, 7:26 p.m. UTC | #6 
On Fri, 09 Feb, 2024 11:24:04 -0800 Joe Damato <jdamato@fastly.com> wrote:
> On Thu, Feb 08, 2024 at 08:43:57PM -0800, Rahul Rameshbabu wrote:
>> 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/
>
> OK, I had wrapped them in the next version I was going to send to 80, but
> I'll adjust that to 90.
>
>> >> 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
>
> I had made the change that Tariq had suggested in the previous message of
> using sq->netdev and sq->cq.napi, but I can also tie that into
> MLX5E_PTP_STATE_RX.

Right, Tariq's comments are needed for the TX queue associations and are
needed as well. We also need to make sure we do not create an RX queue
association for an RX queue that may not exist for this special PTP
channel.

>
> I'll add this change as well, test locally again and resend shortly.
>

--
Thanks,

Rahul Rameshbabu
diff mbox series

Patch

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);
 
 	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);
 	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);
 }
 
 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);
 	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