[dpdk-dev] [PATCH v4] net/ice: add flow mark hint support

[dpdk-dev] [PATCH v4] net/ice: add flow mark hint support

[Qi Zhang]

Since not all data paths support flow mark, the driver needs
a hint from application to select the correct data path if
flow mark is required. The patch introduces a devarg
"flow-mark-support" as a workaround solution, since a standard
way is still ongoing.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
Acked-by: Qiming Yang <qiming.yang@intel.com>
---

v4:
- remove debug code, fix typos

v3:
- add "experimental notification" in ice.rst

v2:
- fix typos

 doc/guides/nics/ice.rst               | 12 ++++++++++++
 drivers/net/ice/ice_ethdev.c          | 12 +++++++++++-
 drivers/net/ice/ice_ethdev.h          |  1 +
 drivers/net/ice/ice_rxtx_vec_common.h |  6 ++++++
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 7ff33d7f0..652620d16 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -98,6 +98,18 @@ Runtime Config Options
 
     -w 80:00.0,pipeline-mode-support=1
 
+- ``Flow Mark Support`` (default ``0``)
+
+  This is a hint to the driver to select the data path that supports flow mark extraction
+  by default.
+  NOTE: This is an experimental devarg, it will be removed when any of below conditions
+  is ready.
+  1) all data paths support flow mark (currently vPMD does not)
+  2) a new offload like RTE_DEV_RX_OFFLOAD_FLOW_MARK be introduced as a standard way to hint.
+  Example::
+
+    -w 80:00.0,flow-mark-support=1
+
 - ``Protocol extraction for per queue``
 
   Configure the RX queues to do protocol extraction into mbuf for protocol
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index ecfc6c9e8..de189daba 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -23,11 +23,13 @@
 /* devargs */
 #define ICE_SAFE_MODE_SUPPORT_ARG "safe-mode-support"
 #define ICE_PIPELINE_MODE_SUPPORT_ARG  "pipeline-mode-support"
+#define ICE_FLOW_MARK_SUPPORT_ARG	"flow-mark-support"
 #define ICE_PROTO_XTR_ARG         "proto_xtr"
 
 static const char * const ice_valid_args[] = {
 	ICE_SAFE_MODE_SUPPORT_ARG,
 	ICE_PIPELINE_MODE_SUPPORT_ARG,
+	ICE_FLOW_MARK_SUPPORT_ARG,
 	ICE_PROTO_XTR_ARG,
 	NULL
 };
@@ -1987,6 +1989,13 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
 
 	ret = rte_kvargs_process(kvlist, ICE_PIPELINE_MODE_SUPPORT_ARG,
 				 &parse_bool, &ad->devargs.pipe_mode_support);
+	if (ret)
+		goto bail;
+
+	ret = rte_kvargs_process(kvlist, ICE_FLOW_MARK_SUPPORT_ARG,
+				 &parse_bool, &ad->devargs.flow_mark_support);
+	if (ret)
+		goto bail;
 
 bail:
 	rte_kvargs_free(kvlist);
