Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, September 25, 2017 5:32 PM
> To: Zhao1, Wei <wei.zh...@intel.com>
> Cc: dev@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.zh...@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?
> >>
> >
> > 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
>
> <...>
