From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 2283A3230 for ; Mon, 25 Sep 2017 11:31:45 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP; 25 Sep 2017 02:31:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,435,1500966000"; d="scan'208";a="903549497" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by FMSMGA003.fm.intel.com with ESMTP; 25 Sep 2017 02:31:42 -0700 To: "Zhao1, Wei" Cc: "dev@dpdk.org" References: <1505282644-40415-1-git-send-email-wei.zhao1@intel.com> <1505445188-70251-1-git-send-email-wei.zhao1@intel.com> <1505445188-70251-2-git-send-email-wei.zhao1@intel.com> <0508ed6a-4301-de07-7bb1-a74a5ff2a871@intel.com> From: Ferruh Yigit Message-ID: <1109bef6-39c2-155e-f60d-a8f4147ba359@intel.com> Date: Mon, 25 Sep 2017 10:31:42 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Sep 2017 09:31:46 -0000 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 ; 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. <...>                                                                 > 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)); <...> >> > +                      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; <...> >> > +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. <...> >> > + >> > +      memset(&hw->local_dcbx_config, 0, >> > +      sizeof(struct i40e_dcbx_config)); >> >> Wrong indentation. Reminder of this one incase missed. <...> >> > +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. 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. > >> > +      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. >> > +                      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. <...> >> > @@ -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. <...> >> > +      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. <...> >> 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. >> >> > + * that port as the command otion type >> >> s/otion/options >> Reminder of this one incase missed. <...> >> > +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. <...>