Re: [PATCH v3 1/2] net/i40e: queue region set and flush

["Zhao1, Wei" <wei.zhao1@intel.com>]

Thank you.

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, September 28, 2017 3:13 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush
> 
> On 9/26/2017 9:54 AM, Zhao1, Wei wrote:
> > Hi,  Ferruh
> 
> Hi Wei,
> 
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Wednesday, September 20, 2017 6:36 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@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?
> >>
> >> And is this feature only supported with RSS? Can it be part of RSS
> >> configuration instead of PMD specific API?
> >>
> >> > This patch enable mapping between different priorities (UP) and
> >>
> >> User priorities (UP)
> >>
> >> > different traffic classes.It also enable mapping between a region
> >> > index and a sepcific flowtype(PCTYPE).It also provide the solution
> >> > of flush all configuration about queue region the above described.
> >> >
> >> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com
> > <mailto:wei.zhao1@intel.com>>
> >> > ---
> >> >  drivers/net/i40e/i40e_ethdev.c            |  19 +-
> >> >  drivers/net/i40e/i40e_ethdev.h            |  30 ++
> >> >  drivers/net/i40e/rte_pmd_i40e.c           | 482
> >> ++++++++++++++++++++++++++++++
> >> >  drivers/net/i40e/rte_pmd_i40e.h           |  38 +++
> >> >  drivers/net/i40e/rte_pmd_i40e_version.map |   1 +
> >> >  5 files changed, 566 insertions(+), 4 deletions(-)
> >> >
> >>
> >> <...>
> >>
> >> > +static int
> >> > +i40e_vsi_update_queue_region_mapping(struct i40e_hw *hw,
> >> > +                                            struct i40e_pf *pf) {
> >> > +      uint16_t i;
> >> > +      struct i40e_vsi *vsi = pf->main_vsi;
> >> > +      uint16_t queue_offset, bsf, tc_index;
> >> > +      struct i40e_vsi_context ctxt;
> >> > +      struct i40e_aqc_vsi_properties_data *vsi_info;
> >> > +      struct i40e_queue_region_info *region_info =
> >> > +
> > &pf->queue_region;
> >> > +      uint32_t ret = -EINVAL;
> >> > +
> >> > +      if (!region_info->queue_region_number) {
> >
> > ....................
> >
> >> > +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.
> >
> > I have already "if (!is_i40e_supported(dev))" code in v3 in function
> >  rte_pmd_i40e_queue_region_conf.
> >
> > So, I do not know what is your meaning.
> 
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> 
> Normally we don't need to worry about this in PMD because abstraction
> layer APIs do this check already. But since this is public API, user can give any
> value as port_id parameter, better to verify it.
> 
> >
> >>
> >> > +      struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> >> >dev_private);
> >> > +      struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >> >dev_private);
> >> > +      enum rte_pmd_i40e_queue_region_op op_type = conf_ptr->op;
> >> > +      uint32_t ret;
> >>
> >> This should be signed variable, since you are using it for return and
> > assigning
> >> negative values.
> >>
> >> > +
> >> > +      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;
> >>