Re: [dpdk-dev] [PATCH v2 01/12] ethdev: add port representor item to flow API

[Ori Kam <orika@nvidia.com>]

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.malov@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.malov@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