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

[tanhuazhong]

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

> .