Re: [net-next,V5,12/15] net/mlx5: Store QoS sched nodes in the sh_devlink

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net/mlx5: Store QoS sched nodes in the sh_devlink

This commit moves the QoS scheduling nodes from per-eswitch storage to
the shared devlink instance. It replaces per-eswitch locking with the
shared devlink lock and removes explicit locking from devlink rate
callbacks with the expectation that devlink holds the shd lock.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c
> index 8c3a026b8db4..0d187399d846 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c

[ ... ]

> static void esw_assert_qos_lock_held(struct mlx5_eswitch *esw)
> {
>-	lockdep_assert_held(&esw->qos.domain->lock);
>+	devl_assert_locked(esw->dev->shd);
> }

[ ... ]

> int mlx5_esw_devlink_rate_leaf_tx_share_set(struct devlink_rate *rate_leaf, void *priv,
> 					    u64 tx_share, struct netlink_ext_ack *extack)
> {
> 	struct mlx5_vport *vport = priv;
>-	struct mlx5_eswitch *esw;
> 	int err;
>
>-	esw = vport->dev->priv.eswitch;
>-	if (!mlx5_esw_allowed(esw))
>+	if (!mlx5_esw_allowed(vport->dev->priv.eswitch))
> 		return -EPERM;
>
> 	err = esw_qos_devlink_rate_to_mbps(vport->dev, "tx_share", &tx_share, extack);
> 	if (err)
> 		return err;
>
>-	esw_qos_lock(esw);
> 	err = mlx5_esw_qos_set_vport_min_rate(vport, tx_share, extack);
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

At this commit, the esw_qos_lock/unlock calls are removed with the
expectation that "all calls originating from devlink rate already have
the shd lock held" (per the commit message). However, the devlink rate
infrastructure only locks dev->shd when supported_cross_device_rate_nodes
is set in the devlink_ops, which does not happen until commit ef6db4a7f381
("net/mlx5: qos: Enable cross-device scheduling").

The call chain is:
  devlink_nl_rate_set_doit() -> devl_rate_lock() ->
  mlx5_esw_devlink_rate_leaf_tx_share_set() ->
  mlx5_esw_qos_set_vport_min_rate() -> esw_assert_qos_lock_held() ->
  devl_assert_locked(esw->dev->shd)

Since devl_rate_lock() only locks the nested-in devlink when
supported_cross_device_rate_nodes is true, and that flag is not set at
this commit, the assertion will fail with lockdep enabled, and QoS data
structures will be accessed without proper locking.

This creates a bisect window between this commit and ef6db4a7f381 where
the QoS locking is broken. Could the setting of supported_cross_device_rate_nodes
be moved into this patch to avoid the bisect issue?

>-	if (err)
>-		goto out;
>-	esw_vport_qos_prune_empty(vport);
>-out:
>-	esw_qos_unlock(esw);
>+	if (!err)
>+		esw_vport_qos_prune_empty(vport);
> 	return err;
> }

[ ... ]