@@ -4562,7 +4571,8 @@ RTE_PMD_REGISTER_KMOD_DEP(net_ice, "* igb_uio | uio_pci_generic | vfio-pci");
 RTE_PMD_REGISTER_PARAM_STRING(net_ice,
 			      ICE_PROTO_XTR_ARG "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp>"
 			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
-			      ICE_PIPELINE_MODE_SUPPORT_ARG "=<0|1>");
+			      ICE_PIPELINE_MODE_SUPPORT_ARG "=<0|1>"
+			      ICE_FLOW_MARK_SUPPORT_ARG "=<0|1>");
 
 RTE_INIT(ice_init_log)
 {
diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
index 510f6869d..f2186e1ff 100644
--- a/drivers/net/ice/ice_ethdev.h
+++ b/drivers/net/ice/ice_ethdev.h
@@ -391,6 +391,7 @@ struct ice_devargs {
 	int safe_mode_support;
 	uint8_t proto_xtr_dflt;
 	int pipe_mode_support;
+	int flow_mark_support;
 	uint8_t proto_xtr[ICE_MAX_QUEUE_NUM];
 };
 
diff --git a/drivers/net/ice/ice_rxtx_vec_common.h b/drivers/net/ice/ice_rxtx_vec_common.h
index 080ca4175..5e6f89642 100644
--- a/drivers/net/ice/ice_rxtx_vec_common.h
+++ b/drivers/net/ice/ice_rxtx_vec_common.h
@@ -268,6 +268,12 @@ ice_rx_vec_dev_check_default(struct rte_eth_dev *dev)
 {
 	int i;
 	struct ice_rx_queue *rxq;
+	struct ice_adapter *ad =
+		ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+
+	/* vPMD does not support flow mark. */
+	if (ad->devargs.flow_mark_support)
+		return -1;
 
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		rxq = dev->data->rx_queues[i];
-- 
2.13.6

Re: [dpdk-dev] [PATCH v4] net/ice: add flow mark hint support

[Ye Xiaolong]

On 11/19, Qi Zhang wrote:
>Since not all data paths support flow mark, the driver needs
>a hint from application to select the correct data path if
>flow mark is required. The patch introduces a devarg
>"flow-mark-support" as a workaround solution, since a standard
>way is still ongoing.
>
>Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>Acked-by: Qiming Yang <qiming.yang@intel.com>
>---
>
>v4:
>- remove debug code, fix typos
>
>v3:
>- add "experimental notification" in ice.rst
>
>v2:
>- fix typos
>
> doc/guides/nics/ice.rst               | 12 ++++++++++++
> drivers/net/ice/ice_ethdev.c          | 12 +++++++++++-
> drivers/net/ice/ice_ethdev.h          |  1 +
> drivers/net/ice/ice_rxtx_vec_common.h |  6 ++++++
> 4 files changed, 30 insertions(+), 1 deletion(-)
>
>diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
>index 7ff33d7f0..652620d16 100644
>--- a/doc/guides/nics/ice.rst
>+++ b/doc/guides/nics/ice.rst
>@@ -98,6 +98,18 @@ Runtime Config Options
> 
>     -w 80:00.0,pipeline-mode-support=1
> 
>+- ``Flow Mark Support`` (default ``0``)
>+
>+  This is a hint to the driver to select the data path that supports flow mark extraction
>+  by default.
>+  NOTE: This is an experimental devarg, it will be removed when any of below conditions
>+  is ready.
>+  1) all data paths support flow mark (currently vPMD does not)
>+  2) a new offload like RTE_DEV_RX_OFFLOAD_FLOW_MARK be introduced as a standard way to hint.
>+  Example::
>+
>+    -w 80:00.0,flow-mark-support=1
>+
> - ``Protocol extraction for per queue``
> 
>   Configure the RX queues to do protocol extraction into mbuf for protocol
>diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
>index ecfc6c9e8..de189daba 100644
>--- a/drivers/net/ice/ice_ethdev.c
>+++ b/drivers/net/ice/ice_ethdev.c
>@@ -23,11 +23,13 @@
> /* devargs */
> #define ICE_SAFE_MODE_SUPPORT_ARG "safe-mode-support"
> #define ICE_PIPELINE_MODE_SUPPORT_ARG  "pipeline-mode-support"
>+#define ICE_FLOW_MARK_SUPPORT_ARG	"flow-mark-support"
> #define ICE_PROTO_XTR_ARG         "proto_xtr"
> 
> static const char * const ice_valid_args[] = {
> 	ICE_SAFE_MODE_SUPPORT_ARG,
> 	ICE_PIPELINE_MODE_SUPPORT_ARG,
>+	ICE_FLOW_MARK_SUPPORT_ARG,
> 	ICE_PROTO_XTR_ARG,
> 	NULL
> };
>@@ -1987,6 +1989,13 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
> 
> 	ret = rte_kvargs_process(kvlist, ICE_PIPELINE_MODE_SUPPORT_ARG,
> 				 &parse_bool, &ad->devargs.pipe_mode_support);
>+	if (ret)
>+		goto bail;
>+
>+	ret = rte_kvargs_process(kvlist, ICE_FLOW_MARK_SUPPORT_ARG,
>+				 &parse_bool, &ad->devargs.flow_mark_support);
>+	if (ret)
>+		goto bail;
> 
> bail:
> 	rte_kvargs_free(kvlist);
>@@ -4562,7 +4571,8 @@ RTE_PMD_REGISTER_KMOD_DEP(net_ice, "* igb_uio | uio_pci_generic | vfio-pci");
> RTE_PMD_REGISTER_PARAM_STRING(net_ice,
> 			      ICE_PROTO_XTR_ARG "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp>"
> 			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
>-			      ICE_PIPELINE_MODE_SUPPORT_ARG "=<0|1>");
>+			      ICE_PIPELINE_MODE_SUPPORT_ARG "=<0|1>"
>+			      ICE_FLOW_MARK_SUPPORT_ARG "=<0|1>");
> 
> RTE_INIT(ice_init_log)
> {
>diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
>index 510f6869d..f2186e1ff 100644
>--- a/drivers/net/ice/ice_ethdev.h
>+++ b/drivers/net/ice/ice_ethdev.h
>@@ -391,6 +391,7 @@ struct ice_devargs {
> 	int safe_mode_support;
> 	uint8_t proto_xtr_dflt;
> 	int pipe_mode_support;
>+	int flow_mark_support;
> 	uint8_t proto_xtr[ICE_MAX_QUEUE_NUM];
> };
> 
>diff --git a/drivers/net/ice/ice_rxtx_vec_common.h b/drivers/net/ice/ice_rxtx_vec_common.h
>index 080ca4175..5e6f89642 100644
>--- a/drivers/net/ice/ice_rxtx_vec_common.h
>+++ b/drivers/net/ice/ice_rxtx_vec_common.h
>@@ -268,6 +268,12 @@ ice_rx_vec_dev_check_default(struct rte_eth_dev *dev)
> {
> 	int i;
> 	struct ice_rx_queue *rxq;
>+	struct ice_adapter *ad =
>+		ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
>+
>+	/* vPMD does not support flow mark. */
>+	if (ad->devargs.flow_mark_support)
>+		return -1;
> 
> 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> 		rxq = dev->data->rx_queues[i];
>-- 
>2.13.6
>

Applied to dpdk-next-net-intel, Thanks.

Re: [dpdk-dev] [PATCH v4] net/ice: add flow mark hint support

[Thomas Monjalon]

19/11/2019 07:14, Qi Zhang:
> Since not all data paths support flow mark, the driver needs
> a hint from application to select the correct data path if
> flow mark is required. The patch introduces a devarg
> "flow-mark-support" as a workaround solution, since a standard
> way is still ongoing.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> Acked-by: Qiming Yang <qiming.yang@intel.com>
> ---
> 
> v4:
> - remove debug code, fix typos
> 
> v3:
> - add "experimental notification" in ice.rst
> 
> v2:
> - fix typos

Please use --in-reply-to to help tracking all the versions together.

> +- ``Flow Mark Support`` (default ``0``)
> +
> +  This is a hint to the driver to select the data path that supports flow mark extraction
> +  by default.
> +  NOTE: This is an experimental devarg, it will be removed when any of below conditions
> +  is ready.
> +  1) all data paths support flow mark (currently vPMD does not)
> +  2) a new offload like RTE_DEV_RX_OFFLOAD_FLOW_MARK be introduced as a standard way to hint.

