[v4,1/4] ethdev: add GRE key field to flow API

  Add new rte_flow_item_gre_key in order to match the optional key field.

Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
---
 doc/guides/prog_guide/rte_flow.rst | 8 ++++++++
 lib/librte_ethdev/rte_flow.c       | 1 +
 lib/librte_ethdev/rte_flow.h       | 7 +++++++
 3 files changed, 16 insertions(+)
  

02/07/2019 11:45, Xiaoyu Min:
> +	/**
> +	 * Matches a GRE optional key field.
> +	 *
> +	 * The value should a big-endian 32bit integer.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_GRE_KEY,

We probably want to use the same format as in Dekel's patch
for doxygen documentation of the action value:

       /**
        * Increase sequence number in the outermost TCP header.
        *
        * Action configuration specifies the value to increase
        * TCP sequence number as a big-endian 32 bit integer.
        *
        * @p conf type:
        * @code rte_be32_t * @endcode
        *
        * Using this action on non-matching traffic will result in
        * undefined behavior.
        */
       RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ,
  

On Tue, Jul 02, 2019 at 05:45:52PM +0800, Xiaoyu Min wrote:
> Add new rte_flow_item_gre_key in order to match the optional key field.
> 
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>

OK with adding this feature, however I still have a bunch of comments below.

> ---
>  doc/guides/prog_guide/rte_flow.rst | 8 ++++++++
>  lib/librte_ethdev/rte_flow.c       | 1 +
>  lib/librte_ethdev/rte_flow.h       | 7 +++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index a34d012e55..f4b7baa3c3 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -980,6 +980,14 @@ Matches a GRE header.
>  - ``protocol``: protocol type.
>  - Default ``mask`` matches protocol only.
>  
> +Item: ``GRE_KEY``
> +^^^^^^^^^^^^^^^^^
> +
> +Matches a GRE key field.
> +This should be preceded by item ``GRE``

Nit: missing ending "."

> +
> +- Value to be matched is a big-endian 32 bit integer
> +
>  Item: ``FUZZY``
>  ^^^^^^^^^^^^^^^
>  
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 3277be1edb..f3e56d0bbe 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -55,6 +55,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
>  	MK_FLOW_ITEM(NVGRE, sizeof(struct rte_flow_item_nvgre)),
>  	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
>  	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
> +	MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),

Hmm? Adding a new item in the middle?

>  	MK_FLOW_ITEM(FUZZY, sizeof(struct rte_flow_item_fuzzy)),
>  	MK_FLOW_ITEM(GTP, sizeof(struct rte_flow_item_gtp)),
>  	MK_FLOW_ITEM(GTPC, sizeof(struct rte_flow_item_gtp)),
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index f3a8fb103f..5d3702a44c 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -289,6 +289,13 @@ enum rte_flow_item_type {
>  	 */
>  	RTE_FLOW_ITEM_TYPE_GRE,
>  
> +	/**
> +	 * Matches a GRE optional key field.
> +	 *
> +	 * The value should a big-endian 32bit integer.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_GRE_KEY,
> +

Same comment. While I understand the intent to group GRE and GRE_KEY, doing
so causes ABI breakage by shifting the value of all subsequent pattern
items (see IPV6 and IPV6_EXT for instance).

We could later decide to sort them while knowingly breaking ABI on purpose,
however right now there's no choice but adding new pattern items and actions
at the end of their respective enums, please do that.
  

On Wed, 19-07-03, 16:06, Thomas Monjalon wrote:
> 02/07/2019 11:45, Xiaoyu Min:
> > +	/**
> > +	 * Matches a GRE optional key field.
> > +	 *
> > +	 * The value should a big-endian 32bit integer.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_GRE_KEY,
> 
> We probably want to use the same format as in Dekel's patch
> for doxygen documentation of the action value:
> 

OK, I'll update it accordingly.
Thanks ~

>        /**
>         * Increase sequence number in the outermost TCP header.
>         *
>         * Action configuration specifies the value to increase
>         * TCP sequence number as a big-endian 32 bit integer.
>         *
>         * @p conf type:
>         * @code rte_be32_t * @endcode
>         *
>         * Using this action on non-matching traffic will result in
>         * undefined behavior.
>         */
>        RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ,
> 
> 
>
  

On Wed, 19-07-03, 17:25, Adrien Mazarguil wrote:
> On Tue, Jul 02, 2019 at 05:45:52PM +0800, Xiaoyu Min wrote:
> > Add new rte_flow_item_gre_key in order to match the optional key field.
> > 
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> 
> OK with adding this feature, however I still have a bunch of comments below.
> 
> > ---
> >  doc/guides/prog_guide/rte_flow.rst | 8 ++++++++
> >  lib/librte_ethdev/rte_flow.c       | 1 +
> >  lib/librte_ethdev/rte_flow.h       | 7 +++++++
> >  3 files changed, 16 insertions(+)
> > 
> > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> > index a34d012e55..f4b7baa3c3 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -980,6 +980,14 @@ Matches a GRE header.
> >  - ``protocol``: protocol type.
> >  - Default ``mask`` matches protocol only.
> >  
> > +Item: ``GRE_KEY``
> > +^^^^^^^^^^^^^^^^^
> > +
> > +Matches a GRE key field.
> > +This should be preceded by item ``GRE``
> 
> Nit: missing ending "."
> 

Ok, I'll add it.

> > +
> > +- Value to be matched is a big-endian 32 bit integer
> > +
> >  Item: ``FUZZY``
> >  ^^^^^^^^^^^^^^^
> >  
> > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > index 3277be1edb..f3e56d0bbe 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -55,6 +55,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
> >  	MK_FLOW_ITEM(NVGRE, sizeof(struct rte_flow_item_nvgre)),
> >  	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
> >  	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
> > +	MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),
> 
> Hmm? Adding a new item in the middle?
> 

I'll add it at the end.

> >  	MK_FLOW_ITEM(FUZZY, sizeof(struct rte_flow_item_fuzzy)),
> >  	MK_FLOW_ITEM(GTP, sizeof(struct rte_flow_item_gtp)),
> >  	MK_FLOW_ITEM(GTPC, sizeof(struct rte_flow_item_gtp)),
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index f3a8fb103f..5d3702a44c 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -289,6 +289,13 @@ enum rte_flow_item_type {
> >  	 */
> >  	RTE_FLOW_ITEM_TYPE_GRE,
> >  
> > +	/**
> > +	 * Matches a GRE optional key field.
> > +	 *
> > +	 * The value should a big-endian 32bit integer.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_GRE_KEY,
> > +
> 
> Same comment. While I understand the intent to group GRE and GRE_KEY, doing
> so causes ABI breakage by shifting the value of all subsequent pattern
> items (see IPV6 and IPV6_EXT for instance).
> 

Oh, I was't aware of this. Thank you for explaination.

> We could later decide to sort them while knowingly breaking ABI on purpose,
> however right now there's no choice but adding new pattern items and actions
> at the end of their respective enums, please do that.
> 

Yes, I'll do this.

> -- 
> Adrien Mazarguil
> 6WIND