Re: [dpdk-dev] [PATCH v4 1/6] ethdev: Add tunnel encap/decap actions

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

On Wed, Apr 18, 2018 at 10:04:18PM +0100, Declan Doherty wrote:
> Add new flow action types and associated action data structure to
> support the encapsulation and decapsulation of VXLAN/NVGRE tunnel
> endpoints.
> 
> The RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP or
> RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP actions will cause the matching
> flow to be encapsulated in the tunnel endpoint overlay
> defined in the rte_flow_action_tunnel_encap action definition.
> 
> The RTE_FLOW_ACTION_TYPE_VXLAN_DECAP and
> RTE_FLOW_ACTION_TYPE_NVGRE_DECAP actions will cause all headers
> associated with the outermost VXLAN/NVGRE tunnel overlay to be
> decapsulated from the matching flow.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>

So far so good, more comments below.

> ---
>  doc/guides/prog_guide/rte_flow.rst | 89 ++++++++++++++++++++++++++++++++++++++
>  lib/librte_ether/rte_flow.h        | 67 +++++++++++++++++++++++++++-
>  2 files changed, 155 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 961943dda..672473d33 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1486,6 +1486,95 @@ fields in the pattern items.
>     | 1     | END      |
>     +-------+----------+
>  
> +Action: ``VXLAN_ENCAP``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Performs a VXLAN encapsulation action by encapsulating the matched flow in the
> +VXLAN tunnel as defined in the``rte_flow_action_tunnel_encap`` flow items
> +definition.

Missing space before ``rte_flow_action_tunnel_encap``. However please define
a dedicated rte_flow_action_vxlan_encap and another for nvgre instead, even
if their contents are identical, in order to keep them synchronized with
their type name. There's not much overhead involved and it helps clarifying
the API.

> +
> +This action modifies the payload of matched flows. The flow definition specified
> +in the ``rte_flow_action_tunnel_encap`` action structure must defined a valid

defined => define

> +VLXAN network overlay which conforms with RFC 7348 (Virtual eXtensible Local Area
> +Network (VXLAN): A Framework for Overlaying Virtualized Layer 2 Networks over
> +Layer 3 Networks). The pattern must be terminated with
> +the RTE_FLOW_ITEM_TYPE_END item type.
> +
> +- Non-terminating by default.
> +
> +.. _table_rte_flow_action_tunnel_encap:
> +
> +.. table:: TUNNEL_ENCAP
> +
> +   +----------------+-------------------------------------+
> +   | Field          | Value                               |
> +   +================+=====================================+
> +   | ``definition`` | Tunnel end-point overlay definition |
> +   +----------------+-------------------------------------+
> +
> +.. _table_rte_flow_action_tunnel_encap_example:
> +
> +.. table:: IPv4 VxLAN flow pattern example.
> +
> +   +-------+----------+
> +   | Index | Item     |
> +   +=======+==========+
> +   | 0     | Ethernet |
> +   +-------+----------+
> +   | 1     | IPv4     |
> +   +-------+----------+
> +   | 2     | UDP      |
> +   +-------+----------+
> +   | 3     | VXLAN    |
> +   +-------+----------+
> +   | 4     | END      |
> +   +-------+----------+
> +
> +Action: ``VXLAN_DECAP``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Performs a decapsulation action by stripping all headers of the VXLAN tunnel
> +network overlay from the matched flow.
> +
> +This action modifies the payload of matched flows.

You should document "undefined behavior" when this action faces traffic
which does not contain a recognized VXLAN header.

> +
> +Action: ``NVGRE_ENCAP``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Performs a NVGRE encapsulation action by encapsulating the matched flow in the
> +NVGRE tunnel as defined in the``rte_flow_action_tunnel_encap`` flow item
> +definition.
> +
> +This action modifies the payload of matched flows. The flow definition specified
> +in the ``rte_flow_action_tunnel_encap`` action structure must defined a valid
> +NVGRE network overlay which conforms with RFC 7637 (NVGRE: Network Virtualization
> +Using Generic Routing Encapsulation). The pattern must be terminated with
> +the RTE_FLOW_ITEM_TYPE_END item type.

Same comment as for VXLAN.

