Re: [dpdk-dev] [RFC] ethdev: enhance metadata as flow rule criteria

[Yongseok Koh <yskoh@mellanox.com>]

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
>