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. > And is this feature only supported with RSS? Can it be part of RSS > configuration instead of PMD specific API? > Yes, only valid after rss enable.May be in the feature, in rte_flow mode, we can combine them together. > > This patch enable mapping between different priorities (UP) and > > User priorities (UP) oK, change in v4 > > > 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.zh...@intel.com<mailto:wei.zh...@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) { > > + PMD_INIT_LOG(ERR, "there is no that region id been > > set > before"); > > Can you please re-check the log message. oK, change in v4 > > > + return ret; > > + } > > + > > + memset(&ctxt, 0, sizeof(struct i40e_vsi_context)); > > + > > + /* Update Queue Pairs Mapping for currently enabled UPs */ > > + ctxt.seid = vsi->seid; > > + ctxt.pf_num = hw->pf_id; > > + ctxt.vf_num = 0; > > + ctxt.uplink_seid = vsi->uplink_seid; > > + ctxt.info = vsi->info; > > + vsi_info = &ctxt.info; > > + > > + memset(vsi_info->tc_mapping, 0, sizeof(uint16_t) * 8); > > + memset(vsi_info->queue_mapping, 0, sizeof(uint16_t) * 16); > > + > > + /** > > + * Configure queue region and queue mapping parameters, > > + * for enabled queue region, allocate queues to this region. > > + */ > > + > > + for (i = 0; i < region_info->queue_region_number; i++) { > > + tc_index = region_info->region[i].region_id; > > + bsf = rte_bsf32(region_info->region[i].queue_num); > > + queue_offset = > > region_info->region[i].queue_start_index; > > + vsi_info->tc_mapping[tc_index] = rte_cpu_to_le_16( > > + (queue_offset << > I40E_AQ_VSI_TC_QUE_OFFSET_SHIFT) | > > + (bsf << > I40E_AQ_VSI_TC_QUE_NUMBER_SHIFT)); > > + } > > + > > + /* Associate queue number with VSI, Keep vsi->nb_qps unchanged > */ > > + if (vsi->type == I40E_VSI_SRIOV) { > > + vsi_info->mapping_flags |= > > + > rte_cpu_to_le_16(I40E_AQ_VSI_QUE_MAP_NONCONTIG); > > + for (i = 0; i < vsi->nb_qps; i++) > > + vsi_info->queue_mapping[i] = > > + > > rte_cpu_to_le_16(vsi->base_queue + i); > > + } else { > > + vsi_info->mapping_flags |= > > + > rte_cpu_to_le_16(I40E_AQ_VSI_QUE_MAP_CONTIG); > > + vsi_info->queue_mapping[0] = rte_cpu_to_le_16(vsi- > >base_queue); > > + } > > + vsi_info->valid_sections |= > > + > rte_cpu_to_le_16(I40E_AQ_VSI_PROP_QUEUE_MAP_VALID); > > + > > + /* Update the VSI after updating the VSI queue-mapping > information */ > > + ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL); > > + if (ret) { > > + PMD_INIT_LOG(ERR, "Failed to configure queue region " > > + "mapping = %d ", > > hw->aq.asq_last_status); > > Please don't split log message. oK, change in v4 > > > + return ret; > > + } > > + /* update the local VSI info with updated queue map */ > > + (void)rte_memcpy(&vsi->info.tc_mapping, &ctxt.info.tc_mapping, > > + > > 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. > > > + vsi->info.mapping_flags = ctxt.info.mapping_flags; > > + vsi->info.valid_sections = 0; > > + > > + return 0; > > +} > > + > > + > > +static int > > +i40e_set_queue_region(struct i40e_pf *pf, > > + struct > > rte_i40e_rss_region_conf *conf_ptr) { > > + uint16_t tc_size_tb[7] = {1, 2, 4, 8, 16, 32, 64}; > > + uint16_t i; > > + struct i40e_vsi *main_vsi = pf->main_vsi; > > + struct i40e_queue_region_info *info = &pf->queue_region; > > + uint32_t ret = -EINVAL; > > + > > + for (i = 0; i < I40E_REGION_MAX_INDEX; i++) > > + if (conf_ptr->queue_num == tc_size_tb[i]) > > + break; > > + > > + if (i == I40E_REGION_MAX_INDEX) { > > + printf("The region sizes should be any of the > > following " > > + "values: 1, 2, 4, 8, 16, 32, 64 as long as the " > > + "total number of queues do not exceed the VSI > > allocation"); > > + return ret; > > + } > > + > > + if (conf_ptr->region_id >= I40E_REGION_MAX_INDEX) { > > + PMD_INIT_LOG(ERR, "the queue region max index is 7"); > > + return ret; > > + } > > + > > + if ((conf_ptr->queue_start_index + conf_ptr->queue_num) > > + > > > main_vsi->nb_used_qps) { > > + PMD_INIT_LOG(ERR, "the queue index exceeds the VSI > range"); > > + return ret; > > + } > > + > > + if (info->queue_region_number) { > > + for (i = 0; i < info->queue_region_number; i++) > > + if (conf_ptr->region_id == > > info->region[i].region_id) > > + break; > > + > > + 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) { ........................... } } > > > + info->region[i].region_id = > > conf_ptr->region_id; > > + info->region[i].queue_num = > > conf_ptr->queue_num; > > + info->region[i].queue_start_index = > > + > > conf_ptr->queue_start_index; > > + info->queue_region_number++; > > + } else { > > + PMD_INIT_LOG(ERR, "queue region > > number " > > + "exceeds maxnum 8 or > > the " > > + "queue region id has > > " > > + "been set before"); > > please don't split log message. oK, change in v4 > > > + return ret; > > + } > > + } else { > > + info->region[0].region_id = conf_ptr->region_id; > > + info->region[0].queue_num = conf_ptr->queue_num; > > + info->region[0].queue_start_index = > > + conf_ptr->queue_start_index; > > + info->queue_region_number++; > > + } > > + > > + return 0; > > +} > > + > > +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 of i40e_set_ as prefix? Because many set is not related directly to queue region. > > > +{ > > + uint32_t pfqf_hregion; > > + uint32_t ret = -EINVAL; > > + struct i40e_queue_region_info *info = &pf->queue_region; > > + uint16_t i, j, index, flowtype_set = 0; > > + uint16_t region_index, flowtype_index; > > + > > + if (conf_ptr->region_id > I40E_PFQF_HREGION_MAX_INDEX) { > > + PMD_INIT_LOG(ERR, "the queue region max index is 7"); > > + return ret; > > + } > > + > > + if (conf_ptr->hw_flowtype >= I40E_FILTER_PCTYPE_MAX) { > > + PMD_INIT_LOG(ERR, "the hw_flowtype or PCTYPE max > > index > is 63"); > > + return ret; > > + } > > + > > + if (info->queue_region_number) { > > + for (i = 0; i < info->queue_region_number; i++) > > + if (conf_ptr->region_id == > > info->region[i].region_id) > > + break; > > + > > + if (i == info->queue_region_number) { > > + PMD_INIT_LOG(ERR, "that region id > > has not " > > + "been set before"); > > + return ret; > > + } > > + region_index = i; > > + > > + for (i = 0; i < info->queue_region_number; i++) { > > + for (j = 0; j < > > info->region[i].flowtype_num; j++) { > > + if > > (conf_ptr->hw_flowtype == > > + > > info->region[i].hw_flowtype[j]) { > > + > > flowtype_set = 1; > > + > > break; > > + } > > + } > > + } > > + > > + if (flowtype_set) { > > + PMD_INIT_LOG(ERR, "that hw_flowtype " > > + "has been set > > before"); > > Please don't split log messages. Ok. > > > + return ret; > > + } > > + flowtype_index = > > info->region[region_index].flowtype_num; > > + > > info->region[region_index].hw_flowtype[flowtype_index] = > > + > > conf_ptr->hw_flowtype; > > + info->region[region_index].flowtype_num++; > > + } else { > > + PMD_INIT_LOG(ERR, "there is no that region id been > > set > before"); > > + return ret; > > + } > > + > > + index = conf_ptr->hw_flowtype >> 3; > > + pfqf_hregion = i40e_read_rx_ctl(hw, I40E_PFQF_HREGION(index)); > > + > > + if ((conf_ptr->hw_flowtype & 0x7) == 0) { > > + pfqf_hregion |= conf_ptr->region_id << > > + > > I40E_PFQF_HREGION_REGION_0_SHIFT; > > + pfqf_hregion |= 1 << > I40E_PFQF_HREGION_OVERRIDE_ENA_0_SHIFT; > > + } else if ((conf_ptr->hw_flowtype & 0x7) == 1) { > > + pfqf_hregion |= conf_ptr->region_id << > > + > > I40E_PFQF_HREGION_REGION_1_SHIFT; > > + pfqf_hregion |= 1 << > I40E_PFQF_HREGION_OVERRIDE_ENA_1_SHIFT; > > + } else if ((conf_ptr->hw_flowtype & 0x7) == 2) { > > + pfqf_hregion |= conf_ptr->region_id << > > + > > I40E_PFQF_HREGION_REGION_2_SHIFT; > > + pfqf_hregion |= 1 << > I40E_PFQF_HREGION_OVERRIDE_ENA_2_SHIFT; > > + } else if ((conf_ptr->hw_flowtype & 0x7) == 3) { > > + pfqf_hregion |= conf_ptr->region_id << > > + > > I40E_PFQF_HREGION_REGION_3_SHIFT; > > + pfqf_hregion |= 1 << > I40E_PFQF_HREGION_OVERRIDE_ENA_3_SHIFT; > > + } else if ((conf_ptr->hw_flowtype & 0x7) == 4) { > > + pfqf_hregion |= conf_ptr->region_id << > > + > > I40E_PFQF_HREGION_REGION_4_SHIFT; > > + pfqf_hregion |= 1 << > I40E_PFQF_HREGION_OVERRIDE_ENA_4_SHIFT; > > + } else if ((conf_ptr->hw_flowtype & 0x7) == 5) { > > + pfqf_hregion |= conf_ptr->region_id << > > + > > I40E_PFQF_HREGION_REGION_5_SHIFT; > > + pfqf_hregion |= 1 << > I40E_PFQF_HREGION_OVERRIDE_ENA_5_SHIFT; > > + } else if ((conf_ptr->hw_flowtype & 0x7) == 6) { > > + pfqf_hregion |= conf_ptr->region_id << > > + > > I40E_PFQF_HREGION_REGION_6_SHIFT; > > + pfqf_hregion |= 1 << > I40E_PFQF_HREGION_OVERRIDE_ENA_6_SHIFT; > > + } else { > > + pfqf_hregion |= conf_ptr->region_id << > > + > > I40E_PFQF_HREGION_REGION_7_SHIFT; > > + pfqf_hregion |= 1 << > I40E_PFQF_HREGION_OVERRIDE_ENA_7_SHIFT; > > + } > > + > > + i40e_write_rx_ctl(hw, I40E_PFQF_HREGION(index), pfqf_hregion); > > + > > + return 0; > > +} > > + > > +static int > > +i40e_set_up_region(struct i40e_pf *pf, > > + struct > > rte_i40e_rss_region_conf *conf_ptr) > > The API name looks like "setup" but that up part is user_priority, can you > please long form against "up"? Ok, I will change to “user_priority” instead of “up” > > > +{ > > + struct i40e_queue_region_info *info = &pf->queue_region; > > + uint32_t ret = -EINVAL; > > + uint16_t i, j, region_index, up_set = 0; > > Same abrevation comment for "upset", please use long form. > > > + > > + if (conf_ptr->user_priority > > I40E_REGION_USERPRIORITY_MAX_INDEX) { > > + PMD_INIT_LOG(ERR, "the queue region max index is 7"); > > + return ret; > > + } > > + > > + if (conf_ptr->region_id >= I40E_REGION_MAX_INDEX) { > > + PMD_INIT_LOG(ERR, "the region_id max index is 7"); > > + return ret; > > + } > > + > > + if (info->queue_region_number) { > > + for (i = 0; i < info->queue_region_number; i++) > > + if (conf_ptr->region_id == > > info->region[i].region_id) > > + break; > > + > > + if (i == info->queue_region_number) { > > + PMD_INIT_LOG(ERR, "that region id " > > + "has not been set > > before"); > > + return ret; > > + } > > + region_index = i; > > + > > + for (i = 0; i < info->queue_region_number; i++) { > > + for (j = 0; j < > > + > > info->region[i].user_priority_num; j++) { > > Please break line after ";" Ok. > > > + if > > (info->region[i].user_priority[j] == > > + > > conf_ptr->user_priority) { > > + > > up_set = 1; > > + > > break; > > + } > > + } > > + } > > + > > + if (up_set) { > > + PMD_INIT_LOG(ERR, "that user > > priority " > > + "has been set > > before"); > > + return ret; > > + } > > + > > + j = info->region[region_index].user_priority_num; > > + info->region[region_index].user_priority[j] = > > + > > conf_ptr->user_priority; > > + info->region[region_index].user_priority_num++; > > + } else { > > + PMD_INIT_LOG(ERR, "there is no that region id been > > set > before"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int > > +i40e_queue_region_dcb_configure(struct i40e_hw *hw, > > + struct i40e_pf *pf) > > +{ > > + struct i40e_dcbx_config dcb_cfg_local; > > + struct i40e_dcbx_config *dcb_cfg; > > + struct i40e_queue_region_info *info = &pf->queue_region; > > + struct i40e_dcbx_config *old_cfg = &hw->local_dcbx_config; > > + uint32_t ret = -EINVAL; > > Please use signed variable, there are more above. Ok > > > + uint16_t i, j, prio_index, region_index; > > + uint8_t tc_map, tc_bw, bw_lf; > > + > > + if (!info->queue_region_number) { > > + PMD_INIT_LOG(ERR, "there is no that region id been > > set > before"); > > + return ret; > > + } > > + > > + dcb_cfg = &dcb_cfg_local; > > + memset(dcb_cfg, 0, sizeof(struct i40e_dcbx_config)); > > + > > + /* assume each tc has the same bw */ > > + tc_bw = I40E_MAX_PERCENT / info->queue_region_number; > > + for (i = 0; i < info->queue_region_number; i++) > > + dcb_cfg->etscfg.tcbwtable[i] = tc_bw; > > + /* to ensure the sum of tcbw is equal to 100 */ > > + bw_lf = I40E_MAX_PERCENT % info->queue_region_number; > > + for (i = 0; i < bw_lf; i++) > > + dcb_cfg->etscfg.tcbwtable[i]++; > > + > > + /* assume each tc has the same Transmission Selection Algorithm */ > > + for (i = 0; i < info->queue_region_number; i++) > > + dcb_cfg->etscfg.tsatable[i] = I40E_IEEE_TSA_ETS; > > + > > + for (i = 0; i < info->queue_region_number; i++) { > > + for (j = 0; j < info->region[i].user_priority_num; > > j++) { > > + prio_index = > > info->region[i].user_priority[j]; > > + region_index = > > info->region[i].region_id; > > + > > dcb_cfg->etscfg.prioritytable[prio_index] = > > + > > region_index; > > + } > > + } > > + > > + /* FW needs one App to configure HW */ > > + dcb_cfg->numapps = I40E_DEFAULT_DCB_APP_NUM; > > + dcb_cfg->app[0].selector = I40E_APP_SEL_ETHTYPE; > > + dcb_cfg->app[0].priority = I40E_DEFAULT_DCB_APP_PRIO; > > + dcb_cfg->app[0].protocolid = I40E_APP_PROTOID_FCOE; > > + > > + tc_map = RTE_LEN2MASK(info->queue_region_number, uint8_t); > > + > > + dcb_cfg->pfc.willing = 0; > > + dcb_cfg->pfc.pfccap = I40E_MAX_TRAFFIC_CLASS; > > + dcb_cfg->pfc.pfcenable = tc_map; > > + > > + /* Copy the new config to the current config */ > > + *old_cfg = *dcb_cfg; > > + old_cfg->etsrec = old_cfg->etscfg; > > + ret = i40e_set_dcb_config(hw); > > + > > + if (ret) { > > + PMD_INIT_LOG(ERR, "Set queue region DCB Config > > failed, > err" > > + " %s aq_err %s", > > Please don't split the log message. Ok > > > + i40e_stat_str(hw, ret), > > + i40e_aq_str(hw, > > hw->aq.asq_last_status)); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int > > +i40e_queue_region_dcb_configure_default(struct i40e_hw *hw) { > > + uint16_t i; > > + uint32_t ret = -EINVAL; > > Please use signed variable here. Ok > > > + > > + memset(&hw->local_dcbx_config, 0, > > + sizeof(struct i40e_dcbx_config)); > > Wrong indentation. > > > + /* set dcb default configuration */ > > + hw->local_dcbx_config.etscfg.willing = 0; > > + hw->local_dcbx_config.etscfg.maxtcs = 0; > > + hw->local_dcbx_config.etscfg.tcbwtable[0] = 100; > > + hw->local_dcbx_config.etscfg.tsatable[0] = > > + I40E_IEEE_TSA_ETS; > > + /* all UPs mapping to region 0 */ > > + for (i = 0; i < I40E_MAX_USER_PRIORITY; i++) > > + hw->local_dcbx_config.etscfg.prioritytable[i] = 0; > > + hw->local_dcbx_config.etsrec = > > + hw->local_dcbx_config.etscfg; > > + hw->local_dcbx_config.pfc.willing = 0; > > + hw->local_dcbx_config.pfc.pfccap = > > + I40E_MAX_TRAFFIC_CLASS; > > + /* FW needs one App to configure HW */ > > + hw->local_dcbx_config.numapps = 1; > > + hw->local_dcbx_config.app[0].selector = > > + I40E_APP_SEL_ETHTYPE; > > + hw->local_dcbx_config.app[0].priority = 3; > > + hw->local_dcbx_config.app[0].protocolid = > > + I40E_APP_PROTOID_FCOE; > > + ret = i40e_set_dcb_config(hw); > > + if (ret) { > > + PMD_INIT_LOG(ERR, > > + "default dcb config fails. err = %d, aq_err = %d.", > > + ret, hw->aq.asq_last_status); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +int > > +i40e_flush_region_all_conf(struct i40e_hw *hw, struct i40e_pf *pf, > > + uint16_t on) > > +{ > > + uint16_t i; > > + struct i40e_queue_region_info *info = &pf->queue_region; > > + > > + if (on) { > > + i40e_vsi_update_queue_region_mapping(hw, pf); > > + i40e_queue_region_dcb_configure(hw, pf); > > + return 0; > > + } > > + > > + info->queue_region_number = 1; > > + info->region[0].queue_num = 64; > > + info->region[0].queue_start_index = 0; > > + > > + i40e_vsi_update_queue_region_mapping(hw, pf); > > + i40e_queue_region_dcb_configure_default(hw); > > + > > + for (i = 0; i < I40E_PFQF_HREGION_MAX_INDEX; i++) > > + i40e_write_rx_ctl(hw, I40E_PFQF_HREGION(i), 0); > > + > > + memset(info, 0, sizeof(struct i40e_queue_region_info)); > > + > > + return 0; > > +} > > + > > +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. > > > + 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; > > Does it make sense to add another type to get the current queue region > config? > > > + 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? > > > + 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. > > > + break; > > + > > + default: > > + PMD_DRV_LOG(WARNING, "op type (%d) not supported", > > + op_type); > > Line break not required. Ok, delete in v4 > > > + ret = -EINVAL; > > + break; > > + } > > + > > + I40E_WRITE_FLUSH(hw); > > + > > + return ret; > > +} > > diff --git a/drivers/net/i40e/rte_pmd_i40e.h > > b/drivers/net/i40e/rte_pmd_i40e.h index 155b7e8..a1329f4 100644 > > --- a/drivers/net/i40e/rte_pmd_i40e.h > > +++ b/drivers/net/i40e/rte_pmd_i40e.h > > @@ -91,6 +91,20 @@ enum rte_pmd_i40e_package_info { > > RTE_PMD_I40E_PKG_INFO_MAX = 0xFFFFFFFF }; > > > > +/** > > + * Option types of queue region. > > + */ > > +enum rte_pmd_i40e_queue_region_op { > > + RTE_PMD_I40E_REGION_UNDEFINED = 0, > > No need to set to zero, it is default value. Ok > > > + RTE_PMD_I40E_QUEUE_REGION_SET, /**< add queue region > set*/ > > I would suggest using same namespace for the enum ites, there are > "RTE_PMD_I40E_REGION" and "RTE_PMD_I40E_QUEUE_REGION". > "RTE_PMD_I40E_QUEUE_REGION" looks better. Or if it makes sense, > perhaps "RTE_PMD_I40E_RSS_QUEUE_REGION"? But please stick with one. > > > + RTE_PMD_I40E_REGION_FLOWTYPE_PF_SET, /**< add pf region > pctype set */ > > + RTE_PMD_I40E_REGION_FLOWTYPE_VF_SET, /**< add vf region > pctype set */ > > + RTE_PMD_I40E_UP_REGION_SET, /**< add queue region pctype > set */ > > is this comment correct, this type should be for user priority Ok, fix in v4 > > > + RTE_PMD_I40E_REGION_ALL_FLUSH_ON, /**< flush on all > configuration */ > > + RTE_PMD_I40E_REGION_ALL_FLUSH_OFF, /**< flush off all > configuration */ > > + RTE_PMD_I40E_QUEUE_REGION_OP_MAX > > +}; > > + > > #define RTE_PMD_I40E_DDP_NAME_SIZE 32 > > > > /** > > @@ -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 > > > + */ > > +struct rte_i40e_rss_region_conf { > > This is now public struct, can you please comment items. Ok. Add in v4 > > > + uint8_t region_id; > > + uint8_t hw_flowtype; > > + uint8_t queue_start_index; > > + uint8_t queue_num; > > + 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” > > > +}; > > + > > +/** > > * Notify VF when PF link status changes. > > * > > * @param port > > @@ -657,4 +683,16 @@ int > 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 > > > + * that port as the command otion type > > s/otion/options > > > + * > > + * @param port > > + * pointer to port identifier of the device > > Not pointer. > Also this will conflict with patch that updates port storage to uint16_t. Can Ok, change in v4 > you please follow the status of the patch, and if this merged after that one, > please ping to remind to update the storage. Ok ，I will commit a new v4 to fix these , then remind you of the new version > > > + * @param conf_ptr > > + * pointer to the struct that contain all the > > + * region configuration parameters > > + */ > > +int rte_pmd_i40e_queue_region_conf(uint8_t port, > > Can you please use port_id as variable name to be consistent. > > > + struct rte_i40e_rss_region_conf *conf_ptr); > > conf_ptr variable name doesn't say much, although it is OK in this context. I > would suggest something like rss_region_conf Ok. > > > #endif /* _PMD_I40E_H_ */ > > diff --git a/drivers/net/i40e/rte_pmd_i40e_version.map > > b/drivers/net/i40e/rte_pmd_i40e_version.map > > index ef8882b..c3ee2da 100644 > > --- a/drivers/net/i40e/rte_pmd_i40e_version.map > > +++ b/drivers/net/i40e/rte_pmd_i40e_version.map > > @@ -50,5 +50,6 @@ DPDK_17.11 { > > global: > > > > rte_pmd_i40e_add_vf_mac_addr; > > + rte_pmd_i40e_queue_region_conf; > > The feature has been mention as "rss queue region" above a few places, do > you think rte_pmd_i40e_rss_queue_region_conf() or > rte_pmd_i40e_queue_region_conf() is better for API name? Ok, I will change to rte_pmd_i40e_rss_queue_region_conf > > > > > } DPDK_17.08; > >
