linux-kernel - Re: [PATCH net-next v3 10/12] net/mlx5e: Implement queue mgmt ops and single channel swap

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: <CAHS8izOX8t-Xu+mseiRBvLDYmk6G+iH=tX6t4SWY2TKBau7r-Q@mail.gmail.com>
Date: Wed, 11 Jun 2025 22:33:41 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Mark Bloch <mbloch@...dia.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, saeedm@...dia.com, gal@...dia.com, 
	leonro@...dia.com, tariqt@...dia.com, Leon Romanovsky <leon@...nel.org>, 
	Simon Horman <horms@...nel.org>, Richard Cochran <richardcochran@...il.com>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, netdev@...r.kernel.org, 
	linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org, bpf@...r.kernel.org, 
	Dragos Tatulea <dtatulea@...dia.com>, Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH net-next v3 10/12] net/mlx5e: Implement queue mgmt ops and
 single channel swap

On Mon, Jun 9, 2025 at 8:08 AM Mark Bloch <mbloch@...dia.com> wrote:
>
> From: Saeed Mahameed <saeedm@...dia.com>
>
> The bulk of the work is done in mlx5e_queue_mem_alloc, where we allocate
> and create the new channel resources, similar to
> mlx5e_safe_switch_params, but here we do it for a single channel using
> existing params, sort of a clone channel.
> To swap the old channel with the new one, we deactivate and close the
> old channel then replace it with the new one, since the swap procedure
> doesn't fail in mlx5, we do it all in one place (mlx5e_queue_start).
>
> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> Reviewed-by: Dragos Tatulea <dtatulea@...dia.com>
> Signed-off-by: Cosmin Ratiu <cratiu@...dia.com>
> Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> Signed-off-by: Mark Bloch <mbloch@...dia.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/en_main.c | 97 +++++++++++++++++++
>  1 file changed, 97 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index a51e204bd364..90687392545c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -5494,6 +5494,102 @@ static const struct netdev_stat_ops mlx5e_stat_ops = {
>         .get_base_stats      = mlx5e_get_base_stats,
>  };
>
> +struct mlx5_qmgmt_data {
> +       struct mlx5e_channel *c;
> +       struct mlx5e_channel_param cparam;
> +};
> +
> +static int mlx5e_queue_mem_alloc(struct net_device *dev, void *newq,
> +                                int queue_index)
> +{
> +       struct mlx5_qmgmt_data *new = (struct mlx5_qmgmt_data *)newq;
> +       struct mlx5e_priv *priv = netdev_priv(dev);
> +       struct mlx5e_channels *chs = &priv->channels;
> +       struct mlx5e_params params = chs->params;
> +       struct mlx5_core_dev *mdev;
> +       int err;
> +
> +       mutex_lock(&priv->state_lock);
> +       if (!test_bit(MLX5E_STATE_OPENED, &priv->state)) {
> +               err = -ENODEV;
> +               goto unlock;
> +       }
> +
> +       if (queue_index >= chs->num) {
> +               err = -ERANGE;
> +               goto unlock;
> +       }
> +
> +       if (MLX5E_GET_PFLAG(&chs->params, MLX5E_PFLAG_TX_PORT_TS) ||
> +           chs->params.ptp_rx   ||
> +           chs->params.xdp_prog ||
> +           priv->htb) {
> +               netdev_err(priv->netdev,
> +                          "Cloning channels with Port/rx PTP, XDP or HTB is not supported\n");
> +               err = -EOPNOTSUPP;
> +               goto unlock;
> +       }
> +
> +       mdev = mlx5_sd_ch_ix_get_dev(priv->mdev, queue_index);
> +       err = mlx5e_build_channel_param(mdev, &params, &new->cparam);
> +       if (err) {
> +               return err;
> +               goto unlock;
> +       }
> +
> +       err = mlx5e_open_channel(priv, queue_index, &params, NULL, &new->c);
> +unlock:
> +       mutex_unlock(&priv->state_lock);
> +       return err;
> +}
> +
> +static void mlx5e_queue_mem_free(struct net_device *dev, void *mem)
> +{
> +       struct mlx5_qmgmt_data *data = (struct mlx5_qmgmt_data *)mem;
> +
> +       /* not supposed to happen since mlx5e_queue_start never fails
> +        * but this is how this should be implemented just in case
> +        */
> +       if (data->c)
> +               mlx5e_close_channel(data->c);
> +}
> +
> +static int mlx5e_queue_stop(struct net_device *dev, void *oldq, int queue_index)
> +{
> +       /* mlx5e_queue_start does not fail, we stop the old queue there */
> +       return 0;
> +}

Is this really better than maintaining uniformity of behavior between
the drivers that support the queue mgmt api and just doing the
mlx5e_deactivate_priv_channels and mlx5e_close_channel in the stop
like core sorta expects?

We currently use the ndos to restart a queue, but I'm imagining in the
future we can expand it to create queues on behalf of the queues. The
stop queue API may be reused in other contexts, like maybe to kill a
dynamically created devmem queue or something, and this specific
driver may stop working because stop actually doesn't do anything?

-- 
Thanks,
Mina