Hi Ivan,
>From the new patches I saw you choose port_representor and represented_port
Didn't we agree to go with ETHDEV_PORT and SHADOW_PORT?
The only thing that worries me is that the naming are very easy to get wrong.
port_representor and represented_port.
Also there is an issue with wording if we assume like in previous thread that
DPDK can have both the representor port and also the represented_port.
While if we look at for example ethdev_port and shadow port are better defined
as ethdev_port -> the port that is closest to the DPDK ethdev while shadow port
is defined as the other side of the ethdev port.
What do you think?
> -----Original Message-----
> From: Ivan Malov <ivan.ma...@oktetlabs.ru>
> Sent: Sunday, October 10, 2021 3:05 AM
> Subject: [PATCH v2 01/12] ethdev: add port representor item to flow API
> ?
> For use in "transfer" flows. Supposed to match traffic entering the embedded
> switch from the given ethdev.
I would also add a comment that says the since we are in transfer it means
that all rules are located in the E-Switch which means that the matching is done
on the source port of the traffic.
I think this will also help understand the view point that we are looking from
the point
of the switch, and to add the drawing.
>
> Must not be combined with direction attributes.
>
> Signed-off-by: Ivan Malov <ivan.ma...@oktetlabs.ru>
> ---
> app/test-pmd/cmdline_flow.c | 27 ++++++++++
> doc/guides/prog_guide/rte_flow.rst | 59 +++++++++++++++++++++
> doc/guides/rel_notes/release_21_11.rst | 2 +
> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 4 ++
> lib/ethdev/rte_flow.c | 1 +
> lib/ethdev/rte_flow.h | 27 ++++++++++
> 6 files changed, 120 insertions(+)
>
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c index
> bb22294dd3..a912a8d815 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -306,6 +306,8 @@ enum index {
> ITEM_POL_PORT,
> ITEM_POL_METER,
> ITEM_POL_POLICY,
> + ITEM_PORT_REPRESENTOR,
> + ITEM_PORT_REPRESENTOR_PORT_ID,
>
> /* Validate/create actions. */
> ACTIONS,
> @@ -1000,6 +1002,7 @@ static const enum index next_item[] = {
> ITEM_GENEVE_OPT,
> ITEM_INTEGRITY,
> ITEM_CONNTRACK,
> + ITEM_PORT_REPRESENTOR,
> END_SET,
> ZERO,
> };
> @@ -1368,6 +1371,12 @@ static const enum index item_integrity_lv[] = {
> ZERO,
> };
>
> +static const enum index item_port_representor[] = {
> + ITEM_PORT_REPRESENTOR_PORT_ID,
> + ITEM_NEXT,
> + ZERO,
> +};
> +
> static const enum index next_action[] = {
> ACTION_END,
> ACTION_VOID,
> @@ -3608,6 +3617,21 @@ static const struct token token_list[] = {
> item_param),
> .args = ARGS(ARGS_ENTRY(struct rte_flow_item_conntrack,
> flags)),
> },
> + [ITEM_PORT_REPRESENTOR] = {
> + .name = "port_representor",
> + .help = "match traffic entering the embedded switch from the
> given ethdev",
> + .priv = PRIV_ITEM(PORT_REPRESENTOR,
> + sizeof(struct rte_flow_item_ethdev)),
> + .next = NEXT(item_port_representor),
> + .call = parse_vc,
> + },
> + [ITEM_PORT_REPRESENTOR_PORT_ID] = {
> + .name = "port_id",
> + .help = "ethdev port ID",
> + .next = NEXT(item_port_representor,
> NEXT_ENTRY(COMMON_UNSIGNED),
> + item_param),
> + .args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
> port_id)),
> + },
> /* Validate/create actions. */
> [ACTIONS] = {
> .name = "actions",
> @@ -8343,6 +8367,9 @@ flow_item_default_mask(const struct rte_flow_item
> *item)
> case RTE_FLOW_ITEM_TYPE_PFCP:
> mask = &rte_flow_item_pfcp_mask;
> break;
> + case RTE_FLOW_ITEM_TYPE_PORT_REPRESENTOR:
> + mask = &rte_flow_item_ethdev_mask;
> + break;
> default:
> break;
> }
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index 2b42d5ec8c..2e0f590777 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1425,6 +1425,65 @@ Matches a conntrack state after conntrack action.
> - ``flags``: conntrack packet state flags.
> - Default ``mask`` matches all state bits.
>
> +Item: ``PORT_REPRESENTOR``
> +^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Matches traffic entering the embedded switch from the given ethdev.
> +
> +Term **ethdev** and the concept of **port representor** are synonymous.
> +The **represented port** is an *entity* plugged to the embedded switch
> +at the opposite end of the "wire" leading to the ethdev.
> +
> +::
> +
> + .------------------------.
> + | PORT_REPRESENTOR | Ethdev (Application Port Referred to by its
> ID)
> + '------------------------'
> + ||
> + \/
> + .------------------------.
> + | Embedded Switch Port | Logical Port
> + '------------------------'
> + ||
> + ||
> + ||
> + \/
> + .------------------------.
> + | Embedded Flow Engine |
> + '------------------------'
> + :
> + :
> + :
> + :
> + .------------------------.
> + | Embedded Switch Port | Logical Port
> + '------------------------'
> + :
> + :
> + .------------------------.
> + | REPRESENTED_PORT | Net / Guest / Another Ethdev (Same
> Application)
> + '------------------------'
> +
> +
I think this drawing is harder to understand than the one
you draw in the previous thread (A-> ethdev, b-> embedded switch, switch , c->
embedded switch, d -> represented entity (shadow port).
> +- Incompatibe with `Attribute: Traffic direction`_.
> +- Requires `Attribute: Transfer`_.
> +
> +.. _table_rte_flow_item_ethdev:
> +
> +.. table:: ``struct rte_flow_item_ethdev``
> +
> + +----------+-------------+---------------------------+
> + | Field | Subfield | Value |
> + +==========+=============+===========================+
> + | ``spec`` | ``port_id`` | ethdev port ID |
> + +----------+-------------+---------------------------+
> + | ``last`` | ``port_id`` | upper range value |
> + +----------+-------------+---------------------------+
> + | ``mask`` | ``port_id`` | zeroed for wildcard match |
> + +----------+-------------+---------------------------+
> +
> +- Default ``mask`` provides exact match behaviour.
> +
> Actions
> ~~~~~~~
>
> diff --git a/doc/guides/rel_notes/release_21_11.rst
> b/doc/guides/rel_notes/release_21_11.rst
> index 89d4b33ef1..1261cb2bf3 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -188,6 +188,8 @@ API Changes
> Also, make sure to start the actual text at the margin.
> =======================================================
>
> +* ethdev: Added item ``PORT_REPRESENTOR`` to flow API.
> +
> * kvargs: The experimental function ``rte_kvargs_strcmp()`` has been
> removed. Its usages have been replaced by a new function
> ``rte_kvargs_get_with_value()``.
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 8ead7a4a71..dcb9f47d98 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3795,6 +3795,10 @@ This section lists supported pattern items and their
> attributes, if any.
>
> - ``conntrack``: match conntrack state.
>
> +- ``port_representor``: match traffic entering the embedded switch from
> +the given ethdev
> +
> + - ``port_id {unsigned}``: ethdev port ID
> +
> Actions list
> ^^^^^^^^^^^^
>
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> 8cb7a069c8..5e9317c6d1 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -100,6 +100,7 @@ static const struct rte_flow_desc_data
> rte_flow_desc_item[] = {
> MK_FLOW_ITEM(GENEVE_OPT, sizeof(struct
> rte_flow_item_geneve_opt)),
> MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
> MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
> + MK_FLOW_ITEM(PORT_REPRESENTOR, sizeof(struct
> rte_flow_item_ethdev)),
> };
>
> /** Generate flow_action[] entry. */
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> 7b1ed7f110..3625fd2c12 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -574,6 +574,15 @@ enum rte_flow_item_type {
> * @see struct rte_flow_item_conntrack.
> */
> RTE_FLOW_ITEM_TYPE_CONNTRACK,
> +
> + /**
> + * [META]
> + *
> + * Matches traffic entering the embedded switch from the given ethdev.
> + *
> + * @see struct rte_flow_item_ethdev
> + */
> + RTE_FLOW_ITEM_TYPE_PORT_REPRESENTOR,
> };
>
> /**
> @@ -1799,6 +1808,24 @@ static const struct rte_flow_item_conntrack
> rte_flow_item_conntrack_mask = { }; #endif
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * Provides an ethdev port ID for use with the following items:
> + * RTE_FLOW_ITEM_TYPE_PORT_REPRESENTOR.
> + */
> +struct rte_flow_item_ethdev {
> + uint16_t port_id; /**< ethdev port ID */ };
> +
> +/** Default mask for items based on struct rte_flow_item_ethdev */
> +#ifndef __cplusplus static const struct rte_flow_item_ethdev
> +rte_flow_item_ethdev_mask = {
> + .port_id = 0xffff,
> +};
> +#endif
> +
> /**
> * Matching pattern item definition.
> *
> --
> 2.20.1