When the data path is selected?
I suppose such decision should be done when starting the port,
after everything is configured.
So you can check if a rte_flow rule was added for mark action.
Why the user needs to use an explicit option?

Re: [dpdk-dev] [PATCH v4] net/ice: add flow mark hint support

[Zhang, Qi Z]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, November 21, 2019 2:57 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; arybchenko@solarflare.com; orika@mellanox.com
> Subject: Re: [dpdk-dev] [PATCH v4] net/ice: add flow mark hint support
> 
> 19/11/2019 07:14, Qi Zhang:
> > Since not all data paths support flow mark, the driver needs a hint
> > from application to select the correct data path if flow mark is
> > required. The patch introduces a devarg "flow-mark-support" as a
> > workaround solution, since a standard way is still ongoing.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > Acked-by: Qiming Yang <qiming.yang@intel.com>
> > ---
> >
> > v4:
> > - remove debug code, fix typos
> >
> > v3:
> > - add "experimental notification" in ice.rst
> >
> > v2:
> > - fix typos
> 
> Please use --in-reply-to to help tracking all the versions together.
> 
> > +- ``Flow Mark Support`` (default ``0``)
> > +
> > +  This is a hint to the driver to select the data path that supports
> > + flow mark extraction  by default.
> > +  NOTE: This is an experimental devarg, it will be removed when any
> > + of below conditions  is ready.
> > +  1) all data paths support flow mark (currently vPMD does not)
> > +  2) a new offload like RTE_DEV_RX_OFFLOAD_FLOW_MARK be introduced
> as a standard way to hint.
> 
> When the data path is selected?

