Re: [dpdk-dev] [PATCH v5] net/i40e: add interface to choose latest vector path

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/12/2018 11:12 AM, Xiaoyun Li wrote:
> Right now, vector path is limited to only use on later platform.

i40e supports vector instructions for intel, arm and powerpc,
this behavior is only for Intel vector drivers, can be good to clarify,
also it can better to explain a little more what "limited to only use on later
platform" means.

> This patch adds a devarg use-latest-vec to allow the users to
> use the latest vector path that the platform supported. Namely,
> using AVX2 vector path on broadwell is possible.

Again, this is for intel only, and can you put a matrix to clarify what is
supported:

no devarg:
Machine		PMD
avx512		avx2
avx2		sse4.2
sse4.2		sse4.2
< sse4.2	not supported

with devarg:
Machine		PMD
avx512		avx2
avx2		avx2
sse4.2		sse4.2
< sse4.2	not supported


And I am not sure about name of the devarg "use-latest-vec", I can see it has
been discussed already.
What about "use-latest-supported-vec"? Too verbose?
Do you have any other suggestion?

<...>

> @@ -163,6 +163,14 @@ Runtime Config Options
>    Currently hot-plugging of representor ports is not supported so all required
>    representors must be specified on the creation of the PF.
>  
> +- ``Use latest vector`` (default ``disable``)
> +
> +  Vector path was limited to use only on later platform. But users may want the
> +  latest vector path. For example, VPP users may want to use AVX2 vector path on HSW/BDW
> +  because it can get better perf. So ``devargs`` parameter ``use-latest-vec`` is
> +  introduced, for example::
> +    -w 84:00.0,use-latest-vec=1

Do we need to name a specific consumer of DPDK on i40e document? Why not say any
application?

> +
>  Driver compilation and testing
>  ------------------------------
>  
> diff --git a/doc/guides/rel_notes/release_18_11.rst b/doc/guides/rel_notes/release_18_11.rst
> index 3ae6b3f58..34af591a2 100644
> --- a/doc/guides/rel_notes/release_18_11.rst
> +++ b/doc/guides/rel_notes/release_18_11.rst
> @@ -54,6 +54,10 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =========================================================
>  
> +* **Added a devarg to use the latest vector path.**
> +  A new devarg ``use-latest-vec`` was introduced to allow users to choose
> +  the latest vector path that the platform supported. For example, VPP users
> +  can use AVX2 vector path on BDW/HSW to get better performance.

Same, do we need to name a specific consumer of DPDK on release notes?

<...>

> @@ -1201,6 +1203,46 @@ i40e_aq_debug_write_global_register(struct i40e_hw *hw,
>  	return i40e_aq_debug_write_register(hw, reg_addr, reg_val, cmd_details);
>  }
>  
> +static int
> +i40e_parse_latest_vec(struct rte_eth_dev *dev)
> +{
> +	struct i40e_adapter *ad =
> +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	int kvargs_count, use_latest_vec;
> +	struct rte_kvargs *kvlist;
> +
> +	ad->use_latest_vec = false;
> +
> +	if (!dev->device->devargs)
> +		return 0;
> +
> +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> +	if (!kvlist)
> +		return -EINVAL;

Agree with Qi to prevent rte_kvargs_parse() call for each devarg, in the future.

> +
> +	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_USE_LATEST_VEC);
> +	if (!kvargs_count) {
> +		rte_kvargs_free(kvlist);
> +		return 0;
> +	}
> +
> +	if (kvargs_count > 1)
> +		PMD_DRV_LOG(WARNING, "More than one argument \"%s\" and only "
> +			    "the first one is used !",
> +			    ETH_I40E_USE_LATEST_VEC);
> +
> +	use_latest_vec = atoi((&kvlist->pairs[0])->value);

Instead of accessing directly kvlist internals, please use rte_kvargs_process()

<...>

> @@ -12527,4 +12570,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
>  			      ETH_I40E_FLOATING_VEB_ARG "=1"
>  			      ETH_I40E_FLOATING_VEB_LIST_ARG "=<string>"
>  			      ETH_I40E_QUEUE_NUM_PER_VF_ARG "=1|2|4|8|16"
> -			      ETH_I40E_SUPPORT_MULTI_DRIVER "=1");
> +			      ETH_I40E_SUPPORT_MULTI_DRIVER "=1"
> +			      ETH_I40E_USE_LATEST_VEC "=1");

= 0|1 ?