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

On 2020/12/11 4:24, Saeed Mahameed wrote:

On Thu, 2020-12-10 at 20:27 +0800, tanhuazhong wrote:

quoted

On 2020/12/10 12:50, Saeed Mahameed wrote:

quoted

On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:

quoted

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.

[...]

quoted

quoted

quoted

+	}
+	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;
}

quoted

quoted

quoted

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

.