[dpdk-dev] [PATCH 0/3] net/i40e: improve rte_flow offload with MARK + RSS

[dpdk-dev] [PATCH 0/3] net/i40e: improve rte_flow offload with MARK + RSS

[Mesut Ali Ergin]

Applications using DPDK, including but not limited to OVS DPDK,
utilize rte_flow to benefit from hardware flow offloads. Three
patches in this set improves i40e offload capabilities by

(*) Enhancing Flow Director to support MARK + RSS action combination
(*) Giving applications ability to disable vector RX at runtime, since
Flow Director is not currently supported using the vector RX path

For example, with this patchset, OVS DPDK's existing hw-offload feature
becomes functional using i40e, improving phy-to-phy switching
performance more than 200% for a use case with 1,000,000 UDP flows
switched by 1,000 rules in Open Flow tables.

Mesut Ali Ergin (3):
  net/i40e: add support for MARK + RSS action in rte_flow
  net/i40e: add runtime option to disable vector rx
  net/i40e: fix inadvertent override of vector RX allowance

 doc/guides/nics/i40e.rst                | 14 +++++++
 drivers/net/i40e/i40e_ethdev.c          | 70 ++++++++++++++++++++++++++++++++-
 drivers/net/i40e/i40e_ethdev.h          |  1 +
 drivers/net/i40e/i40e_flow.c            | 29 ++++++++++++++
 drivers/net/i40e/i40e_rxtx.c            |  4 ++
 drivers/net/i40e/i40e_rxtx_vec_common.h |  4 ++
 6 files changed, 121 insertions(+), 1 deletion(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/3] net/i40e: add support for MARK + RSS action in rte_flow

[Mesut Ali Ergin]

Currently, i40e Flow Director action parser only allows following nine
action combinations: (QUEUE, PASSTHRU, DROP, QUEUE + MARK, PASSTHRU +
MARK, DROP + MARK, QUEUE + FLAG, PASSTHRU + FLAG, DROP + FLAG)

Using the existing Cloud Filter profile on the NIC, it is possible to
add support for two more combinations as: (MARK + RSS, MARK + FLAG +
RSS)

Addition of these new combinations would allow more applications to
utilize DPDK rte_flow to implement hardware flow offloads with Intel
Ethernet 700 series network adapters, including but not limited to the
existing OVS DPDK partial hardware flow offload feature.

Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
---
 drivers/net/i40e/i40e_flow.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
index 5447e4e..d4d564f 100644
--- a/drivers/net/i40e/i40e_flow.c
+++ b/drivers/net/i40e/i40e_flow.c
@@ -3078,6 +3078,12 @@ i40e_flow_parse_fdir_action(struct rte_eth_dev *dev,
 	case RTE_FLOW_ACTION_TYPE_PASSTHRU:
 		filter->action.behavior = I40E_FDIR_PASSTHRU;
 		break;
+	case RTE_FLOW_ACTION_TYPE_MARK:
+		filter->action.behavior = I40E_FDIR_PASSTHRU;
+		mark_spec = act->conf;
+		filter->action.report_status = I40E_FDIR_REPORT_ID;
+		filter->soft_id = mark_spec->id;
+	break;
 	default:
 		rte_flow_error_set(error, EINVAL,
 				   RTE_FLOW_ERROR_TYPE_ACTION, act,
@@ -3090,13 +3096,36 @@ i40e_flow_parse_fdir_action(struct rte_eth_dev *dev,
 	NEXT_ITEM_OF_ACTION(act, actions, index);
 	switch (act->type) {
 	case RTE_FLOW_ACTION_TYPE_MARK:
+		if (!mark_spec) {
+			/* Double MARK actions requested */
+			rte_flow_error_set(error, EINVAL,
+			   RTE_FLOW_ERROR_TYPE_ACTION, act,
+			   "Invalid action.");
+			return -rte_errno;
+		}
 		mark_spec = act->conf;
 		filter->action.report_status = I40E_FDIR_REPORT_ID;
 		filter->soft_id = mark_spec->id;
 		break;
 	case RTE_FLOW_ACTION_TYPE_FLAG:
+		if (!mark_spec) {
+			/* MARK + FLAG not supported */
+			rte_flow_error_set(error, EINVAL,
+					   RTE_FLOW_ERROR_TYPE_ACTION, act,
+					   "Invalid action.");
+			return -rte_errno;
+		}
 		filter->action.report_status = I40E_FDIR_NO_REPORT_STATUS;
 		break;
+	case RTE_FLOW_ACTION_TYPE_RSS:
+		if (filter->action.behavior != I40E_FDIR_PASSTHRU) {
+			/* RSS filter won't be next if FDIR did not pass thru */
+			rte_flow_error_set(error, EINVAL,
+					   RTE_FLOW_ERROR_TYPE_ACTION, act,
+					   "Invalid action.");
+			return -rte_errno;
+		}
+		break;
 	case RTE_FLOW_ACTION_TYPE_END:
 		return 0;
 	default:
-- 
2.7.4

[dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Mesut Ali Ergin]

Vector RX functions are not at feature parity with non-vector ones and
currently the vector RX path is enabled by default. Hence, the only
option to force selection of non-vector variants and be able to retain
functionality is to disable vector PMD globally at compile time via the
config file option.

This patch introduces a new runtime device config option named
disable-vec-rx to allow users to limit the driver to make a choice among
non-vector RX functions, on a per device basis. This runtime option
defaults to a value of zero, allowing vector RX functions to be
selected (current behavior). When disable-vec-rx=1 is specified for a
device, even if all the other requirements to select a vector RX
function are satisfied, the driver will still pick one out of the
appropriate non-vector RX functions.

Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
---
 doc/guides/nics/i40e.rst       | 14 +++++++++
 drivers/net/i40e/i40e_ethdev.c | 70 +++++++++++++++++++++++++++++++++++++++++-
 drivers/net/i40e/i40e_ethdev.h |  1 +
 drivers/net/i40e/i40e_rxtx.c   |  4 +++
 4 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index a97484c..532cc64 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -183,6 +183,20 @@ Runtime Config Options
 
   -w 84:00.0,use-latest-supported-vec=1
 
+- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
+
+  When config file option for the vector PMD is enabled, vector RX functions may
+  be the default choice of the driver at device initialization time, if certain
+  conditions apply. However, vector RX functions may not always be at feature
+  parity with non-vector ones. In order to allow users to override vector RX
+  function selection behavior at runtime, the ``devargs`` parameter
+  ``disable-vec-rx`` is introduced, such that
+
+  -w DBDF,disable-vec-rx=1
+
+  would force driver to limit its choices to non-vector RX function variants for
+  the device specified by the DBDF.
+
 Vector RX Pre-conditions
 ~~~~~~~~~~~~~~~~~~~~~~~~
 For Vector RX it is assumed that the number of descriptor rings will be a power
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index cab440f..19fbd23 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -44,6 +44,7 @@
 #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
 #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
 #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
+#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
 
 #define I40E_CLEAR_PXE_WAIT_MS     200
 
@@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
 	ETH_I40E_SUPPORT_MULTI_DRIVER,
 	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
 	ETH_I40E_USE_LATEST_VEC,
+	ETH_I40E_DISABLE_VEC_RX,
 	NULL};
 
 static const struct rte_pci_id pci_id_i40e_map[] = {
@@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
 	return 0;
 }
 
+static int
+i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
+				const char *value,
+				void *opaque)
+{
+	struct i40e_adapter *ad;
+
+	ad = (struct i40e_adapter *)opaque;
+
+	switch (atoi(value)) {
+	case 0:
+		/* Selection of RX vector functions left untouched*/
+		break;
+	case 1:
+		/* Disable RX vector functions as requested*/
+		ad->rx_vec_allowed = false;
+	break;
+	default:
+		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as 1!");
+	break;
+	}
+
+	return 0;
+}
+
+int
+i40e_disable_vec_rx(struct rte_eth_dev *dev)
+{
+	struct i40e_adapter *ad =
+		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+	struct rte_kvargs *kvlist;
+	int kvargs_count;
+
+
+	if (!dev->device->devargs)
+		return 0;
+
+	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
+	if (!kvlist)
+		return -EINVAL;
+
+	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_DISABLE_VEC_RX);
+	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 invalid or last valid one is used !",
+			    ETH_I40E_DISABLE_VEC_RX);
+
+	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
+				i40e_parse_disable_vec_rx_handler, ad) < 0) {
+		rte_kvargs_free(kvlist);
+		return -EINVAL;
+	}
+
+	rte_kvargs_free(kvlist);
+	return 0;
+}
+
 #define I40E_ALARM_INTERVAL 50000 /* us */
 
 static int
@@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
 	ad->tx_simple_allowed = true;
 	ad->tx_vec_allowed = true;
 
+	/* Check if users wanted to disable vector RX functions */
+	i40e_disable_vec_rx(dev);
+
 	/* Only legacy filter API needs the following fdir config. So when the
 	 * legacy filter API is deprecated, the following codes should also be
 	 * removed.
@@ -12790,4 +12857,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
 			      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_USE_LATEST_VEC "=0|1");
+			      ETH_I40E_USE_LATEST_VEC "=0|1"
+			      ETH_I40E_DISABLE_VEC_RX "=0|1");
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 9855038..906bfe9 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -1248,6 +1248,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);
+int i40e_disable_vec_rx(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 1489552..7e66f59 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
 		 */
 		ad->rx_bulk_alloc_allowed = true;
 		ad->rx_vec_allowed = true;
+
+		/* Check if users wanted to disable vector RX functions */
+		i40e_disable_vec_rx(dev);
+
 		dev->data->scattered_rx = use_scattered_rx;
 		if (use_def_burst_func)
 			ad->rx_bulk_alloc_allowed = false;
-- 
2.7.4

[dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance

[Mesut Ali Ergin]

When i40e_rx_vec_dev_conf_condition_check_default() determines whether
vector receive functions would be allowed during initialization phase,
it should honor previously recorded disallowance during configuration
phase, and not override simply because it is for the first queue.

Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
---
 drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
index 0e6ffa0..f30cab4 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_common.h
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -212,6 +212,10 @@ i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
 	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
 		return -1;
 
+	/* Should not override if vector was already disallowed */
+	if (!ad->rx_vec_allowed)
+	return -1;
+
 	/**
 	 * Vector mode is allowed only when number of Rx queue
 	 * descriptor is power of 2.
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Maxime Coquelin]

On 5/16/19 6:28 AM, Mesut Ali Ergin wrote:
> Vector RX functions are not at feature parity with non-vector ones and
> currently the vector RX path is enabled by default. Hence, the only
> option to force selection of non-vector variants and be able to retain
> functionality is to disable vector PMD globally at compile time via the
> config file option.
> 
> This patch introduces a new runtime device config option named
> disable-vec-rx to allow users to limit the driver to make a choice among
> non-vector RX functions, on a per device basis. This runtime option
> defaults to a value of zero, allowing vector RX functions to be
> selected (current behavior). When disable-vec-rx=1 is specified for a
> device, even if all the other requirements to select a vector RX
> function are satisfied, the driver will still pick one out of the
> appropriate non-vector RX functions.
> 
> Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> ---
>   doc/guides/nics/i40e.rst       | 14 +++++++++
>   drivers/net/i40e/i40e_ethdev.c | 70 +++++++++++++++++++++++++++++++++++++++++-
>   drivers/net/i40e/i40e_ethdev.h |  1 +
>   drivers/net/i40e/i40e_rxtx.c   |  4 +++
>   4 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> index a97484c..532cc64 100644
> --- a/doc/guides/nics/i40e.rst
> +++ b/doc/guides/nics/i40e.rst
> @@ -183,6 +183,20 @@ Runtime Config Options
>   
>     -w 84:00.0,use-latest-supported-vec=1
>   
> +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> +
> +  When config file option for the vector PMD is enabled, vector RX functions may
> +  be the default choice of the driver at device initialization time, if certain
> +  conditions apply. However, vector RX functions may not always be at feature
> +  parity with non-vector ones. In order to allow users to override vector RX
> +  function selection behavior at runtime, the ``devargs`` parameter
> +  ``disable-vec-rx`` is introduced, such that
> +
> +  -w DBDF,disable-vec-rx=1
> +
> +  would force driver to limit its choices to non-vector RX function variants for
> +  the device specified by the DBDF.
> +
>   Vector RX Pre-conditions
>   ~~~~~~~~~~~~~~~~~~~~~~~~
>   For Vector RX it is assumed that the number of descriptor rings will be a power
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index cab440f..19fbd23 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -44,6 +44,7 @@
>   #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
>   #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
>   #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
>   
>   #define I40E_CLEAR_PXE_WAIT_MS     200
>   
> @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
>   	ETH_I40E_SUPPORT_MULTI_DRIVER,
>   	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
>   	ETH_I40E_USE_LATEST_VEC,
> +	ETH_I40E_DISABLE_VEC_RX,
>   	NULL};
>   
>   static const struct rte_pci_id pci_id_i40e_map[] = {
> @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
>   	return 0;
>   }
>   
> +static int
> +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> +				const char *value,
> +				void *opaque)
> +{
> +	struct i40e_adapter *ad;
> +
> +	ad = (struct i40e_adapter *)opaque;
> +
> +	switch (atoi(value)) {
> +	case 0:
> +		/* Selection of RX vector functions left untouched*/
> +		break;
> +	case 1:
> +		/* Disable RX vector functions as requested*/
> +		ad->rx_vec_allowed = false;
> +	break;
> +	default:
> +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as 1!");

You may want to propagate the error here.

> +	break;
> +	}
> +
> +	return 0;
> +}
> +
> +int
> +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> +{
> +	struct i40e_adapter *ad =
> +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	struct rte_kvargs *kvlist;
> +	int kvargs_count;
> +
> +
> +	if (!dev->device->devargs)
> +		return 0;
> +
> +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> +	if (!kvlist)
> +		return -EINVAL;
> +
> +	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_DISABLE_VEC_RX);
> +	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 invalid or last valid one is used !",
> +			    ETH_I40E_DISABLE_VEC_RX);
> +
> +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> +				i40e_parse_disable_vec_rx_handler, ad) < 0) {
> +		rte_kvargs_free(kvlist);
> +		return -EINVAL;
> +	}
> +
> +	rte_kvargs_free(kvlist);
> +	return 0;
> +}
> +
>   #define I40E_ALARM_INTERVAL 50000 /* us */
>   
>   static int
> @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
>   	ad->tx_simple_allowed = true;
>   	ad->tx_vec_allowed = true;
>   
> +	/* Check if users wanted to disable vector RX functions */
> +	i40e_disable_vec_rx(dev);

Ditto, error should be propagated.

> +
>   	/* Only legacy filter API needs the following fdir config. So when the
>   	 * legacy filter API is deprecated, the following codes should also be
>   	 * removed.
> @@ -12790,4 +12857,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
>   			      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_USE_LATEST_VEC "=0|1");
> +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index 9855038..906bfe9 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -1248,6 +1248,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);
> +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
>   		 */
>   		ad->rx_bulk_alloc_allowed = true;
>   		ad->rx_vec_allowed = true;
> +
> +		/* Check if users wanted to disable vector RX functions */
> +		i40e_disable_vec_rx(dev);
> +

Ditto, error should be propagated.

>   		dev->data->scattered_rx = use_scattered_rx;
>   		if (use_def_burst_func)
>   			ad->rx_bulk_alloc_allowed = false;
> 

Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance

[Maxime Coquelin]

On 5/16/19 6:28 AM, Mesut Ali Ergin wrote:
> When i40e_rx_vec_dev_conf_condition_check_default() determines whether
> vector receive functions would be allowed during initialization phase,
> it should honor previously recorded disallowance during configuration
> phase, and not override simply because it is for the first queue.
> 
> Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> ---
>   drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index 0e6ffa0..f30cab4 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -212,6 +212,10 @@ i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>   	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
>   		return -1;
>   
> +	/* Should not override if vector was already disallowed */
> +	if (!ad->rx_vec_allowed)
> +	return -1;

nit: wrong indentation.

> +
>   	/**
>   	 * Vector mode is allowed only when number of Rx queue
>   	 * descriptor is power of 2.
> 

Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance

[Ergin, Mesut A]

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, May 16, 2019 1:18 AM
> To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector
> RX allowance
> 
> 
> 
> On 5/16/19 6:28 AM, Mesut Ali Ergin wrote:
> > When i40e_rx_vec_dev_conf_condition_check_default() determines whether
> > vector receive functions would be allowed during initialization phase,
> > it should honor previously recorded disallowance during configuration
> > phase, and not override simply because it is for the first queue.
> >
> > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > ---
> >   drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > index 0e6ffa0..f30cab4 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > @@ -212,6 +212,10 @@
> i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> >   	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> >   		return -1;
> >
> > +	/* Should not override if vector was already disallowed */
> > +	if (!ad->rx_vec_allowed)
> > +	return -1;
> 
> nit: wrong indentation.

Thanks. Fixed, and queued for v2 -- my checkpatch misses this for some reason.
> 
> > +
> >   	/**
> >   	 * Vector mode is allowed only when number of Rx queue
> >   	 * descriptor is power of 2.
> >

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ergin, Mesut A]

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, May 16, 2019 1:17 AM
> To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable
> vector rx
> 
> 
> 
> On 5/16/19 6:28 AM, Mesut Ali Ergin wrote:
> > Vector RX functions are not at feature parity with non-vector ones and
> > currently the vector RX path is enabled by default. Hence, the only
> > option to force selection of non-vector variants and be able to retain
> > functionality is to disable vector PMD globally at compile time via the
> > config file option.
> >
> > This patch introduces a new runtime device config option named
> > disable-vec-rx to allow users to limit the driver to make a choice among
> > non-vector RX functions, on a per device basis. This runtime option
> > defaults to a value of zero, allowing vector RX functions to be
> > selected (current behavior). When disable-vec-rx=1 is specified for a
> > device, even if all the other requirements to select a vector RX
> > function are satisfied, the driver will still pick one out of the
> > appropriate non-vector RX functions.
> >
> > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > ---
> >   doc/guides/nics/i40e.rst       | 14 +++++++++
> >   drivers/net/i40e/i40e_ethdev.c | 70
> +++++++++++++++++++++++++++++++++++++++++-
> >   drivers/net/i40e/i40e_ethdev.h |  1 +
> >   drivers/net/i40e/i40e_rxtx.c   |  4 +++
> >   4 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> > index a97484c..532cc64 100644
> > --- a/doc/guides/nics/i40e.rst
> > +++ b/doc/guides/nics/i40e.rst
> > @@ -183,6 +183,20 @@ Runtime Config Options
> >
> >     -w 84:00.0,use-latest-supported-vec=1
> >
> > +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> > +
> > +  When config file option for the vector PMD is enabled, vector RX functions
> may
> > +  be the default choice of the driver at device initialization time, if certain
> > +  conditions apply. However, vector RX functions may not always be at
> feature
> > +  parity with non-vector ones. In order to allow users to override vector RX
> > +  function selection behavior at runtime, the ``devargs`` parameter
> > +  ``disable-vec-rx`` is introduced, such that
> > +
> > +  -w DBDF,disable-vec-rx=1
> > +
> > +  would force driver to limit its choices to non-vector RX function variants for
> > +  the device specified by the DBDF.
> > +
> >   Vector RX Pre-conditions
> >   ~~~~~~~~~~~~~~~~~~~~~~~~
> >   For Vector RX it is assumed that the number of descriptor rings will be a
> power
> > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> > index cab440f..19fbd23 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -44,6 +44,7 @@
> >   #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
> >   #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
> >   #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> > +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
> >
> >   #define I40E_CLEAR_PXE_WAIT_MS     200
> >
> > @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
> >   	ETH_I40E_SUPPORT_MULTI_DRIVER,
> >   	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
> >   	ETH_I40E_USE_LATEST_VEC,
> > +	ETH_I40E_DISABLE_VEC_RX,
> >   	NULL};
> >
> >   static const struct rte_pci_id pci_id_i40e_map[] = {
> > @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
> >   	return 0;
> >   }
> >
> > +static int
> > +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> > +				const char *value,
> > +				void *opaque)
> > +{
> > +	struct i40e_adapter *ad;
> > +
> > +	ad = (struct i40e_adapter *)opaque;
> > +
> > +	switch (atoi(value)) {
> > +	case 0:
> > +		/* Selection of RX vector functions left untouched*/
> > +		break;
> > +	case 1:
> > +		/* Disable RX vector functions as requested*/
> > +		ad->rx_vec_allowed = false;
> > +	break;
> > +	default:
> > +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as
> 1!");
> 
> You may want to propagate the error here.
> 
Agreed, done and queued up for v2.

> > +	break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int
> > +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> > +{
> > +	struct i40e_adapter *ad =
> > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > +	struct rte_kvargs *kvlist;
> > +	int kvargs_count;
> > +
> > +
> > +	if (!dev->device->devargs)
> > +		return 0;
> > +
> > +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> > +	if (!kvlist)
> > +		return -EINVAL;
> > +
> > +	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_DISABLE_VEC_RX);
> > +	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 invalid or last valid one is used !",
> > +			    ETH_I40E_DISABLE_VEC_RX);
> > +
> > +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> > +				i40e_parse_disable_vec_rx_handler, ad) < 0) {
> > +		rte_kvargs_free(kvlist);
> > +		return -EINVAL;
> > +	}
> > +
> > +	rte_kvargs_free(kvlist);
> > +	return 0;
> > +}
> > +
> >   #define I40E_ALARM_INTERVAL 50000 /* us */
> >
> >   static int
> > @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
> >   	ad->tx_simple_allowed = true;
> >   	ad->tx_vec_allowed = true;
> >
> > +	/* Check if users wanted to disable vector RX functions */
> > +	i40e_disable_vec_rx(dev);
> 
> Ditto, error should be propagated.
> 
Likewise, agreed, done and queued up for v2.

> > +
> >   	/* Only legacy filter API needs the following fdir config. So when the
> >   	 * legacy filter API is deprecated, the following codes should also be
> >   	 * removed.
> > @@ -12790,4 +12857,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
> >   			      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_USE_LATEST_VEC "=0|1");
> > +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> > +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> > diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> > index 9855038..906bfe9 100644
> > --- a/drivers/net/i40e/i40e_ethdev.h
> > +++ b/drivers/net/i40e/i40e_ethdev.h
> > @@ -1248,6 +1248,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);
> > +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct
> rte_eth_dev *dev,
> >   		 */
> >   		ad->rx_bulk_alloc_allowed = true;
> >   		ad->rx_vec_allowed = true;
> > +
> > +		/* Check if users wanted to disable vector RX functions */
> > +		i40e_disable_vec_rx(dev);
> > +
> 
> Ditto, error should be propagated.

Likewise, agreed, done and queued up for v2.

> 
> >   		dev->data->scattered_rx = use_scattered_rx;
> >   		if (use_def_burst_func)
> >   			ad->rx_bulk_alloc_allowed = false;
> >

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ananyev, Konstantin]

Hi

> 
> Vector RX functions are not at feature parity with non-vector ones and
> currently the vector RX path is enabled by default. Hence, the only
> option to force selection of non-vector variants and be able to retain
> functionality is to disable vector PMD globally at compile time via the
> config file option.
> 
> This patch introduces a new runtime device config option named
> disable-vec-rx to allow users to limit the driver to make a choice among
> non-vector RX functions, on a per device basis. This runtime option
> defaults to a value of zero, allowing vector RX functions to be
> selected (current behavior). When disable-vec-rx=1 is specified for a
> device, even if all the other requirements to select a vector RX
> function are satisfied, the driver will still pick one out of the
> appropriate non-vector RX functions.

