Re: [PATCH net-next 2/7] net: hns3: add support for tc mqprio offload

[tanhuazhong <tanhuazhong@huawei.com>]

On 2020/12/11 4:24, Saeed Mahameed wrote:
> On Thu, 2020-12-10 at 20:27 +0800, tanhuazhong wrote:
>>
>> On 2020/12/10 12:50, Saeed Mahameed wrote:
>>> On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
>>>> From: Jian Shen <shenjian15@huawei.com>
>>>>
>>>> Currently, the HNS3 driver only supports offload for tc number
>>>> and prio_tc. This patch adds support for other qopts, including
>>>> queues count and offset for each tc.
>>>>
>>>> When enable tc mqprio offload, it's not allowed to change
>>>> queue numbers by ethtool. For hardware limitation, the queue
>>>> number of each tc should be power of 2.
>>>>
>>>> For the queues is not assigned to each tc by average, so it's
>>>> should return vport->alloc_tqps for hclge_get_max_channels().
>>>>
>>>
>>> The commit message needs some improvements, it is not really clear
>>> what
>>> the last two sentences are about.
>>>
>>
>> The hclge_get_max_channels() returns the max queue number of each TC
>> for
>> user can set by command ethool -L. In previous implement, the queues
>> are
>> assigned to each TC by average, so we return it by vport-:
>> alloc_tqps / num_tc. And now we can assign differrent queue number
>> for
>> each TC, so it shouldn't be divided by num_tc.
> 
> What do you mean by "queues assigned to each tc by average" ?
> 

In previous implement the number of queue in each TC is same, now, we
allow that the number of queue in each TC is different.

> [...]
> 
>>    
>>>> +	}
>>>> +	if (hdev->vport[0].alloc_tqps < queue_sum) {
>>>
>>> can't you just allocate new tqps according to the new mqprio input
>>> like
>>> other drivers do ? how the user allocates those tqps ?
>>>
>>
>> maybe the name of 'alloc_tqps' is a little bit misleading, the
>> meaning
>> of this field is the total number of the available tqps in this
>> vport.
>>
> 
> from your driver code it seems alloc_tqps is number of rings allocated
> via ethool -L.
> 
> My point is, it seems like in this patch you demand for the queues to
> be preallocated, but what other drivers do on setup tc, they just
> duplicate what ever number of queues was configured prior to setup tc,
> num_tc times.
> 

The maximum number of queues is defined by 'alloc_tqps', not 
preallocated queues. The behavior on setup tc of HNS3 is same as other 
driver.

'alloc_tqps' in HNS3 has the same means as 'num_queue_pairs' in i40e below
if (vsi->num_queue_pairs <
	    (mqprio_qopt->qopt.offset[i] + mqprio_qopt->qopt.count[i])) {
		return -EINVAL;
}

>>>> +		dev_err(&hdev->pdev->dev,
>>>> +			"qopt queue count sum should be less than
>>>> %u\n",
>>>> +			hdev->vport[0].alloc_tqps);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void hclge_sync_mqprio_qopt(struct hnae3_tc_info
>>>> *tc_info,
>>>> +				   struct tc_mqprio_qopt_offload
>>>> *mqprio_qopt)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	memset(tc_info, 0, sizeof(*tc_info));
>>>> +	tc_info->num_tc = mqprio_qopt->qopt.num_tc;
>>>> +	memcpy(tc_info->prio_tc, mqprio_qopt->qopt.prio_tc_map,
>>>> +	       sizeof_field(struct hnae3_tc_info, prio_tc));
>>>> +	memcpy(tc_info->tqp_count, mqprio_qopt->qopt.count,
>>>> +	       sizeof_field(struct hnae3_tc_info, tqp_count));
>>>> +	memcpy(tc_info->tqp_offset, mqprio_qopt->qopt.offset,
>>>> +	       sizeof_field(struct hnae3_tc_info, tqp_offset));
>>>> +
>>>
>>> isn't it much easier to just store a copy of tc_mqprio_qopt in you
>>> tc_info and then just:
>>> tc_info->qopt = mqprio->qopt;
>>>
>>> [...]
>>
>> The tc_mqprio_qopt_offload still contains a lot of opt hns3 driver
>> does
>> not use yet, even if the hns3 use all the opt, I still think it is
>> better to create our own struct, if struct tc_mqprio_qopt_offload
>> changes in the future, we can limit the change to the
>> tc_mqprio_qopt_offload convertion.
>>
> 
> ok.
> 

Thanks.
Huazhong.

> 
> 
> .
>