netdev - Re: [PATCH net-next v2] eth: mlx5: link NAPI instances to queues and IRQs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o7cul2nf.fsf@nvidia.com>
Date: Mon, 05 Feb 2024 18:52:45 -0800
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: Joe Damato <jdamato@...tly.com>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org, tariqt@...dia.com,
 Saeed Mahameed <saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, "open list:MELLANOX MLX5 core VPI driver"
 <linux-rdma@...r.kernel.org>
Subject: Re: [PATCH net-next v2] eth: mlx5: link NAPI instances to queues
 and IRQs

On Tue, 06 Feb, 2024 02:49:56 +0000 Joe Damato <jdamato@...tly.com> wrote:
> Make mlx5 compatible with the newly added netlink queue GET APIs.
>
> 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.
>   - Fix white space where IRQ is stored in mlx5e_open_channel
>
> Signed-off-by: Joe Damato <jdamato@...tly.com>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index c8e8f512803e..3e74c7de6050 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -2560,6 +2560,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,6 +2603,9 @@ 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_TX, &c->napi);
> +	netif_queue_set_napi(c->netdev, c->ix, NETDEV_QUEUE_TYPE_RX, &c->napi);
>  }
>  
>  static void mlx5e_deactivate_channel(struct mlx5e_channel *c)
> @@ -2619,6 +2623,9 @@ static void mlx5e_deactivate_channel(struct mlx5e_channel *c)
>  		mlx5e_deactivate_txqsq(&c->sq[tc]);
>  	mlx5e_qos_deactivate_queues(c);
>  
> +	netif_queue_set_napi(c->netdev, c->ix, NETDEV_QUEUE_TYPE_TX, NULL);
> +	netif_queue_set_napi(c->netdev, c->ix, NETDEV_QUEUE_TYPE_RX, NULL);
> +

I think it would be better to clean the associations before actually
deactivating the queues. Your teardown becomes LIFO/flipped order of
what is done in mlx5_activate_channel.

>  	napi_disable(&c->napi);
>  }

In general, the netdev community maintains a rule for not reposting new
versions of patches in 24hr periods to avoid these types of situations.

Lets add the feedback of updating the commit message feedback in the v1
thread into v3.

       https://lore.kernel.org/netdev/20240206025153.GA11388@fastly.com/T/#mcbf987c817c0d06c29364410ba8ab10b144c753d

Lets send out that v3 a day from now if that's alright. This way we can
pick up feedback from others if needed, but I think we are converging
here.

--
Thanks,

Rahul Rameshbabu