[dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush

[Ferruh Yigit]

On 9/25/2017 8:40 AM, Zhao1, Wei wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, September 20, 2017 6:36 PM
>> To: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and
> flush
>>
>> On 9/15/2017 4:13 AM, Wei Zhao wrote:
>> > This feature enable queue regions configuration for RSS in PF/VF, so
>> > that different traffic classes or different packet classification
>> > types can be separated to different queues in different queue
>> > regions.This patch can set queue region range, it include queue number
>> > in a region and the index of first queue.
>>
>> Is following correct:
>> So instead of distributing packets to the multiple queues, this will
> distribute
>> packets into queue reqions which may consists of multiple queues.
>>
>> If so, is there a way to control how packets distributed within same queue
>> region to multiple queues?
>>
> 
> distributed within same queue region is based on a rss algorithm, it is
> implemented by NIC self, software can not control it.

I was thinking RSS used to select queue region, but no first queue
region selected, later RSS used to select queue within the region.
Thanks for clarification.

<...>
                                                               
> sizeof(vsi->info.tc_mapping));
>> > +      (void)rte_memcpy(&vsi->info.queue_mapping,
>> > +                                      &ctxt.info.queue_mapping,
>> > +                      sizeof(vsi->info.queue_mapping));
>>
>> Please keep line allignment same with above line.
> 
> oK, change in v4, but can not the same.
> (void)rte_memcpy(&vsi->info.queue_mapping, &ctxt.info.queue_mapping,
>                                                
> sizeof(vsi->info.queue_mapping));
> will over 80 maxnumber.

Someting like following, new lines aligned:

	(void)rte_memcpy(&vsi->info.queue_mapping,
		&ctxt.info.queue_mapping,
		sizeof(vsi->info.queue_mapping));

<...>

>> > +                      if ((i == info->queue_region_number) &&
>> > +                                      (i <= I40E_REGION_MAX_INDEX)) {
>>
>>
>> Please use one more level indentaion here, and pharantesis looks extra can
>> you please double check?
> 
> You mean,you want the following moede ?
>         if (i == info->queue_region_number) {
>         	if(i <= I40E_REGION_MAX_INDEX) {
> 			...........................
>         	}
>        }
> 

No, continuation of the "if" should be easy to recognized from body of
the "if", something like:

	if ((i == info->queue_region_number) &&
			(i <= I40E_REGION_MAX_INDEX)) {
		info->region[i].region_id = conf_ptr->region_id;
		info->region[i].queue_num = conf_ptr->queue_num;

<...>


>> > +static int
>> > +i40e_set_region_flowtype_pf(struct i40e_hw *hw,
>> > +                      struct i40e_pf *pf, struct
> rte_i40e_rss_region_conf
>> *conf_ptr)
>>
>> What do you think starting all functions, even they are static, with
>> "i40e_queue_region_" prefix? 
>                                 ret = i40e_set_queue_region(pf, conf_ptr);
>                                 break;
>                 case RTE_PMD_I40E_REGION_FLOWTYPE_PF_SET:
>                                 ret = i40e_set_region_flowtype_pf(hw,
> pf, conf_ptr);
>                                 break;
>                 case RTE_PMD_I40E_REGION_FLOWTYPE_VF_SET:
>                                 ret = -EINVAL;
>                                 break;
>                 case RTE_PMD_I40E_UP_REGION_SET:
>                                 ret = i40e_set_up_region(pf, conf_ptr);
> 
> I have use the format ofi40e_set_  as prefix?
> Because many set is not related directly to queue region.

When looking to this patch what functions are doing is clear, but it
also should be clear when looking the source code some time later what
that function is about.

That is why I believe a namespace is useful, like
"i40e_queue_region_xx", so you can immediately say this function is
about "RSS queue region" feature.

This function is "rte_pmd_i40e_queue_region_conf", it is applying
provided queue region configuration based of region op type, so I
assumed all these functions are directly related to the fature.

If you believe function name will be wrong with that prefix, of course
just don't use it, but please try to stick with a name space, that will
make easy to understand these features in all source code.

<...>


>> > +
>> > +      memset(&hw->local_dcbx_config, 0,
>> > +      sizeof(struct i40e_dcbx_config));
>>
>> Wrong indentation.

Reminder of this one incase missed.

<...>

>> > +int rte_pmd_i40e_queue_region_conf(uint8_t port,
>> > +                                      struct
> rte_i40e_rss_region_conf *conf_ptr) {
>> > +      struct rte_eth_dev *dev = &rte_eth_devices[port];
>>
>> you need to verify port_id, since this is public API now. Please check
> other
>> APIs in this file.

Reminder of this one incase missed.

I mean is_i40e_supported() call that other APIs have.

<...>

>> > +
>> > +      if (!is_i40e_supported(dev))
>> > +                      return -ENOTSUP;
>> > +
>> > +      switch (op_type) {
>> > +      case RTE_PMD_I40E_QUEUE_REGION_SET:
>> > +                      ret = i40e_set_queue_region(pf, conf_ptr);
>> > +                      break;
>>
>> Does it make sense to add another type to get the current queue region
>> config?

Reminder of this one incase missed.

> 
>> > +      case RTE_PMD_I40E_REGION_FLOWTYPE_PF_SET:
>> > +                      ret = i40e_set_region_flowtype_pf(hw, pf,
> conf_ptr);
>> > +                      break;
>> > +      case RTE_PMD_I40E_REGION_FLOWTYPE_VF_SET:
>> > +                      ret = -EINVAL;
>>
>> Will this be implemented later, or not a valid case at all?

Reminder of this one incase missed.

>> > +                      break;
>> > +      case RTE_PMD_I40E_UP_REGION_SET:> +                         
> ret =
>> i40e_set_up_region(pf, conf_ptr);
>> > +                      break;
>> > +      case RTE_PMD_I40E_REGION_ALL_FLUSH_ON:
>> > +                      ret = i40e_flush_region_all_conf(hw, pf, 1);
>> > +                      break;
>> > +      case RTE_PMD_I40E_REGION_ALL_FLUSH_OFF:
>> > +                      ret = i40e_flush_region_all_conf(hw, pf, 0);
>>
>> Can you please describe what flush_on and flush_off are, you can comment
>> to code if also.

Reminder of this one incase missed.

<...>

>> > @@ -146,6 +160,18 @@ struct rte_pmd_i40e_ptype_mapping {  };
>> >
>> >  /**
>> > + * Queue region information get from CLI.
>>
>> It doesn't need to be from CLI, can drop that part

Reminder of this one incase missed.

<...>

>> > +      uint8_t user_priority;
>> > +      enum rte_pmd_i40e_queue_region_op  op;
>>
>> Extra space before "op"
> 
> Maybe, I SHOULD add uint8_t raw[3] before “op”

Sorry, I didn't get this one.

<...>

>> rte_pmd_i40e_ptype_mapping_replace(uint8_t
>> > port,  int rte_pmd_i40e_add_vf_mac_addr(uint8_t port, uint16_t vf_id,
>> >                                                           struct
> ether_addr *mac_addr);
>> >
>> > +/**
>> > + * Get RSS queue region info from CLI and do configuration for
>>
>> Again now this is an API, not just for testpmd, please drop CLI

Reminder of this one incase missed.

>>
>> > + * that port as the command otion type
>>
>> s/otion/options
>>

Reminder of this one incase missed.

<...>

>> > +int rte_pmd_i40e_queue_region_conf(uint8_t port,
>>
>> Can you please use port_id as variable name to be consistent.

Reminder of this one incase missed.

<...>