[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.

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(+)
  

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
  

Hi Ori,

Thanks a lot for reviewing this.

On 10/10/2021 14:15, Ori Kam wrote:
> 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?

Yes, in the previous thread I suggested a different option. But
I gave it more thought and decided to go for PORT_REPRESENTOR
and REPRESENTED_PORT instead. In any case, I apologise for it
being such short notice.

> The only thing that worries me is that the naming are very easy to get wrong.
> port_representor and represented_port.

Is that so? In "port representor", the key word is "representor", a
noun. In "represented port", the key word is "port". Word "represented"
acts like an adjective. More to that, the order of words is chosen on
purpose to prevent someone from accidentally writing "representOR_port"
in their code, for example. That simply won't compile.

Well, at least, these two are harder to get wrong compared to "employer"
and "employee", for example.

The new names are better because they are paired terms. Each one
suggests the existence of the other one. This spares one from the
need to have wordy explanations of the symmetry of the diagram.
The new names speak themselves.

Saying "shadow" is in fact vague. The reader doesn't see clearly
that it's the shadow of the ethdev. And, in turn, "ethdev" is also
not as good as "representor". It's even hard to pronounce because
of being a combination of multiple contractions.

I hope you get the idea.

> 
> 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?

The semantics hasn't changed since the previous thread.
Representor coincides with the ethdev, so it's still the closest port.
And represented port is still the other side. It's all the same in fact.

If both VF and its representor are plugged to DPDK, they represent each
other in fact. Mutual representors. You can equally name the first one
a representor and the other one a represented port, - and vice versa.

The new naming doesn't deny both these ports being ethdevs, while
the previous option (ethdev and shadow) may trick the reader into
thinking that only one of the two can be an ethdev. The reader may
think that this use case (when VF and its representor are plugged to
the application) is completely wrong. Let's avoid this mistake.

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

Let me put it clear: I don't mind adding more comments. I can do that,
yes. But look. You say "matching is done on the source port". Isn't that
what "entering the embedded switch FROM the given ethdev" already says?

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

Well, as I said, technically, I don't object adding more comments. But
the above description says: "entering the embedded switch". Doesn't it
indicate the viewpoint clear enough? Should I reword this?

Also, "flow engine" is showed in the diagram. And there are arrows.
Doesn't that explain the viewpoint fully?

I'm ready to discuss / improve.

> 
>>
>> 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).

Is that so? Here's the previous drawing:


      [ A ]       <-- ethdev
        |
      [ B ]       <-- embedded switch (logical) port
        |
        |
        |
===============  <-- plane of symmetry
        |
        |
        |
      [ C ]       <-- embedded switch (logical) port
        |
      [ D ]       <-- represented entity


Technically, these two are exactly the same. Yes, I use precise names
instead of letters (A, B, C, D) and more formatting, but that should
help, not confuse.

And, as I say, in the new one, I don't have to use complex terms
like "plane of symmetry". The names speak for it themselves.

> 
>> +- 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
  

Hi Ivan.

> -----Original Message-----
> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
> Sent: Sunday, October 10, 2021 4:30 PM
> Subject: Re: [PATCH v2 01/12] ethdev: add port representor item to flow API
> 
> Hi Ori,
> 
> Thanks a lot for reviewing this.
> 
> On 10/10/2021 14:15, Ori Kam wrote:
> > 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?
> 
> Yes, in the previous thread I suggested a different option. But I gave it more thought and decided to go for
> PORT_REPRESENTOR and REPRESENTED_PORT instead. In any case, I apologise for it being such short
> notice.
> 
> > The only thing that worries me is that the naming are very easy to get wrong.
> > port_representor and represented_port.
> 
> Is that so? In "port representor", the key word is "representor", a noun. In "represented port", the key
> word is "port". Word "represented"
> acts like an adjective. More to that, the order of words is chosen on purpose to prevent someone from
> accidentally writing "representOR_port"
> in their code, for example. That simply won't compile.
> 
> Well, at least, these two are harder to get wrong compared to "employer"
> and "employee", for example.
> 
> The new names are better because they are paired terms. Each one suggests the existence of the other
> one. This spares one from the need to have wordy explanations of the symmetry of the diagram.
> The new names speak themselves.
> 
> Saying "shadow" is in fact vague. The reader doesn't see clearly that it's the shadow of the ethdev. And, in
> turn, "ethdev" is also not as good as "representor". It's even hard to pronounce because of being a
> combination of multiple contractions.
> 
> I hope you get the idea.
> 