dev_start

> I suppose such decision should be done when starting the port, after everything
> is configured.
> So you can check if a rte_flow rule was added for mark action.
> Why the user needs to use an explicit option?

A rte_flow with mark can be issued at any time after dev_start when it is need, in that case, we have to reject the flow, this has been complained a lot base on previous feedback by users, since inconsistent behavior (sometimes mark works, some time it does not) is not expected
Also this option is overwhelmed by option 1 if we plan to do a clean fix in driver.







> 

Re: [dpdk-dev] [PATCH v4] net/ice: add flow mark hint support

[Thomas Monjalon]

21/11/2019 02:19, Zhang, Qi Z:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 19/11/2019 07:14, Qi Zhang:
> > > Since not all data paths support flow mark, the driver needs a hint
> > > from application to select the correct data path if flow mark is
> > > required. The patch introduces a devarg "flow-mark-support" as a
> > > workaround solution, since a standard way is still ongoing.
> > >
> > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > Acked-by: Qiming Yang <qiming.yang@intel.com>
> > > ---
> > > +- ``Flow Mark Support`` (default ``0``)
> > > +
> > > +  This is a hint to the driver to select the data path that supports
> > > + flow mark extraction  by default.
> > > +  NOTE: This is an experimental devarg, it will be removed when any
> > > + of below conditions  is ready.
> > > +  1) all data paths support flow mark (currently vPMD does not)
> > > +  2) a new offload like RTE_DEV_RX_OFFLOAD_FLOW_MARK be introduced
> > as a standard way to hint.
> > 
> > When the data path is selected?
> 
> dev_start
> 
> > I suppose such decision should be done when starting the port, after everything
> > is configured.
> > So you can check if a rte_flow rule was added for mark action.
> > Why the user needs to use an explicit option?
> 
> A rte_flow with mark can be issued at any time after dev_start when it is need, in that case, we have to reject the flow, this has been complained a lot base on previous feedback by users, since inconsistent behavior (sometimes mark works, some time it does not) is not expected

OK so you confirm the problem is only when a configuration
is changed at runtime without stopping the port.

> Also this option is overwhelmed by option 1 if we plan to do a clean fix in driver.

You want the application (or the user) to announce in advance which
configuration could be applied during the runtime.
I think we should consider the problem for any runtime configuration.
We never clearly defined which configuration is allowed at runtime.

Which other configs may be setup at runtime? MTU? VLAN? mirroring?
tunneling checksum? promiscuous? supported packet types? IEEE1588?

Re: [dpdk-dev] [PATCH v4] net/ice: add flow mark hint support

[Zhang, Qi Z]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, November 21, 2019 3:37 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; arybchenko@solarflare.com; orika@mellanox.com
> Subject: Re: [dpdk-dev] [PATCH v4] net/ice: add flow mark hint support
> 
> 21/11/2019 02:19, Zhang, Qi Z:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 19/11/2019 07:14, Qi Zhang:
> > > > Since not all data paths support flow mark, the driver needs a
> > > > hint from application to select the correct data path if flow mark
> > > > is required. The patch introduces a devarg "flow-mark-support" as
> > > > a workaround solution, since a standard way is still ongoing.
> > > >
> > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > Acked-by: Qiming Yang <qiming.yang@intel.com>
> > > > ---
> > > > +- ``Flow Mark Support`` (default ``0``)
> > > > +
> > > > +  This is a hint to the driver to select the data path that
> > > > + supports flow mark extraction  by default.
> > > > +  NOTE: This is an experimental devarg, it will be removed when
> > > > + any of below conditions  is ready.
> > > > +  1) all data paths support flow mark (currently vPMD does not)
> > > > +  2) a new offload like RTE_DEV_RX_OFFLOAD_FLOW_MARK be
> > > > + introduced
> > > as a standard way to hint.
> > >
> > > When the data path is selected?
> >
> > dev_start
> >
> > > I suppose such decision should be done when starting the port, after
> > > everything is configured.
> > > So you can check if a rte_flow rule was added for mark action.
> > > Why the user needs to use an explicit option?
> >
> > A rte_flow with mark can be issued at any time after dev_start when it
> > is need, in that case, we have to reject the flow, this has been
> > complained a lot base on previous feedback by users, since
> > inconsistent behavior (sometimes mark works, some time it does not) is
> > not expected
> 
> OK so you confirm the problem is only when a configuration is changed at
> runtime without stopping the port.
> 
> > Also this option is overwhelmed by option 1 if we plan to do a clean fix in
> driver.
> 
> You want the application (or the user) to announce in advance which
> configuration could be applied during the runtime.
> I think we should consider the problem for any runtime configuration.
> We never clearly defined which configuration is allowed at runtime.
> 
> Which other configs may be setup at runtime? MTU? VLAN? mirroring?
> tunneling checksum? promiscuous? supported packet types? IEEE1588?