Not sure I understand the logic of that patch.
If i40e RX PMD selects wrong RX function that doesn't provide
requested by user functionality (which one?) -   
then it is a bug in i40e RX function selection code that needs to be fixed.
No point to introduce some new config options for that.
Konstantin

> 
> Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> ---
>  doc/guides/nics/i40e.rst       | 14 +++++++++
>  drivers/net/i40e/i40e_ethdev.c | 70 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/net/i40e/i40e_ethdev.h |  1 +
>  drivers/net/i40e/i40e_rxtx.c   |  4 +++
>  4 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> index a97484c..532cc64 100644
> --- a/doc/guides/nics/i40e.rst
> +++ b/doc/guides/nics/i40e.rst
> @@ -183,6 +183,20 @@ Runtime Config Options
> 
>    -w 84:00.0,use-latest-supported-vec=1
> 
> +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> +
> +  When config file option for the vector PMD is enabled, vector RX functions may
> +  be the default choice of the driver at device initialization time, if certain
> +  conditions apply. However, vector RX functions may not always be at feature
> +  parity with non-vector ones. In order to allow users to override vector RX
> +  function selection behavior at runtime, the ``devargs`` parameter
> +  ``disable-vec-rx`` is introduced, such that
> +
> +  -w DBDF,disable-vec-rx=1
> +
> +  would force driver to limit its choices to non-vector RX function variants for
> +  the device specified by the DBDF.
> +
>  Vector RX Pre-conditions
>  ~~~~~~~~~~~~~~~~~~~~~~~~
>  For Vector RX it is assumed that the number of descriptor rings will be a power
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index cab440f..19fbd23 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -44,6 +44,7 @@
>  #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
>  #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
>  #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
> 
>  #define I40E_CLEAR_PXE_WAIT_MS     200
> 
> @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
>  	ETH_I40E_SUPPORT_MULTI_DRIVER,
>  	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
>  	ETH_I40E_USE_LATEST_VEC,
> +	ETH_I40E_DISABLE_VEC_RX,
>  	NULL};
> 
>  static const struct rte_pci_id pci_id_i40e_map[] = {
> @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
>  	return 0;
>  }
> 
> +static int
> +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> +				const char *value,
> +				void *opaque)
> +{
> +	struct i40e_adapter *ad;
> +
> +	ad = (struct i40e_adapter *)opaque;
> +
> +	switch (atoi(value)) {
> +	case 0:
> +		/* Selection of RX vector functions left untouched*/
> +		break;
> +	case 1:
> +		/* Disable RX vector functions as requested*/
> +		ad->rx_vec_allowed = false;
> +	break;
> +	default:
> +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as 1!");
> +	break;
> +	}
> +
> +	return 0;
> +}
> +
> +int
> +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> +{
> +	struct i40e_adapter *ad =
> +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	struct rte_kvargs *kvlist;
> +	int kvargs_count;
> +
> +
> +	if (!dev->device->devargs)
> +		return 0;
> +
> +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> +	if (!kvlist)
> +		return -EINVAL;
> +
> +	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_DISABLE_VEC_RX);
> +	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 invalid or last valid one is used !",
> +			    ETH_I40E_DISABLE_VEC_RX);
> +
> +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> +				i40e_parse_disable_vec_rx_handler, ad) < 0) {
> +		rte_kvargs_free(kvlist);
> +		return -EINVAL;
> +	}
> +
> +	rte_kvargs_free(kvlist);
> +	return 0;
> +}
> +
>  #define I40E_ALARM_INTERVAL 50000 /* us */
> 
>  static int
> @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
>  	ad->tx_simple_allowed = true;
>  	ad->tx_vec_allowed = true;
> 
> +	/* Check if users wanted to disable vector RX functions */
> +	i40e_disable_vec_rx(dev);
> +
>  	/* Only legacy filter API needs the following fdir config. So when the
>  	 * legacy filter API is deprecated, the following codes should also be
>  	 * removed.
> @@ -12790,4 +12857,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
>  			      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_USE_LATEST_VEC "=0|1");
> +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index 9855038..906bfe9 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -1248,6 +1248,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);
> +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
>  		 */
>  		ad->rx_bulk_alloc_allowed = true;
>  		ad->rx_vec_allowed = true;
> +
> +		/* Check if users wanted to disable vector RX functions */
> +		i40e_disable_vec_rx(dev);
> +
>  		dev->data->scattered_rx = use_scattered_rx;
>  		if (use_def_burst_func)
>  			ad->rx_bulk_alloc_allowed = false;
> --
> 2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ergin, Mesut A]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, May 20, 2019 1:29 AM
> To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable
> vector rx
> 
> 
> Hi
> 
> >
> > Vector RX functions are not at feature parity with non-vector ones and
> > currently the vector RX path is enabled by default. Hence, the only
> > option to force selection of non-vector variants and be able to retain
> > functionality is to disable vector PMD globally at compile time via the
> > config file option.
> >
> > This patch introduces a new runtime device config option named
> > disable-vec-rx to allow users to limit the driver to make a choice among
> > non-vector RX functions, on a per device basis. This runtime option
> > defaults to a value of zero, allowing vector RX functions to be
> > selected (current behavior). When disable-vec-rx=1 is specified for a
> > device, even if all the other requirements to select a vector RX
> > function are satisfied, the driver will still pick one out of the
> > appropriate non-vector RX functions.
> 
> Not sure I understand the logic of that patch.
> If i40e RX PMD selects wrong RX function that doesn't provide
> requested by user functionality (which one?) -
> then it is a bug in i40e RX function selection code that needs to be fixed.
> No point to introduce some new config options for that.
> Konstantin
> 
Current design of RX function selection for the PMDs make it an 
initialization time deal. However, the first rte_flow request (thus the need
to enable FD / RTE_FDIR_MODE_PERFECT) may come in at any point in 
time, well after the RX function was selected (e.g., OVS does this when the 
first packet that can be offloaded arrives). The design in this patch gives 
such applications a choice to restrict possible RX functions to non-vector 
paths proactively.

> >
> > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > ---
> >  doc/guides/nics/i40e.rst       | 14 +++++++++
> >  drivers/net/i40e/i40e_ethdev.c | 70
> +++++++++++++++++++++++++++++++++++++++++-
> >  drivers/net/i40e/i40e_ethdev.h |  1 +
> >  drivers/net/i40e/i40e_rxtx.c   |  4 +++
> >  4 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> > index a97484c..532cc64 100644
> > --- a/doc/guides/nics/i40e.rst
> > +++ b/doc/guides/nics/i40e.rst
> > @@ -183,6 +183,20 @@ Runtime Config Options
> >
> >    -w 84:00.0,use-latest-supported-vec=1
> >
> > +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> > +
> > +  When config file option for the vector PMD is enabled, vector RX functions
> may
> > +  be the default choice of the driver at device initialization time, if certain
> > +  conditions apply. However, vector RX functions may not always be at
> feature
> > +  parity with non-vector ones. In order to allow users to override vector RX
> > +  function selection behavior at runtime, the ``devargs`` parameter
> > +  ``disable-vec-rx`` is introduced, such that
> > +
> > +  -w DBDF,disable-vec-rx=1
> > +
> > +  would force driver to limit its choices to non-vector RX function variants for
> > +  the device specified by the DBDF.
> > +
> >  Vector RX Pre-conditions
> >  ~~~~~~~~~~~~~~~~~~~~~~~~
> >  For Vector RX it is assumed that the number of descriptor rings will be a power
> > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> > index cab440f..19fbd23 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -44,6 +44,7 @@
> >  #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
> >  #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
> >  #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> > +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
> >
> >  #define I40E_CLEAR_PXE_WAIT_MS     200
> >
> > @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
> >  	ETH_I40E_SUPPORT_MULTI_DRIVER,
> >  	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
> >  	ETH_I40E_USE_LATEST_VEC,
> > +	ETH_I40E_DISABLE_VEC_RX,
> >  	NULL};
> >
> >  static const struct rte_pci_id pci_id_i40e_map[] = {
> > @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
> >  	return 0;
> >  }
> >
> > +static int
> > +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> > +				const char *value,
> > +				void *opaque)
> > +{
> > +	struct i40e_adapter *ad;
> > +
> > +	ad = (struct i40e_adapter *)opaque;
> > +
> > +	switch (atoi(value)) {
> > +	case 0:
> > +		/* Selection of RX vector functions left untouched*/
> > +		break;
> > +	case 1:
> > +		/* Disable RX vector functions as requested*/
> > +		ad->rx_vec_allowed = false;
> > +	break;
> > +	default:
> > +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as
> 1!");
> > +	break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int
> > +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> > +{
> > +	struct i40e_adapter *ad =
> > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > +	struct rte_kvargs *kvlist;
> > +	int kvargs_count;
> > +
> > +
> > +	if (!dev->device->devargs)
> > +		return 0;
> > +
> > +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> > +	if (!kvlist)
> > +		return -EINVAL;
> > +
> > +	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_DISABLE_VEC_RX);
> > +	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 invalid or last valid one is used !",
> > +			    ETH_I40E_DISABLE_VEC_RX);
> > +
> > +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> > +				i40e_parse_disable_vec_rx_handler, ad) < 0) {
> > +		rte_kvargs_free(kvlist);
> > +		return -EINVAL;
> > +	}
> > +
> > +	rte_kvargs_free(kvlist);
> > +	return 0;
> > +}
> > +
> >  #define I40E_ALARM_INTERVAL 50000 /* us */
> >
> >  static int
> > @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
> >  	ad->tx_simple_allowed = true;
> >  	ad->tx_vec_allowed = true;
> >
> > +	/* Check if users wanted to disable vector RX functions */
> > +	i40e_disable_vec_rx(dev);
> > +
> >  	/* Only legacy filter API needs the following fdir config. So when the
> >  	 * legacy filter API is deprecated, the following codes should also be
> >  	 * removed.
> > @@ -12790,4 +12857,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
> >  			      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_USE_LATEST_VEC "=0|1");
> > +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> > +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> > diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> > index 9855038..906bfe9 100644
> > --- a/drivers/net/i40e/i40e_ethdev.h
> > +++ b/drivers/net/i40e/i40e_ethdev.h
> > @@ -1248,6 +1248,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);
> > +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct
> rte_eth_dev *dev,
> >  		 */
> >  		ad->rx_bulk_alloc_allowed = true;
> >  		ad->rx_vec_allowed = true;
> > +
> > +		/* Check if users wanted to disable vector RX functions */
> > +		i40e_disable_vec_rx(dev);
> > +
> >  		dev->data->scattered_rx = use_scattered_rx;
> >  		if (use_def_burst_func)
> >  			ad->rx_bulk_alloc_allowed = false;
> > --
> > 2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ananyev, Konstantin]

> >
> > Hi
> >
> > >
> > > Vector RX functions are not at feature parity with non-vector ones and
> > > currently the vector RX path is enabled by default. Hence, the only
> > > option to force selection of non-vector variants and be able to retain
> > > functionality is to disable vector PMD globally at compile time via the
> > > config file option.
> > >
> > > This patch introduces a new runtime device config option named
> > > disable-vec-rx to allow users to limit the driver to make a choice among
> > > non-vector RX functions, on a per device basis. This runtime option
> > > defaults to a value of zero, allowing vector RX functions to be
> > > selected (current behavior). When disable-vec-rx=1 is specified for a
> > > device, even if all the other requirements to select a vector RX
> > > function are satisfied, the driver will still pick one out of the
> > > appropriate non-vector RX functions.
> >
> > Not sure I understand the logic of that patch.
> > If i40e RX PMD selects wrong RX function that doesn't provide
> > requested by user functionality (which one?) -
> > then it is a bug in i40e RX function selection code that needs to be fixed.
> > No point to introduce some new config options for that.
> > Konstantin
> >
> Current design of RX function selection for the PMDs make it an
> initialization time deal. However, the first rte_flow request (thus the need
> to enable FD / RTE_FDIR_MODE_PERFECT) may come in at any point in
> time, well after the RX function was selected (e.g., OVS does this when the
> first packet that can be offloaded arrives). The design in this patch gives
> such applications a choice to restrict possible RX functions to non-vector
> paths proactively.

