From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (unknown [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id BA1FD23A for ; Wed, 22 Nov 2017 03:46:55 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Nov 2017 18:46:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,435,1505804400"; d="scan'208";a="4584624" Received: from pgsmsx107.gar.corp.intel.com ([10.221.44.105]) by fmsmga004.fm.intel.com with ESMTP; 21 Nov 2017 18:46:53 -0800 Received: from pgsmsx112.gar.corp.intel.com ([169.254.3.145]) by PGSMSX107.gar.corp.intel.com ([169.254.7.200]) with mapi id 14.03.0319.002; Wed, 22 Nov 2017 10:46:52 +0800 From: "Dai, Wei" To: "Ananyev, Konstantin" , "Wu, Jingjing" , "Xing, Beilei" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v3] net/i40e: determine number of queues per VF during run time Thread-Index: AQHTYfNAkjd1TIR7vUqE3rZqREKgoaMfr9SA Date: Wed, 22 Nov 2017 02:46:50 +0000 Message-ID: <49759EB36A64CF4892C1AFEC9231E8D65156543C@PGSMSX112.gar.corp.intel.com> References: <1511077425-27468-1-git-send-email-wei.dai@intel.com> <1511100484-42264-1-git-send-email-wei.dai@intel.com> <2601191342CEEE43887BDE71AB9772585FABEAC3@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB9772585FABEAC3@irsmsx105.ger.corp.intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzkzODU2NmItZGQyNi00ZjBmLTg4MzctMDYwZDlmOGMzNzc2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IlJxcjY2bzZPREZnbWRHVWx2clVtdHRoZGNJNVFIcTh1bGl1bmpzRXFXWGM9In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [172.30.20.206] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3] net/i40e: determine number of queues per VF during run time 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: Wed, 22 Nov 2017 02:46:56 -0000 Thank you, Konstantin for your quick feedback. Please see my inline comments below. > -----Original Message----- > From: Ananyev, Konstantin > Sent: Monday, November 20, 2017 7:32 PM > To: Dai, Wei ; Wu, Jingjing ; > Xing, Beilei > Cc: dev@dpdk.org; Dai, Wei > Subject: RE: [dpdk-dev] [PATCH v3] net/i40e: determine number of queues > per VF during run time >=20 >=20 >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Dai > > Sent: Sunday, November 19, 2017 2:08 PM > > To: Wu, Jingjing ; Xing, Beilei > > > > Cc: dev@dpdk.org; Dai, Wei > > Subject: [dpdk-dev] [PATCH v3] net/i40e: determine number of queues > > per VF during run time > > > > Without this patch, the number of queues per i40e VF is defined as 4 > > by CONFIG_RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF=3D4 in > config/common_base. > > It is fixed value determined in building time and can't be changed > > during run time. > > With this patch, the number of queues per i40e VF can be determinated > > during run time. For example, if the PCI address of an i40e VF is > > aaaa:bb.cc, with the EAL parameter -w > > aaaa:bb.cc,i40e-queue-num-per-vf=3D8 > > , the number of queues per VF is 8. > > If there is no "i40e-queue-num-per-vf" setting in EAL parameters, it > > is 4 by default as before. And if the value after the > > "i40e-queue-num-per-vf" is invalid, it is set as 4 forcibly. > > The valid values include 1, 2, 4, 8, 16 . > > > > Signed-off-by: Wei Dai > > > > --- > > v3: fix WARNING of coding style issues from checkpatch@dpdk.org > > v2: fix WARNING of coding style issues from checkpatch@dpdk.org > > --- > > config/common_base | 1 - > > drivers/net/i40e/i40e_ethdev.c | 67 > > ++++++++++++++++++++++++++++++++++++++++-- > > 2 files changed, 65 insertions(+), 3 deletions(-) > > > > diff --git a/config/common_base b/config/common_base index > > e74febe..4e20389 100644 > > --- a/config/common_base > > +++ b/config/common_base > > @@ -208,7 +208,6 @@ > CONFIG_RTE_LIBRTE_I40E_RX_ALLOW_BULK_ALLOC=3Dy > > CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=3Dy > > CONFIG_RTE_LIBRTE_I40E_16BYTE_RX_DESC=3Dn > > CONFIG_RTE_LIBRTE_I40E_QUEUE_NUM_PER_PF=3D64 > > -CONFIG_RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF=3D4 > > CONFIG_RTE_LIBRTE_I40E_QUEUE_NUM_PER_VM=3D4 > > # interval up to 8160 us, aligned to 2 (or default value) > > CONFIG_RTE_LIBRTE_I40E_ITR_INTERVAL=3D-1 > > diff --git a/drivers/net/i40e/i40e_ethdev.c > > b/drivers/net/i40e/i40e_ethdev.c index 811cc9f..b23a39e 100644 > > --- a/drivers/net/i40e/i40e_ethdev.c > > +++ b/drivers/net/i40e/i40e_ethdev.c > > @@ -3971,6 +3971,67 @@ i40e_get_cap(struct i40e_hw *hw) > > return ret; > > } > > > > +static int i40e_pf_config_vf_rxq_number(struct rte_eth_dev *dev) { > > +#define RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF 4 > > +#define I40E_QUEUE_NUM_PER_VF_ARG > "i40e-queue-num-per-vf" >=20 > Does it need to have i40e prefix? > Provably other HW type would support similar devarg? Yes, I can't be sure if other HW type support this. Will remove i40e prefix in next version of patch. >=20 > > + > > + struct i40e_pf *pf =3D > I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > > + char *s; > > + char *ps0, *ps1, *pv; > > + char *end; > > + unsigned long value; > > + > > + if (!is_i40e_supported(dev)) > > + return -ENOTSUP; >=20 > Not a big deal - but that function is static, so I suppose the check is > redundant. Yes, this is a static function, so it can't be called by other type of NIC. I add this in order to prevent it being called by other NIC. Now I see it is unnecessary. I will remove it in next version of patch. >=20 > > + > > + /* set default queue number per VF as 4 */ > > + pf->vf_nb_qp_max =3D RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF; > > + > > + if (dev->device->devargs =3D=3D NULL) > > + return 0; > > + > > + s =3D rte_zmalloc(__func__, strlen(dev->device->devargs->args) + 1, > > +0); >=20 > No need to allocate temp memory from hugepages here. > Normal malloc, or even alloca() would do, I think. Yes, no need to allocate memory from hugepage. I will use normal malloc/freee instead of rte_zmalloc/rte_free. >=20 >=20 > > + if (s =3D=3D NULL) > > + return -(ENOMEM); > > + strcpy(s, dev->device->devargs->args); > > + >=20 > What is the point to write your own parsing function from scratch? > Why just not use rte_kvargs API as most other PMDs do? > BTW, i40e does the same in few other places. Thanks for your guidance. I will use rte_kvargs in next version of patch. I write my own parsing function only for performance. As it is only in control path and for SW maintenance, it is better to use r= te_kvargs. >=20 > > + ps0 =3D s; > > + do { > > + while (isblank(*ps0)) > > + ps0++; > > + pv =3D strchr(ps0, '=3D'); > > + if (pv =3D=3D NULL) > > + break; > > + ps1 =3D pv - 1; > > + *pv++ =3D 0; > > + > > + while (isblank(*ps1)) > > + *ps1-- =3D 0; > > + > > + if (!strcmp(ps0, I40E_QUEUE_NUM_PER_VF_ARG)) { > > + errno =3D 0; > > + value =3D strtoul(pv, &end, 0); > > + if (errno !=3D 0 || end =3D=3D pv) >=20 > That wouldn't catch something like ...=3D1blah, Should be *end !=3D '0' (= or > whatever your stop-character is). > Anyway would be much better to use kvargs. > Even better to have a common function, something like: > int devargs_get_uint(const struct rte_devargs *devargs, const char > *name,uint64_t *value); and use it in all appropriate places. Will follow your guidance in next version of patch. >=20 > Konstantin >=20 > > + break; > > + if (value <=3D 16 && rte_is_power_of_2(value)) > > + pf->vf_nb_qp_max =3D (uint16_t)value; > > + else > > + PMD_DRV_LOG(ERR, "Wrong VF queue number =3D %lu," > > + " it must be power of 2 and equal or" > > + " less than 16 !", value); > > + break; > > + } > > + ps0 =3D strchr(pv, ','); > > + if (ps0 =3D=3D NULL) > > + break; > > + ps0++; > > + } while (1); > > + > > + rte_free(s); > > + return 0; > > +} > > + > > static int > > i40e_pf_parameter_init(struct rte_eth_dev *dev) { @@ -3983,6 > +4044,9 > > @@ i40e_pf_parameter_init(struct rte_eth_dev *dev) > > PMD_INIT_LOG(ERR, "HW configuration doesn't support SRIOV"); > > return -EINVAL; > > } > > + > > + i40e_pf_config_vf_rxq_number(dev); > > + > > /* Add the parameter init for LFC */ > > pf->fc_conf.pause_time =3D I40E_DEFAULT_PAUSE_TIME; > > pf->fc_conf.high_water[I40E_MAX_TRAFFIC_CLASS] =3D > > I40E_DEFAULT_HIGH_WATER; @@ -3992,7 +4056,6 @@ > i40e_pf_parameter_init(struct rte_eth_dev *dev) > > pf->max_num_vsi =3D hw->func_caps.num_vsis; > > pf->lan_nb_qp_max =3D RTE_LIBRTE_I40E_QUEUE_NUM_PER_PF; > > pf->vmdq_nb_qp_max =3D RTE_LIBRTE_I40E_QUEUE_NUM_PER_VM; > > - pf->vf_nb_qp_max =3D RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF; > > > > /* FDir queue/VSI allocation */ > > pf->fdir_qp_offset =3D 0; > > @@ -4022,7 +4085,7 @@ i40e_pf_parameter_init(struct rte_eth_dev > *dev) > > pf->vf_qp_offset =3D pf->lan_qp_offset + pf->lan_nb_qps; > > if (hw->func_caps.sr_iov_1_1 && pci_dev->max_vfs) { > > pf->flags |=3D I40E_FLAG_SRIOV; > > - pf->vf_nb_qps =3D RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF; > > + pf->vf_nb_qps =3D pf->vf_nb_qp_max; > > pf->vf_num =3D pci_dev->max_vfs; > > PMD_DRV_LOG(DEBUG, > > "%u VF VSIs, %u queues per VF VSI, in total %u queues", > > -- > > 2.7.5