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

[Saeed Mahameed]

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

[...]

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

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