If you plan to use FD mode on your device, why not enable it
at setup phase via rte_eth_dev_configure()?
Then proper rx function would be selected. 

> 
> > >
> > > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > > ---
> > >  doc/guides/nics/i40e.rst       | 14 +++++++++
> > >  drivers/net/i40e/i40e_ethdev.c | 70
> > +++++++++++++++++++++++++++++++++++++++++-
> > >  drivers/net/i40e/i40e_ethdev.h |  1 +
> > >  drivers/net/i40e/i40e_rxtx.c   |  4 +++
> > >  4 files changed, 88 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> > > index a97484c..532cc64 100644
> > > --- a/doc/guides/nics/i40e.rst
> > > +++ b/doc/guides/nics/i40e.rst
> > > @@ -183,6 +183,20 @@ Runtime Config Options
> > >
> > >    -w 84:00.0,use-latest-supported-vec=1
> > >
> > > +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> > > +
> > > +  When config file option for the vector PMD is enabled, vector RX functions
> > may
> > > +  be the default choice of the driver at device initialization time, if certain
> > > +  conditions apply. However, vector RX functions may not always be at
> > feature
> > > +  parity with non-vector ones. In order to allow users to override vector RX
> > > +  function selection behavior at runtime, the ``devargs`` parameter
> > > +  ``disable-vec-rx`` is introduced, such that
> > > +
> > > +  -w DBDF,disable-vec-rx=1
> > > +
> > > +  would force driver to limit its choices to non-vector RX function variants for
> > > +  the device specified by the DBDF.
> > > +
> > >  Vector RX Pre-conditions
> > >  ~~~~~~~~~~~~~~~~~~~~~~~~
> > >  For Vector RX it is assumed that the number of descriptor rings will be a power
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> > > index cab440f..19fbd23 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -44,6 +44,7 @@
> > >  #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
> > >  #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
> > >  #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> > > +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
> > >
> > >  #define I40E_CLEAR_PXE_WAIT_MS     200
> > >
> > > @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
> > >  	ETH_I40E_SUPPORT_MULTI_DRIVER,
> > >  	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
> > >  	ETH_I40E_USE_LATEST_VEC,
> > > +	ETH_I40E_DISABLE_VEC_RX,
> > >  	NULL};
> > >
> > >  static const struct rte_pci_id pci_id_i40e_map[] = {
> > > @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
> > >  	return 0;
> > >  }
> > >
> > > +static int
> > > +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> > > +				const char *value,
> > > +				void *opaque)
> > > +{
> > > +	struct i40e_adapter *ad;
> > > +
> > > +	ad = (struct i40e_adapter *)opaque;
> > > +
> > > +	switch (atoi(value)) {
> > > +	case 0:
> > > +		/* Selection of RX vector functions left untouched*/
> > > +		break;
> > > +	case 1:
> > > +		/* Disable RX vector functions as requested*/
> > > +		ad->rx_vec_allowed = false;
> > > +	break;
> > > +	default:
> > > +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as
> > 1!");
> > > +	break;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int
> > > +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> > > +{
> > > +	struct i40e_adapter *ad =
> > > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > > +	struct rte_kvargs *kvlist;
> > > +	int kvargs_count;
> > > +
> > > +
> > > +	if (!dev->device->devargs)
> > > +		return 0;
> > > +
> > > +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> > > +	if (!kvlist)
> > > +		return -EINVAL;
> > > +
> > > +	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_DISABLE_VEC_RX);
> > > +	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 invalid or last valid one is used !",
> > > +			    ETH_I40E_DISABLE_VEC_RX);
> > > +
> > > +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> > > +				i40e_parse_disable_vec_rx_handler, ad) < 0) {
> > > +		rte_kvargs_free(kvlist);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	rte_kvargs_free(kvlist);
> > > +	return 0;
> > > +}
> > > +
> > >  #define I40E_ALARM_INTERVAL 50000 /* us */
> > >
> > >  static int
> > > @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
> > >  	ad->tx_simple_allowed = true;
> > >  	ad->tx_vec_allowed = true;
> > >
> > > +	/* Check if users wanted to disable vector RX functions */
> > > +	i40e_disable_vec_rx(dev);
> > > +
> > >  	/* Only legacy filter API needs the following fdir config. So when the
> > >  	 * legacy filter API is deprecated, the following codes should also be
> > >  	 * removed.
> > > @@ -12790,4 +12857,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
> > >  			      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_USE_LATEST_VEC "=0|1");
> > > +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> > > +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> > > diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> > > index 9855038..906bfe9 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.h
> > > +++ b/drivers/net/i40e/i40e_ethdev.h
> > > @@ -1248,6 +1248,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);
> > > +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct
> > rte_eth_dev *dev,
> > >  		 */
> > >  		ad->rx_bulk_alloc_allowed = true;
> > >  		ad->rx_vec_allowed = true;
> > > +
> > > +		/* Check if users wanted to disable vector RX functions */
> > > +		i40e_disable_vec_rx(dev);
> > > +
> > >  		dev->data->scattered_rx = use_scattered_rx;
> > >  		if (use_def_burst_func)
> > >  			ad->rx_bulk_alloc_allowed = false;
> > > --
> > > 2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ergin, Mesut A]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, May 20, 2019 4:11 PM
> To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable
> vector rx
> 
> 
> 
> > >
> > > Hi
> > >
> > > >
> > > > Vector RX functions are not at feature parity with non-vector ones and
> > > > currently the vector RX path is enabled by default. Hence, the only
> > > > option to force selection of non-vector variants and be able to retain
> > > > functionality is to disable vector PMD globally at compile time via the
> > > > config file option.
> > > >
> > > > This patch introduces a new runtime device config option named
> > > > disable-vec-rx to allow users to limit the driver to make a choice among
> > > > non-vector RX functions, on a per device basis. This runtime option
> > > > defaults to a value of zero, allowing vector RX functions to be
> > > > selected (current behavior). When disable-vec-rx=1 is specified for a
> > > > device, even if all the other requirements to select a vector RX
> > > > function are satisfied, the driver will still pick one out of the
> > > > appropriate non-vector RX functions.
> > >
> > > Not sure I understand the logic of that patch.
> > > If i40e RX PMD selects wrong RX function that doesn't provide
> > > requested by user functionality (which one?) -
> > > then it is a bug in i40e RX function selection code that needs to be fixed.
> > > No point to introduce some new config options for that.
> > > Konstantin
> > >
> > Current design of RX function selection for the PMDs make it an
> > initialization time deal. However, the first rte_flow request (thus the need
> > to enable FD / RTE_FDIR_MODE_PERFECT) may come in at any point in
> > time, well after the RX function was selected (e.g., OVS does this when the
> > first packet that can be offloaded arrives). The design in this patch gives
> > such applications a choice to restrict possible RX functions to non-vector
> > paths proactively.
> 
> If you plan to use FD mode on your device, why not enable it
> at setup phase via rte_eth_dev_configure()?
> Then proper rx function would be selected.
> 

FDIR_MODE was designed to bind late automatically -- it is set when the first 
filter rule is parsed, and unset when the last rule is removed. As there are likely
other features not yet supported by the vector path, it felt more appropriate 
to have a generic flag for applications to not choose vector path. In fact, the 
proposed option is the dual of already shipping "use-latest-supported-vec"
flag that lets the apps do the opposite (i.e. choose the "most" vectorized path).

> >
> > > >
> > > > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > > > ---
> > > >  doc/guides/nics/i40e.rst       | 14 +++++++++
> > > >  drivers/net/i40e/i40e_ethdev.c | 70
> > > +++++++++++++++++++++++++++++++++++++++++-
> > > >  drivers/net/i40e/i40e_ethdev.h |  1 +
> > > >  drivers/net/i40e/i40e_rxtx.c   |  4 +++
> > > >  4 files changed, 88 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> > > > index a97484c..532cc64 100644
> > > > --- a/doc/guides/nics/i40e.rst
> > > > +++ b/doc/guides/nics/i40e.rst
> > > > @@ -183,6 +183,20 @@ Runtime Config Options
> > > >
> > > >    -w 84:00.0,use-latest-supported-vec=1
> > > >
> > > > +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> > > > +
> > > > +  When config file option for the vector PMD is enabled, vector RX
> functions
> > > may
> > > > +  be the default choice of the driver at device initialization time, if certain
> > > > +  conditions apply. However, vector RX functions may not always be at
> > > feature
> > > > +  parity with non-vector ones. In order to allow users to override vector
> RX
> > > > +  function selection behavior at runtime, the ``devargs`` parameter
> > > > +  ``disable-vec-rx`` is introduced, such that
> > > > +
> > > > +  -w DBDF,disable-vec-rx=1
> > > > +
> > > > +  would force driver to limit its choices to non-vector RX function variants
> for
> > > > +  the device specified by the DBDF.
> > > > +
> > > >  Vector RX Pre-conditions
> > > >  ~~~~~~~~~~~~~~~~~~~~~~~~
> > > >  For Vector RX it is assumed that the number of descriptor rings will be a
> power
> > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> b/drivers/net/i40e/i40e_ethdev.c
> > > > index cab440f..19fbd23 100644
> > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > @@ -44,6 +44,7 @@
> > > >  #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
> > > >  #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
> > > >  #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> > > > +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
> > > >
> > > >  #define I40E_CLEAR_PXE_WAIT_MS     200
> > > >
> > > > @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
> > > >  	ETH_I40E_SUPPORT_MULTI_DRIVER,
> > > >  	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
> > > >  	ETH_I40E_USE_LATEST_VEC,
> > > > +	ETH_I40E_DISABLE_VEC_RX,
> > > >  	NULL};
> > > >
> > > >  static const struct rte_pci_id pci_id_i40e_map[] = {
> > > > @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int
> > > > +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> > > > +				const char *value,
> > > > +				void *opaque)
> > > > +{
> > > > +	struct i40e_adapter *ad;
> > > > +
> > > > +	ad = (struct i40e_adapter *)opaque;
> > > > +
> > > > +	switch (atoi(value)) {
> > > > +	case 0:
> > > > +		/* Selection of RX vector functions left untouched*/
> > > > +		break;
> > > > +	case 1:
> > > > +		/* Disable RX vector functions as requested*/
> > > > +		ad->rx_vec_allowed = false;
> > > > +	break;
> > > > +	default:
> > > > +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as
> > > 1!");
> > > > +	break;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int
> > > > +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> > > > +{
> > > > +	struct i40e_adapter *ad =
> > > > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > > > +	struct rte_kvargs *kvlist;
> > > > +	int kvargs_count;
> > > > +
> > > > +
> > > > +	if (!dev->device->devargs)
> > > > +		return 0;
> > > > +
> > > > +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> > > > +	if (!kvlist)
> > > > +		return -EINVAL;
> > > > +
> > > > +	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_DISABLE_VEC_RX);
> > > > +	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 invalid or last valid one is used !",
> > > > +			    ETH_I40E_DISABLE_VEC_RX);
> > > > +
> > > > +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> > > > +				i40e_parse_disable_vec_rx_handler, ad) < 0) {
> > > > +		rte_kvargs_free(kvlist);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	rte_kvargs_free(kvlist);
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  #define I40E_ALARM_INTERVAL 50000 /* us */
> > > >
> > > >  static int
> > > > @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
> > > >  	ad->tx_simple_allowed = true;
> > > >  	ad->tx_vec_allowed = true;
> > > >
> > > > +	/* Check if users wanted to disable vector RX functions */
> > > > +	i40e_disable_vec_rx(dev);
> > > > +
> > > >  	/* Only legacy filter API needs the following fdir config. So when the
> > > >  	 * legacy filter API is deprecated, the following codes should also be
> > > >  	 * removed.
> > > > @@ -12790,4 +12857,5 @@
> RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
> > > >  			      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_USE_LATEST_VEC "=0|1");
> > > > +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> > > > +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> > > > diff --git a/drivers/net/i40e/i40e_ethdev.h
> b/drivers/net/i40e/i40e_ethdev.h
> > > > index 9855038..906bfe9 100644
> > > > --- a/drivers/net/i40e/i40e_ethdev.h
> > > > +++ b/drivers/net/i40e/i40e_ethdev.h
> > > > @@ -1248,6 +1248,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);
> > > > +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct
> > > rte_eth_dev *dev,
> > > >  		 */
> > > >  		ad->rx_bulk_alloc_allowed = true;
> > > >  		ad->rx_vec_allowed = true;
> > > > +
> > > > +		/* Check if users wanted to disable vector RX functions */
> > > > +		i40e_disable_vec_rx(dev);
> > > > +
> > > >  		dev->data->scattered_rx = use_scattered_rx;
> > > >  		if (use_def_burst_func)
> > > >  			ad->rx_bulk_alloc_allowed = false;
> > > > --
> > > > 2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ananyev, Konstantin]

