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

[tanhuazhong]

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

> > Signed-off-by: Jian Shen <shenjian15@xxxxxxxxxx>
> > Signed-off-by: Huazhong Tan <tanhuazhong@xxxxxxxxxx>
> > ---
> ...
>
> >   
> > +	if (kinfo->tc_info.mqprio_active) {
> > +		dev_err(&netdev->dev,
>
> why not use netdev_err() and friends ?
> anyway I see your driver is using dev_err(&netdev->dev, ...)
> intensively,
> maybe submit a follow up patch to fix all your prints ?

yes, will fix it with another patch.

> ...]
> >   
> > +static int hclge_mqprio_qopt_check(struct hclge_dev *hdev,
> > +				   struct tc_mqprio_qopt_offload
> > *mqprio_qopt)
> > +{
> > +	u16 queue_sum = 0;
> > +	int ret;
> > +	int i;
> > +
> > +	if (!mqprio_qopt->qopt.num_tc) {
> > +		mqprio_qopt->qopt.num_tc = 1;
> > +		return 0;
> > +	}
> > +
> > +	ret = hclge_dcb_common_validate(hdev, mqprio_qopt->qopt.num_tc,
> > +					mqprio_qopt->qopt.prio_tc_map);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < mqprio_qopt->qopt.num_tc; i++) {
> > +		if (!is_power_of_2(mqprio_qopt->qopt.count[i])) {
> > +			dev_err(&hdev->pdev->dev,
> > +				"qopt queue count must be power of
> > 2\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (mqprio_qopt->qopt.count[i] > hdev->rss_size_max) {
> > +			dev_err(&hdev->pdev->dev,
> > +				"qopt queue count should be no more
> > than %u\n",
> > +				hdev->rss_size_max);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (mqprio_qopt->qopt.offset[i] != queue_sum) {
> > +			dev_err(&hdev->pdev->dev,
> > +				"qopt queue offset must start from 0,
> > and being continuous\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (mqprio_qopt->min_rate[i] || mqprio_qopt-
> > > max_rate[i]) {
> > +			dev_err(&hdev->pdev->dev,
> > +				"qopt tx_rate is not supported\n");
> > +			return -EOPNOTSUPP;
> > +		}
> > +
> > +		queue_sum = mqprio_qopt->qopt.offset[i];
> > +		queue_sum += mqprio_qopt->qopt.count[i];
>
> it will make more sense if you moved this queue summing outside of the
> loop

this queue_sum is used in this loop:
if (mqprio_qopt->qopt.offset[i] != queue_sum) {
...

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

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

> > -	hclge_tm_schd_info_update(hdev, tc);
> > -	hclge_tm_prio_tc_info_update(hdev, prio_tc);
> > -
> > -	ret = hclge_tm_init_hw(hdev, false);
> > -	if (ret)
> > -		goto err_out;
> > +	kinfo = &vport->nic.kinfo;
> > +	memcpy(&old_tc_info, &kinfo->tc_info, sizeof(old_tc_info));
>
> if those are of the same kind, just normal assignment would be much
> cleaner.

yes, normal assignment seems cleaner.

> > +	hclge_sync_mqprio_qopt(&kinfo->tc_info, mqprio_qopt);
> > +	kinfo->tc_info.mqprio_active = tc > 0;
> >   
> > -	ret = hclge_client_setup_tc(hdev);
> > +	ret = hclge_config_tc(hdev, &kinfo->tc_info);
> >   	if (ret)
> >   		goto err_out;
> >   
> > @@ -436,6 +534,12 @@ static int hclge_setup_tc(struct hnae3_handle
> > *h, u8 tc, u8 *prio_tc)
> >   	return hclge_notify_init_up(hdev);
> >   
> >   err_out:
> > +	/* roll-back */
> > +	memcpy(&kinfo->tc_info, &old_tc_info, sizeof(old_tc_info));
> same.
>
>
>
> .