Re: [dpdk-dev] [PATCH v4 1/3] ethdev: add raw encapsulation action

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/17/18 12:41 AM, Ori Kam wrote:
> Currenlty the encap/decap actions only support encapsulation
> of VXLAN and NVGRE L2 packets (L2 encapsulation is where
> the inner packet has a valid Ethernet header, while L3 encapsulation
> is where the inner packet doesn't have the Ethernet header).
> In addtion the parameter to to the encap action is a list of rte items,
> this results in 2 extra translation, between the application to the
> actioni and from the action to the NIC. This results in negetive impact
> on the insertion performance.
>
> Looking forward there are going to be a need to support many more tunnel
> encapsulations. For example MPLSoGRE, MPLSoUDP.
> Adding the new encapsulation will result in duplication of code.
> For example the code for handling NVGRE and VXLAN are exactly the same,
> and each new tunnel will have the same exact structure.
>
> This patch introduce a raw encapsulation that can support L2 tunnel types
> and L3 tunnel types. In addtion the new
> encapsulations commands are using raw buffer inorder to save the
> converstion time, both for the application and the PMD.
>
> In order to encapsulate L3 tunnel type there is a need to use both
> actions in the same rule: The decap to remove the L2 of the original
> packet, and then encap command to encapsulate the packet with the
> tunnel.
> For decap L3 there is also a need to use both commands in the same flow
> first the decap command to remove the outer tunnel header and then encap
> to add the L2 header.
>
> Signed-off-by: Ori Kam <orika@mellanox.com>

[...]

> +Action: ``RAW_DECAP``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Remove outer header whose template is provided in it's data buffer,

Typo: 'it's' -> its

> +as defined in hte ``rte_flow_action_raw_decap``

Typo above 'hte' -> 'the'.

> +
> +This action modifies the payload of matched flows. The data supplied must
> +be a valid header, either holding layer 2 data in case of removing layer 2
> +before incapsulation of layer 3 tunnel (for example MPLSoGRE) or complete
> +tunnel definition starting from layer 2 and moving to the tunel item itself.
> +When applied to the original packet the resulting packet must be a
> +valid packet.
> +
> +.. _table_rte_flow_action_raw_decap:
> +
> +.. table:: RAW_DECAP
> +
> +   +----------------+----------------------------------------+
> +   | Field          | Value                                  |
> +   +================+========================================+
> +   | ``data``       | Decapsulation data                     |

Sorry, I've missed the point why it is here. Is it used for matching?
Why is the size insufficient?

> +   +----------------+----------------------------------------+
> +   | ``size``       | Size of data                           |
> +   +----------------+----------------------------------------+
> +
>   Action: ``SET_IPV4_SRC``
>   ^^^^^^^^^^^^^^^^^^^^^^^^
>   
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index bc9e719..1e5cd73 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -123,6 +123,8 @@ struct rte_flow_desc_data {
>   	MK_FLOW_ACTION(VXLAN_DECAP, 0),
>   	MK_FLOW_ACTION(NVGRE_ENCAP, sizeof(struct rte_flow_action_vxlan_encap)),
>   	MK_FLOW_ACTION(NVGRE_DECAP, 0),
> +	MK_FLOW_ACTION(RAW_ENCAP, sizeof(struct rte_flow_action_raw_encap)),
> +	MK_FLOW_ACTION(RAW_DECAP, sizeof(struct rte_flow_action_raw_decap)),
>   	MK_FLOW_ACTION(SET_IPV4_SRC,
>   		       sizeof(struct rte_flow_action_set_ipv4)),
>   	MK_FLOW_ACTION(SET_IPV4_DST,
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 68bbf57..3ae9de3 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1508,6 +1508,20 @@ enum rte_flow_action_type {
>   	RTE_FLOW_ACTION_TYPE_NVGRE_DECAP,
>   
>   	/**
> +	 * Adds outer header whose template is provided in it's data buffer

Adds -> Add, it's -> its

> +	 *
> +	 * See struct rte_flow_action_raw_encap.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_RAW_ENCAP,
> +
> +	/**
> +	 * Remove outer header whose tempalte is provided in it's data buffer.

it's -> its

> +	 *
> +	 * See struct rte_flow_action_raw_decap
> +	 */
> +	RTE_FLOW_ACTION_TYPE_RAW_DECAP,
> +
> +	/**
>   	 * Modify IPv4 source address in the outermost IPv4 header.
>   	 *
>   	 * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_IPV4,
> @@ -1946,6 +1960,51 @@ struct rte_flow_action_nvgre_encap {
>    * @warning
>    * @b EXPERIMENTAL: this structure may change without prior notice
>    *
> + * RTE_FLOW_ACTION_TYPE_RAW_ENCAP
> + *
> + * Raw tunnel end-point encapsulation data definition.
> + *
> + * The data holds the headers definitions to be applied on the packet.
> + * The data must start with ETH header up to the tunnel item header itself.
> + * When used right after RAW_DECAP (for decapsulating L3 tunnel type for
> + * example MPLSoGRE) the data will just hold layer 2 header.
> + *
> + * The preserve parameter holds which bits in the packet the PMD is not allowd

allowd -> allowed