> > > >
> > > > >
> > > > > Vector RX functions are not at feature parity with non-vector ones and
> > > > > currently the vector RX path is enabled by default. Hence, the only
> > > > > option to force selection of non-vector variants and be able to retain
> > > > > functionality is to disable vector PMD globally at compile time via the
> > > > > config file option.
> > > > >
> > > > > This patch introduces a new runtime device config option named
> > > > > disable-vec-rx to allow users to limit the driver to make a choice among
> > > > > non-vector RX functions, on a per device basis. This runtime option
> > > > > defaults to a value of zero, allowing vector RX functions to be
> > > > > selected (current behavior). When disable-vec-rx=1 is specified for a
> > > > > device, even if all the other requirements to select a vector RX
> > > > > function are satisfied, the driver will still pick one out of the
> > > > > appropriate non-vector RX functions.
> > > >
> > > > Not sure I understand the logic of that patch.
> > > > If i40e RX PMD selects wrong RX function that doesn't provide
> > > > requested by user functionality (which one?) -
> > > > then it is a bug in i40e RX function selection code that needs to be fixed.
> > > > No point to introduce some new config options for that.
> > > > Konstantin
> > > >
> > > Current design of RX function selection for the PMDs make it an
> > > initialization time deal. However, the first rte_flow request (thus the need
> > > to enable FD / RTE_FDIR_MODE_PERFECT) may come in at any point in
> > > time, well after the RX function was selected (e.g., OVS does this when the
> > > first packet that can be offloaded arrives). The design in this patch gives
> > > such applications a choice to restrict possible RX functions to non-vector
> > > paths proactively.
> >
> > If you plan to use FD mode on your device, why not enable it
> > at setup phase via rte_eth_dev_configure()?
> > Then proper rx function would be selected.
> >
> 
> FDIR_MODE was designed to bind late automatically -- it is set when the first
> filter rule is parsed, and unset when the last rule is removed.

Why is that, if you can define it at configuration stage, and RX function
selection is based on it?

> As there are likely
> other features not yet supported by the vector path, it felt more appropriate
> to have a generic flag for applications to not choose vector path.

Once again - if some vector RX doesn't support particular requested at config
feature, it wouldn't be selected. If it is not the case, then it is a bug in
rx function selection code and should be fixed, instead of introducing workaround flags.

> In fact, the
> proposed option is the dual of already shipping use-latest-supported-vec
> flag that lets the apps do the opposite (i.e. choose the "most" vectorized path).

I don't think that's the same thing.
From my perspective 'use-latest-supported-vec' is to overcome HW limitations.

In your case, as I can see, all you need is to declare to HW/PMD that you plan
to use FD HW capabilities and proper RX function will be selected automatically
(pretty much as with other HW offloads, TX cksum, RX multi-seg, etc.). 


> 
> > >
> > > > >
> > > > > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > > > > ---
> > > > >  doc/guides/nics/i40e.rst       | 14 +++++++++
> > > > >  drivers/net/i40e/i40e_ethdev.c | 70
> > > > +++++++++++++++++++++++++++++++++++++++++-
> > > > >  drivers/net/i40e/i40e_ethdev.h |  1 +
> > > > >  drivers/net/i40e/i40e_rxtx.c   |  4 +++
> > > > >  4 files changed, 88 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> > > > > index a97484c..532cc64 100644
> > > > > --- a/doc/guides/nics/i40e.rst
> > > > > +++ b/doc/guides/nics/i40e.rst
> > > > > @@ -183,6 +183,20 @@ Runtime Config Options
> > > > >
> > > > >    -w 84:00.0,use-latest-supported-vec=1
> > > > >
> > > > > +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> > > > > +
> > > > > +  When config file option for the vector PMD is enabled, vector RX
> > functions
> > > > may
> > > > > +  be the default choice of the driver at device initialization time, if certain
> > > > > +  conditions apply. However, vector RX functions may not always be at
> > > > feature
> > > > > +  parity with non-vector ones. In order to allow users to override vector
> > RX
> > > > > +  function selection behavior at runtime, the ``devargs`` parameter
> > > > > +  ``disable-vec-rx`` is introduced, such that
> > > > > +
> > > > > +  -w DBDF,disable-vec-rx=1
> > > > > +
> > > > > +  would force driver to limit its choices to non-vector RX function variants
> > for
> > > > > +  the device specified by the DBDF.
> > > > > +
> > > > >  Vector RX Pre-conditions
> > > > >  ~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >  For Vector RX it is assumed that the number of descriptor rings will be a
> > power
> > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c
> > > > > index cab440f..19fbd23 100644
> > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > @@ -44,6 +44,7 @@
> > > > >  #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
> > > > >  #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
> > > > >  #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> > > > > +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
> > > > >
> > > > >  #define I40E_CLEAR_PXE_WAIT_MS     200
> > > > >
> > > > > @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
> > > > >  	ETH_I40E_SUPPORT_MULTI_DRIVER,
> > > > >  	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
> > > > >  	ETH_I40E_USE_LATEST_VEC,
> > > > > +	ETH_I40E_DISABLE_VEC_RX,
> > > > >  	NULL};
> > > > >
> > > > >  static const struct rte_pci_id pci_id_i40e_map[] = {
> > > > > @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +static int
> > > > > +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> > > > > +				const char *value,
> > > > > +				void *opaque)
> > > > > +{
> > > > > +	struct i40e_adapter *ad;
> > > > > +
> > > > > +	ad = (struct i40e_adapter *)opaque;
> > > > > +
> > > > > +	switch (atoi(value)) {
> > > > > +	case 0:
> > > > > +		/* Selection of RX vector functions left untouched*/
> > > > > +		break;
> > > > > +	case 1:
> > > > > +		/* Disable RX vector functions as requested*/
> > > > > +		ad->rx_vec_allowed = false;
> > > > > +	break;
> > > > > +	default:
> > > > > +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as
> > > > 1!");
> > > > > +	break;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +int
> > > > > +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> > > > > +{
> > > > > +	struct i40e_adapter *ad =
> > > > > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > > > > +	struct rte_kvargs *kvlist;
> > > > > +	int kvargs_count;
> > > > > +
> > > > > +
> > > > > +	if (!dev->device->devargs)
> > > > > +		return 0;
> > > > > +
> > > > > +	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
> > > > > +	if (!kvlist)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_DISABLE_VEC_RX);
> > > > > +	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 invalid or last valid one is used !",
> > > > > +			    ETH_I40E_DISABLE_VEC_RX);
> > > > > +
> > > > > +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> > > > > +				i40e_parse_disable_vec_rx_handler, ad) < 0) {
> > > > > +		rte_kvargs_free(kvlist);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	rte_kvargs_free(kvlist);
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  #define I40E_ALARM_INTERVAL 50000 /* us */
> > > > >
> > > > >  static int
> > > > > @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
> > > > >  	ad->tx_simple_allowed = true;
> > > > >  	ad->tx_vec_allowed = true;
> > > > >
> > > > > +	/* Check if users wanted to disable vector RX functions */
> > > > > +	i40e_disable_vec_rx(dev);
> > > > > +
> > > > >  	/* Only legacy filter API needs the following fdir config. So when the
> > > > >  	 * legacy filter API is deprecated, the following codes should also be
> > > > >  	 * removed.
> > > > > @@ -12790,4 +12857,5 @@
> > RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
> > > > >  			      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_USE_LATEST_VEC "=0|1");
> > > > > +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> > > > > +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> > > > > diff --git a/drivers/net/i40e/i40e_ethdev.h
> > b/drivers/net/i40e/i40e_ethdev.h
> > > > > index 9855038..906bfe9 100644
> > > > > --- a/drivers/net/i40e/i40e_ethdev.h
> > > > > +++ b/drivers/net/i40e/i40e_ethdev.h
> > > > > @@ -1248,6 +1248,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);
> > > > > +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct
> > > > rte_eth_dev *dev,
> > > > >  		 */
> > > > >  		ad->rx_bulk_alloc_allowed = true;
> > > > >  		ad->rx_vec_allowed = true;
> > > > > +
> > > > > +		/* Check if users wanted to disable vector RX functions */
> > > > > +		i40e_disable_vec_rx(dev);
> > > > > +
> > > > >  		dev->data->scattered_rx = use_scattered_rx;
> > > > >  		if (use_def_burst_func)
> > > > >  			ad->rx_bulk_alloc_allowed = false;
> > > > > --
> > > > > 2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ergin, Mesut A]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, May 21, 2019 10:21 AM
> To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable
> vector rx
> 
> 
> 
> > > > >
> > > > > >
> > > > > > Vector RX functions are not at feature parity with non-vector ones and
> > > > > > currently the vector RX path is enabled by default. Hence, the only
> > > > > > option to force selection of non-vector variants and be able to retain
> > > > > > functionality is to disable vector PMD globally at compile time via the
> > > > > > config file option.
> > > > > >
> > > > > > This patch introduces a new runtime device config option named
> > > > > > disable-vec-rx to allow users to limit the driver to make a choice among
> > > > > > non-vector RX functions, on a per device basis. This runtime option
> > > > > > defaults to a value of zero, allowing vector RX functions to be
> > > > > > selected (current behavior). When disable-vec-rx=1 is specified for a
> > > > > > device, even if all the other requirements to select a vector RX
> > > > > > function are satisfied, the driver will still pick one out of the
> > > > > > appropriate non-vector RX functions.
> > > > >
> > > > > Not sure I understand the logic of that patch.
> > > > > If i40e RX PMD selects wrong RX function that doesn't provide
> > > > > requested by user functionality (which one?) -
> > > > > then it is a bug in i40e RX function selection code that needs to be fixed.
> > > > > No point to introduce some new config options for that.
> > > > > Konstantin
> > > > >
> > > > Current design of RX function selection for the PMDs make it an
> > > > initialization time deal. However, the first rte_flow request (thus the need
> > > > to enable FD / RTE_FDIR_MODE_PERFECT) may come in at any point in
> > > > time, well after the RX function was selected (e.g., OVS does this when the
> > > > first packet that can be offloaded arrives). The design in this patch gives
> > > > such applications a choice to restrict possible RX functions to non-vector
> > > > paths proactively.
> > >
> > > If you plan to use FD mode on your device, why not enable it
> > > at setup phase via rte_eth_dev_configure()?
> > > Then proper rx function would be selected.
> > >
> >
> > FDIR_MODE was designed to bind late automatically -- it is set when the first
> > filter rule is parsed, and unset when the last rule is removed.
> 
> Why is that, if you can define it at configuration stage, and RX function
> selection is based on it?