So far, in rte_eth API, we do dev_started check at dev_configure, queue_setup (if runtime queue setup is not supported by PMD).
All other control path API is case by case depends on hardware capability.
Take i40e as an example; we have to stop the port when setting MTU because we have to reconfigure the hardware queue context, which needs to stop queue that impacts data path.
While for VLAN / promiscuous, since it is the case that a rule is added into the on-chip memory, so no need to stop the data path.
 
Maybe it's a good idea to define a rule that which control path is allowed at runtime, which should not be.
But at least I think it's not necessary to prevent users from doing flow configure at runtime.;






> 

Re: [dpdk-dev] [PATCH v4] net/ice: add flow mark hint support

[Thomas Monjalon]

21/11/2019 13:40, Zhang, Qi Z:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 21/11/2019 02:19, Zhang, Qi Z:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 19/11/2019 07:14, Qi Zhang:
> > > > > Since not all data paths support flow mark, the driver needs a
> > > > > hint from application to select the correct data path if flow mark
> > > > > is required. The patch introduces a devarg "flow-mark-support" as
> > > > > a workaround solution, since a standard way is still ongoing.
> > > > >
> > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > Acked-by: Qiming Yang <qiming.yang@intel.com>
> > > > > ---
> > > > > +- ``Flow Mark Support`` (default ``0``)
> > > > > +
> > > > > +  This is a hint to the driver to select the data path that
> > > > > + supports flow mark extraction  by default.
> > > > > +  NOTE: This is an experimental devarg, it will be removed when
> > > > > + any of below conditions  is ready.
> > > > > +  1) all data paths support flow mark (currently vPMD does not)
> > > > > +  2) a new offload like RTE_DEV_RX_OFFLOAD_FLOW_MARK be
> > > > > + introduced
> > > > as a standard way to hint.
> > > >
> > > > When the data path is selected?
> > >
> > > dev_start
> > >
> > > > I suppose such decision should be done when starting the port, after
> > > > everything is configured.
> > > > So you can check if a rte_flow rule was added for mark action.
> > > > Why the user needs to use an explicit option?
> > >
> > > A rte_flow with mark can be issued at any time after dev_start when it
> > > is need, in that case, we have to reject the flow, this has been
> > > complained a lot base on previous feedback by users, since
> > > inconsistent behavior (sometimes mark works, some time it does not) is
> > > not expected
> > 
> > OK so you confirm the problem is only when a configuration is changed at
> > runtime without stopping the port.
> > 
> > > Also this option is overwhelmed by option 1 if we plan to do a clean fix in
> > driver.
> > 
> > You want the application (or the user) to announce in advance which
> > configuration could be applied during the runtime.
> > I think we should consider the problem for any runtime configuration.
> > We never clearly defined which configuration is allowed at runtime.
> > 
> > Which other configs may be setup at runtime? MTU? VLAN? mirroring?
> > tunneling checksum? promiscuous? supported packet types? IEEE1588?
> 
> So far, in rte_eth API, we do dev_started check at dev_configure, queue_setup (if runtime queue setup is not supported by PMD).
> All other control path API is case by case depends on hardware capability.
> Take i40e as an example; we have to stop the port when setting MTU because we have to reconfigure the hardware queue context, which needs to stop queue that impacts data path.
> While for VLAN / promiscuous, since it is the case that a rule is added into the on-chip memory, so no need to stop the data path.
>  
> Maybe it's a good idea to define a rule that which control path is allowed at runtime, which should not be.
> But at least I think it's not necessary to prevent users from doing flow configure at runtime.;

Yes I agree.
Based on the list of allowed runtime config changes,
we could think about a global solution to allow datapath optimization
taking application needs into account.