> +
> +.. _table_rte_flow_action_tunnel_encap_example:
> +
> +.. table:: IPv4 NVGRE flow pattern example.
> +
> +   +-------+----------+
> +   | Index | Item     |
> +   +=======+==========+
> +   | 0     | Ethernet |
> +   +-------+----------+
> +   | 1     | IPv4     |
> +   +-------+----------+
> +   | 2     | NVGRE    |
> +   +-------+----------+
> +   | 3     | END      |
> +   +-------+----------+
> +
> +Action: ``NVGRE_DECAP``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Performs a decapsulation action by stripping all headers of the NVGRE tunnel
> +network overlay from the matched flow.
> +
> +This action modifies the payload of matched flows.
> +

Ditto.

>  Negative types
>  ~~~~~~~~~~~~~~
>  
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 56c733451..537e1bfba 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -1010,7 +1010,35 @@ enum rte_flow_action_type {
>  	 *
>  	 * See struct rte_flow_action_security.
>  	 */
> -	RTE_FLOW_ACTION_TYPE_SECURITY
> +	RTE_FLOW_ACTION_TYPE_SECURITY,
> +
> +	/**
> +	 * Encapsulate flow in VXLAN tunnel as defined in RFC7348
> +	 * using headers defined in rte_flow_action_tunnel_encap
> +	 * structure.
> +	 *
> +	 * See struct rte_flow_action_tunnel_encap.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP,
> +
> +	/**
> +	 * Decapsulate outer most VXLAN tunnel headers from flow
> +	 */
> +	RTE_FLOW_ACTION_TYPE_VXLAN_DECAP,
> +
> +	/**
> +	 * Encapsulate flow in NVGRE tunnel as defined in RFC7637
> +	 * using header defined in the rte_flow_action_tunnel_encap
> +	 * structure.
> +	 *
> +	 * See struct rte_flow_action_tunnel_encap.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP,
> +
> +	/**
> +	 * Decapsulate outer most NVGRE tunnel headers from flow
> +	 */
> +	RTE_FLOW_ACTION_TYPE_NVGRE_DECAP

Please add a trailing comma.

>  };
>  
>  /**
> @@ -1148,6 +1176,43 @@ struct rte_flow_action_security {
>  	void *security_session; /**< Pointer to security session structure. */
>  };
>  
> +/**
> + * RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP
> + *
> + * Tunnel end-point encapsulation data definition
> + *
> + * The tunnel definition is provided through the flow item pattern pattern, the
> + * provided pattern must conform with the corresponding RFC for the
> + * tunnel encapsulation action specified by the action type. The flow
> + * definition must be provided in order from the RTE_FLOW_ITEM_TYPE_ETH
> + * definition up the end item which is specified by RTE_FLOW_ITEM_TYPE_END.
> + *
> + * The mask field allows user to specify which fields in the flow item
> + * definitions can be ignored and which have valid data and can be used
> + * verbatim.
> + *
> + * Note: the last field is not used in the definition of a tunnel and can be
> + * ignored.
> + *
> + * For example if the actions type was RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP
> + * then valid patterns would include:
> + *
> + * - ETH / IPV4 / UDP / VXLAN / END
> + * - ETH / VLAN / IPV4 / UDP / VXLAN / END
> + *
> + * some valid examples for RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP include:
> + *
> + * - ETH / IPV4 / NVGRE / END
> + * - ETH / VLAN / IPV4 / NVGRE / END
> + */

The above must be split between VXLAN and NVGRE, since they should come as
distinct structures. In any case, make sure Doxygen is synchronized with its
rte_flow.rst counterpart.

> +struct rte_flow_action_tunnel_encap {
> +	struct rte_flow_item *definition;
> +	/**<
> +	 * Encapsulating tunnel definition
> +	 * (list terminated by the END pattern item).
> +	 */

Please put this comment before the documented field by replacing the opening
"/**<" with "/**". End result is the same but I find it clearer and more
consistent that way ("/**<" is still fine for one-liners).

> +};
> +
>  /**
>   * Definition of a single action.
>   *
> -- 
> 2.14.3
> 

I'm still unsure about the definition involving a list of items. I imagined
these actions even less generic than that. How about tagging them
EXPERIMENTAL until sorted out through the first PMD implementation?

(We could even provide forward declarations only to prevent applications
from using them in the meantime.)

-- 
Adrien Mazarguil
6WIND