> -----Original Message-----
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Wei Dai
> Sent: Sunday, November 19, 2017 2:08 PM
> To: Wu, Jingjing <jingjing...@intel.com>; Xing, Beilei <beilei.x...@intel.com>
> Cc: dev@dpdk.org; Dai, Wei <wei....@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....@intel.com>
> 
> ---
> v3: fix WARNING of coding style issues from checkpa...@dpdk.org
> v2: fix WARNING of coding style issues from checkpa...@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

Reply via email to