Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add GRE key field to flow API

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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.

-- 
Adrien Mazarguil
6WIND