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

["Doherty, Declan" <declan.doherty@intel.com>]

On 06/04/2018 9:27 PM, Adrien Mazarguil wrote:
> 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.
>

I admit this was a bit future looking :) and probably overkill for what 
is needed today. I'll add the suggested MARK pattern item which will be 
enough to handle the OVS-DPDK tunnel encap/decap requirements.

>> ---
>>   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 :)

sorry.... what can I say I love whitespace :D

> 
>> +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
>>
>