[RFC] ethdev: enhance metadata as flow rule criteria

  Current implementation of rte_flow metadata match sets metadata only
for egress traffic leaving the need for such feature in ingress
flows unsatisfied and the lack of metadata index or ID limits it to
a single metadata action and match per flow which is a drawback in
several scenarios we would like to address.

This RFC proposes an enhancement of the existing implementation of
"[RFC,v2] ethdev: support metadata as flow rule criteria" without
breaking the API or ABI of the existing implementation.
We will do so by introducing new metadata item 'RTE_FLOW_ITEM_METADATA'
with a corresponding 'rte_flow_item_metadata' data structure,
and a new metadata action 'RTE_FLOW_ACTION_TYPE_SET_METADATA' with
a corresponding 'rte_flow_action_set_metadata' data structure.

These new action and item will be super-set of the existing
implementation allowing us to deprecate it later on.

Application will set and match on the new metadata action and item
respectively via the standard rte_flow rules, letting the PMD to worry
about the implementation details.

For example suppose an ingress flow packet has to traverse several
flow-groups before reaching its destination (fate), during this
traversal the packet may be changed several times by the flow rules,
like encap or decap headers, mac address modification, etc. making
the primary match on the packet header useless. How will we track the
flow making sure that the correct rules are applied to it ?
We can do so by setting a unique flow ID to each flow, setting the
metadata of each packet in that flow to that value, and now the rest
of the flow table will match on this unique value.
testpmd commands for such flow could look like:

testpmd> flow create 0 ingress pattern eth ... / end actions
         set_meta_data id 1 data 5 / vxlan_decap / jump group 1 / end

testpmd> flow create 0 ingress group 1 pattern metadata id is 1 data
         is 5 / end actions .... / end

Comments are welcome.

Signed-off-by: Moti Haimovsky <motih@mellanox.com>
---
 app/test-pmd/cmdline_flow.c        | 68 ++++++++++++++++++++++++++++++++++++++
 doc/guides/prog_guide/rte_flow.rst | 50 ++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_flow.c       |  3 ++
 lib/librte_ethdev/rte_flow.h       | 61 ++++++++++++++++++++++++++++++++++
 4 files changed, 182 insertions(+)
  

This has been overridden by another RFC.
http://patches.dpdk.org/project/dpdk/list/?series=4875


Thanks,
Yongseok