I get the idea, since my English is not perfect from my view point those are a bit more confusing and easy
to get wrong but I have no objection to keep them.
Lets see what others will think about it.

> >
> > 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?
> 
> The semantics hasn't changed since the previous thread.
> Representor coincides with the ethdev, so it's still the closest port.
> And represented port is still the other side. It's all the same in fact.
> 
> If both VF and its representor are plugged to DPDK, they represent each other in fact. Mutual
> representors. You can equally name the first one a representor and the other one a represented port, -
> and vice versa.
> 
> The new naming doesn't deny both these ports being ethdevs, while the previous option (ethdev and
> shadow) may trick the reader into thinking that only one of the two can be an ethdev. The reader may
> think that this use case (when VF and its representor are plugged to the application) is completely wrong.
> Let's avoid this mistake.
> 

I agree those are semantics and there is no perfect world.
I accept your case.

> >
> >> -----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.
> 
> Let me put it clear: I don't mind adding more comments. I can do that, yes. But look. You say "matching is
> done on the source port". Isn't that what "entering the embedded switch FROM the given ethdev" already
> says?
> 
Yes,  the idea of this series is to clear everything.
Do you think that someone who just read this commit log can without prior knowledge of RTE flow
and transfer can fully understand the idea?

If so I'm, O.K. with leaving the comment as is.

In most related comment about pharsing I'm giving my view point with all
the comments I got about how much hard it is to create rules and understand the
documentation.

Unless there is something wrong or misleading I can except most answers and comments,
but I steel think that sometime it is worth saying them even if it is just for the developer to think
again if something can be improved.

> >
> > 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.
> 
> Well, as I said, technically, I don't object adding more comments. But the above description says: "entering
> the embedded switch". Doesn't it indicate the viewpoint clear enough? Should I reword this?
> 
> Also, "flow engine" is showed in the diagram. And there are arrows.
> Doesn't that explain the viewpoint fully?
> 
> I'm ready to discuss / improve.

Yes, but I was thinking about also adding it in the commit log.

> 
> >
> >>
> >> 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).
> 
> Is that so? Here's the previous drawing:
> 
> 
>       [ A ]       <-- ethdev
>         |
>       [ B ]       <-- embedded switch (logical) port
>         |
>         |
>         |
> ===============  <-- plane of symmetry
>         |
>         |
>         |
>       [ C ]       <-- embedded switch (logical) port
>         |
>       [ D ]       <-- represented entity
> 
> 
> Technically, these two are exactly the same. Yes, I use precise names instead of letters (A, B, C, D) and
> more formatting, but that should help, not confuse.
> 
> And, as I say, in the new one, I don't have to use complex terms like "plane of symmetry". The names
> speak for it themselves.
> 

That was what I was missing and the only difference between the two,
Th plane of symmetry (I'm not sure I like this name) but it is much cleared them embedded engine.
Embedded engine has meaning for me which is very different.
maybe just replace it with  switch? Or Embedded switch?
 
> >
> >> +- 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
> 
> --
> Ivan M

Best,
Ori
  

Hi Ori,

On 10/10/2021 17:04, Ori Kam wrote:
> Hi Ivan.
> 
>> -----Original Message-----
>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>> Sent: Sunday, October 10, 2021 4:30 PM
>> Subject: Re: [PATCH v2 01/12] ethdev: add port representor item to flow API
>>
>> Hi Ori,
>>
>> Thanks a lot for reviewing this.
>>
>> On 10/10/2021 14:15, Ori Kam wrote:
>>> 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?
>>
>> Yes, in the previous thread I suggested a different option. But I gave it more thought and decided to go for
>> PORT_REPRESENTOR and REPRESENTED_PORT instead. In any case, I apologise for it being such short
>> notice.
>>
>>> The only thing that worries me is that the naming are very easy to get wrong.
>>> port_representor and represented_port.
>>
>> Is that so? In "port representor", the key word is "representor", a noun. In "represented port", the key
>> word is "port". Word "represented"
>> acts like an adjective. More to that, the order of words is chosen on purpose to prevent someone from
>> accidentally writing "representOR_port"
>> in their code, for example. That simply won't compile.
>>
>> Well, at least, these two are harder to get wrong compared to "employer"
>> and "employee", for example.
>>
>> The new names are better because they are paired terms. Each one suggests the existence of the other
>> one. This spares one from the need to have wordy explanations of the symmetry of the diagram.
>> The new names speak themselves.
>>
>> Saying "shadow" is in fact vague. The reader doesn't see clearly that it's the shadow of the ethdev. And, in
>> turn, "ethdev" is also not as good as "representor". It's even hard to pronounce because of being a
>> combination of multiple contractions.
>>
>> I hope you get the idea.
>>
> 
> I get the idea, since my English is not perfect from my view point those are a bit more confusing and easy
> to get wrong but I have no objection to keep them.

Please don't think me forward: my English is not any better. But I just
think that if English is the lingua franca in DPDK and in the community,
then the corresponding rules of the language are supposed to apply.

> Lets see what others will think about it.
> 
>>>
>>> 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?
>>
>> The semantics hasn't changed since the previous thread.
>> Representor coincides with the ethdev, so it's still the closest port.
>> And represented port is still the other side. It's all the same in fact.
>>
>> If both VF and its representor are plugged to DPDK, they represent each other in fact. Mutual
>> representors. You can equally name the first one a representor and the other one a represented port, -
>> and vice versa.
>>
>> The new naming doesn't deny both these ports being ethdevs, while the previous option (ethdev and
>> shadow) may trick the reader into thinking that only one of the two can be an ethdev. The reader may
>> think that this use case (when VF and its representor are plugged to the application) is completely wrong.
>> Let's avoid this mistake.
>>
> 
> I agree those are semantics and there is no perfect world.
> I accept your case.

