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

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

Will send new version later. Thanks.

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, September 13, 2018 21:27
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Yang, Zhiyong <zhiyong.yang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v5] net/i40e: add interface to choose latest
> vector path
> 
> 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.
OK. Will update the commit log.

> 
> > 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
OK.

> 
> 
> 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?
OK. Will take that name. 
> 
> <...>
> 
> > @@ -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?
OK. Will generalize it to users not VPP users.
> 
> > +
> >  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.
OK. I am preparing patch about that.

> 
> > +
> > +	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()
OK.

> 
> <...>
> 
> > @@ -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 ?
Yes. Will correct it.