[dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush
Zhao1, Wei
wei.zhao1 at intel.com
Mon Sep 25 09:40:56 CEST 2017
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.
> 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.zhao1 at intel.com<mailto:wei.zhao1 at 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;
> >
More information about the dev
mailing list