> -----Original Message-----
> From: dev <dev-boun...@dpdk.org> On Behalf Of Viacheslav Ovsiienko
> Sent: Thursday, October 24, 2019 4:12 PM
> To: dev@dpdk.org
> Cc: Thomas Monjalon <tho...@monjalon.net>; arybche...@solarflare.com;
> Matan Azrad <ma...@mellanox.com>; Raslan Darawsheh
> <rasl...@mellanox.com>; Yongseok Koh <ys...@mellanox.com>
> Subject: [dpdk-dev] [PATCH v3] ethdev: add flow tag
>
> A tag is a transient data which can be used during flow match. This can be
> used to store match result from a previous table so that the same pattern
> need not be matched again on the next table. Even if outer header is
> decapsulated on the previous match, the match result can be kept.
>
> Some device expose internal registers of its flow processing pipeline and
> those registers are quite useful for stateful connection tracking as it
> keeps status of flow matching. Multiple tags are supported by specifying
> index.
>
> Example testpmd commands are:
>
> flow create 0 ingress pattern ... / end
> actions set_tag index 2 value 0xaa00bb mask 0xffff00ff /
> set_tag index 3 value 0x123456 mask 0xffffff /
> vxlan_decap / jump group 1 / end
>
> flow create 0 ingress pattern ... / end
> actions set_tag index 2 value 0xcc00 mask 0xff00 /
> set_tag index 3 value 0x123456 mask 0xffffff /
> vxlan_decap / jump group 1 / end
>
> flow create 0 ingress group 1
> pattern tag index is 2 value spec 0xaa00bb value mask 0xffff00ff /
> eth ... / end
> actions ... jump group 2 / end
>
> flow create 0 ingress group 1
> pattern tag index is 2 value spec 0xcc00 value mask 0xff00 /
> tag index is 3 value spec 0x123456 value mask 0xffffff /
> eth ... / end
> actions ... / end
>
> flow create 0 ingress group 2
> pattern tag index is 3 value spec 0x123456 value mask 0xffffff /
> eth ... / end
> actions ... / end
>
> Signed-off-by: Yongseok Koh <ys...@mellanox.com>
> Signed-off-by: Viacheslav Ovsiienko <viachesl...@mellanox.com>
> ---
> v3: rebased, neat updates
> v2:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dp
> dk.org%2Fpatch%2F60909%2F&data=02%7C01%7Corika%40mellanox.com
> %7Cd91f4ba88d40409aac5a08d75883d0f2%7Ca652971c7d2e4d9ba6a4d14925
> 6f461b%7C0%7C0%7C637075195487679773&sdata=XrzhgAa2H%2BuWV%
> 2FxZu3XBnYkFv%2FVauLkjN7fAA0ROSss%3D&reserved=0
> v1:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dp
> dk.org%2Fpatch%2F56104%2F&data=02%7C01%7Corika%40mellanox.com
> %7Cd91f4ba88d40409aac5a08d75883d0f2%7Ca652971c7d2e4d9ba6a4d14925
> 6f461b%7C0%7C0%7C637075195487689768&sdata=e9C9LHb3b%2Fnif%2F
> 8S5ypeGDoEeVH%2FBayN3mX1q4p0arA%3D&reserved=0
> rfc:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dp
> dk.org%2Fpatch%2F54271%2F&data=02%7C01%7Corika%40mellanox.com
> %7Cd91f4ba88d40409aac5a08d75883d0f2%7Ca652971c7d2e4d9ba6a4d14925
> 6f461b%7C0%7C0%7C637075195487689768&sdata=3uP4UubC%2BpoDdtk
> iwSMwu2AwHm7yyBAJhItA%2Be9Q5co%3D&reserved=0
>
> app/test-pmd/cmdline_flow.c | 75
> ++++++++++++++++++++++++++++++++++
> doc/guides/prog_guide/rte_flow.rst | 50 +++++++++++++++++++++++
> doc/guides/rel_notes/release_19_11.rst | 5 +++
> lib/librte_ethdev/rte_flow.c | 2 +
> lib/librte_ethdev/rte_flow.h | 61 +++++++++++++++++++++++++++
> 5 files changed, 193 insertions(+)
>
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index bc89bf9..35852bd 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -206,6 +206,9 @@ enum index {
> ITEM_HIGIG2,
> ITEM_HIGIG2_CLASSIFICATION,
> ITEM_HIGIG2_VID,
> + ITEM_TAG,
> + ITEM_TAG_DATA,
> + ITEM_TAG_INDEX,
>
> /* Validate/create actions. */
> ACTIONS,
> @@ -311,6 +314,10 @@ enum index {
> ACTION_SET_META,
> ACTION_SET_META_DATA,
> ACTION_SET_META_MASK,
> + ACTION_SET_TAG,
> + ACTION_SET_TAG_INDEX,
> + ACTION_SET_TAG_DATA,
> + ACTION_SET_TAG_MASK,
> };
>
Can you please add them alphabetic , (data,index,mask) what do you think?
> /** Maximum size for pattern in struct rte_flow_item_raw. */
> @@ -682,6 +689,7 @@ struct parse_action_priv {
> ITEM_PPPOED,
> ITEM_PPPOE_PROTO_ID,
> ITEM_HIGIG2,
> + ITEM_TAG,
> END_SET,
> ZERO,
> };
> @@ -953,6 +961,13 @@ struct parse_action_priv {
> ZERO,
> };
>
> +static const enum index item_tag[] = {
> + ITEM_TAG_DATA,
> + ITEM_TAG_INDEX,
> + ITEM_NEXT,
> + ZERO,
> +};
> +
> static const enum index next_action[] = {
> ACTION_END,
> ACTION_VOID,
> @@ -1009,6 +1024,7 @@ struct parse_action_priv {
> ACTION_RAW_ENCAP,
> ACTION_RAW_DECAP,
> ACTION_SET_META,
> + ACTION_SET_TAG,
> ZERO,
> };
>
> @@ -1202,6 +1218,14 @@ struct parse_action_priv {
> ZERO,
> };
>
> +static const enum index action_set_tag[] = {
> + ACTION_SET_TAG_INDEX,
> + ACTION_SET_TAG_DATA,
> + ACTION_SET_TAG_MASK,
> + ACTION_NEXT,
> + ZERO,
> +};
> +
Again maybe order of the defines according.
> static int parse_set_raw_encap_decap(struct context *, const struct token *,
> const char *, unsigned int,
> void *, unsigned int);
> @@ -2467,6 +2491,26 @@ static int comp_vc_action_rss_queue(struct context
> *, const struct token *,
> .args = ARGS(ARGS_ENTRY_HTON(struct
> rte_flow_item_higig2_hdr,
> hdr.ppt1.vid)),
> },
> + [ITEM_TAG] = {
> + .name = "tag",
> + .help = "match tag value",
> + .priv = PRIV_ITEM(TAG, sizeof(struct rte_flow_item_tag)),
> + .next = NEXT(item_tag),
> + .call = parse_vc,
> + },
> + [ITEM_TAG_DATA] = {
> + .name = "data",
> + .help = "tag value to match",
> + .next = NEXT(item_tag, NEXT_ENTRY(UNSIGNED),
> item_param),
> + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_tag,
> data)),
> + },
> + [ITEM_TAG_INDEX] = {
> + .name = "index",
> + .help = "index of tag array to match",
> + .next = NEXT(item_tag, NEXT_ENTRY(UNSIGNED),
> + NEXT_ENTRY(ITEM_PARAM_IS)),
> + .args = ARGS(ARGS_ENTRY(struct rte_flow_item_tag, index)),
> + },
I think you are missing mask.
> /* Validate/create actions. */
> [ACTIONS] = {
> .name = "actions",
> @@ -3295,6 +3339,37 @@ static int comp_vc_action_rss_queue(struct context
> *, const struct token *,
> (struct rte_flow_action_set_meta, mask)),
> .call = parse_vc_conf,
> },
> + [ACTION_SET_TAG] = {
> + .name = "set_tag",
> + .help = "set tag",
> + .priv = PRIV_ACTION(SET_TAG,
> + sizeof(struct rte_flow_action_set_tag)),
> + .next = NEXT(action_set_tag),
> + .call = parse_vc,
> + },
> + [ACTION_SET_TAG_INDEX] = {
> + .name = "index",
> + .help = "index of tag array",
> + .next = NEXT(action_set_tag, NEXT_ENTRY(UNSIGNED)),
> + .args = ARGS(ARGS_ENTRY(struct rte_flow_action_set_tag,
> index)),
> + .call = parse_vc_conf,
> + },
> + [ACTION_SET_TAG_DATA] = {
> + .name = "data",
> + .help = "tag value",
> + .next = NEXT(action_set_tag, NEXT_ENTRY(UNSIGNED)),
> + .args = ARGS(ARGS_ENTRY_HTON
> + (struct rte_flow_action_set_tag, data)),
> + .call = parse_vc_conf,
> + },
> + [ACTION_SET_TAG_MASK] = {
> + .name = "mask",
> + .help = "mask for tag value",
> + .next = NEXT(action_set_tag, NEXT_ENTRY(UNSIGNED)),
> + .args = ARGS(ARGS_ENTRY_HTON
> + (struct rte_flow_action_set_tag, mask)),
> + .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 2b49baa..89a29b9 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -684,6 +684,34 @@ action sets metadata for a packet and the metadata
> will be reported via
> | ``mask`` | ``data`` | bit-mask applies to "spec" and "last" |
> +----------+----------+---------------------------------------+
>
> +Item: ``TAG``
> +^^^^^^^^^^^^^
> +
> +Matches tag item set by other flows. Multiple tags are supported by
> specifying
> +``index``.
> +
> +- Default ``mask`` matches the specified tag value and index.
> +
> +.. _table_rte_flow_item_tag:
> +
> +.. table:: TAG
> +
> + +----------+----------+----------------------------------------+
> + | Field | Subfield | Value |
> +
> +==========+===========+=======================================+
> + | ``spec`` | ``data`` | 32 bit flow tag value |
> + | +-----------+---------------------------------------+
> + | | ``index`` | index of flow tag |
> + +----------+-----------+---------------------------------------+
> + | ``last`` | ``data`` | upper range value |
> + | +-----------+ |
> + | | ``index`` | |
> + +----------+-----------+---------------------------------------+
I don't think last is relevant for this. Maybe is should be documented as
ignored.
> + | ``mask`` | ``data`` | bit-mask applies to "spec" and "last" |
> + | +-----------+ |
> + | | ``index`` | |
Should set index as ignored.
> + +----------+-----------+---------------------------------------+
> +
> Data matching item types
> ~~~~~~~~~~~~~~~~~~~~~~~~
>
> @@ -2508,6 +2536,28 @@ the other path depending on HW capability.
> | ``mask`` | bit-mask applies to "data" |
> +----------+----------------------------+
>
> +Action: ``SET_TAG``
> +^^^^^^^^^^^^^^^^^^^
> +
> +Set Tag.
> +
> +Tag is a transient data used during flow matching. This is not delivered to
> +application. Multiple tags are supported by specifying index.
> +
> +.. _table_rte_flow_action_set_tag:
> +
> +.. table:: SET_TAG
> +
> + +-----------+----------------------------+
> + | Field | Value |
> + +===========+============================+
> + | ``data`` | 32 bit tag value |
> + +-----------+----------------------------+
> + | ``mask`` | bit-mask applies to "data" |
> + +-----------+----------------------------+
> + | ``index`` | index of tag to set |
> + +-----------+----------------------------+
> +
> Negative types
> ~~~~~~~~~~~~~~
>
> diff --git a/doc/guides/rel_notes/release_19_11.rst
> b/doc/guides/rel_notes/release_19_11.rst
> index 2c51426..610191b 100644
> --- a/doc/guides/rel_notes/release_19_11.rst
> +++ b/doc/guides/rel_notes/release_19_11.rst
> @@ -208,6 +208,11 @@ New Features
> * Rx metadata is delivered to host via a dynamic field of ``rte_mbuf`` with
> PKT_RX_DYNF_METADATA.
>
> +* **Added flow tag in rte_flow.**
> + SET_TAG action and TAG item have been added to support transient flow
> + tag.
> +
> +
> Removed Items
> -------------
>
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 6090177..ec1d11d 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -82,6 +82,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(TAG, sizeof(struct rte_flow_item_tag)),
> MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),
> MK_FLOW_ITEM(GTP_PSC, sizeof(struct rte_flow_item_gtp_psc)),
> MK_FLOW_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)),
> @@ -166,6 +167,7 @@ struct rte_flow_desc_data {
> MK_FLOW_ACTION(INC_TCP_ACK, sizeof(rte_be32_t)),
> MK_FLOW_ACTION(DEC_TCP_ACK, sizeof(rte_be32_t)),
> MK_FLOW_ACTION(SET_META, sizeof(struct
> rte_flow_action_set_meta)),
> + MK_FLOW_ACTION(SET_TAG, sizeof(struct rte_flow_action_set_tag)),
> };
>
> int
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index b821557..4d56954 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -501,6 +501,15 @@ enum rte_flow_item_type {
> * see struct rte_flow_item_higig2_hdr.
> */
> RTE_FLOW_ITEM_TYPE_HIGIG2,
> +
> + /*
> + * [META]
> + *
Please remove the [META]
> + * Matches a tag value.
> + *
> + * See struct rte_flow_item_tag.
> + */
> + RTE_FLOW_ITEM_TYPE_TAG,
> };
>
> /**
> @@ -1350,6 +1359,27 @@ struct rte_flow_item_pppoe_proto_id {
> * @warning
> * @b EXPERIMENTAL: this structure may change without prior notice
> *
> + * RTE_FLOW_ITEM_TYPE_TAG
> + *
> + * Matches a specified tag value at the specified index.
> + */
> +struct rte_flow_item_tag {
> + uint32_t data;
> + uint8_t index;
> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_TAG. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_tag rte_flow_item_tag_mask = {
> + .data = 0xffffffff,
> + .index = 0xff,
> +};
> +#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
> @@ -1368,6 +1398,13 @@ struct rte_flow_item_mark {
> uint32_t id; /**< Integer value to match against. */
> };
>
> +/** Default mask for RTE_FLOW_ITEM_TYPE_MARK. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_mark rte_flow_item_mark_mask = {
> + .id = 0xffffffff,
> +};
> +#endif
> +
> /**
> * @warning
> * @b EXPERIMENTAL: this structure may change without prior notice
> @@ -1960,6 +1997,15 @@ enum rte_flow_action_type {
> * See struct rte_flow_action_set_meta.
> */
> RTE_FLOW_ACTION_TYPE_SET_META,
> +
> + /**
> + * Set Tag.
> + *
> + * Tag is not delivered to application.
> + *
I think we should think positive. Something like tag is for internal flow only
and is not delivered to the application.
What do you think?
> + * See struct rte_flow_action_set_tag.
> + */
> + RTE_FLOW_ACTION_TYPE_SET_TAG,
> };
>
> /**
> @@ -2496,6 +2542,21 @@ struct rte_flow_action_set_meta {
> *RTE_FLOW_DYNF_METADATA(m) = v;
> }
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_SET_TAG
> + *
> + * Set a tag which is a transient data used during flow matching. This is not
> + * delivered to application. Multiple tags are supported by specifying index.
> + */
> +struct rte_flow_action_set_tag {
> + uint32_t data;
> + uint32_t mask;
> + uint8_t index;
> +};
> +
> /*
> * Definition of a single action.
> *
> --
> 1.8.3.1
