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

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

Hi Xiaoyun:

> -----Original Message-----
> From: Li, Xiaoyun
> Sent: Tuesday, September 4, 2018 7:40 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Yang, Zhiyong <zhiyong.yang@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>; Hunt, David <david.hunt@intel.com>; Li,
> Xiaoyun <xiaoyun.li@intel.com>
> Subject: [PATCH v3] net/i40e: add interface to choose latest vector path
> 
> Right now, vector path is limited to only use on later platform.
> This patch adds a devarg enable-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.
> 
> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> ---
> v3:
>  * Polish the doc and commit log.
> v2:
>  * Correct the calling of the wrong function last time.
>  * Fix seg fault bug.
> 
>  doc/guides/nics/i40e.rst               |  8 ++++++
>  doc/guides/rel_notes/release_18_11.rst |  4 +++
>  drivers/net/i40e/i40e_ethdev.c         | 38
> ++++++++++++++++++++++++++
>  drivers/net/i40e/i40e_ethdev.h         |  1 +
>  drivers/net/i40e/i40e_rxtx.c           | 27 ++++++++++++++++++
>  5 files changed, 78 insertions(+)
> 
> diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst index
> 65d87f869..6158e7c34 100644
> --- a/doc/guides/nics/i40e.rst
> +++ b/doc/guides/nics/i40e.rst
> @@ -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.
> 
> +- ``Enable 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 ``enable-latest-vec``  is introduced, for example::
> +    -w 84:00.0,enable-latest-vec=1


How about "use_latest_vec" or "use-lastest-vpmd"?

> +
>  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..f8b0f3189 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 eable the latest vector path.**
> +  A new devarg ``enable-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.
> 
>  API Changes
>  -----------
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 85a6a867f..16b5345fb 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -12513,6 +12513,44 @@ i40e_config_rss_filter(struct i40e_pf *pf,
>  	return 0;
>  }
> 
> +#define ETH_I40E_ENABLE_LATEST_VEC	"enable-latest-vec"

This should be defined along with other exist devargs, please check ETH_I40E_SUPPORT_MULTI_DRIVER for reference
Also it should be registered with RTE_PMD_REGISTER_PARAM_STRING.

> +
> +bool
> +i40e_parse_latest_vec(struct rte_eth_dev *dev) {
> +	static const char *const valid_keys[] = {
> +		ETH_I40E_ENABLE_LATEST_VEC, NULL};
> +	int enable_latest_vec;
> +	struct rte_kvargs *kvlist;
> +
> +	if (!dev->device->devargs)
> +		return 0;
> +
> +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> +	if (!kvlist)
> +		return -EINVAL;
> +
> +	if (!rte_kvargs_count(kvlist, ETH_I40E_ENABLE_LATEST_VEC))
> +		return 0;
> +
> +	if (rte_kvargs_count(kvlist, ETH_I40E_ENABLE_LATEST_VEC) > 1)
> +		PMD_DRV_LOG(WARNING, "More than one argument \"%s\" and
> only "
> +			    "the first one is used !",
> +			    ETH_I40E_ENABLE_LATEST_VEC);
> +
> +	enable_latest_vec = atoi((&kvlist->pairs[0])->value);
> +
> +	rte_kvargs_free(kvlist);
> +
> +	if (enable_latest_vec != 0 && enable_latest_vec != 1)
> +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as 1!");
> +
> +	if (enable_latest_vec)
> +		return true;
> +	else
> +		return false;
> +}

We call rte_kvargs_parse in different place for different parameter which is not necessary.
it's better to merge them into one parse_devargs function at dev_init and then all corresponding field of i40e_adapter can be configured at the same place.
Though this is not this patch's scope, but it's better to introduce a field like i40e_adapter->use_latest_vec
and in i40e_parse_latest_vec, it just assign the value which could be used later.
This will make things easy for future code clean and also it is not necessary to call i40e_parse_latest_vec multiple time in set_rx/tx_function.

> +
>  RTE_INIT(i40e_init_log)
>  {
>  	i40e_logtype_init = rte_log_register("pmd.net.i40e.init");
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index 3fffe5a55..cdf68cd93 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -1243,6 +1243,7 @@ int i40e_config_rss_filter(struct i40e_pf *pf,
>  		struct i40e_rte_flow_rss_conf *conf, bool add);  int
> i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params);  int
> i40e_vf_representor_uninit(struct rte_eth_dev *ethdev);
> +bool i40e_parse_latest_vec(struct rte_eth_dev *dev);
> 
>  #define I40E_DEV_TO_PCI(eth_dev) \
>  	RTE_DEV_TO_PCI((eth_dev)->device)
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index
> 2a28ee348..75f8ec284 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -2960,6 +2960,15 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
>  			if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
>  				dev->rx_pkt_burst =
>  					i40e_recv_scattered_pkts_vec_avx2;
> +			/*
> +			 * Give users chance to use the latest vector path
> +			 * that the platform supported.
> +			 */
> +			if (i40e_parse_latest_vec(dev)) {
> +				if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> +					dev->rx_pkt_burst =
> +					i40e_recv_scattered_pkts_vec_avx2;
> +			}


How about: 
			
			if (adapter->use_latest_vec)
				dev->pkt_burst = get_latest_rx_vec();
			else
				dev->pkt_burst = get_recommended_rx_vec();

this make code easy to understand and in future, if we have avx512 vpmd, it's easy for us to modify the code.


>  #endif
>  		} else {
>  			PMD_INIT_LOG(DEBUG, "Using a Scattered with bulk "
> @@ -2989,6 +2998,15 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
>  		 */
>  		if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
>  			dev->rx_pkt_burst = i40e_recv_pkts_vec_avx2;
> +		/*
> +		 * Give users chance to use the latest vector path
> +		 * that the platform supported.
> +		 */
> +		if (i40e_parse_latest_vec(dev)) {
> +			if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> +				dev->rx_pkt_burst =
> +					i40e_recv_pkts_vec_avx2;
> +		}

Same as above
>  #endif
>  	} else if (ad->rx_bulk_alloc_allowed) {
>  		PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions are "
> @@ -3083,6 +3101,15 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
>  			 */
>  			if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
>  				dev->tx_pkt_burst = i40e_xmit_pkts_vec_avx2;
> +			/*
> +			 * Give users chance to use the latest vector path
> +			 * that the platform supported.
> +			 */
> +			if (i40e_parse_latest_vec(dev)) {
> +				if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> +					dev->tx_pkt_burst =
> +					i40e_xmit_pkts_vec_avx2;
> +			}

Same as above

>  #endif
>  		} else {
>  			PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> --
> 2.17.1

Regards
Qi