* [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support
@ 2019-11-15 3:41 Qi Zhang
2019-11-15 16:37 ` Stillwell Jr, Paul M
0 siblings, 1 reply; 3+ messages in thread
From: Qi Zhang @ 2019-11-15 3:41 UTC (permalink / raw)
To: xiaolong.ye; +Cc: dev, Qi Zhang
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(-)
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support
2019-11-15 3:41 [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support Qi Zhang
@ 2019-11-15 16:37 ` Stillwell Jr, Paul M
2019-11-16 1:52 ` Zhang, Qi Z
0 siblings, 1 reply; 3+ messages in thread
From: Stillwell Jr, Paul M @ 2019-11-15 16:37 UTC (permalink / raw)
To: Zhang, Qi Z, Ye, Xiaolong; +Cc: dev, Zhang, Qi Z
> -----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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support
2019-11-15 16:37 ` Stillwell Jr, Paul M
@ 2019-11-16 1:52 ` Zhang, Qi Z
0 siblings, 0 replies; 3+ messages in thread
From: Zhang, Qi Z @ 2019-11-16 1:52 UTC (permalink / raw)
To: Stillwell Jr, Paul M, Ye, Xiaolong; +Cc: dev
> -----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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-16 1:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-15 3:41 [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support Qi Zhang
2019-11-15 16:37 ` Stillwell Jr, Paul M
2019-11-16 1:52 ` Zhang, Qi Z
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).