I don't know why it was designed that way -- maybe maintainers know the 
historical context.

> 
> > As there are likely
> > other features not yet supported by the vector path, it felt more appropriate
> > to have a generic flag for applications to not choose vector path.
> 
> Once again - if some vector RX doesn't support particular requested at config
> feature, it wouldn't be selected. If it is not the case, then it is a bug in
> rx function selection code and should be fixed, instead of introducing
> workaround flags.
> 

In any case, this has to be tied to a command line option, either on the application
side (i.e. OVS) or DPDK devargs side, since we don't want to hard code it to limit
RX functions to non-vector ones application-wide at compile time.

As far as I can see, passing FDIR configuration via the rte_eth_conf struct:
   struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
was deprecated. I suspect in favor of the late binding design mentioned, but 
again, I don't know the history on that. IMO, this made devargs a better choice.

> > In fact, the
> > proposed option is the dual of already shipping use-latest-supported-vec
> > flag that lets the apps do the opposite (i.e. choose the "most" vectorized
> path).
> 
> I don't think that's the same thing.
> From my perspective 'use-latest-supported-vec' is to overcome HW limitations.
>

That's fine, we seem to have different opinions on them. To me, both are 
runtime configuration options that let users select what RX functions they 
prefer to use with their HW.
 
> In your case, as I can see, all you need is to declare to HW/PMD that you plan
> to use FD HW capabilities and proper RX function will be selected automatically
> (pretty much as with other HW offloads, TX cksum, RX multi-seg, etc.).
> 

Offload configurations you mentioned can be passed on to DPDK from the 
application via offloads field in rte_eth_conf, FD cannot.

> 
> >
> > > >
> > > > > >
> > > > > > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > > > > > ---
> > > > > >  doc/guides/nics/i40e.rst       | 14 +++++++++
> > > > > >  drivers/net/i40e/i40e_ethdev.c | 70
> > > > > +++++++++++++++++++++++++++++++++++++++++-
> > > > > >  drivers/net/i40e/i40e_ethdev.h |  1 +
> > > > > >  drivers/net/i40e/i40e_rxtx.c   |  4 +++
> > > > > >  4 files changed, 88 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> > > > > > index a97484c..532cc64 100644
> > > > > > --- a/doc/guides/nics/i40e.rst
> > > > > > +++ b/doc/guides/nics/i40e.rst
> > > > > > @@ -183,6 +183,20 @@ Runtime Config Options
> > > > > >
> > > > > >    -w 84:00.0,use-latest-supported-vec=1
> > > > > >
> > > > > > +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> > > > > > +
> > > > > > +  When config file option for the vector PMD is enabled, vector RX
> > > functions
> > > > > may
> > > > > > +  be the default choice of the driver at device initialization time, if
> certain
> > > > > > +  conditions apply. However, vector RX functions may not always be at
> > > > > feature
> > > > > > +  parity with non-vector ones. In order to allow users to override
> vector
> > > RX
> > > > > > +  function selection behavior at runtime, the ``devargs`` parameter
> > > > > > +  ``disable-vec-rx`` is introduced, such that
> > > > > > +
> > > > > > +  -w DBDF,disable-vec-rx=1
> > > > > > +
> > > > > > +  would force driver to limit its choices to non-vector RX function
> variants
> > > for
> > > > > > +  the device specified by the DBDF.
> > > > > > +
> > > > > >  Vector RX Pre-conditions
> > > > > >  ~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > >  For Vector RX it is assumed that the number of descriptor rings will be
> a
> > > power
> > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > b/drivers/net/i40e/i40e_ethdev.c
> > > > > > index cab440f..19fbd23 100644
> > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > @@ -44,6 +44,7 @@
> > > > > >  #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
> > > > > >  #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
> > > > > >  #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> > > > > > +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
> > > > > >
> > > > > >  #define I40E_CLEAR_PXE_WAIT_MS     200
> > > > > >
> > > > > > @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
> > > > > >  	ETH_I40E_SUPPORT_MULTI_DRIVER,
> > > > > >  	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
> > > > > >  	ETH_I40E_USE_LATEST_VEC,
> > > > > > +	ETH_I40E_DISABLE_VEC_RX,
> > > > > >  	NULL};
> > > > > >
> > > > > >  static const struct rte_pci_id pci_id_i40e_map[] = {
> > > > > > @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev
> *dev)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int
> > > > > > +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> > > > > > +				const char *value,
> > > > > > +				void *opaque)
> > > > > > +{
> > > > > > +	struct i40e_adapter *ad;
> > > > > > +
> > > > > > +	ad = (struct i40e_adapter *)opaque;
> > > > > > +
> > > > > > +	switch (atoi(value)) {
> > > > > > +	case 0:
> > > > > > +		/* Selection of RX vector functions left untouched*/
> > > > > > +		break;
> > > > > > +	case 1:
> > > > > > +		/* Disable RX vector functions as requested*/
> > > > > > +		ad->rx_vec_allowed = false;
> > > > > > +	break;
> > > > > > +	default:
> > > > > > +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set
> it as
> > > > > 1!");
> > > > > > +	break;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +int
> > > > > > +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> > > > > > +{
> > > > > > +	struct i40e_adapter *ad =
> > > > > > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data-
> >dev_private);
> > > > > > +	struct rte_kvargs *kvlist;
> > > > > > +	int kvargs_count;
> > > > > > +
> > > > > > +
> > > > > > +	if (!dev->device->devargs)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	kvlist = rte_kvargs_parse(dev->device->devargs->args,
> valid_keys);
> > > > > > +	if (!kvlist)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	kvargs_count = rte_kvargs_count(kvlist,
> ETH_I40E_DISABLE_VEC_RX);
> > > > > > +	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 invalid or last valid one is used !",
> > > > > > +			    ETH_I40E_DISABLE_VEC_RX);
> > > > > > +
> > > > > > +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> > > > > > +				i40e_parse_disable_vec_rx_handler,
> ad) < 0) {
> > > > > > +		rte_kvargs_free(kvlist);
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	rte_kvargs_free(kvlist);
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  #define I40E_ALARM_INTERVAL 50000 /* us */
> > > > > >
> > > > > >  static int
> > > > > > @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev
> *dev)
> > > > > >  	ad->tx_simple_allowed = true;
> > > > > >  	ad->tx_vec_allowed = true;
> > > > > >
> > > > > > +	/* Check if users wanted to disable vector RX functions */
> > > > > > +	i40e_disable_vec_rx(dev);
> > > > > > +
> > > > > >  	/* Only legacy filter API needs the following fdir config. So
> when the
> > > > > >  	 * legacy filter API is deprecated, the following codes should
> also be
> > > > > >  	 * removed.
> > > > > > @@ -12790,4 +12857,5 @@
> > > RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
> > > > > >  			      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_USE_LATEST_VEC "=0|1");
> > > > > > +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> > > > > > +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.h
> > > b/drivers/net/i40e/i40e_ethdev.h
> > > > > > index 9855038..906bfe9 100644
> > > > > > --- a/drivers/net/i40e/i40e_ethdev.h
> > > > > > +++ b/drivers/net/i40e/i40e_ethdev.h
> > > > > > @@ -1248,6 +1248,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);
> > > > > > +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct
> > > > > rte_eth_dev *dev,
> > > > > >  		 */
> > > > > >  		ad->rx_bulk_alloc_allowed = true;
> > > > > >  		ad->rx_vec_allowed = true;
> > > > > > +
> > > > > > +		/* Check if users wanted to disable vector RX functions
> */
> > > > > > +		i40e_disable_vec_rx(dev);
> > > > > > +
> > > > > >  		dev->data->scattered_rx = use_scattered_rx;
> > > > > >  		if (use_def_burst_func)
> > > > > >  			ad->rx_bulk_alloc_allowed = false;
> > > > > > --
> > > > > > 2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ananyev, Konstantin]

> > > > > > >
> > > > > > > Vector RX functions are not at feature parity with non-vector ones and
> > > > > > > currently the vector RX path is enabled by default. Hence, the only
> > > > > > > option to force selection of non-vector variants and be able to retain
> > > > > > > functionality is to disable vector PMD globally at compile time via the
> > > > > > > config file option.
> > > > > > >
> > > > > > > This patch introduces a new runtime device config option named
> > > > > > > disable-vec-rx to allow users to limit the driver to make a choice among
> > > > > > > non-vector RX functions, on a per device basis. This runtime option
> > > > > > > defaults to a value of zero, allowing vector RX functions to be
> > > > > > > selected (current behavior). When disable-vec-rx=1 is specified for a
> > > > > > > device, even if all the other requirements to select a vector RX
> > > > > > > function are satisfied, the driver will still pick one out of the
> > > > > > > appropriate non-vector RX functions.
> > > > > >
> > > > > > Not sure I understand the logic of that patch.
> > > > > > If i40e RX PMD selects wrong RX function that doesn't provide
> > > > > > requested by user functionality (which one?) -
> > > > > > then it is a bug in i40e RX function selection code that needs to be fixed.
> > > > > > No point to introduce some new config options for that.
> > > > > > Konstantin
> > > > > >
> > > > > Current design of RX function selection for the PMDs make it an
> > > > > initialization time deal. However, the first rte_flow request (thus the need
> > > > > to enable FD / RTE_FDIR_MODE_PERFECT) may come in at any point in
> > > > > time, well after the RX function was selected (e.g., OVS does this when the
> > > > > first packet that can be offloaded arrives). The design in this patch gives
> > > > > such applications a choice to restrict possible RX functions to non-vector
> > > > > paths proactively.
> > > >
> > > > If you plan to use FD mode on your device, why not enable it
> > > > at setup phase via rte_eth_dev_configure()?
> > > > Then proper rx function would be selected.
> > > >
> > >
> > > FDIR_MODE was designed to bind late automatically -- it is set when the first
> > > filter rule is parsed, and unset when the last rule is removed.
> >
> > Why is that, if you can define it at configuration stage, and RX function
> > selection is based on it?
> 
> I don't know why it was designed that way -- maybe maintainers know the
> historical context.

What I am trying to say - if particular feature (FD) enablement/disablement
affects RX/TX function selection, then there should be an ability to enable/disable
that feature at configuration state (dev_config/queue_setup).
Function to change value of that feature at runtime (after dev_start/queue_start)
should return an error if new value can't be supported with already selected RX/TX
function. 
If that's is not the case, then it sounds like a bug/gap in i40e driver that needs to be fixed.
 
> 
> >
> > > As there are likely
> > > other features not yet supported by the vector path, it felt more appropriate
> > > to have a generic flag for applications to not choose vector path.
> >
> > Once again - if some vector RX doesn't support particular requested at config
> > feature, it wouldn't be selected. If it is not the case, then it is a bug in
> > rx function selection code and should be fixed, instead of introducing
> > workaround flags.
> >
> 
> In any case, this has to be tied to a command line option, either on the application
> side (i.e. OVS)
> or DPDK devargs side, since we don't want to hard code it to limit
> RX functions to non-vector ones application-wide at compile time.

I am not forcing you to use hard-coded values, I am saying that we need to
follow standard API to enable/disable HW/PMD features.
If there is a problem in that API/implementation - we need to fix it,
not introduce some hackery workarounds (new devargs or so).
Obviously i40e RX function variations (vector/scalar/etc.) 
are PMD specific details that user shouldn't know about.
Let say tomorrow someone can add support for FD into i40e vector-RX,
or completely new RX function will be introduced for i40e that will
void current selection logic.
Again i40e is not the only PMD that supports FD, so we need generic and well
defined way to enable/disable it. 

> 
> As far as I can see, passing FDIR configuration via the rte_eth_conf struct:
>    struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> was deprecated. I suspect in favor of the late binding design mentioned, but
> again, I don't know the history on that. IMO, this made devargs a better choice.

