[dpdk-dev,v4,4/6] ethdev: add mark flow item to flow item types

Message ID 20180418210423.13847-5-declan.doherty@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Doherty, Declan April 18, 2018, 9:04 p.m. UTC
  Introduces a new action type RTE_FLOW_ITEM_TYPE_MARK which enables
flow patterns to specify arbitrary integer values to match aginst
which are set by the RTE_FLOW_ACTION_TYPE_MARK action in a 
previously matched flow from a higher prioriry group.

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
 doc/guides/prog_guide/rte_flow.rst | 28 ++++++++++++++++++++++++++++
 lib/librte_ether/rte_flow.h        | 24 ++++++++++++++++++++++++
 2 files changed, 52 insertions(+)
  

Comments

Adrien Mazarguil April 19, 2018, 1:03 p.m. UTC | #1
On Wed, Apr 18, 2018 at 10:04:21PM +0100, Declan Doherty wrote:
> Introduces a new action type RTE_FLOW_ITEM_TYPE_MARK which enables
> flow patterns to specify arbitrary integer values to match aginst

Typo on "aginst".

> which are set by the RTE_FLOW_ACTION_TYPE_MARK action in a 
> previously matched flow from a higher prioriry group.

prioriry => priority, however this last addition is unnecessary, it could be
any prior flow rule that happens to use PASSTHRU.

> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst | 28 ++++++++++++++++++++++++++++
>  lib/librte_ether/rte_flow.h        | 24 ++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 325010544..6f23ad909 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -598,6 +598,34 @@ associated with a port_id should be retrieved by other means.
>     | ``mask`` | ``index`` | zeroed to match any port index |
>     +----------+-----------+--------------------------------+
>  
> +Item: ``MARK``
> +^^^^^^^^^^^^^^
> +
> +Matches packets coming from a previously matched flow in a higher priority group

See above re "higher priority group".

