Re: [PATCH net-next 08/15] net/mlx5: Refactor vport QoS to use scheduling node structure

[Simon Horman <horms@kernel.org>]

On Sun, Oct 13, 2024 at 09:45:33AM +0300, Tariq Toukan wrote:
> From: Carolina Jubran <cjubran@nvidia.com>
> 
> Refactor the vport QoS structure by moving group membership and
> scheduling details into the `mlx5_esw_sched_node` structure.
> 
> This change consolidates the vport into the rate hierarchy by unifying
> the handling of different types of scheduling element nodes.
> 
> In addition, add a direct reference to the mlx5_vport within the
> mlx5_esw_sched_node structure, to ensure that the vport is easily
> accessible when a scheduling node is associated with a vport.
> 
> Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>

Hi Carolina and Tariq,

Some minor feedback from my side.

...

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c

...

> +struct mlx5_esw_sched_node *
> +mlx5_esw_qos_vport_get_parent(const struct mlx5_vport *vport)
>  {
> -	list_del_init(&vport->qos.parent_entry);
> -	vport->qos.parent = parent;
> -	list_add_tail(&vport->qos.parent_entry, &parent->children);
> +	if (!vport->qos.sched_node)
> +		return 0;

As the return type of this function is a pointer,
perhaps returning NULL would be more appropriate.

...

> @@ -718,18 +750,26 @@ static int esw_qos_vport_enable(struct mlx5_vport *vport,
>  		return err;
>  
>  	err = esw_qos_vport_create_sched_element(vport, esw->qos.node0, max_rate, bw_share,
> -						 &vport->qos.esw_sched_elem_ix);
> +						 &sched_elem_ix);
>  	if (err)
>  		goto err_out;
>  
> -	INIT_LIST_HEAD(&vport->qos.parent_entry);
> -	esw_qos_vport_set_parent(vport, esw->qos.node0);
> +	vport->qos.sched_node = __esw_qos_alloc_rate_node(esw, sched_elem_ix, SCHED_NODE_TYPE_VPORT,
> +							  esw->qos.node0);
> +	if (!vport->qos.sched_node)

Should err be set to a negative error value here so that value will be
returned?

> +		goto err_alloc;
>  
>  	vport->qos.enabled = true;
> +	vport->qos.sched_node->vport = vport;
> +
>  	trace_mlx5_esw_vport_qos_create(vport->dev, vport, bw_share, max_rate);
>  
>  	return 0;
>  
> +err_alloc:
> +	if (mlx5_destroy_scheduling_element_cmd(esw->dev,
> +						SCHEDULING_HIERARCHY_E_SWITCH, sched_elem_ix))
> +		esw_warn(esw->dev, "E-Switch destroy vport scheduling element failed.\n");
>  err_out:
>  	esw_qos_put(esw);
>  

...

> @@ -746,20 +787,23 @@ void mlx5_esw_qos_vport_disable(struct mlx5_vport *vport)
>  	esw_qos_lock(esw);
>  	if (!vport->qos.enabled)
>  		goto unlock;
> -	WARN(vport->qos.parent != esw->qos.node0,
> +	vport_node = vport->qos.sched_node;
> +	WARN(vport_node->parent != esw->qos.node0,
>  	     "Disabling QoS on port before detaching it from node");
>  
> -	dev = vport->qos.parent->esw->dev;
> +	trace_mlx5_esw_vport_qos_destroy(dev, vport);

dev does not appear to be initialised here.

> +
> +	dev = vport_node->esw->dev;
>  	err = mlx5_destroy_scheduling_element_cmd(dev,
>  						  SCHEDULING_HIERARCHY_E_SWITCH,
> -						  vport->qos.esw_sched_elem_ix);
> +						  vport_node->ix);
>  	if (err)
>  		esw_warn(dev,
>  			 "E-Switch destroy vport scheduling element failed (vport=%d,err=%d)\n",
>  			 vport->vport, err);
>  
> +	__esw_qos_free_node(vport_node);
>  	memset(&vport->qos, 0, sizeof(vport->qos));
> -	trace_mlx5_esw_vport_qos_destroy(dev, vport);
>  
>  	esw_qos_put(esw);
>  unlock:

...