Ok, then it looks like there is a flaw in ethdev level API that needs to be fixed:
We deprecated old way to request FD usage without introducing new one.
CC-ing to ethdev maintainers -
Guys is there a new way to request FD enablement, instead of deprecated fdir_config?
Seems like not, unless I missed something obvious.
If not, then we probably need either to re-deprecate fdir_config, or introduce some new method.
My first thought would be to add new  DEV_RX_OFFLOAD_* flag(s).
Does it make sense?
Konstantin

> 
> > > In fact, the
> > > proposed option is the dual of already shipping use-latest-supported-vec
> > > flag that lets the apps do the opposite (i.e. choose the "most" vectorized
> > path).
> >
> > I don't think that's the same thing.
> > From my perspective 'use-latest-supported-vec' is to overcome HW limitations.
> >
> 
> That's fine, we seem to have different opinions on them. To me, both are
> runtime configuration options that let users select what RX functions they
> prefer to use with their HW.
> 
> > In your case, as I can see, all you need is to declare to HW/PMD that you plan
> > to use FD HW capabilities and proper RX function will be selected automatically
> > (pretty much as with other HW offloads, TX cksum, RX multi-seg, etc.).
> >
> 
> Offload configurations you mentioned can be passed on to DPDK from the
> application via offloads field in rte_eth_conf, FD cannot.
> 
> >
> > >
> > > > >
> > > > > > >
> > > > > > > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > > > > > > ---
> > > > > > >  doc/guides/nics/i40e.rst       | 14 +++++++++
> > > > > > >  drivers/net/i40e/i40e_ethdev.c | 70
> > > > > > +++++++++++++++++++++++++++++++++++++++++-
> > > > > > >  drivers/net/i40e/i40e_ethdev.h |  1 +
> > > > > > >  drivers/net/i40e/i40e_rxtx.c   |  4 +++
> > > > > > >  4 files changed, 88 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> > > > > > > index a97484c..532cc64 100644
> > > > > > > --- a/doc/guides/nics/i40e.rst
> > > > > > > +++ b/doc/guides/nics/i40e.rst
> > > > > > > @@ -183,6 +183,20 @@ Runtime Config Options
> > > > > > >
> > > > > > >    -w 84:00.0,use-latest-supported-vec=1
> > > > > > >
> > > > > > > +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> > > > > > > +
> > > > > > > +  When config file option for the vector PMD is enabled, vector RX
> > > > functions
> > > > > > may
> > > > > > > +  be the default choice of the driver at device initialization time, if
> > certain
> > > > > > > +  conditions apply. However, vector RX functions may not always be at
> > > > > > feature
> > > > > > > +  parity with non-vector ones. In order to allow users to override
> > vector
> > > > RX
> > > > > > > +  function selection behavior at runtime, the ``devargs`` parameter
> > > > > > > +  ``disable-vec-rx`` is introduced, such that
> > > > > > > +
> > > > > > > +  -w DBDF,disable-vec-rx=1
> > > > > > > +
> > > > > > > +  would force driver to limit its choices to non-vector RX function
> > variants
> > > > for
> > > > > > > +  the device specified by the DBDF.
> > > > > > > +
> > > > > > >  Vector RX Pre-conditions
> > > > > > >  ~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > >  For Vector RX it is assumed that the number of descriptor rings will be
> > a
> > > > power
> > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > index cab440f..19fbd23 100644
> > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > @@ -44,6 +44,7 @@
> > > > > > >  #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
> > > > > > >  #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
> > > > > > >  #define ETH_I40E_USE_LATEST_VEC	"use-latest-supported-vec"
> > > > > > > +#define ETH_I40E_DISABLE_VEC_RX	"disable-vec-rx"
> > > > > > >
> > > > > > >  #define I40E_CLEAR_PXE_WAIT_MS     200
> > > > > > >
> > > > > > > @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
> > > > > > >  	ETH_I40E_SUPPORT_MULTI_DRIVER,
> > > > > > >  	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
> > > > > > >  	ETH_I40E_USE_LATEST_VEC,
> > > > > > > +	ETH_I40E_DISABLE_VEC_RX,
> > > > > > >  	NULL};
> > > > > > >
> > > > > > >  static const struct rte_pci_id pci_id_i40e_map[] = {
> > > > > > > @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev
> > *dev)
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +static int
> > > > > > > +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> > > > > > > +				const char *value,
> > > > > > > +				void *opaque)
> > > > > > > +{
> > > > > > > +	struct i40e_adapter *ad;
> > > > > > > +
> > > > > > > +	ad = (struct i40e_adapter *)opaque;
> > > > > > > +
> > > > > > > +	switch (atoi(value)) {
> > > > > > > +	case 0:
> > > > > > > +		/* Selection of RX vector functions left untouched*/
> > > > > > > +		break;
> > > > > > > +	case 1:
> > > > > > > +		/* Disable RX vector functions as requested*/
> > > > > > > +		ad->rx_vec_allowed = false;
> > > > > > > +	break;
> > > > > > > +	default:
> > > > > > > +		PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set
> > it as
> > > > > > 1!");
> > > > > > > +	break;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +int
> > > > > > > +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> > > > > > > +{
> > > > > > > +	struct i40e_adapter *ad =
> > > > > > > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data-
> > >dev_private);
> > > > > > > +	struct rte_kvargs *kvlist;
> > > > > > > +	int kvargs_count;
> > > > > > > +
> > > > > > > +
> > > > > > > +	if (!dev->device->devargs)
> > > > > > > +		return 0;
> > > > > > > +
> > > > > > > +	kvlist = rte_kvargs_parse(dev->device->devargs->args,
> > valid_keys);
> > > > > > > +	if (!kvlist)
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	kvargs_count = rte_kvargs_count(kvlist,
> > ETH_I40E_DISABLE_VEC_RX);
> > > > > > > +	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 invalid or last valid one is used !",
> > > > > > > +			    ETH_I40E_DISABLE_VEC_RX);
> > > > > > > +
> > > > > > > +	if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> > > > > > > +				i40e_parse_disable_vec_rx_handler,
> > ad) < 0) {
> > > > > > > +		rte_kvargs_free(kvlist);
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	rte_kvargs_free(kvlist);
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  #define I40E_ALARM_INTERVAL 50000 /* us */
> > > > > > >
> > > > > > >  static int
> > > > > > > @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev
> > *dev)
> > > > > > >  	ad->tx_simple_allowed = true;
> > > > > > >  	ad->tx_vec_allowed = true;
> > > > > > >
> > > > > > > +	/* Check if users wanted to disable vector RX functions */
> > > > > > > +	i40e_disable_vec_rx(dev);
> > > > > > > +
> > > > > > >  	/* Only legacy filter API needs the following fdir config. So
> > when the
> > > > > > >  	 * legacy filter API is deprecated, the following codes should
> > also be
> > > > > > >  	 * removed.
> > > > > > > @@ -12790,4 +12857,5 @@
> > > > RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
> > > > > > >  			      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_USE_LATEST_VEC "=0|1");
> > > > > > > +			      ETH_I40E_USE_LATEST_VEC "=0|1"
> > > > > > > +			      ETH_I40E_DISABLE_VEC_RX "=0|1");
> > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.h
> > > > b/drivers/net/i40e/i40e_ethdev.h
> > > > > > > index 9855038..906bfe9 100644
> > > > > > > --- a/drivers/net/i40e/i40e_ethdev.h
> > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.h
> > > > > > > @@ -1248,6 +1248,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);
> > > > > > > +int i40e_disable_vec_rx(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 1489552..7e66f59 100644
> > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct
> > > > > > rte_eth_dev *dev,
> > > > > > >  		 */
> > > > > > >  		ad->rx_bulk_alloc_allowed = true;
> > > > > > >  		ad->rx_vec_allowed = true;
> > > > > > > +
> > > > > > > +		/* Check if users wanted to disable vector RX functions
> > */
> > > > > > > +		i40e_disable_vec_rx(dev);
> > > > > > > +
> > > > > > >  		dev->data->scattered_rx = use_scattered_rx;
> > > > > > >  		if (use_def_burst_func)
> > > > > > >  			ad->rx_bulk_alloc_allowed = false;
> > > > > > > --
> > > > > > > 2.7.4

Re: [dpdk-dev] [PATCH 1/3] net/i40e: add support for MARK + RSS action in rte_flow

[Zhang, Qi Z]

> -----Original Message-----
> From: Ergin, Mesut A
> Sent: Thursday, May 16, 2019 12:28 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> Subject: [PATCH 1/3] net/i40e: add support for MARK + RSS action in rte_flow
> 
> Currently, i40e Flow Director action parser only allows following nine action
> combinations: (QUEUE, PASSTHRU, DROP, QUEUE + MARK, PASSTHRU + MARK,
> DROP + MARK, QUEUE + FLAG, PASSTHRU + FLAG, DROP + FLAG)
> 
> Using the existing Cloud Filter profile on the NIC, it is possible to add support
> for two more combinations as: (MARK + RSS, MARK + FLAG +
> RSS)
> 
> Addition of these new combinations would allow more applications to utilize
> DPDK rte_flow to implement hardware flow offloads with Intel Ethernet 700
> series network adapters, including but not limited to the existing OVS DPDK
> partial hardware flow offload feature.
> 
> Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance

[Zhang, Qi Z]

Hi Mesut:

> -----Original Message-----
> From: Ergin, Mesut A
> Sent: Thursday, May 16, 2019 12:28 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> Subject: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance
> 
> When i40e_rx_vec_dev_conf_condition_check_default() determines whether
> vector receive functions would be allowed during initialization phase, it should
> honor previously recorded disallowance during configuration phase, and not
> override simply because it is for the first queue.
> 
> Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index 0e6ffa0..f30cab4 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -212,6 +212,10 @@
> i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
>  		return -1;
> 
> +	/* Should not override if vector was already disallowed */

It is possible a device be reconfigured between dev_stop/dev_start, vector mode may fit for the new configure, so the old rx_vec_allowd should be ignored, 

Regards
Qi

> +	if (!ad->rx_vec_allowed)
> +	return -1;
> +
>  	/**
>  	 * Vector mode is allowed only when number of Rx queue
>  	 * descriptor is power of 2.
> --
> 2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Thomas Monjalon]

Hi,

22/05/2019 13:01, Ananyev, Konstantin:
> > As far as I can see, passing FDIR configuration via the rte_eth_conf struct:
> >    struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */
> > was deprecated. I suspect in favor of the late binding design mentioned, but
> > again, I don't know the history on that. IMO, this made devargs a better choice.
> 
> Ok, then it looks like there is a flaw in ethdev level API that needs to be fixed:
> We deprecated old way to request FD usage without introducing new one.
> CC-ing to ethdev maintainers -
> Guys is there a new way to request FD enablement, instead of deprecated fdir_config?
> Seems like not, unless I missed something obvious.
> If not, then we probably need either to re-deprecate fdir_config, or introduce some new method.
> My first thought would be to add new  DEV_RX_OFFLOAD_* flag(s).
> Does it make sense?

Sorry,I have not read the full thread so I may be out of topic.
Please be aware that the flow director API is deprecated in favor
of the more generic rte_flow API.

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Zhang, Qi Z]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, May 22, 2019 10:05 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org;
> Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable
> vector rx
> 
> Hi,
> 
> 22/05/2019 13:01, Ananyev, Konstantin:
> > > As far as I can see, passing FDIR configuration via the rte_eth_conf struct:
> > >    struct rte_fdir_conf fdir_conf; /**< FDIR configuration.
> > > DEPRECATED */ was deprecated. I suspect in favor of the late binding
> > > design mentioned, but again, I don't know the history on that. IMO, this
> made devargs a better choice.
> >
> > Ok, then it looks like there is a flaw in ethdev level API that needs to be fixed:
> > We deprecated old way to request FD usage without introducing new one.
> > CC-ing to ethdev maintainers -
> > Guys is there a new way to request FD enablement, instead of deprecated
> fdir_config?
> > Seems like not, unless I missed something obvious.
> > If not, then we probably need either to re-deprecate fdir_config, or introduce
> some new method.
> > My first thought would be to add new  DEV_RX_OFFLOAD_* flag(s).
> > Does it make sense?
> 
> Sorry,I have not read the full thread so I may be out of topic.
> Please be aware that the flow director API is deprecated in favor of the more
> generic rte_flow API.
> 

