Re: [dpdk-dev] [PATCH v3 4/4] ethdev: Add metadata flow and action items support

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

On Fri, Apr 06, 2018 at 01:24:03PM +0100, Declan Doherty wrote:
> Introduces a new action type RTE_FLOW_ACTION_TYPE_METADATA which enables
> metadata extraction from a packet into a specified metadata container
> for consumption on further pipeline stages or for propagation to the host
> interface.
> 
> As complementary function to the new metadata action type this patch also
> introduces a new flow item type which enables flow patterns to specify a
> specific metadata container as a matching criteria for a flow rule.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>

Is there already HW support for extraction and subsequent matching?

Otherwise it looks overkill. If it's meant to be implemented in software by
PMDs, isn't the existing MARK action with arbitrary value good enough?
All it needs is a corresponding MARK pattern item (which reminds me I forgot
to add it...)

My suggestion would be to submit this new item/action combo at the same
time as the first actual PMD implementation. In the meantime I think a much
simpler MARK pattern item could satisfy the most pressing needs.

> ---
>  doc/guides/prog_guide/rte_flow.rst | 85 ++++++++++++++++++++++++++++++++++++++
>  lib/librte_ether/rte_flow.h        | 42 +++++++++++++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 2f0a47a..9b2b0e3 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1580,6 +1580,91 @@ group on that device.
>     +--------------+---------------------------------+
>  
>  
> +

You keep adding these empty lines :)

> +Action: ``METADATA``
> +^^^^^^^^^^^^^^^^^^^^
> +
> +Action extracts data from packet into user specified metadata field in pipeline
> +for use in downstream processing stages or for propagation to host interface.
> +
> +The pattern mask is used to define the data which is to be extracted from the
> +packet. The mask pattern types defined in the action metadata pattern must
> +match the flow pattern definitions up to the last flow item from which data is
> +to be extracted.
> +
> +- Non-terminating by default.

Usual comment [1] regarding this property.

[1] "ethdev: alter behavior of flow API actions"
    http://dpdk.org/ml/archives/dev/2018-April/095779.html

> +
> +.. _table_rte_flow_action_metadata:
> +
> +.. table:: METADATA
> +
> +   +--------------+---------------------------+
> +   | Field        | Value                     |
> +   +==============+===========================+
> +   | ``id``       | Metadata field Identifier |
> +   +--------------+---------------------------+
> +   | ``pattern``  | Extraction mask pattern   |
> +   +--------------+---------------------------+
> +
> +The example below demonstrates how the extraction mask to extract the source/
> +destination IPv4 address, the UDP destination port and and the VxLAN VNI can be
> +specified.
> +
> +.. _table_rte_flow_action_metadata_example:
> +
> +.. table:: IPv4 VxLAN metadata extraction
> +
> +   +-------+--------------------------+-----------------------------------+
> +   | Index | Flow Item Type           | Flow Mask                         |
> +   +=======+==========================+===================================+
> +   | 0     | RTE_FLOW_ITEM_TYPE_ETH   | .dst = "\x00\x00\x00\x00\x00\x00" |
> +   |       |                          +-----------------------------------+
> +   |       |                          | .src = "\x00\x00\x00\x00\x00\x00" |
> +   |       |                          +-----------------------------------+
> +   |       |                          | .type = RTE_BE16(0x0)             |
> +   +-------+--------------------------+-----------------------------------+
> +   | 1     | RTE_FLOW_ITEM_TYPE_IPV4  | .src_addr = RTE_BE32(0xffffffff)  |
> +   |       |                          +-----------------------------------+
> +   |       |                          | .dst_addr = RTE_BE32(0xffffffff)  |
> +   +-------+--------------------------+-----------------------------------+
> +   | 2     | RTE_FLOW_ITEM_TYPE_UDP   | .src_port = RTE_BE16(0x0)         |
> +   |       |                          +-----------------------------------+
> +   |       |                          | .dst_port = RTE_BE16(0xffff)      |
> +   +-------+--------------------------+-----------------------------------+
> +   | 3     | RTE_FLOW_ITEM_TYPE_VXLAN | .vni = "\xff\xff\xff"             |
> +   +-------+--------------------------+-----------------------------------+
> +   | 4     | RTE_FLOW_ITEM_TYPE_END   | NULL                              |
> +   +-------+--------------------------+-----------------------------------+
> +
> +If only the VxLAN VNI extraction was required then the extraction mask would be
> +as follows.

