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

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] [day] [month] [year] [list]

Message-ID: <202a36b6-8940-4d65-8743-4997300aa1c8@gmail.com>
Date: Mon, 14 Oct 2024 23:47:26 +0300
From: Tariq Toukan <ttoukan.linux@...il.com>
To: Simon Horman <horms@...nel.org>, Tariq Toukan <tariqt@...dia.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
 Saeed Mahameed <saeedm@...dia.com>, Gal Pressman <gal@...dia.com>,
 Leon Romanovsky <leonro@...dia.com>, cjubran@...dia.com, cratiu@...dia.com
Subject: Re: [PATCH net-next 08/15] net/mlx5: Refactor vport QoS to use
 scheduling node structure



On 14/10/2024 12:33, Simon Horman wrote:
> On Sun, Oct 13, 2024 at 09:45:33AM +0300, Tariq Toukan wrote:
>> From: Carolina Jubran <cjubran@...dia.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@...dia.com>
>> Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> 
> Hi Carolina and Tariq,
> 
> Some minor feedback from my side.
> 
> ...
> 

Hi Simon,
Thanks for your feedback.

>> 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.
> 
> ...
> 

Sure. Agree. It was not intended.

>> @@ -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?
> 

Yes. Will fix.

>> +		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.
> 

Will fix.

>> +
>> +	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:
> 
> ...
>