What we need is a software mark when a flow is hit, it is stored in mbuf->fdir (there is another discussion to change the name "fdir" to a more generic one)
For intel driver, vector rx Path does not support software mark, so currently we use rte_eth_conf-> rte_fdir_conf->mode to prevent vector path be selected when fdir is required.
actually this is not make very sense, vector path is only necessary to be disabled when software mark is required, but not for general fdir
Now since it will be removed, it's a good chance to improve this, a new offload flag such as DEV_RX_OFFLOAD_FLOW_MARK looks like what we needed..

Regards
Qi

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Jerin Jacob Kollanukkaran]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Zhang, Qi Z
> Sent: Wednesday, May 22, 2019 8:03 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable
> vector rx
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Wednesday, May 22, 2019 10:05 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>
> > Subject: Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to
> > disable vector rx
> >
> > Hi,
> >
> > 22/05/2019 13:01, Ananyev, Konstantin:
> > > > As far as I can see, passing FDIR configuration via the rte_eth_conf
> struct:
> > > >    struct rte_fdir_conf fdir_conf; /**< FDIR configuration.
> > > > DEPRECATED */ was deprecated. I suspect in favor of the late
> > > > binding design mentioned, but again, I don't know the history on
> > > > that. IMO, this
> > made devargs a better choice.
> > >
> > > Ok, then it looks like there is a flaw in ethdev level API that needs to be
> fixed:
> > > We deprecated old way to request FD usage without introducing new
> one.
> > > CC-ing to ethdev maintainers -
> > > Guys is there a new way to request FD enablement, instead of
> > > deprecated
> > fdir_config?
> > > Seems like not, unless I missed something obvious.
> > > If not, then we probably need either to re-deprecate fdir_config, or
> > > introduce
> > some new method.
> > > My first thought would be to add new  DEV_RX_OFFLOAD_* flag(s).
> > > Does it make sense?
> >
> > Sorry,I have not read the full thread so I may be out of topic.
> > Please be aware that the flow director API is deprecated in favor of
> > the more generic rte_flow API.
> >
> 
> What we need is a software mark when a flow is hit, it is stored in mbuf->fdir
> (there is another discussion to change the name "fdir" to a more generic
> one) For intel driver, vector rx Path does not support software mark, so
> currently we use rte_eth_conf-> rte_fdir_conf->mode to prevent vector
> path be selected when fdir is required.
> actually this is not make very sense, vector path is only necessary to be
> disabled when software mark is required, but not for general fdir Now since
> it will be removed, it's a good chance to improve this, a new offload flag such
> as DEV_RX_OFFLOAD_FLOW_MARK looks like what we needed..

+1 to a new offload flag.


> 
> Regards
> Qi
> 

Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx

[Ergin, Mesut A]

> > > > > If you plan to use FD mode on your device, why not enable it
> > > > > at setup phase via rte_eth_dev_configure()?
> > > > > Then proper rx function would be selected.
> > > > >
> > > >
> > > > FDIR_MODE was designed to bind late automatically -- it is set when the
> first
> > > > filter rule is parsed, and unset when the last rule is removed.
> > >
> > > Why is that, if you can define it at configuration stage, and RX function
> > > selection is based on it?
> >
> > I don't know why it was designed that way -- maybe maintainers know the
> > historical context.
> 
> What I am trying to say - if particular feature (FD) enablement/disablement
> affects RX/TX function selection, then there should be an ability to
> enable/disable
> that feature at configuration state (dev_config/queue_setup).
> Function to change value of that feature at runtime (after
> dev_start/queue_start)
> should return an error if new value can't be supported with already selected
> RX/TX
> function.
> If that's is not the case, then it sounds like a bug/gap in i40e driver that needs to
> be fixed.

Will send a fix for that shortly.

Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance

[Ergin, Mesut A]

Hi Qi,

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Wednesday, May 22, 2019 5:42 AM
> To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX
> allowance
> 
> Hi Mesut:
> 
> > -----Original Message-----
> > From: Ergin, Mesut A
> > Sent: Thursday, May 16, 2019 12:28 PM
> > To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> > Subject: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance
> >
> > When i40e_rx_vec_dev_conf_condition_check_default() determines whether
> > vector receive functions would be allowed during initialization phase, it should
> > honor previously recorded disallowance during configuration phase, and not
> > override simply because it is for the first queue.
> >
> > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > ---
> >  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > index 0e6ffa0..f30cab4 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > @@ -212,6 +212,10 @@
> > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> >  		return -1;
> >
> > +	/* Should not override if vector was already disallowed */
> 
> It is possible a device be reconfigured between dev_stop/dev_start, vector
> mode may fit for the new configure, so the old rx_vec_allowd should be
> ignored,
> 

i40e_dev_configure would reset rx_vec_allowed already. Am I missing 
another reconfiguration path?

Mesut

> Regards
> Qi
> 
> > +	if (!ad->rx_vec_allowed)
> > +	return -1;
> > +
> >  	/**
> >  	 * Vector mode is allowed only when number of Rx queue
> >  	 * descriptor is power of 2.
> > --
> > 2.7.4

Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance

[Zhang, Qi Z]

Hi Meset:

> -----Original Message-----
> From: Ergin, Mesut A
> Sent: Friday, May 24, 2019 2:26 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX
> allowance
> 
> Hi Qi,
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Wednesday, May 22, 2019 5:42 AM
> > To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>
> > Cc: dev@dpdk.org
> > Subject: RE: [PATCH 3/3] net/i40e: fix inadvertent override of vector
> > RX allowance
> >
> > Hi Mesut:
> >
> > > -----Original Message-----
> > > From: Ergin, Mesut A
> > > Sent: Thursday, May 16, 2019 12:28 PM
> > > To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> > > Subject: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX
> > > allowance
> > >
> > > When i40e_rx_vec_dev_conf_condition_check_default() determines
> > > whether vector receive functions would be allowed during
> > > initialization phase, it should honor previously recorded	
> > > disallowance during configuration phase, and not override simply because
> it is for the first queue.
> > >
> > > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > index 0e6ffa0..f30cab4 100644
> > > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > @@ -212,6 +212,10 @@
> > > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> > >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> > >  		return -1;
> > >
> > > +	/* Should not override if vector was already disallowed */
> >
> > It is possible a device be reconfigured between dev_stop/dev_start,
> > vector mode may fit for the new configure, so the old rx_vec_allowd
> > should be ignored,
> >
> 
> i40e_dev_configure would reset rx_vec_allowed already. Am I missing another
> reconfiguration path?

Look at below scenario,

1. dev_configure (rx_vec_allowed is reset to true)
2. queue_setup (the ring size is not power of 2) 
3. dev_start (vector will not be selected due to ring size, rx_vec_allowed set to false) 
4. dev_stop 
5. queue_setup  (this time, with power of 2 ring size) 
6. dev_start (assume vector path should be selected, and rx_vec_allowed should be overwrite to true, but your patch will prevent it)

Also, I may not get the point of the gap you observed, would you share more detail scenario?

Regards
Qi

> 
> Mesut
> 
> > Regards
> > Qi
> >
> > > +	if (!ad->rx_vec_allowed)
> > > +	return -1;
> > > +
> > >  	/**
> > >  	 * Vector mode is allowed only when number of Rx queue
> > >  	 * descriptor is power of 2.
> > > --
> > > 2.7.4

Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance

[Ergin, Mesut A]

Hi,

> > > > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> > > >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> > > >  		return -1;
> > > >
> > > > +	/* Should not override if vector was already disallowed */
> > >
> > > It is possible a device be reconfigured between dev_stop/dev_start,
> > > vector mode may fit for the new configure, so the old rx_vec_allowd
> > > should be ignored,
> > >
> >
> > i40e_dev_configure would reset rx_vec_allowed already. Am I missing another
> > reconfiguration path?
> 
> Look at below scenario,
> 
> 1. dev_configure (rx_vec_allowed is reset to true)
> 2. queue_setup (the ring size is not power of 2)
> 3. dev_start (vector will not be selected due to ring size, rx_vec_allowed set to
> false)
> 4. dev_stop
> 5. queue_setup  (this time, with power of 2 ring size)
> 6. dev_start (assume vector path should be selected, and rx_vec_allowed should
> be overwrite to true, but your patch will prevent it)
> 
> Also, I may not get the point of the gap you observed, would you share more
> detail scenario?
> 
> Regards
> Qi

It seems like queue setup should reset vector_allowed flag, too. Because 
i40e_dev_rx_queue_setup_runtime is already doing it, though covering
only half of the state space (i.e. device already started).

Do you think this patch plus that would be a preferred approach?

> 
> >
> > Mesut
> >
> > > Regards
> > > Qi
> > >
> > > > +	if (!ad->rx_vec_allowed)
> > > > +	return -1;
> > > > +
> > > >  	/**
> > > >  	 * Vector mode is allowed only when number of Rx queue
> > > >  	 * descriptor is power of 2.
> > > > --
> > > > 2.7.4

Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance

[Zhang, Qi Z]

Hi Muset:

> -----Original Message-----
> From: Ergin, Mesut A
> Sent: Saturday, May 25, 2019 2:09 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX
> allowance
> 
> Hi,
> 
> > > > > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev
> *dev)
> > > > >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> > > > >  		return -1;
> > > > >
> > > > > +	/* Should not override if vector was already disallowed */
> > > >
> > > > It is possible a device be reconfigured between
> > > > dev_stop/dev_start, vector mode may fit for the new configure, so
> > > > the old rx_vec_allowd should be ignored,
> > > >
> > >
> > > i40e_dev_configure would reset rx_vec_allowed already. Am I missing
> > > another reconfiguration path?
> >
> > Look at below scenario,
> >
> > 1. dev_configure (rx_vec_allowed is reset to true) 2. queue_setup (the
> > ring size is not power of 2) 3. dev_start (vector will not be selected
> > due to ring size, rx_vec_allowed set to
> > false)
> > 4. dev_stop
> > 5. queue_setup  (this time, with power of 2 ring size) 6. dev_start
> > (assume vector path should be selected, and rx_vec_allowed should be
> > overwrite to true, but your patch will prevent it)
> >
> > Also, I may not get the point of the gap you observed, would you share
> > more detail scenario?
> >
> > Regards
> > Qi
> 
> It seems like queue setup should reset vector_allowed flag, too. Because
> i40e_dev_rx_queue_setup_runtime is already doing it, though covering only
> half of the state space (i.e. device already started).
> 
> Do you think this patch plus that would be a preferred approach?

First, I need to understand your gap, that might be obvious, but so far I just did not figure out it base your commit log yet, so please give me more heads up :)
Secondary, I think move below codes from dev_configure to dev_start might be a better solution

        ad->rx_bulk_alloc_allowed = true;
        ad->rx_vec_allowed = true;
        ad->tx_simple_allowed = true;
        ad->tx_vec_allowed = true;

Regards
Qi
> 
> >
> > >
> > > Mesut
> > >
> > > > Regards
> > > > Qi
> > > >
> > > > > +	if (!ad->rx_vec_allowed)
> > > > > +	return -1;
> > > > > +
> > > > >  	/**
> > > > >  	 * Vector mode is allowed only when number of Rx queue
> > > > >  	 * descriptor is power of 2.
> > > > > --
> > > > > 2.7.4