Re: [dpdk-dev] [PATCH 1/2] ethdev: add GENEVE flow pattern item

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

On Mon, Nov 20, 2017 at 08:22:00AM +0000, Andrew Rybchenko wrote:
> From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> 
> Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
> This commit also adds default mask for these item.
> 
> Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

OK, the main issue in this patch is you're inserting GENEVE in the middle of
everything (enums, documentation, etc). Just append it to the enum to avoid
ABI breakage as described in my previous message and use the same position
everywhere else for consistency.

It must appear after RTE_FLOW_ITEM_TYPE_ESP.

More comments below.

> ---
>  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
>  lib/librte_ether/rte_flow.c        |  1 +
>  lib/librte_ether/rte_flow.h        | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d158be5..2f96623 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -863,6 +863,18 @@ Matches a VXLAN header (RFC 7348).
>  - ``rsvd1``: reserved, normally 0x00.
>  - Default ``mask`` matches VNI only.
>  
> +Item: ``GENEVE``
> +^^^^^^^^^^^^^^^
> +
> +Matches a GENEVE header.
> +
> +- ``ver_opt_len_o_c_rsvd0``: version (2b), length of the options fields (6b),
> +  OAM packet (1b), critical options present (1b), reserved 0 (6b).
> +- ``protocol``: protocol type.
> +- ``vni``: virtual network identifier.
> +- ``rsvd1``: reserved, normally 0x00.
> +- Default ``mask`` matches protocol type and VNI.
> +

I'm not sure about the default mask. It should be the least common
denominator, not necessarily what the first implementation supports. How
about making it like VXLAN, i.e. VNI only? Does that make sense at all?

>  Item: ``E_TAG``
>  ^^^^^^^^^^^^^^^
>  
> diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
> index 6659063..bf1b253 100644
> --- a/lib/librte_ether/rte_flow.c
> +++ b/lib/librte_ether/rte_flow.c
> @@ -77,6 +77,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
>  	MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)),
>  	MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)),
>  	MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)),
> +	MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),

You should add it at the end but some items are already missing from that
list. Since I plan to send an overhaul for this function, you can leave this
change out for the time being.

>  	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
>  	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
>  	MK_FLOW_ITEM(E_TAG, sizeof(struct rte_flow_item_e_tag)),
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 47c88ea..29d81d4 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -272,6 +272,13 @@ enum rte_flow_item_type {
>  	RTE_FLOW_ITEM_TYPE_VXLAN,
>  
>  	/**
> +	 * Matches a GENEVE header.
> +	 *
> +	 * See struct rte_flow_item_geneve.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_GENEVE,
> +
> +	/**
>  	 * Matches a E_TAG header.
>  	 *
>  	 * See struct rte_flow_item_e_tag.
> @@ -651,6 +658,30 @@ static const struct rte_flow_item_vxlan rte_flow_item_vxlan_mask = {
>  #endif
>  
>  /**
> + * RTE_FLOW_ITEM_TYPE_GENEVE.
> + *
> + * Matches a GENEVE header.
> + */
> +struct rte_flow_item_geneve {
> +	/**
> +	 * Version (2b), length of the options fields (6b), OAM packet (1b),
> +	 * critical options present (1b), reserved 0 (6b).
> +	 */
> +	rte_be16_t ver_opt_len_o_c_rsvd0;
> +	rte_be16_t protocol; /**< Protocol type. */
> +	uint8_t vni[3]; /**< Virtual Network Identifier. */
> +	uint8_t rsvd1; /**< Reserved, normally 0x00. */
> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_GENEVE. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
> +	.protocol = RTE_BE16(0xffff),
> +	.vni = "\xff\xff\xff",
> +};
> +#endif

So how about removing .protocol from the default mask?

> +
> +/**
>   * RTE_FLOW_ITEM_TYPE_E_TAG.
>   *
>   * Matches a E-tag header.
> -- 
> 2.7.4
> 

-- 
Adrien Mazarguil
6WIND