From: "Dai, Wei" <wei.dai@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
"Wu, Jingjing" <jingjing.wu@intel.com>,
"Xing, Beilei" <beilei.xing@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3] net/i40e: determine number of queues per VF during run time
Date: Wed, 22 Nov 2017 02:46:50 +0000 [thread overview]
Message-ID: <49759EB36A64CF4892C1AFEC9231E8D65156543C@PGSMSX112.gar.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB9772585FABEAC3@irsmsx105.ger.corp.intel.com>
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 <wei.dai@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Dai, Wei <wei.dai@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v3] net/i40e: determine number of queues
> per VF during run time
>
>
>
> > -----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 <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; Dai, Wei <wei.dai@intel.com>
> > 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=4 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=8
> > , 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 <wei.dai@intel.com>
> >
> > ---
> > 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=y
> > CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=y
> > CONFIG_RTE_LIBRTE_I40E_16BYTE_RX_DESC=n
> > CONFIG_RTE_LIBRTE_I40E_QUEUE_NUM_PER_PF=64
> > -CONFIG_RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF=4
> > CONFIG_RTE_LIBRTE_I40E_QUEUE_NUM_PER_VM=4
> > # interval up to 8160 us, aligned to 2 (or default value)
> > CONFIG_RTE_LIBRTE_I40E_ITR_INTERVAL=-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"
>
> 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.
>
> > +
> > + struct i40e_pf *pf =
> 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;
>
> 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.
>
> > +
> > + /* set default queue number per VF as 4 */
> > + pf->vf_nb_qp_max = RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF;
> > +
> > + if (dev->device->devargs == NULL)
> > + return 0;
> > +
> > + s = rte_zmalloc(__func__, strlen(dev->device->devargs->args) + 1,
> > +0);
>
> 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.
>
>
> > + if (s == NULL)
> > + return -(ENOMEM);
> > + strcpy(s, dev->device->devargs->args);
> > +
>
> 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 rte_kvargs.
>
> > + ps0 = s;
> > + do {
> > + while (isblank(*ps0))
> > + ps0++;
> > + pv = strchr(ps0, '=');
> > + if (pv == NULL)
> > + break;
> > + ps1 = pv - 1;
> > + *pv++ = 0;
> > +
> > + while (isblank(*ps1))
> > + *ps1-- = 0;
> > +
> > + if (!strcmp(ps0, I40E_QUEUE_NUM_PER_VF_ARG)) {
> > + errno = 0;
> > + value = strtoul(pv, &end, 0);
> > + if (errno != 0 || end == pv)
>
> That wouldn't catch something like ...=1blah, Should be *end != '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.
>
> Konstantin
>
> > + break;
> > + if (value <= 16 && rte_is_power_of_2(value))
> > + pf->vf_nb_qp_max = (uint16_t)value;
> > + else
> > + PMD_DRV_LOG(ERR, "Wrong VF queue number = %lu,"
> > + " it must be power of 2 and equal or"
> > + " less than 16 !", value);
> > + break;
> > + }
> > + ps0 = strchr(pv, ',');
> > + if (ps0 == 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 = I40E_DEFAULT_PAUSE_TIME;
> > pf->fc_conf.high_water[I40E_MAX_TRAFFIC_CLASS] =
> > I40E_DEFAULT_HIGH_WATER; @@ -3992,7 +4056,6 @@
> i40e_pf_parameter_init(struct rte_eth_dev *dev)
> > pf->max_num_vsi = hw->func_caps.num_vsis;
> > pf->lan_nb_qp_max = RTE_LIBRTE_I40E_QUEUE_NUM_PER_PF;
> > pf->vmdq_nb_qp_max = RTE_LIBRTE_I40E_QUEUE_NUM_PER_VM;
> > - pf->vf_nb_qp_max = RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF;
> >
> > /* FDir queue/VSI allocation */
> > pf->fdir_qp_offset = 0;
> > @@ -4022,7 +4085,7 @@ i40e_pf_parameter_init(struct rte_eth_dev
> *dev)
> > pf->vf_qp_offset = pf->lan_qp_offset + pf->lan_nb_qps;
> > if (hw->func_caps.sr_iov_1_1 && pci_dev->max_vfs) {
> > pf->flags |= I40E_FLAG_SRIOV;
> > - pf->vf_nb_qps = RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF;
> > + pf->vf_nb_qps = pf->vf_nb_qp_max;
> > pf->vf_num = pci_dev->max_vfs;
> > PMD_DRV_LOG(DEBUG,
> > "%u VF VSIs, %u queues per VF VSI, in total %u queues",
> > --
> > 2.7.5
next prev parent reply other threads:[~2017-11-22 2:46 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-19 7:43 [dpdk-dev] [PATCH v2] " Wei Dai
2017-11-19 14:08 ` [dpdk-dev] [PATCH v3] " Wei Dai
2017-11-20 11:32 ` Ananyev, Konstantin
2017-11-22 2:46 ` Dai, Wei [this message]
2017-11-22 14:10 ` [dpdk-dev] [PATCH v4] " Wei Dai
2017-11-24 6:12 ` [dpdk-dev] [PATCH v5] " Wei Dai
2017-11-27 8:08 ` [dpdk-dev] [PATCH v6] " Wei Dai
2017-12-03 11:19 ` Ananyev, Konstantin
2017-12-08 1:53 ` [dpdk-dev] [PATCH v7] " Wei Dai
2017-12-11 2:33 ` Peng, Yuan
2017-12-20 5:59 ` Zhang, Helin
2017-12-25 11:45 ` [dpdk-dev] [PATCH v8] " Wei Dai
2017-12-26 15:24 ` [dpdk-dev] [PATCH v9] " Wei Dai
2018-01-08 6:00 ` Zhang, Helin
2018-01-09 9:09 ` Dai, Wei
2018-01-08 13:23 ` Ferruh Yigit
2018-01-09 8:56 ` [dpdk-dev] [PATCH v10] " Wei Dai
2018-01-09 14:36 ` Zhang, Helin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=49759EB36A64CF4892C1AFEC9231E8D65156543C@PGSMSX112.gar.corp.intel.com \
--to=wei.dai@intel.com \
--cc=beilei.xing@intel.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.com \
--cc=konstantin.ananyev@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).