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

Zhao1, Wei wei.zhao1 at intel.com
Tue Sep 26 09:46:55 CEST 2017


Hi,  Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, September 25, 2017 5:32 PM
> To: Zhao1, Wei <wei.zhao1 at intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush
> 
> 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.

OK

> 
> <...>
> 
> > 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));


Ok, change in v4

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

Ok, change in v4

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

Ok, fix in v4


> 
> 
> >> > +
> >> > +      memset(&hw->local_dcbx_config, 0,
> >> > +      sizeof(struct i40e_dcbx_config));
> >>
> >> Wrong indentation.
> 
> Reminder of this one incase missed.

Ok, change in v4

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

Ok, change in v4

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

I will add a get command.

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

Ok, change in v4

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

I will comment to code in v4.
ALL config from CLI at first will only keep in DPDK software stored in driver, only after " FLUSH_ON ", it commit all configure to HW.
Because I have to set hardware config at a time, so I have to record all CLI command at first.
" FLUSH_OFF " is just clean all configuration about queue region  just now, and restore all to DPDK i40e driver default config when start up.
The following is my test process in CLI: 

./x86_64-native-linuxapp-gcc/app/testpmd -c 1ffff -n 4 -- -i --nb-cores=8  --rxq=8 --txq=8 --port-topology=chained set fwd rxonly port config all rss all queue-region set port 0 region_id 0 queue_start_index 2 queue_num 2 queue-region set port 0 region_id 1 queue_start_index 4 queue_num 2 
queue-region set pf port 0 region_id 0 flowtype 31 
queue-region set pf port 0 region_id 1 flowtype 33 
queue-region set pf port 0 region_id 1 flowtype 34 
queue-region set port 0 UP 1 region_id 0 
queue-region set port 0 UP 3 region_id 1 
queue-region flush on  port 0 
start set verbose 1 sendp([Ether(dst="3C:FD:FE:9F:70:B8", src="52:00:00:00:00:00")/Dot1Q(prio=3)/IP(src="10.0.0.1",dst="192.168.0.2")/TCP(dport=80, sport=80 )/("X"*48)], iface="ens5f2", count=10) sendp([Ether(dst="3C:FD:FE:9F:70:B8", src="52:00:00:00:00:00")/IP(src="10.0.0.1",dst="192.168.0.2")/SCTP(dport=80, sport=80 )/("X"*48)], iface="ens5f2", count=10) sendp([Ether(dst="3C:FD:FE:9F:70:B8", src="52:00:00:00:00:00")/IP(src="10.0.0.1",dst="192.168.0.2")/UDP(dport=80, sport=80 )/("X"*48)], iface="ens5f2", count=10)

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

Ok, change in v4

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

Ok

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

Ok, change in v4

> 
> >>
> >> > + * that port as the command otion type
> >>
> >> s/otion/options
> >>
> 
> Reminder of this one incase missed.

Ok, change in v4

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

Ok, change in v4

> 
> <...>


More information about the dev mailing list