Thank you.

> 
>>>
>>>> -----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.
>>
>> Let me put it clear: I don't mind adding more comments. I can do that, yes. But look. You say "matching is
>> done on the source port". Isn't that what "entering the embedded switch FROM the given ethdev" already
>> says?
>>
> Yes,  the idea of this series is to clear everything.
> Do you think that someone who just read this commit log can without prior knowledge of RTE flow
> and transfer can fully understand the idea?

Well, the commit log is just an introduction to the diff itself. And,
in the diff, there is more commentary, diagrams, etc. Yes, of course,
I could have added some brief introduction to the commit log on what
RTE flow is and what "transfer" flows stand for, but I believe it's
already described in the existing documentation. However, I have no
strong opinion.

> 
> If so I'm, O.K. with leaving the comment as is.

I don't object changing it and probably I should do that in the next 
version should more reviewers indicate the same issue.

> 
> In most related comment about pharsing I'm giving my view point with all
> the comments I got about how much hard it is to create rules and understand the
> documentation.

I see.

> 
> Unless there is something wrong or misleading I can except most answers and comments,
> but I steel think that sometime it is worth saying them even if it is just for the developer to think
> again if something can be improved.

I'm just trying to use shorter comments in the code and commit logs as a 
starting point for being more concise. If necessary, I can extend that.

> 
>>>
>>> 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.
>>
>> Well, as I said, technically, I don't object adding more comments. But the above description says: "entering
>> the embedded switch". Doesn't it indicate the viewpoint clear enough? Should I reword this?
>>
>> Also, "flow engine" is showed in the diagram. And there are arrows.
>> Doesn't that explain the viewpoint fully?
>>
>> I'm ready to discuss / improve.
> 
> Yes, but I was thinking about also adding it in the commit log.

I see. In the new version I improve the diagram as suggested. Maybe that
makes the concept clearer. But I keep in mind improving my commit logs.

> 
>>
>>>
>>>>
>>>> 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).
>>
>> Is that so? Here's the previous drawing:
>>
>>
>>        [ A ]       <-- ethdev
>>          |
>>        [ B ]       <-- embedded switch (logical) port
>>          |
>>          |
>>          |
>> ===============  <-- plane of symmetry
>>          |
>>          |
>>          |
>>        [ C ]       <-- embedded switch (logical) port
>>          |
>>        [ D ]       <-- represented entity
>>
>>
>> Technically, these two are exactly the same. Yes, I use precise names instead of letters (A, B, C, D) and
>> more formatting, but that should help, not confuse.
>>
>> And, as I say, in the new one, I don't have to use complex terms like "plane of symmetry". The names
>> speak for it themselves.
>>
> 
> That was what I was missing and the only difference between the two,
> Th plane of symmetry (I'm not sure I like this name) but it is much cleared them embedded engine.
> Embedded engine has meaning for me which is very different.
> maybe just replace it with  switch? Or Embedded switch?

Agreed. Please see the new (v3) version. I improved the diagrams.
Do they look clearer now? I'm ready to discuss.

>   
>>>
>>>> +- 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
>>
>> --
>> Ivan M
> 
> Best,
> Ori
>