Make mlx5 compatible with the newly added netlink queue GET APIs.
Signed-off-by: Joe Damato <[email protected]>
---
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);
}
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
--
2.25.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 <[email protected]>
nVidia folks, I'm also waiting to use this, so I'd like to apply
this directly. Please review.
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> >---
> >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
On Thu, 08 Feb, 2024 21:13:25 +0200 Tariq Toukan <[email protected]> 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 <[email protected]>
>> ---
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/[email protected]/
>> 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
On Thu, Feb 08, 2024 at 08:43:57PM -0800, Rahul Rameshbabu wrote:
> On Thu, 08 Feb, 2024 21:13:25 +0200 Tariq Toukan <[email protected]> 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 <[email protected]>
> >> ---
>
> 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/[email protected]/
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
On Fri, 09 Feb, 2024 11:24:04 -0800 Joe Damato <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> >> ---
>>
>> 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/[email protected]/
>
> 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