> +with an arbitrary integer value which was set using the ``MARK`` action in the
> +previously matched rule.
> +
> +This item can only specified once as a match criteria as the ``MARK`` action can
> +only be specified once in a flow action.
> +
> +Note the value of MARK field is arbitrary and application defined.
> +
> +- Default ``mask`` matches any integer value.
> +
> +.. _table_rte_flow_item_mark:
> +
> +.. table:: MARK
> +
> +   +----------+----------+---------------------------+
> +   | Field    | Subfield | Value                     |
> +   +==========+==========+===========================+
> +   | ``spec`` | ``id``   | integer value             |
> +   +----------+--------------------------------------+
> +   | ``last`` | ``id``   | upper range value         |
> +   +----------+----------+---------------------------+
> +   | ``mask`` | ``id``   | zeroed to match any value |
> +   +----------+------- --+---------------------------+
> +
>  Data matching item types
>  ~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 91544f62b..56e262a23 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -323,6 +323,13 @@ enum rte_flow_item_type {
>  	 * See struct rte_flow_item_geneve.
>  	 */
>  	RTE_FLOW_ITEM_TYPE_GENEVE,
> +
> +	/**
> +	 * Matches specified mark field.
> +	 *
> +	 * See struct rte_flow_item_mark.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_MARK

Remember to add a trailing comma.

>  };
>  
>  /**
> @@ -814,6 +821,23 @@ static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
>  };
>  #endif
>  
> +/**
> + * RTE_FLOW_ITEM_TYPE_MARK
> + *
> + * Allow arbitrary integer value set using MARK action in a flow rule in a
> + * higher priority group to be matched against in flow rule in a lower
> + * priority group.
> + *
> + * This value is arbitrary and application-defined. Maximum allowed value
> + * depends on the underlying implementation.
> + *
> + * Depending on the underlying implementation the MARK item may be supported in
> + * the physical device, virtually in logical groups in the PMD or not at all.

See above regarding groups. Also please synchronize this with the contents
of rte_flow.rst. They should be the same.

> + */
> +struct rte_flow_item_mark {
> +	uint32_t id; /**< Integer value to match against. */
> +};
> +
>  /**
>   * Matching pattern item definition.
>   *
> -- 
> 2.14.3
> 

Otherwise this patch looks good.
  
Shahaf Shuler April 23, 2018, 11:10 a.m. UTC | #2
Thursday, April 19, 2018 4:04 PM, Adrien Mazarguil:
>
> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ethdev: add mark flow item to flow
> item types
> 
> On Wed, Apr 18, 2018 at 10:04:21PM +0100, Declan Doherty wrote:
> > Introduces a new action type RTE_FLOW_ITEM_TYPE_MARK which enables
> > flow patterns to specify arbitrary integer values to match aginst
> 
> Typo on "aginst".
> 
> > which are set by the RTE_FLOW_ACTION_TYPE_MARK action in a
> previously
> > matched flow from a higher prioriry group.
> 
> prioriry => priority, however this last addition is unnecessary, it could be any
> prior flow rule that happens to use PASSTHRU.
> 
> >
> > Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> > ---
> >  doc/guides/prog_guide/rte_flow.rst | 28
> ++++++++++++++++++++++++++++
> >  lib/librte_ether/rte_flow.h        | 24 ++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index 325010544..6f23ad909 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -598,6 +598,34 @@ associated with a port_id should be retrieved by
> other means.
> >     | ``mask`` | ``index`` | zeroed to match any port index |
> >     +----------+-----------+--------------------------------+
> >
> > +Item: ``MARK``
> > +^^^^^^^^^^^^^^
> > +
> > +Matches packets coming from a previously matched flow in a higher
> > +priority group
> 
> See above re "higher priority group".
> 
> > +with an arbitrary integer value which was set using the ``MARK``
> > +action in the previously matched rule.
> > +

Why we have to bind It with the MARK? It is HW limitation or design consideration? 

My understanding is you want flow action of setting metadata to be used later as a matching item for the flows on other group.
It doesn't have to, but can be, bounded with the specific mark the application wants to receive.
  
Adrien Mazarguil April 23, 2018, 11:49 a.m. UTC | #3
On Mon, Apr 23, 2018 at 11:10:11AM +0000, Shahaf Shuler wrote:
> Thursday, April 19, 2018 4:04 PM, Adrien Mazarguil:
> >
> > Subject: Re: [dpdk-dev] [PATCH v4 4/6] ethdev: add mark flow item to flow
> > item types
> > 
> > On Wed, Apr 18, 2018 at 10:04:21PM +0100, Declan Doherty wrote:
> > > Introduces a new action type RTE_FLOW_ITEM_TYPE_MARK which enables
> > > flow patterns to specify arbitrary integer values to match aginst
> > 
> > Typo on "aginst".
> > 
> > > which are set by the RTE_FLOW_ACTION_TYPE_MARK action in a
> > previously
> > > matched flow from a higher prioriry group.
> > 
> > prioriry => priority, however this last addition is unnecessary, it could be any
> > prior flow rule that happens to use PASSTHRU.
> > 
> > >
> > > Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> > > ---
> > >  doc/guides/prog_guide/rte_flow.rst | 28
> > ++++++++++++++++++++++++++++
> > >  lib/librte_ether/rte_flow.h        | 24 ++++++++++++++++++++++++
> > >  2 files changed, 52 insertions(+)
> > >
> > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > > b/doc/guides/prog_guide/rte_flow.rst
> > > index 325010544..6f23ad909 100644
> > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > @@ -598,6 +598,34 @@ associated with a port_id should be retrieved by
> > other means.
> > >     | ``mask`` | ``index`` | zeroed to match any port index |
> > >     +----------+-----------+--------------------------------+
> > >
> > > +Item: ``MARK``
> > > +^^^^^^^^^^^^^^
> > > +
> > > +Matches packets coming from a previously matched flow in a higher
> > > +priority group
> > 
> > See above re "higher priority group".
> > 
> > > +with an arbitrary integer value which was set using the ``MARK``
> > > +action in the previously matched rule.
> > > +
> 
> Why we have to bind It with the MARK? It is HW limitation or design consideration? 
> 
> My understanding is you want flow action of setting metadata to be used later as a matching item for the flows on other group.
> It doesn't have to, but can be, bounded with the specific mark the application wants to receive. 

Yes, no problem with that, I was only commenting the wording of the
description. It reads like it'll only work if a prior MARK action found in a
*different* group set a mark to match. My point was that it could also be a
prior rule in the same group. It could even be added by the device by some
other means.
  

Patch

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 325010544..6f23ad909 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -598,6 +598,34 @@  associated with a port_id should be retrieved by other means.
    | ``mask`` | ``index`` | zeroed to match any port index |
    +----------+-----------+--------------------------------+
 
+Item: ``MARK``
+^^^^^^^^^^^^^^
+
+Matches packets coming from a previously matched flow in a higher priority group
+with an arbitrary integer value which was set using the ``MARK`` action in the
+previously matched rule.
+
+This item can only specified once as a match criteria as the ``MARK`` action can
+only be specified once in a flow action.
+
+Note the value of MARK field is arbitrary and application defined.
+
+- Default ``mask`` matches any integer value.
+
+.. _table_rte_flow_item_mark:
+
+.. table:: MARK
+
+   +----------+----------+---------------------------+
+   | Field    | Subfield | Value                     |
+   +==========+==========+===========================+
+   | ``spec`` | ``id``   | integer value             |
+   +----------+--------------------------------------+
+   | ``last`` | ``id``   | upper range value         |
+   +----------+----------+---------------------------+
+   | ``mask`` | ``id``   | zeroed to match any value |
+   +----------+------- --+---------------------------+
+
 Data matching item types
 ~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index 91544f62b..56e262a23 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -323,6 +323,13 @@  enum rte_flow_item_type {
 	 * See struct rte_flow_item_geneve.
 	 */
 	RTE_FLOW_ITEM_TYPE_GENEVE,
+
+	/**
+	 * Matches specified mark field.
+	 *
+	 * See struct rte_flow_item_mark.
+	 */
+	RTE_FLOW_ITEM_TYPE_MARK
 };
 
 /**
@@ -814,6 +821,23 @@  static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
 };
 #endif
 
+/**
+ * RTE_FLOW_ITEM_TYPE_MARK
+ *
+ * Allow arbitrary integer value set using MARK action in a flow rule in a
+ * higher priority group to be matched against in flow rule in a lower
+ * priority group.
+ *
+ * This value is arbitrary and application-defined. Maximum allowed value
+ * depends on the underlying implementation.
+ *
+ * Depending on the underlying implementation the MARK item may be supported in
+ * the physical device, virtually in logical groups in the PMD or not at all.
+ */
+struct rte_flow_item_mark {
+	uint32_t id; /**< Integer value to match against. */
+};
+
 /**
  * Matching pattern item definition.
  *