[dpdk-dev,v3] net/i40e: determine number of queues per VF during run time
Checks
Commit Message
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(-)
Comments
> -----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?
> +
> + 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.
> +
> + /* 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.
> + 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.
> + 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.
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
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
@@ -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
@@ -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"
+
+ 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;
+
+ /* 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);
+ if (s == NULL)
+ return -(ENOMEM);
+ strcpy(s, dev->device->devargs->args);
+
+ 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)
+ 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",