> On May 16, 2019, at 8:31 AM, Moti Haimovsky <motih@mellanox.com> wrote:
> 
> Current implementation of rte_flow metadata match sets metadata only
> for egress traffic leaving the need for such feature in ingress
> flows unsatisfied and the lack of metadata index or ID limits it to
> a single metadata action and match per flow which is a drawback in
> several scenarios we would like to address.
> 
> This RFC proposes an enhancement of the existing implementation of
> "[RFC,v2] ethdev: support metadata as flow rule criteria" without
> breaking the API or ABI of the existing implementation.
> We will do so by introducing new metadata item 'RTE_FLOW_ITEM_METADATA'
> with a corresponding 'rte_flow_item_metadata' data structure,
> and a new metadata action 'RTE_FLOW_ACTION_TYPE_SET_METADATA' with
> a corresponding 'rte_flow_action_set_metadata' data structure.
> 
> These new action and item will be super-set of the existing
> implementation allowing us to deprecate it later on.
> 
> Application will set and match on the new metadata action and item
> respectively via the standard rte_flow rules, letting the PMD to worry
> about the implementation details.
> 
> For example suppose an ingress flow packet has to traverse several
> flow-groups before reaching its destination (fate), during this
> traversal the packet may be changed several times by the flow rules,
> like encap or decap headers, mac address modification, etc. making
> the primary match on the packet header useless. How will we track the
> flow making sure that the correct rules are applied to it ?
> We can do so by setting a unique flow ID to each flow, setting the
> metadata of each packet in that flow to that value, and now the rest
> of the flow table will match on this unique value.
> testpmd commands for such flow could look like:
> 
> testpmd> flow create 0 ingress pattern eth ... / end actions
>         set_meta_data id 1 data 5 / vxlan_decap / jump group 1 / end
> 
> testpmd> flow create 0 ingress group 1 pattern metadata id is 1 data
>         is 5 / end actions .... / end
> 
> Comments are welcome.
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> ---
> app/test-pmd/cmdline_flow.c        | 68 ++++++++++++++++++++++++++++++++++++++
> doc/guides/prog_guide/rte_flow.rst | 50 ++++++++++++++++++++++++++++
> lib/librte_ethdev/rte_flow.c       |  3 ++
> lib/librte_ethdev/rte_flow.h       | 61 ++++++++++++++++++++++++++++++++++
> 4 files changed, 182 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 3070e0e..d10f511 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -181,6 +181,9 @@ enum index {
> 	ITEM_ICMP6_ND_OPT_TLA_ETH_TLA,
> 	ITEM_META,
> 	ITEM_META_DATA,
> +	ITEM_METADATA,
> +	ITEM_METADATA_ID,
> +	ITEM_METADATA_DATA,
> 
> 	/* Validate/create actions. */
> 	ACTIONS,
> @@ -272,6 +275,9 @@ enum index {
> 	ACTION_SET_MAC_SRC_MAC_SRC,
> 	ACTION_SET_MAC_DST,
> 	ACTION_SET_MAC_DST_MAC_DST,
> +	ACTION_SET_METADATA,
> +	ACTION_SET_METADATA_ID,
> +	ACTION_SET_METADATA_DATA,
> };
> 
> /** Maximum size for pattern in struct rte_flow_item_raw. */
> @@ -610,6 +616,7 @@ struct parse_action_priv {
> 	ITEM_ICMP6_ND_OPT_SLA_ETH,
> 	ITEM_ICMP6_ND_OPT_TLA_ETH,
> 	ITEM_META,
> +	ITEM_METADATA,
> 	ZERO,
> };
> 
> @@ -836,6 +843,13 @@ struct parse_action_priv {
> 	ZERO,
> };
> 
> +static const enum index item_metadata[] = {
> +	ITEM_METADATA_ID,
> +	ITEM_METADATA_DATA,
> +	ITEM_NEXT,
> +	ZERO,
> +};
> +
> static const enum index next_action[] = {
> 	ACTION_END,
> 	ACTION_VOID,
> @@ -885,6 +899,7 @@ struct parse_action_priv {
> 	ACTION_SET_TTL,
> 	ACTION_SET_MAC_SRC,
> 	ACTION_SET_MAC_DST,
> +	ACTION_SET_METADATA,
> 	ZERO,
> };
> 
> @@ -1047,6 +1062,14 @@ struct parse_action_priv {
> 	ZERO,
> };
> 
> +static const enum index action_set_metadata[] = {
> +	ACTION_SET_METADATA,
> +	ACTION_SET_METADATA_ID,
> +	ACTION_SET_METADATA_DATA,
> +	ACTION_NEXT,
> +	ZERO,
> +};
> +
> static int parse_init(struct context *, const struct token *,
> 		      const char *, unsigned int,
> 		      void *, unsigned int);
> @@ -2147,6 +2170,27 @@ static int comp_vc_action_rss_queue(struct context *, const struct token *,
> 		.name = "data",
> 		.help = "metadata value",
> 		.next = NEXT(item_meta, NEXT_ENTRY(UNSIGNED), item_param),
> +		.args = ARGS(ARGS_ENTRY_MASK(struct rte_flow_item_meta,
> +					     data, "\xff\xff\xff\xff")),
> +	},
> +	[ITEM_METADATA] = {
> +		.name = "metadata",
> +		.help = "match on metadata info",
> +		.priv = PRIV_ITEM(METADATA,
> +				  sizeof(struct rte_flow_item_metadata)),
> +		.next = NEXT(item_metadata),
> +		.call = parse_vc,
> +	},
> +	[ITEM_METADATA_ID] = {
> +		.name = "id",
> +		.help = " device-specific location of metadata",
> +		.next = NEXT(item_meta, NEXT_ENTRY(UNSIGNED), item_param),
> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_metadata, id)),
> +	},
> +	[ITEM_METADATA_DATA] = {
> +		.name = "data",
> +		.help = "metadata data value",
> +		.next = NEXT(item_meta, NEXT_ENTRY(UNSIGNED), item_param),
> 		.args = ARGS(ARGS_ENTRY_MASK_HTON(struct rte_flow_item_meta,
> 						  data, "\xff\xff\xff\xff")),
> 	},
> @@ -2854,6 +2898,30 @@ static int comp_vc_action_rss_queue(struct context *, const struct token *,
> 			     (struct rte_flow_action_set_mac, mac_addr)),
> 		.call = parse_vc_conf,
> 	},
> +	[ACTION_SET_METADATA] = {
> +		.name = "set_meta_data",
> +		.help = "attach metadata info to flow packets",
> +		.priv = PRIV_ACTION(SET_METADATA,
> +			sizeof(struct rte_flow_action_set_metadata)),
> +		.next = NEXT(action_set_metadata),
> +		.call = parse_vc,
> +	},
> +	[ACTION_SET_METADATA_ID] = {
> +		.name = "id",
> +		.help = "device-specific metadata id to use",
> +		.next = NEXT(action_set_metadata, NEXT_ENTRY(UNSIGNED)),
> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_set_metadata,
> +					id)),
> +		.call = parse_vc_conf,
> +	},
> +	[ACTION_SET_METADATA_DATA] = {
> +		.name = "data",
> +		.help = "data to use",
> +		.next = NEXT(action_set_metadata, NEXT_ENTRY(UNSIGNED)),
> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_set_metadata,
> +					data)),
> +		.call = parse_vc_conf,
> +	},
> };
> 
> /** Remove and return last entry from argument stack. */
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 937f52b..743c357 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1210,6 +1210,30 @@ Matches an application specific 32 bit metadata item.
>    | ``mask`` | ``data`` | bit-mask applies to "spec" and "last" |
>    +----------+----------+---------------------------------------+
> 
> +Item: ``METADATA``
> +^^^^^^^^^^^^^^^^^^
> +
> +Matches an application specific 32 bit metadata item present in the specified
> +location of a device-specific, per-packet metadata information.
> +This information is set either by a ``METADATA`` action in a previously matched
> +rule or by the device hardware itself.
> +
> +This item may be specified several times as a match criteria on several metadata
> +fields.
> +
> +Note the value of the data field is arbitrary and is PMD or application defined.
> +The value of the id field is device-dependent.
> +
> +Depending on the underlying implementation the METADATA item may be supported
> +as a set of registers or a memory region on the physical device,
> +with virtual groups in the PMD or not supported at all.
> +
> +- ``id``: The metadata field id to match.
> +- ``data``: The data to match against.
> +
> +- Default ``mask`` matches the specified integer value in the specified
> +  metadata location.
> +
> Actions
> ~~~~~~~
> 
> @@ -1476,6 +1500,32 @@ sets the ``PKT_RX_FDIR`` mbuf flag.
>    | no properties |
>    +---------------+
> 
> +Action: ``METADATA``
> +^^^^^^^^^^^^^^^^^^^^
> +
> +Insetrs a 32 bit integer value in the specified location of a device-specific
> +metadata information attached to each packet of a flow.
> +
> +This value is arbitrary and is application or PMD defined.
> +Maximum allowed value depends on the underlying implementation.
> +
> +Depending on the underlying implementation the METADATA action may be supported
> +as a set of registers or a memory region on the physical device, with virtual
> +groups in the PMD or not supported at all.
> +
> +.. _table_rte_flow_action_metadata:
> +
> +.. table:: METADATA
> +
> +   +----------+-----------------------------------------------------------+
> +   | Field    | Value                                                     |
> +   +==========+===========================================================+
> +   | ``id``   | unsigned integer value indicating where to write the data |
> +   |          | in the metadata info.                                     |
> +   +----------+-----------------------------------------------------------+
> +   | ``data`` | unsigned integer value of data to write                   |
> +   +----------+-----------------------------------------------------------+
> +
> Action: ``QUEUE``
> ^^^^^^^^^^^^^^^^^
> 
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 3277be1..ea2b9a0 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -74,6 +74,7 @@ struct rte_flow_desc_data {
> 		     sizeof(struct rte_flow_item_icmp6_nd_opt_tla_eth)),
> 	MK_FLOW_ITEM(MARK, sizeof(struct rte_flow_item_mark)),
> 	MK_FLOW_ITEM(META, sizeof(struct rte_flow_item_meta)),
> +	MK_FLOW_ITEM(METADATA, sizeof(struct rte_flow_item_metadata)),
> };
> 
> /** Generate flow_action[] entry. */
> @@ -143,6 +144,8 @@ struct rte_flow_desc_data {
> 	MK_FLOW_ACTION(SET_TTL, sizeof(struct rte_flow_action_set_ttl)),
> 	MK_FLOW_ACTION(SET_MAC_SRC, sizeof(struct rte_flow_action_set_mac)),
> 	MK_FLOW_ACTION(SET_MAC_DST, sizeof(struct rte_flow_action_set_mac)),
> +	MK_FLOW_ACTION(SET_METADATA,
> +		       sizeof(struct rte_flow_action_set_metadata)),
> };
> 
> static int
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 63f84fc..801831f 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -421,6 +421,15 @@ enum rte_flow_item_type {
> 	 * See struct rte_flow_item_meta.
> 	 */
> 	RTE_FLOW_ITEM_TYPE_META,
> +
> +	/**
> +	 * [METADATA]
> +	 *
> +	 * Matches a metadata value specified in device-specific metadata field
> +	 * attached to each flow.
> +	 * See struct rte_flow_item_metadata.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_METADATA,
> };
> 
> /**
> @@ -1183,6 +1192,33 @@ struct rte_flow_item_meta {
>  * @warning
>  * @b EXPERIMENTAL: this structure may change without prior notice
>  *
> + * RTE_FLOW_ITEM_TYPE_METADATA.
> + *
> + * Matches an arbitrary integer value in the device-specific per-flow metadata
> + * location identified by the given id.
> + * This data value may have been set using the `METADATA` action in a previously
> + * matched rule or by the underlying hardware itself.
> + *
> + * The offset field is device-specific and its interpretation may change
> + * according to the device in use and its current configuration.
> + */
> +struct rte_flow_item_metadata {
> +	uint32_t id;
> +	rte_be32_t data;
> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_METADATA. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_metadata rte_flow_item_metadata_mask = {
> +	.id = UINT32_MAX,
> +	.data = RTE_BE32(UINT32_MAX),
> +};
> +#endif
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
>  * RTE_FLOW_ITEM_TYPE_MARK
>  *
>  * Matches an arbitrary integer value which was set using the ``MARK`` action
> @@ -1650,6 +1686,14 @@ enum rte_flow_action_type {
> 	 * See struct rte_flow_action_set_mac.
> 	 */
> 	RTE_FLOW_ACTION_TYPE_SET_MAC_DST,
> +
> +	/**
> +	 * Attaches an integer value to device-specific metadata info attached
> +	 * to each packet in a flow.
> +	 *
> +	 * See struct rte_flow_action_set_metadata.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_SET_METADATA,
> };
> 
> /**
> @@ -2131,6 +2175,23 @@ struct rte_flow_action_set_mac {
> 	uint8_t mac_addr[ETHER_ADDR_LEN];
> };
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_SET_METADATA.
> + *
> + * Attaches an integer value to device-specific metadata info attached
> + * to each packet in a flow.
> + *
> + * The offset field is device-specific and its interpretation may change
> + * according to the device in use and its current configuration.
> + */
> +struct rte_flow_action_set_metadata {
> +	uint32_t id;
> +	rte_be32_t data;
> +};
> +
> /*
>  * Definition of a single action.
>  *
> -- 
> 1.8.3.1
>