diff mbox series

[v2] net/ice: add flow mark hint support

Message ID 20191115034152.21429-1-qi.z.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: xiaolong ye
Headers
Series [v2] net/ice: add flow mark hint support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-compilation success Compile Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed

Commit Message

Qi Zhang Nov. 15, 2019, 3:41 a.m. UTC 
  Since not all data paths support flow mark, the driver need
a hint from application to select the correct data path if
flow mark is required. The patch introduce 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>
---

v2:
- fix typo

 doc/guides/nics/ice.rst               | 10 ++++++++++
 drivers/net/ice/ice_ethdev.c          | 11 ++++++++++-
 drivers/net/ice/ice_ethdev.h          |  1 +
 drivers/net/ice/ice_rxtx_vec_common.h |  5 +++++
 4 files changed, 26 insertions(+), 1 deletion(-)

Comments

Stillwell Jr, Paul M Nov. 15, 2019, 4:37 p.m. UTC | #1 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Qi Zhang
> Sent: Thursday, November 14, 2019 7:42 PM
> To: Ye, Xiaolong <xiaolong.ye@intel.com>
> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support
> 
> Since not all data paths support flow mark, the driver need a hint from
> application to select the correct data path if flow mark is required. The patch
> introduce a devarg "flow-mark-support" as a workaround solution, since a
> standard way is still ongoing.
> 

I understand the need for this, but this seems problematic. Once a customer starts using this, then it has to be in the code forever because they are going to expect it to work forever. Maybe this should be marked with something that indicates it is temporary? Something like the experimental tagging that is done in other parts of the code?

> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> 
> v2:
> - fix typo
> 
>  doc/guides/nics/ice.rst               | 10 ++++++++++
>  drivers/net/ice/ice_ethdev.c          | 11 ++++++++++-
>  drivers/net/ice/ice_ethdev.h          |  1 +
>  drivers/net/ice/ice_rxtx_vec_common.h |  5 +++++
>  4 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst index
> 1a426438d..f7016d338 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -80,6 +80,16 @@ 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 support
> + flow mark extraction  by default. This hint should be removed when any of
> below condition ready.
> +  1) all data path 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 abf00d404..9f2cb2f40 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,12 @@ 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);
> +	printf("flow_mark = %d\n", ad->devargs.flow_mark_support);
> 
>  bail:
>  	rte_kvargs_free(kvlist);
> @@ -4571,7 +4579,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 4a0d37b32..4d35339a7 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..086428898 100644
> --- a/drivers/net/ice/ice_rxtx_vec_common.h
> +++ b/drivers/net/ice/ice_rxtx_vec_common.h
> @@ -268,6 +268,11 @@ 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);
> +
> +	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
Qi Zhang Nov. 16, 2019, 1:52 a.m. UTC | #2 
> -----Original Message-----
> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>
> Sent: Saturday, November 16, 2019 12:37 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Qi Zhang
> > Sent: Thursday, November 14, 2019 7:42 PM
> > To: Ye, Xiaolong <xiaolong.ye@intel.com>
> > Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Subject: [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support
> >
> > Since not all data paths support flow mark, the driver need a hint
> > from application to select the correct data path if flow mark is
> > required. The patch introduce a devarg "flow-mark-support" as a
> > workaround solution, since a standard way is still ongoing.
> >
> 
> I understand the need for this, but this seems problematic. Once a customer
> starts using this, then it has to be in the code forever because they are going to
> expect it to work forever. Maybe this should be marked with something that
> indicates it is temporary? Something like the experimental tagging that is done
> in other parts of the code?

Yes, since this is a workaround solution, claim it as an experimental looks like a good idea.
But I didn't see there is any documented standard way to claim a devarg as experimental.
Not sure if that should be part of the ABI policy or already some effort to standardize devargs is on the way.

Anyway as you can see, I have below statement in ice.rst to claim this is a workaround solution, 

	This hint should be removed when any of below condition ready.
	1) all data path 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. 

And I can add words "This is experimental" to emphasized it.
 
Regards
Qi
> 
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >
> > v2:
> > - fix typo
> >
> >  doc/guides/nics/ice.rst               | 10 ++++++++++
> >  drivers/net/ice/ice_ethdev.c          | 11 ++++++++++-
> >  drivers/net/ice/ice_ethdev.h          |  1 +
> >  drivers/net/ice/ice_rxtx_vec_common.h |  5 +++++
> >  4 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst index
> > 1a426438d..f7016d338 100644
> > --- a/doc/guides/nics/ice.rst
> > +++ b/doc/guides/nics/ice.rst
> > @@ -80,6 +80,16 @@ 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 support
> > + flow mark extraction  by default. This hint should be removed when
> > + any of
> > below condition ready.
> > +  1) all data path 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 abf00d404..9f2cb2f40 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,12 @@ 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);
> > +printf("flow_mark = %d\n", ad->devargs.flow_mark_support);
> >
> >  bail:
> >  rte_kvargs_free(kvlist);
> > @@ -4571,7 +4579,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 4a0d37b32..4d35339a7 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..086428898 100644
> > --- a/drivers/net/ice/ice_rxtx_vec_common.h
> > +++ b/drivers/net/ice/ice_rxtx_vec_common.h
> > @@ -268,6 +268,11 @@ 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);
> > +
> > +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
>
diff mbox series

Patch

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 1a426438d..f7016d338 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -80,6 +80,16 @@  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 support flow mark extraction
+  by default. This hint should be removed when any of below condition ready.
+  1) all data path 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 abf00d404..9f2cb2f40 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,12 @@  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);
+	printf("flow_mark = %d\n", ad->devargs.flow_mark_support);
 
 bail:
 	rte_kvargs_free(kvlist);
@@ -4571,7 +4579,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 4a0d37b32..4d35339a7 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..086428898 100644
--- a/drivers/net/ice/ice_rxtx_vec_common.h
+++ b/drivers/net/ice/ice_rxtx_vec_common.h
@@ -268,6 +268,11 @@  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);
+
+	if (ad->devargs.flow_mark_support)
+		return -1;
 
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		rxq = dev->data->rx_queues[i];