VxLAN => VXLAN

> +
> +.. _table_rte_flow_action_metadata_example_2:
> +
> +.. table::  VxLAN VNI metadata extraction
> +
> +   +-------+--------------------------+-----------------------------------+
> +   | Index | Flow Item Type           | Flow Mask                         |
> +   +=======+==========================+===================================+
> +   | 0     | RTE_FLOW_ITEM_TYPE_ETH   | .dst = "\x00\x00\x00\x00\x00\x00" |
> +   |       |                          +-----------------------------------+
> +   |       |                          | .src = "\x00\x00\x00\x00\x00\x00" |
> +   |       |                          +-----------------------------------+
> +   |       |                          | .type = RTE_BE16(0x0)             |
> +   +-------+--------------------------+-----------------------------------+
> +   | 1     | RTE_FLOW_ITEM_TYPE_IPV4  | .src_addr = RTE_BE32(0x0)         |
> +   |       |                          +-----------------------------------+
> +   |       |                          | .dst_addr = RTE_BE32(0x0)         |
> +   +-------+--------------------------+-----------------------------------+
> +   | 2     | RTE_FLOW_ITEM_TYPE_UDP   | .src_port = RTE_BE16(0x0)         |
> +   |       |                          +-----------------------------------+
> +   |       |                          | .dst_port = RTE_BE16(0x0)         |
> +   +-------+--------------------------+-----------------------------------+
> +   | 3     | RTE_FLOW_ITEM_TYPE_VXLAN | .vni = "\xff\xff\xff"             |
> +   +-------+--------------------------+-----------------------------------+
> +   | 4     | RTE_FLOW_ITEM_TYPE_END   | NULL                              |
> +   +-------+--------------------------+-----------------------------------+
> +
>  Negative types
>  ~~~~~~~~~~~~~~
>  
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 968a23b..f75dee7 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -323,6 +323,13 @@ enum rte_flow_item_type {
>  	 * See struct rte_flow_item_geneve.
>  	 */
>  	RTE_FLOW_ITEM_TYPE_GENEVE,
> +
> +	/**
> +	 * Matches specified pipeline metadata field.
> +	 *
> +	 * See struct rte_flow_item_metadata.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_METADATA

Please add the trailing comma to ease subsequent contributions.

>  };
>  
>  /**
> @@ -815,6 +822,17 @@ static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
>  #endif
>  
>  /**
> + * RTE_FLOW_ITEM_TYPE_METADATA
> + *
> + * Allow arbitrary pipeline metadata to be used in specification flow pattern
> + */
> +struct rte_flow_item_metadata {
> +	uint32_t id;		/**< field identifier */
> +	uint32_t size;		/**< field size */
> +	uint8_t *bytes;		/**< field value */
> +};

One last nit regarding this definition (assuming we keep it), please remove
extra spacing before comments (no alignment) and add caps to format it like
the rest of that file.

> +
> +/**
>   * Matching pattern item definition.
>   *
>   * A pattern is formed by stacking items starting from the lowest protocol
> @@ -1266,6 +1284,30 @@ struct rte_flow_action_group {
>  };
>  
>  /**
> + * RTE_FLOW_ACTION_TYPE_METADATA

Definition in enum rte_flow_action_type should be part of this commit.

> + *
> + * Action extracts data from packet into specified metadata field in pipeline
> + * for use in downstream processing stages or for propagation to host interface.
> + *
> + * Pattern mask is use to define the extraction data for packet. The mask
> + * pattern types defined in the action metadata pattern must match the flow
> + * pattern definitions up to the last item from which data is to be extracted.
> + *
> + * Non-terminating by default.
> + */
> +struct rte_flow_action_metadata {
> +	uint32_t id;		/**< field identifier */
> +
> +	struct rte_flow_action_mask_item {
> +		enum rte_flow_item_type type;
> +		/**< Flow item type. */
> +		const void *mask;
> +		/**< Flow item mask. */
> +	} *pattern;
> +	/** metadata extraction pattern mask specification */
> +};

Same comment as in prior commit regarding this structure. You should use
struct rte_flow_item directly, except this time a pointer to a pattern
with multiple items seems warranted.

> +
> +/**
>   * Definition of a single action.
>   *
>   * A list of actions is terminated by a END action.
> -- 
> 2.7.4
> 

-- 
Adrien Mazarguil
6WIND