[v1,02/12] ethdev: add eswitch port item to flow API

Message ID 20211001134716.1608857-3-andrew.rybchenko@oktetlabs.ru (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: rework transfer flow API |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Andrew Rybchenko Oct. 1, 2021, 1:47 p.m. UTC
  From: Ivan Malov <ivan.malov@oktetlabs.ru>

For use with "transfer" flows. Supposed to match traffic entering
the e-switch from the external world (network, guests) via the
port which is logically connected with the given ethdev.

Must not be combined with attributes "ingress" / "egress".

This item is meant to use the same structure as ethdev item.

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 app/test-pmd/cmdline_flow.c                 | 27 +++++++++++++++++++++
 doc/guides/prog_guide/rte_flow.rst          | 22 +++++++++++++++++
 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                       | 12 ++++++++-
 6 files changed, 66 insertions(+), 2 deletions(-)
  

Comments

Ori Kam Oct. 3, 2021, 12:40 p.m. UTC | #1
Hi Andrew and Ivan,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, October 1, 2021 4:47 PM
> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
> 
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> 
> For use with "transfer" flows. Supposed to match traffic entering the e-switch
> from the external world (network, guests) via the port which is logically
> connected with the given ethdev.
> 
> Must not be combined with attributes "ingress" / "egress".
> 
> This item is meant to use the same structure as ethdev item.
> 

In case the app is not working with representors, meaning
each switch port is mapped to ethdev.
both items (ethdev and eswitch port ) have the same meaning?

> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
>  app/test-pmd/cmdline_flow.c                 | 27 +++++++++++++++++++++
>  doc/guides/prog_guide/rte_flow.rst          | 22 +++++++++++++++++
>  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                       | 12 ++++++++-
>  6 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index e05b0d83d2..188d0ee39d 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -308,6 +308,8 @@ enum index {
>  	ITEM_POL_POLICY,
>  	ITEM_ETHDEV,
>  	ITEM_ETHDEV_ID,
> +	ITEM_ESWITCH_PORT,
> +	ITEM_ESWITCH_PORT_ETHDEV_ID,

Like my comment from previous patch, I'm not sure the correct
term for ETHDEV is ID is should be port.

> 
>  	/* Validate/create actions. */
>  	ACTIONS,
> @@ -1003,6 +1005,7 @@ static const enum index next_item[] = {
>  	ITEM_INTEGRITY,
>  	ITEM_CONNTRACK,
>  	ITEM_ETHDEV,
> +	ITEM_ESWITCH_PORT,
>  	END_SET,
>  	ZERO,
>  };
> @@ -1377,6 +1380,12 @@ static const enum index item_ethdev[] = {
>  	ZERO,
>  };
> 
> +static const enum index item_eswitch_port[] = {
> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
> +	ITEM_NEXT,
> +	ZERO,
> +};
> +
>  static const enum index next_action[] = {
>  	ACTION_END,
>  	ACTION_VOID,
> @@ -3632,6 +3641,21 @@ static const struct token token_list[] = {
>  			     item_param),
>  		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev, id)),
>  	},
> +	[ITEM_ESWITCH_PORT] = {
> +		.name = "eswitch_port",
> +		.help = "match traffic at e-switch going from the external port
> associated with the given ethdev",

Missing the word logically since if we are talking about representor the connected port
is the PF while we want to match traffic on one of the FVs.

> +		.priv = PRIV_ITEM(ESWITCH_PORT,
> +				  sizeof(struct rte_flow_item_ethdev)),
> +		.next = NEXT(item_eswitch_port),
> +		.call = parse_vc,
> +	},
> +	[ITEM_ESWITCH_PORT_ETHDEV_ID] = {
> +		.name = "ethdev_id",
> +		.help = "ethdev ID",
> +		.next = NEXT(item_eswitch_port,
> NEXT_ENTRY(COMMON_UNSIGNED),
> +			     item_param),
> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev, id)),
> +	},
>  	/* Validate/create actions. */
>  	[ACTIONS] = {
>  		.name = "actions",
> @@ -8370,6 +8394,9 @@ flow_item_default_mask(const struct
> rte_flow_item *item)
>  	case RTE_FLOW_ITEM_TYPE_ETHDEV:
>  		mask = &rte_flow_item_ethdev_mask;
>  		break;
> +	case RTE_FLOW_ITEM_TYPE_ESWITCH_PORT:
> +		mask = &rte_flow_item_ethdev_mask;
> +		break;

Not sure maybe merged the two cases?

>  	default:
>  		break;
>  	}
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index ab628d9139..292bb42410 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1460,6 +1460,28 @@ Use this with attribute **transfer**. Attributes
> **ingress** and
>     | ``mask`` | ``id``   | zeroed for wildcard match |
>     +----------+----------+---------------------------+
> 
> +Item: ``ESWITCH_PORT``
> +^^^^^^^^^^^^^^^^^^^^^^
> +
> +Matches traffic at e-switch going from the external port associated
> +with the given ethdev, for example, traffic from net. port or guest.

Maybe replace external with e-switch?

> +
> +::
> +
> +   *    (Ethdev) ~~~~~~~~~~~~ (Internal Port) ~~~~ [] <<<< (External Port)
> +   *    :  SW                 :   Logical                    Net / Guest :
> +   *    :                     :                                          :
> +   *    | ---- PMD Layer ---- | ------------ E-Switch Layer ------------ |
> +   *
> +   *    [] shows the effective ("transfer") standpoint, the match engine;
> +   *    << shows the traffic flow in question hitting the match engine;
> +   *    ~~ shows logical interconnects between the endpoints.
> +

I'm not sure I understand this diagram.

> +Use this with attribute **transfer**. Attributes **ingress** and
> +**egress** (`Attribute: Traffic direction`_) must not be used.
> +
> +This item is meant to use the same structure as `Item: ETHDEV`_.
> +
>  Actions
>  ~~~~~~~
> 
> diff --git a/doc/guides/rel_notes/release_21_11.rst
> b/doc/guides/rel_notes/release_21_11.rst
> index 91631adb4e..b2b27de3f0 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -167,7 +167,7 @@ API Changes
>     Also, make sure to start the actual text at the margin.
>     =======================================================
> 
> -* ethdev: Added item ``ETHDEV`` to flow API.
> +* ethdev: Added items ``ETHDEV``, ``ESWITCH_PORT`` to flow API.
> 
>  * cryptodev: The API rte_cryptodev_pmd_is_valid_dev is modified to
>    rte_cryptodev_is_valid_dev as it can be used by the application as diff --git
> a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 6d5de5457c..9a5c2a2d82 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3824,6 +3824,10 @@ This section lists supported pattern items and
> their attributes, if any.
> 
>    - ``id {unsigned}``: ethdev ID
> 
> +- ``eswitch_port``: match traffic at e-switch going from the external
> +port associated with the given ethdev
> +
> +  - ``ethdev_id {unsigned}``: ethdev ID
> +
>  Actions list
>  ^^^^^^^^^^^^
> 
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> 84eb61cb6c..c4aea5625f 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -101,6 +101,7 @@ static const struct rte_flow_desc_data
> rte_flow_desc_item[] = {
>  	MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
>  	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
>  	MK_FLOW_ITEM(ETHDEV, sizeof(struct rte_flow_item_ethdev)),
> +	MK_FLOW_ITEM(ESWITCH_PORT, 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
> 880502098e..1a7e4c2e3d 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -583,6 +583,16 @@ enum rte_flow_item_type {
>  	 * @see struct rte_flow_item_ethdev
>  	 */
>  	RTE_FLOW_ITEM_TYPE_ETHDEV,
> +
> +	/**
> +	 * [META]
> +	 *
> +	 * Matches traffic at e-switch going from the external port associated
> +	 * with the given ethdev, for example, traffic from net. port or guest.
> +	 *
> +	 * @see struct rte_flow_item_ethdev
> +	 */
> +	RTE_FLOW_ITEM_TYPE_ESWITCH_PORT,
>  };
> 
>  /**
> @@ -1813,7 +1823,7 @@ static const struct rte_flow_item_conntrack
> rte_flow_item_conntrack_mask = {
>   * @b EXPERIMENTAL: this structure may change without prior notice
>   *
>   * Provides an ethdev ID for use with items which are as follows:
> - * RTE_FLOW_ITEM_TYPE_ETHDEV.
> + * RTE_FLOW_ITEM_TYPE_ETHDEV,
> RTE_FLOW_ITEM_TYPE_ESWITCH_PORT.
>   */
>  struct rte_flow_item_ethdev {
>  	uint16_t id; /**< Ethdev ID */
> --
> 2.30.2

Best,
Ori
  
Ivan Malov Oct. 3, 2021, 6:10 p.m. UTC | #2
On 03/10/2021 15:40, Ori Kam wrote:
> Hi Andrew and Ivan,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Friday, October 1, 2021 4:47 PM
>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
>>
>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>
>> For use with "transfer" flows. Supposed to match traffic entering the e-switch
>> from the external world (network, guests) via the port which is logically
>> connected with the given ethdev.
>>
>> Must not be combined with attributes "ingress" / "egress".
>>
>> This item is meant to use the same structure as ethdev item.
>>
> 
> In case the app is not working with representors, meaning
> each switch port is mapped to ethdev.
> both items (ethdev and eswitch port ) have the same meaning?

No. Ethdev means ethdev, and e-switch port is the point where this 
ethdev is plugged to. For example, "transfer + ESWITCH_PORT" for a 
regular PF ethdev typically means the network port (maybe you can recall 
the idea that a PF ethdev "represents" the network port it's associated 
with).

I believe, that diagrams which these patches add to 
"doc/guides/prog_guide/rte_flow.rst" may come in handy to understand the 
meaning. Also, you can take a look at our larger diagram from the Sep 14 
gathering.

> 
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---
>>   app/test-pmd/cmdline_flow.c                 | 27 +++++++++++++++++++++
>>   doc/guides/prog_guide/rte_flow.rst          | 22 +++++++++++++++++
>>   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                       | 12 ++++++++-
>>   6 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>> index e05b0d83d2..188d0ee39d 100644
>> --- a/app/test-pmd/cmdline_flow.c
>> +++ b/app/test-pmd/cmdline_flow.c
>> @@ -308,6 +308,8 @@ enum index {
>>   	ITEM_POL_POLICY,
>>   	ITEM_ETHDEV,
>>   	ITEM_ETHDEV_ID,
>> +	ITEM_ESWITCH_PORT,
>> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
> 
> Like my comment from previous patch, I'm not sure the correct
> term for ETHDEV is ID is should be port.

Please see my reply in the previous thread. "ethdev" here is an 
"anchor", a "beacon" of sorts which allows either to refer namely to 
this ethdev or to the e-switch port associated with it.

> 
>>
>>   	/* Validate/create actions. */
>>   	ACTIONS,
>> @@ -1003,6 +1005,7 @@ static const enum index next_item[] = {
>>   	ITEM_INTEGRITY,
>>   	ITEM_CONNTRACK,
>>   	ITEM_ETHDEV,
>> +	ITEM_ESWITCH_PORT,
>>   	END_SET,
>>   	ZERO,
>>   };
>> @@ -1377,6 +1380,12 @@ static const enum index item_ethdev[] = {
>>   	ZERO,
>>   };
>>
>> +static const enum index item_eswitch_port[] = {
>> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
>> +	ITEM_NEXT,
>> +	ZERO,
>> +};
>> +
>>   static const enum index next_action[] = {
>>   	ACTION_END,
>>   	ACTION_VOID,
>> @@ -3632,6 +3641,21 @@ static const struct token token_list[] = {
>>   			     item_param),
>>   		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev, id)),
>>   	},
>> +	[ITEM_ESWITCH_PORT] = {
>> +		.name = "eswitch_port",
>> +		.help = "match traffic at e-switch going from the external port
>> associated with the given ethdev",
> 
> Missing the word logically since if we are talking about representor the connected port
> is the PF while we want to match traffic on one of the FVs.

Doesn't the word "external" say it all?

Representor Ethdev <--> Admin ethdev's PF <--> E-Switch <--> VF 
(external / the most remote endpoint).

> 
>> +		.priv = PRIV_ITEM(ESWITCH_PORT,
>> +				  sizeof(struct rte_flow_item_ethdev)),
>> +		.next = NEXT(item_eswitch_port),
>> +		.call = parse_vc,
>> +	},
>> +	[ITEM_ESWITCH_PORT_ETHDEV_ID] = {
>> +		.name = "ethdev_id",
>> +		.help = "ethdev ID",
>> +		.next = NEXT(item_eswitch_port,
>> NEXT_ENTRY(COMMON_UNSIGNED),
>> +			     item_param),
>> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev, id)),
>> +	},
>>   	/* Validate/create actions. */
>>   	[ACTIONS] = {
>>   		.name = "actions",
>> @@ -8370,6 +8394,9 @@ flow_item_default_mask(const struct
>> rte_flow_item *item)
>>   	case RTE_FLOW_ITEM_TYPE_ETHDEV:
>>   		mask = &rte_flow_item_ethdev_mask;
>>   		break;
>> +	case RTE_FLOW_ITEM_TYPE_ESWITCH_PORT:
>> +		mask = &rte_flow_item_ethdev_mask;
>> +		break;
> 
> Not sure maybe merged the two cases?

A noble idea.

> 
>>   	default:
>>   		break;
>>   	}
>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>> b/doc/guides/prog_guide/rte_flow.rst
>> index ab628d9139..292bb42410 100644
>> --- a/doc/guides/prog_guide/rte_flow.rst
>> +++ b/doc/guides/prog_guide/rte_flow.rst
>> @@ -1460,6 +1460,28 @@ Use this with attribute **transfer**. Attributes
>> **ingress** and
>>      | ``mask`` | ``id``   | zeroed for wildcard match |
>>      +----------+----------+---------------------------+
>>
>> +Item: ``ESWITCH_PORT``
>> +^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +Matches traffic at e-switch going from the external port associated
>> +with the given ethdev, for example, traffic from net. port or guest.
> 
> Maybe replace external with e-switch?

The word "e-switch" is already here. Basically, all "external" ports are 
"e-switch external ports" = ports which connect the e-switch with 
non-DPDK world: network ports, VFs passed to guests, etc.

> 
>> +
>> +::
>> +
>> +   *    (Ethdev) ~~~~~~~~~~~~ (Internal Port) ~~~~ [] <<<< (External Port)
>> +   *    :  SW                 :   Logical                    Net / Guest :
>> +   *    :                     :                                          :
>> +   *    | ---- PMD Layer ---- | ------------ E-Switch Layer ------------ |
>> +   *
>> +   *    [] shows the effective ("transfer") standpoint, the match engine;
>> +   *    << shows the traffic flow in question hitting the match engine;
>> +   *    ~~ shows logical interconnects between the endpoints.
>> +
> 
> I'm not sure I understand this diagram.

Thanks for the feedback. I have no way with drawing fancy diagrams, so 
this one may not be ideal, yes. But could you please elaborate on your 
concerns. Maybe you understand it the right way but just unsure. If you 
describe what you see when looking at the diagram, I'll be able to 
either confirm your vision or debunk any misunderstanding and possibly 
improve the drawing.

> 
>> +Use this with attribute **transfer**. Attributes **ingress** and
>> +**egress** (`Attribute: Traffic direction`_) must not be used.
>> +
>> +This item is meant to use the same structure as `Item: ETHDEV`_.
>> +
>>   Actions
>>   ~~~~~~~
>>
>> diff --git a/doc/guides/rel_notes/release_21_11.rst
>> b/doc/guides/rel_notes/release_21_11.rst
>> index 91631adb4e..b2b27de3f0 100644
>> --- a/doc/guides/rel_notes/release_21_11.rst
>> +++ b/doc/guides/rel_notes/release_21_11.rst
>> @@ -167,7 +167,7 @@ API Changes
>>      Also, make sure to start the actual text at the margin.
>>      =======================================================
>>
>> -* ethdev: Added item ``ETHDEV`` to flow API.
>> +* ethdev: Added items ``ETHDEV``, ``ESWITCH_PORT`` to flow API.
>>
>>   * cryptodev: The API rte_cryptodev_pmd_is_valid_dev is modified to
>>     rte_cryptodev_is_valid_dev as it can be used by the application as diff --git
>> a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> index 6d5de5457c..9a5c2a2d82 100644
>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> @@ -3824,6 +3824,10 @@ This section lists supported pattern items and
>> their attributes, if any.
>>
>>     - ``id {unsigned}``: ethdev ID
>>
>> +- ``eswitch_port``: match traffic at e-switch going from the external
>> +port associated with the given ethdev
>> +
>> +  - ``ethdev_id {unsigned}``: ethdev ID
>> +
>>   Actions list
>>   ^^^^^^^^^^^^
>>
>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
>> 84eb61cb6c..c4aea5625f 100644
>> --- a/lib/ethdev/rte_flow.c
>> +++ b/lib/ethdev/rte_flow.c
>> @@ -101,6 +101,7 @@ static const struct rte_flow_desc_data
>> rte_flow_desc_item[] = {
>>   	MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
>>   	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
>>   	MK_FLOW_ITEM(ETHDEV, sizeof(struct rte_flow_item_ethdev)),
>> +	MK_FLOW_ITEM(ESWITCH_PORT, 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
>> 880502098e..1a7e4c2e3d 100644
>> --- a/lib/ethdev/rte_flow.h
>> +++ b/lib/ethdev/rte_flow.h
>> @@ -583,6 +583,16 @@ enum rte_flow_item_type {
>>   	 * @see struct rte_flow_item_ethdev
>>   	 */
>>   	RTE_FLOW_ITEM_TYPE_ETHDEV,
>> +
>> +	/**
>> +	 * [META]
>> +	 *
>> +	 * Matches traffic at e-switch going from the external port associated
>> +	 * with the given ethdev, for example, traffic from net. port or guest.
>> +	 *
>> +	 * @see struct rte_flow_item_ethdev
>> +	 */
>> +	RTE_FLOW_ITEM_TYPE_ESWITCH_PORT,
>>   };
>>
>>   /**
>> @@ -1813,7 +1823,7 @@ static const struct rte_flow_item_conntrack
>> rte_flow_item_conntrack_mask = {
>>    * @b EXPERIMENTAL: this structure may change without prior notice
>>    *
>>    * Provides an ethdev ID for use with items which are as follows:
>> - * RTE_FLOW_ITEM_TYPE_ETHDEV.
>> + * RTE_FLOW_ITEM_TYPE_ETHDEV,
>> RTE_FLOW_ITEM_TYPE_ESWITCH_PORT.
>>    */
>>   struct rte_flow_item_ethdev {
>>   	uint16_t id; /**< Ethdev ID */
>> --
>> 2.30.2
> 
> Best,
> Ori
>
  
Ori Kam Oct. 4, 2021, 5:45 a.m. UTC | #3
Hi Ivan,

> -----Original Message-----
> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
> Sent: Sunday, October 3, 2021 9:11 PM
> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
> 
> 
> 
> On 03/10/2021 15:40, Ori Kam wrote:
> > Hi Andrew and Ivan,
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Friday, October 1, 2021 4:47 PM
> >> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
> >>
> >> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> >>
> >> For use with "transfer" flows. Supposed to match traffic entering the
> >> e-switch from the external world (network, guests) via the port which
> >> is logically connected with the given ethdev.
> >>
> >> Must not be combined with attributes "ingress" / "egress".
> >>
> >> This item is meant to use the same structure as ethdev item.
> >>
> >
> > In case the app is not working with representors, meaning each switch
> > port is mapped to ethdev.
> > both items (ethdev and eswitch port ) have the same meaning?
> 
> No. Ethdev means ethdev, and e-switch port is the point where this ethdev
> is plugged to. For example, "transfer + ESWITCH_PORT" for a regular PF
> ethdev typically means the network port (maybe you can recall the idea that
> a PF ethdev "represents" the network port it's associated with).
> 
> I believe, that diagrams which these patches add to
> "doc/guides/prog_guide/rte_flow.rst" may come in handy to understand the
> meaning. Also, you can take a look at our larger diagram from the Sep 14
> gathering.
> 

Lets look at the following system:
E-Switch has 3 ports - PF, VF1, VF2
The ports are distributed as follows:
DPDK application:
ethdev(0) pf,
ethdev(1) representor to VF1
ethdev(2) representor to VF2
ethdev(3) VF1

VM:
VF2

As we know all representors are realy connected to the PF(at least in this example)

So matching on ethdev(3)  means matching on traffic sent from DPDK port 3 right?
And matching on eswitch_port(3) means matching in traffic that goes into VF1 which
is the same traffic as ethdev(3) right?

Matching on ethdev(1) means matching on the PF port in the E-Switch but with some
metadata that marks the traffic as coming from DPDK port 1 and not from VF1 E-Switch
port right?

While matching on eswitch_port(2) means matching on traffic coming from the VM right?
 
> >
> >> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> ---
> >>   app/test-pmd/cmdline_flow.c                 | 27 +++++++++++++++++++++
> >>   doc/guides/prog_guide/rte_flow.rst          | 22 +++++++++++++++++
> >>   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                       | 12 ++++++++-
> >>   6 files changed, 66 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-
> pmd/cmdline_flow.c
> >> index e05b0d83d2..188d0ee39d 100644
> >> --- a/app/test-pmd/cmdline_flow.c
> >> +++ b/app/test-pmd/cmdline_flow.c
> >> @@ -308,6 +308,8 @@ enum index {
> >>   	ITEM_POL_POLICY,
> >>   	ITEM_ETHDEV,
> >>   	ITEM_ETHDEV_ID,
> >> +	ITEM_ESWITCH_PORT,
> >> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
> >
> > Like my comment from previous patch, I'm not sure the correct
> > term for ETHDEV is ID is should be port.
> 
> Please see my reply in the previous thread. "ethdev" here is an
> "anchor", a "beacon" of sorts which allows either to refer namely to
> this ethdev or to the e-switch port associated with it.
> 
> >
> >>
> >>   	/* Validate/create actions. */
> >>   	ACTIONS,
> >> @@ -1003,6 +1005,7 @@ static const enum index next_item[] = {
> >>   	ITEM_INTEGRITY,
> >>   	ITEM_CONNTRACK,
> >>   	ITEM_ETHDEV,
> >> +	ITEM_ESWITCH_PORT,
> >>   	END_SET,
> >>   	ZERO,
> >>   };
> >> @@ -1377,6 +1380,12 @@ static const enum index item_ethdev[] = {
> >>   	ZERO,
> >>   };
> >>
> >> +static const enum index item_eswitch_port[] = {
> >> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
> >> +	ITEM_NEXT,
> >> +	ZERO,
> >> +};
> >> +
> >>   static const enum index next_action[] = {
> >>   	ACTION_END,
> >>   	ACTION_VOID,
> >> @@ -3632,6 +3641,21 @@ static const struct token token_list[] = {
> >>   			     item_param),
> >>   		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
> id)),
> >>   	},
> >> +	[ITEM_ESWITCH_PORT] = {
> >> +		.name = "eswitch_port",
> >> +		.help = "match traffic at e-switch going from the external port
> >> associated with the given ethdev",
> >
> > Missing the word logically since if we are talking about representor the
> connected port
> > is the PF while we want to match traffic on one of the FVs.
> 
> Doesn't the word "external" say it all?
> 
> Representor Ethdev <--> Admin ethdev's PF <--> E-Switch <--> VF
> (external / the most remote endpoint).
> 

Until the last comment External had totally different meaning to me.
I think you should add some place the meaning of external or use
the most remote endpoint.

> >
> >> +		.priv = PRIV_ITEM(ESWITCH_PORT,
> >> +				  sizeof(struct rte_flow_item_ethdev)),
> >> +		.next = NEXT(item_eswitch_port),
> >> +		.call = parse_vc,
> >> +	},
> >> +	[ITEM_ESWITCH_PORT_ETHDEV_ID] = {
> >> +		.name = "ethdev_id",
> >> +		.help = "ethdev ID",
> >> +		.next = NEXT(item_eswitch_port,
> >> NEXT_ENTRY(COMMON_UNSIGNED),
> >> +			     item_param),
> >> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
> id)),
> >> +	},
> >>   	/* Validate/create actions. */
> >>   	[ACTIONS] = {
> >>   		.name = "actions",
> >> @@ -8370,6 +8394,9 @@ flow_item_default_mask(const struct
> >> rte_flow_item *item)
> >>   	case RTE_FLOW_ITEM_TYPE_ETHDEV:
> >>   		mask = &rte_flow_item_ethdev_mask;
> >>   		break;
> >> +	case RTE_FLOW_ITEM_TYPE_ESWITCH_PORT:
> >> +		mask = &rte_flow_item_ethdev_mask;
> >> +		break;
> >
> > Not sure maybe merged the two cases?
> 
> A noble idea.
> 
> >
> >>   	default:
> >>   		break;
> >>   	}
> >> diff --git a/doc/guides/prog_guide/rte_flow.rst
> >> b/doc/guides/prog_guide/rte_flow.rst
> >> index ab628d9139..292bb42410 100644
> >> --- a/doc/guides/prog_guide/rte_flow.rst
> >> +++ b/doc/guides/prog_guide/rte_flow.rst
> >> @@ -1460,6 +1460,28 @@ Use this with attribute **transfer**. Attributes
> >> **ingress** and
> >>      | ``mask`` | ``id``   | zeroed for wildcard match |
> >>      +----------+----------+---------------------------+
> >>
> >> +Item: ``ESWITCH_PORT``
> >> +^^^^^^^^^^^^^^^^^^^^^^
> >> +
> >> +Matches traffic at e-switch going from the external port associated
> >> +with the given ethdev, for example, traffic from net. port or guest.
> >
> > Maybe replace external with e-switch?
> 
> The word "e-switch" is already here. Basically, all "external" ports are
> "e-switch external ports" = ports which connect the e-switch with
> non-DPDK world: network ports, VFs passed to guests, etc.
> 

Please see comment above, the word external is not clearly defined.

> >
> >> +
> >> +::
> >> +
> >> +   *    (Ethdev) ~~~~~~~~~~~~ (Internal Port) ~~~~ [] <<<< (External
> Port)
> >> +   *    :  SW                 :   Logical                    Net / Guest :
> >> +   *    :                     :                                          :
> >> +   *    | ---- PMD Layer ---- | ------------ E-Switch Layer ------------ |
> >> +   *
> >> +   *    [] shows the effective ("transfer") standpoint, the match engine;
> >> +   *    << shows the traffic flow in question hitting the match engine;
> >> +   *    ~~ shows logical interconnects between the endpoints.
> >> +
> >
> > I'm not sure I understand this diagram.
> 
> Thanks for the feedback. I have no way with drawing fancy diagrams, so
> this one may not be ideal, yes. But could you please elaborate on your
> concerns. Maybe you understand it the right way but just unsure. If you
> describe what you see when looking at the diagram, I'll be able to
> either confirm your vision or debunk any misunderstanding and possibly
> improve the drawing.
> 


I will try,
Ethdev is the port that the application sees in DPDK right?
Internal port is what the PMD sees, for example in case of representor it will see the PF port.
External (also due to a drawing in one of your comments) is the E-Switch end point.
So this drawing shows that the app select the external port that is connected to the
internal port which is connected to the ethdev port?

Thanks,
Ori
> >
> >> +Use this with attribute **transfer**. Attributes **ingress** and
> >> +**egress** (`Attribute: Traffic direction`_) must not be used.
> >> +
> >> +This item is meant to use the same structure as `Item: ETHDEV`_.
> >> +
> >>   Actions
> >>   ~~~~~~~
> >>
> >> diff --git a/doc/guides/rel_notes/release_21_11.rst
> >> b/doc/guides/rel_notes/release_21_11.rst
> >> index 91631adb4e..b2b27de3f0 100644
> >> --- a/doc/guides/rel_notes/release_21_11.rst
> >> +++ b/doc/guides/rel_notes/release_21_11.rst
> >> @@ -167,7 +167,7 @@ API Changes
> >>      Also, make sure to start the actual text at the margin.
> >>
> =======================================================
> >>
> >> -* ethdev: Added item ``ETHDEV`` to flow API.
> >> +* ethdev: Added items ``ETHDEV``, ``ESWITCH_PORT`` to flow API.
> >>
> >>   * cryptodev: The API rte_cryptodev_pmd_is_valid_dev is modified to
> >>     rte_cryptodev_is_valid_dev as it can be used by the application as diff --
> git
> >> a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >> index 6d5de5457c..9a5c2a2d82 100644
> >> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >> @@ -3824,6 +3824,10 @@ This section lists supported pattern items and
> >> their attributes, if any.
> >>
> >>     - ``id {unsigned}``: ethdev ID
> >>
> >> +- ``eswitch_port``: match traffic at e-switch going from the external
> >> +port associated with the given ethdev
> >> +
> >> +  - ``ethdev_id {unsigned}``: ethdev ID
> >> +
> >>   Actions list
> >>   ^^^^^^^^^^^^
> >>
> >> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> >> 84eb61cb6c..c4aea5625f 100644
> >> --- a/lib/ethdev/rte_flow.c
> >> +++ b/lib/ethdev/rte_flow.c
> >> @@ -101,6 +101,7 @@ static const struct rte_flow_desc_data
> >> rte_flow_desc_item[] = {
> >>   	MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
> >>   	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
> >>   	MK_FLOW_ITEM(ETHDEV, sizeof(struct rte_flow_item_ethdev)),
> >> +	MK_FLOW_ITEM(ESWITCH_PORT, 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
> >> 880502098e..1a7e4c2e3d 100644
> >> --- a/lib/ethdev/rte_flow.h
> >> +++ b/lib/ethdev/rte_flow.h
> >> @@ -583,6 +583,16 @@ enum rte_flow_item_type {
> >>   	 * @see struct rte_flow_item_ethdev
> >>   	 */
> >>   	RTE_FLOW_ITEM_TYPE_ETHDEV,
> >> +
> >> +	/**
> >> +	 * [META]
> >> +	 *
> >> +	 * Matches traffic at e-switch going from the external port associated
> >> +	 * with the given ethdev, for example, traffic from net. port or guest.
> >> +	 *
> >> +	 * @see struct rte_flow_item_ethdev
> >> +	 */
> >> +	RTE_FLOW_ITEM_TYPE_ESWITCH_PORT,
> >>   };
> >>
> >>   /**
> >> @@ -1813,7 +1823,7 @@ static const struct rte_flow_item_conntrack
> >> rte_flow_item_conntrack_mask = {
> >>    * @b EXPERIMENTAL: this structure may change without prior notice
> >>    *
> >>    * Provides an ethdev ID for use with items which are as follows:
> >> - * RTE_FLOW_ITEM_TYPE_ETHDEV.
> >> + * RTE_FLOW_ITEM_TYPE_ETHDEV,
> >> RTE_FLOW_ITEM_TYPE_ESWITCH_PORT.
> >>    */
> >>   struct rte_flow_item_ethdev {
> >>   	uint16_t id; /**< Ethdev ID */
> >> --
> >> 2.30.2
> >
> > Best,
> > Ori
> >
> 
> --
> Ivan M
  
Ivan Malov Oct. 4, 2021, 11:05 a.m. UTC | #4
Hi Ori,

On 04/10/2021 08:45, Ori Kam wrote:
> Hi Ivan,
> 
>> -----Original Message-----
>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>> Sent: Sunday, October 3, 2021 9:11 PM
>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
>>
>>
>>
>> On 03/10/2021 15:40, Ori Kam wrote:
>>> Hi Andrew and Ivan,
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Friday, October 1, 2021 4:47 PM
>>>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
>>>>
>>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>
>>>> For use with "transfer" flows. Supposed to match traffic entering the
>>>> e-switch from the external world (network, guests) via the port which
>>>> is logically connected with the given ethdev.
>>>>
>>>> Must not be combined with attributes "ingress" / "egress".
>>>>
>>>> This item is meant to use the same structure as ethdev item.
>>>>
>>>
>>> In case the app is not working with representors, meaning each switch
>>> port is mapped to ethdev.
>>> both items (ethdev and eswitch port ) have the same meaning?
>>
>> No. Ethdev means ethdev, and e-switch port is the point where this ethdev
>> is plugged to. For example, "transfer + ESWITCH_PORT" for a regular PF
>> ethdev typically means the network port (maybe you can recall the idea that
>> a PF ethdev "represents" the network port it's associated with).
>>
>> I believe, that diagrams which these patches add to
>> "doc/guides/prog_guide/rte_flow.rst" may come in handy to understand the
>> meaning. Also, you can take a look at our larger diagram from the Sep 14
>> gathering.
>>
> 
> Lets look at the following system:
> E-Switch has 3 ports - PF, VF1, VF2
> The ports are distributed as follows:
> DPDK application:
> ethdev(0) pf,
> ethdev(1) representor to VF1
> ethdev(2) representor to VF2
> ethdev(3) VF1
> 
> VM:
> VF2
> 
> As we know all representors are realy connected to the PF(at least in this example)

This example tries to say that the e-switch has 3 ports in total, and, 
given your explanation, one may indeed agree that *in this example* 
representors re-use e-switch port of ethdev=0 (with some metadata to 
distinguish packets, etc.). But one can hardly assume that *all* 
representors with any vendor's NIC are connected to the e-switch the 
same way. It's vendor specific. Well, at least, applications don't have 
this knowledge and don't need to.

> 
> So matching on ethdev(3)  means matching on traffic sent from DPDK port 3 right?

Item ETHDEV (ethdev_id=3) matches traffic sent by DPDK port 3. Looks 
like we're on the same page here.

> And matching on eswitch_port(3) means matching in traffic that goes into VF1 which
> is the same traffic as ethdev(3) right?

I didn't catch the thought about "the same traffic". Direction is not 
the same. Item ESWITCH_PORT (ethdev_id=3) matches traffic sent by DPDK 
port 1.

Yes, in this case neither of the ports (1, 3) is truly "external" (they 
both interface the DPDK application), but, the thing is, they're 
"external" *to each other* in the sense that they sit at the opposite 
ends of the wire.

> 
> Matching on ethdev(1) means matching on the PF port in the E-Switch but with some
> metadata that marks the traffic as coming from DPDK port 1 and not from VF1 E-Switch
> port right?

That's vendor specific. The application doesn't have to know how exactly 
this particular ethdev is connected to the e-switch - whether it re-uses 
the PF's e-switch port or has its own. The e-switch port that connects 
the ethdev with the e-switch is just assumed to exist logically.

> 
> While matching on eswitch_port(2) means matching on traffic coming from the VM right?

Right.

>   
>>>
>>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> ---
>>>>    app/test-pmd/cmdline_flow.c                 | 27 +++++++++++++++++++++
>>>>    doc/guides/prog_guide/rte_flow.rst          | 22 +++++++++++++++++
>>>>    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                       | 12 ++++++++-
>>>>    6 files changed, 66 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-
>> pmd/cmdline_flow.c
>>>> index e05b0d83d2..188d0ee39d 100644
>>>> --- a/app/test-pmd/cmdline_flow.c
>>>> +++ b/app/test-pmd/cmdline_flow.c
>>>> @@ -308,6 +308,8 @@ enum index {
>>>>    	ITEM_POL_POLICY,
>>>>    	ITEM_ETHDEV,
>>>>    	ITEM_ETHDEV_ID,
>>>> +	ITEM_ESWITCH_PORT,
>>>> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
>>>
>>> Like my comment from previous patch, I'm not sure the correct
>>> term for ETHDEV is ID is should be port.
>>
>> Please see my reply in the previous thread. "ethdev" here is an
>> "anchor", a "beacon" of sorts which allows either to refer namely to
>> this ethdev or to the e-switch port associated with it.
>>
>>>
>>>>
>>>>    	/* Validate/create actions. */
>>>>    	ACTIONS,
>>>> @@ -1003,6 +1005,7 @@ static const enum index next_item[] = {
>>>>    	ITEM_INTEGRITY,
>>>>    	ITEM_CONNTRACK,
>>>>    	ITEM_ETHDEV,
>>>> +	ITEM_ESWITCH_PORT,
>>>>    	END_SET,
>>>>    	ZERO,
>>>>    };
>>>> @@ -1377,6 +1380,12 @@ static const enum index item_ethdev[] = {
>>>>    	ZERO,
>>>>    };
>>>>
>>>> +static const enum index item_eswitch_port[] = {
>>>> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
>>>> +	ITEM_NEXT,
>>>> +	ZERO,
>>>> +};
>>>> +
>>>>    static const enum index next_action[] = {
>>>>    	ACTION_END,
>>>>    	ACTION_VOID,
>>>> @@ -3632,6 +3641,21 @@ static const struct token token_list[] = {
>>>>    			     item_param),
>>>>    		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
>> id)),
>>>>    	},
>>>> +	[ITEM_ESWITCH_PORT] = {
>>>> +		.name = "eswitch_port",
>>>> +		.help = "match traffic at e-switch going from the external port
>>>> associated with the given ethdev",
>>>
>>> Missing the word logically since if we are talking about representor the
>> connected port
>>> is the PF while we want to match traffic on one of the FVs.
>>
>> Doesn't the word "external" say it all?
>>
>> Representor Ethdev <--> Admin ethdev's PF <--> E-Switch <--> VF
>> (external / the most remote endpoint).
>>
> 
> Until the last comment External had totally different meaning to me.
> I think you should add some place the meaning of external or use
> the most remote endpoint.

We'll keep this in mind. Given your above example (when both VF and its 
representor) are plugged to the DPDK application, "external" may sound a 
bit off, but maybe it can be retained and just clarified as the "most 
remote endpoint" in the e-switch with respect to the ethdev in question.

> 
>>>
>>>> +		.priv = PRIV_ITEM(ESWITCH_PORT,
>>>> +				  sizeof(struct rte_flow_item_ethdev)),
>>>> +		.next = NEXT(item_eswitch_port),
>>>> +		.call = parse_vc,
>>>> +	},
>>>> +	[ITEM_ESWITCH_PORT_ETHDEV_ID] = {
>>>> +		.name = "ethdev_id",
>>>> +		.help = "ethdev ID",
>>>> +		.next = NEXT(item_eswitch_port,
>>>> NEXT_ENTRY(COMMON_UNSIGNED),
>>>> +			     item_param),
>>>> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
>> id)),
>>>> +	},
>>>>    	/* Validate/create actions. */
>>>>    	[ACTIONS] = {
>>>>    		.name = "actions",
>>>> @@ -8370,6 +8394,9 @@ flow_item_default_mask(const struct
>>>> rte_flow_item *item)
>>>>    	case RTE_FLOW_ITEM_TYPE_ETHDEV:
>>>>    		mask = &rte_flow_item_ethdev_mask;
>>>>    		break;
>>>> +	case RTE_FLOW_ITEM_TYPE_ESWITCH_PORT:
>>>> +		mask = &rte_flow_item_ethdev_mask;
>>>> +		break;
>>>
>>> Not sure maybe merged the two cases?
>>
>> A noble idea.
>>
>>>
>>>>    	default:
>>>>    		break;
>>>>    	}
>>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>>>> b/doc/guides/prog_guide/rte_flow.rst
>>>> index ab628d9139..292bb42410 100644
>>>> --- a/doc/guides/prog_guide/rte_flow.rst
>>>> +++ b/doc/guides/prog_guide/rte_flow.rst
>>>> @@ -1460,6 +1460,28 @@ Use this with attribute **transfer**. Attributes
>>>> **ingress** and
>>>>       | ``mask`` | ``id``   | zeroed for wildcard match |
>>>>       +----------+----------+---------------------------+
>>>>
>>>> +Item: ``ESWITCH_PORT``
>>>> +^^^^^^^^^^^^^^^^^^^^^^
>>>> +
>>>> +Matches traffic at e-switch going from the external port associated
>>>> +with the given ethdev, for example, traffic from net. port or guest.
>>>
>>> Maybe replace external with e-switch?
>>
>> The word "e-switch" is already here. Basically, all "external" ports are
>> "e-switch external ports" = ports which connect the e-switch with
>> non-DPDK world: network ports, VFs passed to guests, etc.
>>
> 
> Please see comment above, the word external is not clearly defined. >
>>>
>>>> +
>>>> +::
>>>> +
>>>> +   *    (Ethdev) ~~~~~~~~~~~~ (Internal Port) ~~~~ [] <<<< (External
>> Port)
>>>> +   *    :  SW                 :   Logical                    Net / Guest :
>>>> +   *    :                     :                                          :
>>>> +   *    | ---- PMD Layer ---- | ------------ E-Switch Layer ------------ |
>>>> +   *
>>>> +   *    [] shows the effective ("transfer") standpoint, the match engine;
>>>> +   *    << shows the traffic flow in question hitting the match engine;
>>>> +   *    ~~ shows logical interconnects between the endpoints.
>>>> +
>>>
>>> I'm not sure I understand this diagram.
>>
>> Thanks for the feedback. I have no way with drawing fancy diagrams, so
>> this one may not be ideal, yes. But could you please elaborate on your
>> concerns. Maybe you understand it the right way but just unsure. If you
>> describe what you see when looking at the diagram, I'll be able to
>> either confirm your vision or debunk any misunderstanding and possibly
>> improve the drawing.
>>
> 
> 
> I will try,
> Ethdev is the port that the application sees in DPDK right?

Exactly.

> Internal port is what the PMD sees, for example in case of representor it will see the PF port.

Vendor-specific, but, yes, let's assume that.

> External (also due to a drawing in one of your comments) is the E-Switch end point.
> So this drawing shows that the app select the external port that is connected to the
> internal port which is connected to the ethdev port?

So what's the problem?

Ethdev ----- E-switch port A -- ... --- E-switch port B------ VF

So, saying "transfer + ESWITCH_PORT" here implies the e-switch port B.
How do we name it? "External"? "The most remote"? Any other options?

> 
> Thanks,
> Ori
>>>
>>>> +Use this with attribute **transfer**. Attributes **ingress** and
>>>> +**egress** (`Attribute: Traffic direction`_) must not be used.
>>>> +
>>>> +This item is meant to use the same structure as `Item: ETHDEV`_.
>>>> +
>>>>    Actions
>>>>    ~~~~~~~
>>>>
>>>> diff --git a/doc/guides/rel_notes/release_21_11.rst
>>>> b/doc/guides/rel_notes/release_21_11.rst
>>>> index 91631adb4e..b2b27de3f0 100644
>>>> --- a/doc/guides/rel_notes/release_21_11.rst
>>>> +++ b/doc/guides/rel_notes/release_21_11.rst
>>>> @@ -167,7 +167,7 @@ API Changes
>>>>       Also, make sure to start the actual text at the margin.
>>>>
>> =======================================================
>>>>
>>>> -* ethdev: Added item ``ETHDEV`` to flow API.
>>>> +* ethdev: Added items ``ETHDEV``, ``ESWITCH_PORT`` to flow API.
>>>>
>>>>    * cryptodev: The API rte_cryptodev_pmd_is_valid_dev is modified to
>>>>      rte_cryptodev_is_valid_dev as it can be used by the application as diff --
>> git
>>>> a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> index 6d5de5457c..9a5c2a2d82 100644
>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> @@ -3824,6 +3824,10 @@ This section lists supported pattern items and
>>>> their attributes, if any.
>>>>
>>>>      - ``id {unsigned}``: ethdev ID
>>>>
>>>> +- ``eswitch_port``: match traffic at e-switch going from the external
>>>> +port associated with the given ethdev
>>>> +
>>>> +  - ``ethdev_id {unsigned}``: ethdev ID
>>>> +
>>>>    Actions list
>>>>    ^^^^^^^^^^^^
>>>>
>>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
>>>> 84eb61cb6c..c4aea5625f 100644
>>>> --- a/lib/ethdev/rte_flow.c
>>>> +++ b/lib/ethdev/rte_flow.c
>>>> @@ -101,6 +101,7 @@ static const struct rte_flow_desc_data
>>>> rte_flow_desc_item[] = {
>>>>    	MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
>>>>    	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
>>>>    	MK_FLOW_ITEM(ETHDEV, sizeof(struct rte_flow_item_ethdev)),
>>>> +	MK_FLOW_ITEM(ESWITCH_PORT, 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
>>>> 880502098e..1a7e4c2e3d 100644
>>>> --- a/lib/ethdev/rte_flow.h
>>>> +++ b/lib/ethdev/rte_flow.h
>>>> @@ -583,6 +583,16 @@ enum rte_flow_item_type {
>>>>    	 * @see struct rte_flow_item_ethdev
>>>>    	 */
>>>>    	RTE_FLOW_ITEM_TYPE_ETHDEV,
>>>> +
>>>> +	/**
>>>> +	 * [META]
>>>> +	 *
>>>> +	 * Matches traffic at e-switch going from the external port associated
>>>> +	 * with the given ethdev, for example, traffic from net. port or guest.
>>>> +	 *
>>>> +	 * @see struct rte_flow_item_ethdev
>>>> +	 */
>>>> +	RTE_FLOW_ITEM_TYPE_ESWITCH_PORT,
>>>>    };
>>>>
>>>>    /**
>>>> @@ -1813,7 +1823,7 @@ static const struct rte_flow_item_conntrack
>>>> rte_flow_item_conntrack_mask = {
>>>>     * @b EXPERIMENTAL: this structure may change without prior notice
>>>>     *
>>>>     * Provides an ethdev ID for use with items which are as follows:
>>>> - * RTE_FLOW_ITEM_TYPE_ETHDEV.
>>>> + * RTE_FLOW_ITEM_TYPE_ETHDEV,
>>>> RTE_FLOW_ITEM_TYPE_ESWITCH_PORT.
>>>>     */
>>>>    struct rte_flow_item_ethdev {
>>>>    	uint16_t id; /**< Ethdev ID */
>>>> --
>>>> 2.30.2
>>>
>>> Best,
>>> Ori
>>>
>>
>> --
>> Ivan M
  
Ori Kam Oct. 4, 2021, 11:37 a.m. UTC | #5
Hi Ivan,

> -----Original Message-----
> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
> Sent: Monday, October 4, 2021 2:06 PM
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
> 
> Hi Ori,
> 
> On 04/10/2021 08:45, Ori Kam wrote:
> > Hi Ivan,
> >
> >> -----Original Message-----
> >> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
> >> Sent: Sunday, October 3, 2021 9:11 PM
> >> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
> >> API
> >>
> >>
> >>
> >> On 03/10/2021 15:40, Ori Kam wrote:
> >>> Hi Andrew and Ivan,
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> Sent: Friday, October 1, 2021 4:47 PM
> >>>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
> >>>>
> >>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> >>>>
> >>>> For use with "transfer" flows. Supposed to match traffic entering
> >>>> the e-switch from the external world (network, guests) via the port
> >>>> which is logically connected with the given ethdev.
> >>>>
> >>>> Must not be combined with attributes "ingress" / "egress".
> >>>>
> >>>> This item is meant to use the same structure as ethdev item.
> >>>>
> >>>
> >>> In case the app is not working with representors, meaning each
> >>> switch port is mapped to ethdev.
> >>> both items (ethdev and eswitch port ) have the same meaning?
> >>
> >> No. Ethdev means ethdev, and e-switch port is the point where this
> >> ethdev is plugged to. For example, "transfer + ESWITCH_PORT" for a
> >> regular PF ethdev typically means the network port (maybe you can
> >> recall the idea that a PF ethdev "represents" the network port it's
> associated with).
> >>
> >> I believe, that diagrams which these patches add to
> >> "doc/guides/prog_guide/rte_flow.rst" may come in handy to understand
> >> the meaning. Also, you can take a look at our larger diagram from the
> >> Sep 14 gathering.
> >>
> >
> > Lets look at the following system:
> > E-Switch has 3 ports - PF, VF1, VF2
> > The ports are distributed as follows:
> > DPDK application:
> > ethdev(0) pf,
> > ethdev(1) representor to VF1
> > ethdev(2) representor to VF2
> > ethdev(3) VF1
> >
> > VM:
> > VF2
> >
> > As we know all representors are realy connected to the PF(at least in
> > this example)
> 
> This example tries to say that the e-switch has 3 ports in total, and, given
> your explanation, one may indeed agree that *in this example* representors
> re-use e-switch port of ethdev=0 (with some metadata to distinguish
> packets, etc.). But one can hardly assume that *all* representors with any
> vendor's NIC are connected to the e-switch the same way. It's vendor
> specific. Well, at least, applications don't have this knowledge and don't need
> to.
> 
> >
> > So matching on ethdev(3)  means matching on traffic sent from DPDK port
> 3 right?
> 
> Item ETHDEV (ethdev_id=3) matches traffic sent by DPDK port 3. Looks like
> we're on the same page here.
> 

Good.

> > And matching on eswitch_port(3) means matching in traffic that goes
> > into VF1 which is the same traffic as ethdev(3) right?
> 
> I didn't catch the thought about "the same traffic". Direction is not the same.
> Item ESWITCH_PORT (ethdev_id=3) matches traffic sent by DPDK port 1.
> 
This is the critical part for my understanding.
Matching on ethdev_id(3) means matching on traffic that is coming from DPDK port3.
So from E-Switch view point it is traffic that goes into VF1?
While matching on E-Switch_port(3) means matching on traffic coming from VF1?

And by the same logic matching on ethdev_id(1) means matching on taffic that was sent
from DPDK port 1 and matching on E-Switch_port(1) means matching on traffic coming from
VF1 

So in this case eswitch_port(3) is equal ot eswitch_port(1) right?
While ethdev(1) is not equal to ethdev(3)

And just to complete the picture, matching on ethdev(2) will result in traffic
coming from the dpdk port and matching on eswitch_port(2) will match
on traffic coming from VF2

> Yes, in this case neither of the ports (1, 3) is truly "external" (they both
> interface the DPDK application), but, the thing is, they're "external" *to each
> other* in the sense that they sit at the opposite ends of the wire.
> 
> >
> > Matching on ethdev(1) means matching on the PF port in the E-Switch but
> with some
> > metadata that marks the traffic as coming from DPDK port 1 and not from
> VF1 E-Switch
> > port right?
> 
> That's vendor specific. The application doesn't have to know how exactly
> this particular ethdev is connected to the e-switch - whether it re-uses
> the PF's e-switch port or has its own. The e-switch port that connects
> the ethdev with the e-switch is just assumed to exist logically.
> 
> >
> > While matching on eswitch_port(2) means matching on traffic coming from
> the VM right?
> 
> Right.
> 

I think the my above question will clear everything for me.

> >
> >>>
> >>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> >>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> ---
> >>>>    app/test-pmd/cmdline_flow.c                 | 27
> +++++++++++++++++++++
> >>>>    doc/guides/prog_guide/rte_flow.rst          | 22 +++++++++++++++++
> >>>>    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                       | 12 ++++++++-
> >>>>    6 files changed, 66 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-
> >> pmd/cmdline_flow.c
> >>>> index e05b0d83d2..188d0ee39d 100644
> >>>> --- a/app/test-pmd/cmdline_flow.c
> >>>> +++ b/app/test-pmd/cmdline_flow.c
> >>>> @@ -308,6 +308,8 @@ enum index {
> >>>>    	ITEM_POL_POLICY,
> >>>>    	ITEM_ETHDEV,
> >>>>    	ITEM_ETHDEV_ID,
> >>>> +	ITEM_ESWITCH_PORT,
> >>>> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
> >>>
> >>> Like my comment from previous patch, I'm not sure the correct
> >>> term for ETHDEV is ID is should be port.
> >>
> >> Please see my reply in the previous thread. "ethdev" here is an
> >> "anchor", a "beacon" of sorts which allows either to refer namely to
> >> this ethdev or to the e-switch port associated with it.
> >>
> >>>
> >>>>
> >>>>    	/* Validate/create actions. */
> >>>>    	ACTIONS,
> >>>> @@ -1003,6 +1005,7 @@ static const enum index next_item[] = {
> >>>>    	ITEM_INTEGRITY,
> >>>>    	ITEM_CONNTRACK,
> >>>>    	ITEM_ETHDEV,
> >>>> +	ITEM_ESWITCH_PORT,
> >>>>    	END_SET,
> >>>>    	ZERO,
> >>>>    };
> >>>> @@ -1377,6 +1380,12 @@ static const enum index item_ethdev[] = {
> >>>>    	ZERO,
> >>>>    };
> >>>>
> >>>> +static const enum index item_eswitch_port[] = {
> >>>> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
> >>>> +	ITEM_NEXT,
> >>>> +	ZERO,
> >>>> +};
> >>>> +
> >>>>    static const enum index next_action[] = {
> >>>>    	ACTION_END,
> >>>>    	ACTION_VOID,
> >>>> @@ -3632,6 +3641,21 @@ static const struct token token_list[] = {
> >>>>    			     item_param),
> >>>>    		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
> >> id)),
> >>>>    	},
> >>>> +	[ITEM_ESWITCH_PORT] = {
> >>>> +		.name = "eswitch_port",
> >>>> +		.help = "match traffic at e-switch going from the external port
> >>>> associated with the given ethdev",
> >>>
> >>> Missing the word logically since if we are talking about representor the
> >> connected port
> >>> is the PF while we want to match traffic on one of the FVs.
> >>
> >> Doesn't the word "external" say it all?
> >>
> >> Representor Ethdev <--> Admin ethdev's PF <--> E-Switch <--> VF
> >> (external / the most remote endpoint).
> >>
> >
> > Until the last comment External had totally different meaning to me.
> > I think you should add some place the meaning of external or use
> > the most remote endpoint.
> 
> We'll keep this in mind. Given your above example (when both VF and its
> representor) are plugged to the DPDK application, "external" may sound a
> bit off, but maybe it can be retained and just clarified as the "most
> remote endpoint" in the e-switch with respect to the ethdev in question.
> 

+1 to the most remote or finding different wording.

> >
> >>>
> >>>> +		.priv = PRIV_ITEM(ESWITCH_PORT,
> >>>> +				  sizeof(struct rte_flow_item_ethdev)),
> >>>> +		.next = NEXT(item_eswitch_port),
> >>>> +		.call = parse_vc,
> >>>> +	},
> >>>> +	[ITEM_ESWITCH_PORT_ETHDEV_ID] = {
> >>>> +		.name = "ethdev_id",
> >>>> +		.help = "ethdev ID",
> >>>> +		.next = NEXT(item_eswitch_port,
> >>>> NEXT_ENTRY(COMMON_UNSIGNED),
> >>>> +			     item_param),
> >>>> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
> >> id)),
> >>>> +	},
> >>>>    	/* Validate/create actions. */
> >>>>    	[ACTIONS] = {
> >>>>    		.name = "actions",
> >>>> @@ -8370,6 +8394,9 @@ flow_item_default_mask(const struct
> >>>> rte_flow_item *item)
> >>>>    	case RTE_FLOW_ITEM_TYPE_ETHDEV:
> >>>>    		mask = &rte_flow_item_ethdev_mask;
> >>>>    		break;
> >>>> +	case RTE_FLOW_ITEM_TYPE_ESWITCH_PORT:
> >>>> +		mask = &rte_flow_item_ethdev_mask;
> >>>> +		break;
> >>>
> >>> Not sure maybe merged the two cases?
> >>
> >> A noble idea.
> >>
> >>>
> >>>>    	default:
> >>>>    		break;
> >>>>    	}
> >>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
> >>>> b/doc/guides/prog_guide/rte_flow.rst
> >>>> index ab628d9139..292bb42410 100644
> >>>> --- a/doc/guides/prog_guide/rte_flow.rst
> >>>> +++ b/doc/guides/prog_guide/rte_flow.rst
> >>>> @@ -1460,6 +1460,28 @@ Use this with attribute **transfer**.
> Attributes
> >>>> **ingress** and
> >>>>       | ``mask`` | ``id``   | zeroed for wildcard match |
> >>>>       +----------+----------+---------------------------+
> >>>>
> >>>> +Item: ``ESWITCH_PORT``
> >>>> +^^^^^^^^^^^^^^^^^^^^^^
> >>>> +
> >>>> +Matches traffic at e-switch going from the external port associated
> >>>> +with the given ethdev, for example, traffic from net. port or guest.
> >>>
> >>> Maybe replace external with e-switch?
> >>
> >> The word "e-switch" is already here. Basically, all "external" ports are
> >> "e-switch external ports" = ports which connect the e-switch with
> >> non-DPDK world: network ports, VFs passed to guests, etc.
> >>
> >
> > Please see comment above, the word external is not clearly defined. >
> >>>
> >>>> +
> >>>> +::
> >>>> +
> >>>> +   *    (Ethdev) ~~~~~~~~~~~~ (Internal Port) ~~~~ [] <<<< (External
> >> Port)
> >>>> +   *    :  SW                 :   Logical                    Net / Guest :
> >>>> +   *    :                     :                                          :
> >>>> +   *    | ---- PMD Layer ---- | ------------ E-Switch Layer ------------ |
> >>>> +   *
> >>>> +   *    [] shows the effective ("transfer") standpoint, the match engine;
> >>>> +   *    << shows the traffic flow in question hitting the match engine;
> >>>> +   *    ~~ shows logical interconnects between the endpoints.
> >>>> +
> >>>
> >>> I'm not sure I understand this diagram.
> >>
> >> Thanks for the feedback. I have no way with drawing fancy diagrams, so
> >> this one may not be ideal, yes. But could you please elaborate on your
> >> concerns. Maybe you understand it the right way but just unsure. If you
> >> describe what you see when looking at the diagram, I'll be able to
> >> either confirm your vision or debunk any misunderstanding and possibly
> >> improve the drawing.
> >>
> >
> >
> > I will try,
> > Ethdev is the port that the application sees in DPDK right?
> 
> Exactly.
> 
> > Internal port is what the PMD sees, for example in case of representor it
> will see the PF port.
> 
> Vendor-specific, but, yes, let's assume that.
> 
> > External (also due to a drawing in one of your comments) is the E-Switch
> end point.
> > So this drawing shows that the app select the external port that is
> connected to the
> > internal port which is connected to the ethdev port?
> 
> So what's the problem?
> 
> Ethdev ----- E-switch port A -- ... --- E-switch port B------ VF
> 
> So, saying "transfer + ESWITCH_PORT" here implies the e-switch port B.
> How do we name it? "External"? "The most remote"? Any other options?
> 
> >
> > Thanks,
> > Ori
> >>>
> >>>> +Use this with attribute **transfer**. Attributes **ingress** and
> >>>> +**egress** (`Attribute: Traffic direction`_) must not be used.
> >>>> +
> >>>> +This item is meant to use the same structure as `Item: ETHDEV`_.
> >>>> +
> >>>>    Actions
> >>>>    ~~~~~~~
> >>>>
> >>>> diff --git a/doc/guides/rel_notes/release_21_11.rst
> >>>> b/doc/guides/rel_notes/release_21_11.rst
> >>>> index 91631adb4e..b2b27de3f0 100644
> >>>> --- a/doc/guides/rel_notes/release_21_11.rst
> >>>> +++ b/doc/guides/rel_notes/release_21_11.rst
> >>>> @@ -167,7 +167,7 @@ API Changes
> >>>>       Also, make sure to start the actual text at the margin.
> >>>>
> >> =======================================================
> >>>>
> >>>> -* ethdev: Added item ``ETHDEV`` to flow API.
> >>>> +* ethdev: Added items ``ETHDEV``, ``ESWITCH_PORT`` to flow API.
> >>>>
> >>>>    * cryptodev: The API rte_cryptodev_pmd_is_valid_dev is modified to
> >>>>      rte_cryptodev_is_valid_dev as it can be used by the application as
> diff --
> >> git
> >>>> a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> index 6d5de5457c..9a5c2a2d82 100644
> >>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> @@ -3824,6 +3824,10 @@ This section lists supported pattern items
> and
> >>>> their attributes, if any.
> >>>>
> >>>>      - ``id {unsigned}``: ethdev ID
> >>>>
> >>>> +- ``eswitch_port``: match traffic at e-switch going from the external
> >>>> +port associated with the given ethdev
> >>>> +
> >>>> +  - ``ethdev_id {unsigned}``: ethdev ID
> >>>> +
> >>>>    Actions list
> >>>>    ^^^^^^^^^^^^
> >>>>
> >>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> >>>> 84eb61cb6c..c4aea5625f 100644
> >>>> --- a/lib/ethdev/rte_flow.c
> >>>> +++ b/lib/ethdev/rte_flow.c
> >>>> @@ -101,6 +101,7 @@ static const struct rte_flow_desc_data
> >>>> rte_flow_desc_item[] = {
> >>>>    	MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
> >>>>    	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
> >>>>    	MK_FLOW_ITEM(ETHDEV, sizeof(struct rte_flow_item_ethdev)),
> >>>> +	MK_FLOW_ITEM(ESWITCH_PORT, 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
> >>>> 880502098e..1a7e4c2e3d 100644
> >>>> --- a/lib/ethdev/rte_flow.h
> >>>> +++ b/lib/ethdev/rte_flow.h
> >>>> @@ -583,6 +583,16 @@ enum rte_flow_item_type {
> >>>>    	 * @see struct rte_flow_item_ethdev
> >>>>    	 */
> >>>>    	RTE_FLOW_ITEM_TYPE_ETHDEV,
> >>>> +
> >>>> +	/**
> >>>> +	 * [META]
> >>>> +	 *
> >>>> +	 * Matches traffic at e-switch going from the external port associated
> >>>> +	 * with the given ethdev, for example, traffic from net. port or guest.
> >>>> +	 *
> >>>> +	 * @see struct rte_flow_item_ethdev
> >>>> +	 */
> >>>> +	RTE_FLOW_ITEM_TYPE_ESWITCH_PORT,
> >>>>    };
> >>>>
> >>>>    /**
> >>>> @@ -1813,7 +1823,7 @@ static const struct rte_flow_item_conntrack
> >>>> rte_flow_item_conntrack_mask = {
> >>>>     * @b EXPERIMENTAL: this structure may change without prior notice
> >>>>     *
> >>>>     * Provides an ethdev ID for use with items which are as follows:
> >>>> - * RTE_FLOW_ITEM_TYPE_ETHDEV.
> >>>> + * RTE_FLOW_ITEM_TYPE_ETHDEV,
> >>>> RTE_FLOW_ITEM_TYPE_ESWITCH_PORT.
> >>>>     */
> >>>>    struct rte_flow_item_ethdev {
> >>>>    	uint16_t id; /**< Ethdev ID */
> >>>> --
> >>>> 2.30.2
> >>>
> >>> Best,
> >>> Ori
> >>>
> >>
> >> --
> >> Ivan M
> 
> --
> Ivan M

Thanks,
Ori
  
Ivan Malov Oct. 4, 2021, 11:58 a.m. UTC | #6
Hi Ori,

On 04/10/2021 14:37, Ori Kam wrote:
> Hi Ivan,
> 
>> -----Original Message-----
>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>> Sent: Monday, October 4, 2021 2:06 PM
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
>>
>> Hi Ori,
>>
>> On 04/10/2021 08:45, Ori Kam wrote:
>>> Hi Ivan,
>>>
>>>> -----Original Message-----
>>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>>>> Sent: Sunday, October 3, 2021 9:11 PM
>>>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
>>>> API
>>>>
>>>>
>>>>
>>>> On 03/10/2021 15:40, Ori Kam wrote:
>>>>> Hi Andrew and Ivan,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Sent: Friday, October 1, 2021 4:47 PM
>>>>>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
>>>>>>
>>>>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>>>
>>>>>> For use with "transfer" flows. Supposed to match traffic entering
>>>>>> the e-switch from the external world (network, guests) via the port
>>>>>> which is logically connected with the given ethdev.
>>>>>>
>>>>>> Must not be combined with attributes "ingress" / "egress".
>>>>>>
>>>>>> This item is meant to use the same structure as ethdev item.
>>>>>>
>>>>>
>>>>> In case the app is not working with representors, meaning each
>>>>> switch port is mapped to ethdev.
>>>>> both items (ethdev and eswitch port ) have the same meaning?
>>>>
>>>> No. Ethdev means ethdev, and e-switch port is the point where this
>>>> ethdev is plugged to. For example, "transfer + ESWITCH_PORT" for a
>>>> regular PF ethdev typically means the network port (maybe you can
>>>> recall the idea that a PF ethdev "represents" the network port it's
>> associated with).
>>>>
>>>> I believe, that diagrams which these patches add to
>>>> "doc/guides/prog_guide/rte_flow.rst" may come in handy to understand
>>>> the meaning. Also, you can take a look at our larger diagram from the
>>>> Sep 14 gathering.
>>>>
>>>
>>> Lets look at the following system:
>>> E-Switch has 3 ports - PF, VF1, VF2
>>> The ports are distributed as follows:
>>> DPDK application:
>>> ethdev(0) pf,
>>> ethdev(1) representor to VF1
>>> ethdev(2) representor to VF2
>>> ethdev(3) VF1
>>>
>>> VM:
>>> VF2
>>>
>>> As we know all representors are realy connected to the PF(at least in
>>> this example)
>>
>> This example tries to say that the e-switch has 3 ports in total, and, given
>> your explanation, one may indeed agree that *in this example* representors
>> re-use e-switch port of ethdev=0 (with some metadata to distinguish
>> packets, etc.). But one can hardly assume that *all* representors with any
>> vendor's NIC are connected to the e-switch the same way. It's vendor
>> specific. Well, at least, applications don't have this knowledge and don't need
>> to.
>>
>>>
>>> So matching on ethdev(3)  means matching on traffic sent from DPDK port
>> 3 right?
>>
>> Item ETHDEV (ethdev_id=3) matches traffic sent by DPDK port 3. Looks like
>> we're on the same page here.
>>
> 
> Good.
> 
>>> And matching on eswitch_port(3) means matching in traffic that goes
>>> into VF1 which is the same traffic as ethdev(3) right?
>>
>> I didn't catch the thought about "the same traffic". Direction is not the same.
>> Item ESWITCH_PORT (ethdev_id=3) matches traffic sent by DPDK port 1.
>>
> This is the critical part for my understanding.
> Matching on ethdev_id(3) means matching on traffic that is coming from DPDK port3.

Right.

> So from E-Switch view point it is traffic that goes into VF1?

No. Above you clearly say "coming from DPDK port3". That is, from the 
VF1. *Not* going into it. Port 3 (ethdev_id=3) *is* VF1.

> While matching on E-Switch_port(3) means matching on traffic coming from VF1?

No. It means matching on traffic coming from ethdev 1. From the VF1's 
representor.

> 
> And by the same logic matching on ethdev_id(1) means matching on taffic that was sent
> from DPDK port 1 and matching on E-Switch_port(1) means matching on traffic coming from
> VF1

In this case, you've got this right. But please see my above notes. By 
the looks of it, you might have run into confusion over there.

> 
> So in this case eswitch_port(3) is equal ot eswitch_port(1) right?
> While ethdev(1) is not equal to ethdev(3)

No.

Item ETHDEV (ethdev_id=1) equals item ESWITCH_PORT (ethdev_id=3).
Item ETHDEV (ethdev_id=3) equals item ESWITCH_PORT (ethdev_id=1).

> 
> And just to complete the picture, matching on ethdev(2) will result in traffic
> coming from the dpdk port and matching on eswitch_port(2) will match
> on traffic coming from VF2

Exactly.


But, Ori, let me draw your attention to the following issue. In order to 
simplify understanding, I suggest that we refrain from saying "traffic 
that GOES TO". Where it goes depends on default rules that are supposed 
to be maintained by the PMD when ports get plugged / unplugged.

The flow items ETHDEV and ESWITH_PORT define the SOURCE of traffic. 
That's it. They define where the traffic "goes FROM".

Say, the DPDK application sends a packet from ethdev 0. This packet 
enters the e-switch. Match engine sits in the e-switch and intercepts 
the packet. It doesn't care where the packet *would go* if it wasn't 
intercepted. It cares about where the packet comes from. And it comes 
from ethdev 0. So, in the focus, we have the SOURCE of the packet.


> 
>> Yes, in this case neither of the ports (1, 3) is truly "external" (they both
>> interface the DPDK application), but, the thing is, they're "external" *to each
>> other* in the sense that they sit at the opposite ends of the wire.
>>
>>>
>>> Matching on ethdev(1) means matching on the PF port in the E-Switch but
>> with some
>>> metadata that marks the traffic as coming from DPDK port 1 and not from
>> VF1 E-Switch
>>> port right?
>>
>> That's vendor specific. The application doesn't have to know how exactly
>> this particular ethdev is connected to the e-switch - whether it re-uses
>> the PF's e-switch port or has its own. The e-switch port that connects
>> the ethdev with the e-switch is just assumed to exist logically.
>>
>>>
>>> While matching on eswitch_port(2) means matching on traffic coming from
>> the VM right?
>>
>> Right.
>>
> 
> I think the my above question will clear everything for me.
> 
>>>
>>>>>
>>>>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> ---
>>>>>>     app/test-pmd/cmdline_flow.c                 | 27
>> +++++++++++++++++++++
>>>>>>     doc/guides/prog_guide/rte_flow.rst          | 22 +++++++++++++++++
>>>>>>     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                       | 12 ++++++++-
>>>>>>     6 files changed, 66 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-
>>>> pmd/cmdline_flow.c
>>>>>> index e05b0d83d2..188d0ee39d 100644
>>>>>> --- a/app/test-pmd/cmdline_flow.c
>>>>>> +++ b/app/test-pmd/cmdline_flow.c
>>>>>> @@ -308,6 +308,8 @@ enum index {
>>>>>>     	ITEM_POL_POLICY,
>>>>>>     	ITEM_ETHDEV,
>>>>>>     	ITEM_ETHDEV_ID,
>>>>>> +	ITEM_ESWITCH_PORT,
>>>>>> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
>>>>>
>>>>> Like my comment from previous patch, I'm not sure the correct
>>>>> term for ETHDEV is ID is should be port.
>>>>
>>>> Please see my reply in the previous thread. "ethdev" here is an
>>>> "anchor", a "beacon" of sorts which allows either to refer namely to
>>>> this ethdev or to the e-switch port associated with it.
>>>>
>>>>>
>>>>>>
>>>>>>     	/* Validate/create actions. */
>>>>>>     	ACTIONS,
>>>>>> @@ -1003,6 +1005,7 @@ static const enum index next_item[] = {
>>>>>>     	ITEM_INTEGRITY,
>>>>>>     	ITEM_CONNTRACK,
>>>>>>     	ITEM_ETHDEV,
>>>>>> +	ITEM_ESWITCH_PORT,
>>>>>>     	END_SET,
>>>>>>     	ZERO,
>>>>>>     };
>>>>>> @@ -1377,6 +1380,12 @@ static const enum index item_ethdev[] = {
>>>>>>     	ZERO,
>>>>>>     };
>>>>>>
>>>>>> +static const enum index item_eswitch_port[] = {
>>>>>> +	ITEM_ESWITCH_PORT_ETHDEV_ID,
>>>>>> +	ITEM_NEXT,
>>>>>> +	ZERO,
>>>>>> +};
>>>>>> +
>>>>>>     static const enum index next_action[] = {
>>>>>>     	ACTION_END,
>>>>>>     	ACTION_VOID,
>>>>>> @@ -3632,6 +3641,21 @@ static const struct token token_list[] = {
>>>>>>     			     item_param),
>>>>>>     		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
>>>> id)),
>>>>>>     	},
>>>>>> +	[ITEM_ESWITCH_PORT] = {
>>>>>> +		.name = "eswitch_port",
>>>>>> +		.help = "match traffic at e-switch going from the external port
>>>>>> associated with the given ethdev",
>>>>>
>>>>> Missing the word logically since if we are talking about representor the
>>>> connected port
>>>>> is the PF while we want to match traffic on one of the FVs.
>>>>
>>>> Doesn't the word "external" say it all?
>>>>
>>>> Representor Ethdev <--> Admin ethdev's PF <--> E-Switch <--> VF
>>>> (external / the most remote endpoint).
>>>>
>>>
>>> Until the last comment External had totally different meaning to me.
>>> I think you should add some place the meaning of external or use
>>> the most remote endpoint.
>>
>> We'll keep this in mind. Given your above example (when both VF and its
>> representor) are plugged to the DPDK application, "external" may sound a
>> bit off, but maybe it can be retained and just clarified as the "most
>> remote endpoint" in the e-switch with respect to the ethdev in question.
>>
> 
> +1 to the most remote or finding different wording.
> 
>>>
>>>>>
>>>>>> +		.priv = PRIV_ITEM(ESWITCH_PORT,
>>>>>> +				  sizeof(struct rte_flow_item_ethdev)),
>>>>>> +		.next = NEXT(item_eswitch_port),
>>>>>> +		.call = parse_vc,
>>>>>> +	},
>>>>>> +	[ITEM_ESWITCH_PORT_ETHDEV_ID] = {
>>>>>> +		.name = "ethdev_id",
>>>>>> +		.help = "ethdev ID",
>>>>>> +		.next = NEXT(item_eswitch_port,
>>>>>> NEXT_ENTRY(COMMON_UNSIGNED),
>>>>>> +			     item_param),
>>>>>> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
>>>> id)),
>>>>>> +	},
>>>>>>     	/* Validate/create actions. */
>>>>>>     	[ACTIONS] = {
>>>>>>     		.name = "actions",
>>>>>> @@ -8370,6 +8394,9 @@ flow_item_default_mask(const struct
>>>>>> rte_flow_item *item)
>>>>>>     	case RTE_FLOW_ITEM_TYPE_ETHDEV:
>>>>>>     		mask = &rte_flow_item_ethdev_mask;
>>>>>>     		break;
>>>>>> +	case RTE_FLOW_ITEM_TYPE_ESWITCH_PORT:
>>>>>> +		mask = &rte_flow_item_ethdev_mask;
>>>>>> +		break;
>>>>>
>>>>> Not sure maybe merged the two cases?
>>>>
>>>> A noble idea.
>>>>
>>>>>
>>>>>>     	default:
>>>>>>     		break;
>>>>>>     	}
>>>>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>>>>>> b/doc/guides/prog_guide/rte_flow.rst
>>>>>> index ab628d9139..292bb42410 100644
>>>>>> --- a/doc/guides/prog_guide/rte_flow.rst
>>>>>> +++ b/doc/guides/prog_guide/rte_flow.rst
>>>>>> @@ -1460,6 +1460,28 @@ Use this with attribute **transfer**.
>> Attributes
>>>>>> **ingress** and
>>>>>>        | ``mask`` | ``id``   | zeroed for wildcard match |
>>>>>>        +----------+----------+---------------------------+
>>>>>>
>>>>>> +Item: ``ESWITCH_PORT``
>>>>>> +^^^^^^^^^^^^^^^^^^^^^^
>>>>>> +
>>>>>> +Matches traffic at e-switch going from the external port associated
>>>>>> +with the given ethdev, for example, traffic from net. port or guest.
>>>>>
>>>>> Maybe replace external with e-switch?
>>>>
>>>> The word "e-switch" is already here. Basically, all "external" ports are
>>>> "e-switch external ports" = ports which connect the e-switch with
>>>> non-DPDK world: network ports, VFs passed to guests, etc.
>>>>
>>>
>>> Please see comment above, the word external is not clearly defined. >
>>>>>
>>>>>> +
>>>>>> +::
>>>>>> +
>>>>>> +   *    (Ethdev) ~~~~~~~~~~~~ (Internal Port) ~~~~ [] <<<< (External
>>>> Port)
>>>>>> +   *    :  SW                 :   Logical                    Net / Guest :
>>>>>> +   *    :                     :                                          :
>>>>>> +   *    | ---- PMD Layer ---- | ------------ E-Switch Layer ------------ |
>>>>>> +   *
>>>>>> +   *    [] shows the effective ("transfer") standpoint, the match engine;
>>>>>> +   *    << shows the traffic flow in question hitting the match engine;
>>>>>> +   *    ~~ shows logical interconnects between the endpoints.
>>>>>> +
>>>>>
>>>>> I'm not sure I understand this diagram.
>>>>
>>>> Thanks for the feedback. I have no way with drawing fancy diagrams, so
>>>> this one may not be ideal, yes. But could you please elaborate on your
>>>> concerns. Maybe you understand it the right way but just unsure. If you
>>>> describe what you see when looking at the diagram, I'll be able to
>>>> either confirm your vision or debunk any misunderstanding and possibly
>>>> improve the drawing.
>>>>
>>>
>>>
>>> I will try,
>>> Ethdev is the port that the application sees in DPDK right?
>>
>> Exactly.
>>
>>> Internal port is what the PMD sees, for example in case of representor it
>> will see the PF port.
>>
>> Vendor-specific, but, yes, let's assume that.
>>
>>> External (also due to a drawing in one of your comments) is the E-Switch
>> end point.
>>> So this drawing shows that the app select the external port that is
>> connected to the
>>> internal port which is connected to the ethdev port?
>>
>> So what's the problem?
>>
>> Ethdev ----- E-switch port A -- ... --- E-switch port B------ VF
>>
>> So, saying "transfer + ESWITCH_PORT" here implies the e-switch port B.
>> How do we name it? "External"? "The most remote"? Any other options?
>>
>>>
>>> Thanks,
>>> Ori
>>>>>
>>>>>> +Use this with attribute **transfer**. Attributes **ingress** and
>>>>>> +**egress** (`Attribute: Traffic direction`_) must not be used.
>>>>>> +
>>>>>> +This item is meant to use the same structure as `Item: ETHDEV`_.
>>>>>> +
>>>>>>     Actions
>>>>>>     ~~~~~~~
>>>>>>
>>>>>> diff --git a/doc/guides/rel_notes/release_21_11.rst
>>>>>> b/doc/guides/rel_notes/release_21_11.rst
>>>>>> index 91631adb4e..b2b27de3f0 100644
>>>>>> --- a/doc/guides/rel_notes/release_21_11.rst
>>>>>> +++ b/doc/guides/rel_notes/release_21_11.rst
>>>>>> @@ -167,7 +167,7 @@ API Changes
>>>>>>        Also, make sure to start the actual text at the margin.
>>>>>>
>>>> =======================================================
>>>>>>
>>>>>> -* ethdev: Added item ``ETHDEV`` to flow API.
>>>>>> +* ethdev: Added items ``ETHDEV``, ``ESWITCH_PORT`` to flow API.
>>>>>>
>>>>>>     * cryptodev: The API rte_cryptodev_pmd_is_valid_dev is modified to
>>>>>>       rte_cryptodev_is_valid_dev as it can be used by the application as
>> diff --
>>>> git
>>>>>> a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>>>> index 6d5de5457c..9a5c2a2d82 100644
>>>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>>>> @@ -3824,6 +3824,10 @@ This section lists supported pattern items
>> and
>>>>>> their attributes, if any.
>>>>>>
>>>>>>       - ``id {unsigned}``: ethdev ID
>>>>>>
>>>>>> +- ``eswitch_port``: match traffic at e-switch going from the external
>>>>>> +port associated with the given ethdev
>>>>>> +
>>>>>> +  - ``ethdev_id {unsigned}``: ethdev ID
>>>>>> +
>>>>>>     Actions list
>>>>>>     ^^^^^^^^^^^^
>>>>>>
>>>>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
>>>>>> 84eb61cb6c..c4aea5625f 100644
>>>>>> --- a/lib/ethdev/rte_flow.c
>>>>>> +++ b/lib/ethdev/rte_flow.c
>>>>>> @@ -101,6 +101,7 @@ static const struct rte_flow_desc_data
>>>>>> rte_flow_desc_item[] = {
>>>>>>     	MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
>>>>>>     	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
>>>>>>     	MK_FLOW_ITEM(ETHDEV, sizeof(struct rte_flow_item_ethdev)),
>>>>>> +	MK_FLOW_ITEM(ESWITCH_PORT, 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
>>>>>> 880502098e..1a7e4c2e3d 100644
>>>>>> --- a/lib/ethdev/rte_flow.h
>>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>>> @@ -583,6 +583,16 @@ enum rte_flow_item_type {
>>>>>>     	 * @see struct rte_flow_item_ethdev
>>>>>>     	 */
>>>>>>     	RTE_FLOW_ITEM_TYPE_ETHDEV,
>>>>>> +
>>>>>> +	/**
>>>>>> +	 * [META]
>>>>>> +	 *
>>>>>> +	 * Matches traffic at e-switch going from the external port associated
>>>>>> +	 * with the given ethdev, for example, traffic from net. port or guest.
>>>>>> +	 *
>>>>>> +	 * @see struct rte_flow_item_ethdev
>>>>>> +	 */
>>>>>> +	RTE_FLOW_ITEM_TYPE_ESWITCH_PORT,
>>>>>>     };
>>>>>>
>>>>>>     /**
>>>>>> @@ -1813,7 +1823,7 @@ static const struct rte_flow_item_conntrack
>>>>>> rte_flow_item_conntrack_mask = {
>>>>>>      * @b EXPERIMENTAL: this structure may change without prior notice
>>>>>>      *
>>>>>>      * Provides an ethdev ID for use with items which are as follows:
>>>>>> - * RTE_FLOW_ITEM_TYPE_ETHDEV.
>>>>>> + * RTE_FLOW_ITEM_TYPE_ETHDEV,
>>>>>> RTE_FLOW_ITEM_TYPE_ESWITCH_PORT.
>>>>>>      */
>>>>>>     struct rte_flow_item_ethdev {
>>>>>>     	uint16_t id; /**< Ethdev ID */
>>>>>> --
>>>>>> 2.30.2
>>>>>
>>>>> Best,
>>>>> Ori
>>>>>
>>>>
>>>> --
>>>> Ivan M
>>
>> --
>> Ivan M
> 
> Thanks,
> Ori
>
  
Ori Kam Oct. 5, 2021, 6:20 a.m. UTC | #7
Hi Ivan,

> -----Original Message-----
> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
> 
> Hi Ori,
> 
> On 04/10/2021 14:37, Ori Kam wrote:
> > Hi Ivan,
> >
> >> -----Original Message-----
> >> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
> >> Sent: Monday, October 4, 2021 2:06 PM
> >> Cc: dev@dpdk.org
> >> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
> >> API
> >>
> >> Hi Ori,
> >>
> >> On 04/10/2021 08:45, Ori Kam wrote:
> >>> Hi Ivan,
> >>>
> >>>> -----Original Message-----
> >>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
> >>>> Sent: Sunday, October 3, 2021 9:11 PM
> >>>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
> >>>> API
> >>>>
> >>>>
> >>>>
> >>>> On 03/10/2021 15:40, Ori Kam wrote:
> >>>>> Hi Andrew and Ivan,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>>>> Sent: Friday, October 1, 2021 4:47 PM
> >>>>>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow
> >>>>>> API
> >>>>>>
> >>>>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> >>>>>>
> >>>>>> For use with "transfer" flows. Supposed to match traffic entering
> >>>>>> the e-switch from the external world (network, guests) via the
> >>>>>> port which is logically connected with the given ethdev.
> >>>>>>
> >>>>>> Must not be combined with attributes "ingress" / "egress".
> >>>>>>
> >>>>>> This item is meant to use the same structure as ethdev item.
> >>>>>>
> >>>>>
> >>>>> In case the app is not working with representors, meaning each
> >>>>> switch port is mapped to ethdev.
> >>>>> both items (ethdev and eswitch port ) have the same meaning?
> >>>>
> >>>> No. Ethdev means ethdev, and e-switch port is the point where this
> >>>> ethdev is plugged to. For example, "transfer + ESWITCH_PORT" for a
> >>>> regular PF ethdev typically means the network port (maybe you can
> >>>> recall the idea that a PF ethdev "represents" the network port it's
> >> associated with).
> >>>>
> >>>> I believe, that diagrams which these patches add to
> >>>> "doc/guides/prog_guide/rte_flow.rst" may come in handy to
> >>>> understand the meaning. Also, you can take a look at our larger
> >>>> diagram from the Sep 14 gathering.
> >>>>
> >>>
> >>> Lets look at the following system:
> >>> E-Switch has 3 ports - PF, VF1, VF2
> >>> The ports are distributed as follows:
> >>> DPDK application:
> >>> ethdev(0) pf,
> >>> ethdev(1) representor to VF1
> >>> ethdev(2) representor to VF2
> >>> ethdev(3) VF1
> >>>
> >>> VM:
> >>> VF2
> >>>
> >>> As we know all representors are realy connected to the PF(at least
> >>> in this example)
> >>
> >> This example tries to say that the e-switch has 3 ports in total,
> >> and, given your explanation, one may indeed agree that *in this
> >> example* representors re-use e-switch port of ethdev=0 (with some
> >> metadata to distinguish packets, etc.). But one can hardly assume
> >> that *all* representors with any vendor's NIC are connected to the
> >> e-switch the same way. It's vendor specific. Well, at least,
> >> applications don't have this knowledge and don't need to.
> >>
> >>>
> >>> So matching on ethdev(3)  means matching on traffic sent from DPDK
> >>> port
> >> 3 right?
> >>
> >> Item ETHDEV (ethdev_id=3) matches traffic sent by DPDK port 3. Looks
> >> like we're on the same page here.
> >>
> >
> > Good.
> >
> >>> And matching on eswitch_port(3) means matching in traffic that goes
> >>> into VF1 which is the same traffic as ethdev(3) right?
> >>
> >> I didn't catch the thought about "the same traffic". Direction is not the
> same.
> >> Item ESWITCH_PORT (ethdev_id=3) matches traffic sent by DPDK port 1.
> >>
> > This is the critical part for my understanding.
> > Matching on ethdev_id(3) means matching on traffic that is coming from
> DPDK port3.
> 
> Right.
> 
> > So from E-Switch view point it is traffic that goes into VF1?
> 
> No. Above you clearly say "coming from DPDK port3". That is, from the VF1.
> *Not* going into it. Port 3 (ethdev_id=3) *is* VF1.
> 
But taffic that goes from DPDK port 3 goes into VF1,
what am I missing?

> > While matching on E-Switch_port(3) means matching on traffic coming
> from VF1?
> 
> No. It means matching on traffic coming from ethdev 1. From the VF1's
> representor.
> 
> >
> > And by the same logic matching on ethdev_id(1) means matching on
> > taffic that was sent from DPDK port 1 and matching on E-Switch_port(1)
> > means matching on traffic coming from
> > VF1
> 
> In this case, you've got this right. But please see my above notes. By the
> looks of it, you might have run into confusion over there.
> 
That is the issue I'm not sure I understand, and I think that 
if I don't underdand this, this miss underdanding will be shared by others.

> >
> > So in this case eswitch_port(3) is equal ot eswitch_port(1) right?
> > While ethdev(1) is not equal to ethdev(3)
> 
> No.
> 
> Item ETHDEV (ethdev_id=1) equals item ESWITCH_PORT (ethdev_id=3).
> Item ETHDEV (ethdev_id=3) equals item ESWITCH_PORT (ethdev_id=1).
> 
I think this was my first undestaning, lets see if I can explain it using my words.
ETHDEV - means that the source port that we are matching on is the closest one
the dpdk application. Example like above ETHDEV(ethdev_id=1) means matching
on traffic coming from the PF with some metadata that marks this DPDK port.
ETHDEV(ethdev_id=3) means matching on traffic coming from VF1.

ESWITCH_PORT meaning matching on the port that is connected to the DPDK port
(the other side of the wire). Example ESWITCH_PORT(ethdev_id=1) means matching
on traffic coming from VF1
While matching on ESWITCH_PORT(ethdev_id=3) means matching on PF with some
metadata.

Everything assume that representors are on the PF and use some metadata.

Did I get it right?

> >
> > And just to complete the picture, matching on ethdev(2) will result in
> > traffic coming from the dpdk port and matching on eswitch_port(2) will
> > match on traffic coming from VF2
> 
> Exactly.
> 
> 
> But, Ori, let me draw your attention to the following issue. In order to
> simplify understanding, I suggest that we refrain from saying "traffic that
> GOES TO". Where it goes depends on default rules that are supposed to be
> maintained by the PMD when ports get plugged / unplugged.
> 
> The flow items ETHDEV and ESWITH_PORT define the SOURCE of traffic.
> That's it. They define where the traffic "goes FROM".
> 
> Say, the DPDK application sends a packet from ethdev 0. This packet enters
> the e-switch. Match engine sits in the e-switch and intercepts the packet. It
> doesn't care where the packet *would go* if it wasn't intercepted. It cares
> about where the packet comes from. And it comes from ethdev 0. So, in the
> focus, we have the SOURCE of the packet.
> 

Agree with you we should only look at coming from,
but something in the patch made me thing otherwise (not sure what part)

> 
> >
> >> Yes, in this case neither of the ports (1, 3) is truly "external"
> >> (they both interface the DPDK application), but, the thing is,
> >> they're "external" *to each
> >> other* in the sense that they sit at the opposite ends of the wire.
> >>
> >>>
> >>> Matching on ethdev(1) means matching on the PF port in the E-Switch
> >>> but
> >> with some
> >>> metadata that marks the traffic as coming from DPDK port 1 and not
> >>> from
> >> VF1 E-Switch
> >>> port right?
> >>
> >> That's vendor specific. The application doesn't have to know how
> >> exactly this particular ethdev is connected to the e-switch - whether
> >> it re-uses the PF's e-switch port or has its own. The e-switch port
> >> that connects the ethdev with the e-switch is just assumed to exist
> logically.
> >>
> >>>
> >>> While matching on eswitch_port(2) means matching on traffic coming
> >>> from
> >> the VM right?
> >>
> >> Right.
> >>
> >
> > I think the my above question will clear everything for me.
> >

[Snip]
> >>>>>
> >>>>> Best,
> >>>>> Ori
> >>>>>
> >>>>
> >>>> --
> >>>> Ivan M
> >>
> >> --
> >> Ivan M
> >
> > Thanks,
> > Ori
> >
> 
> --
> Ivan M
  
Andrew Rybchenko Oct. 5, 2021, 9:19 a.m. UTC | #8
Hi Ori,

On 10/5/21 9:20 AM, Ori Kam wrote:
> Hi Ivan,
> 
>> -----Original Message-----
>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
>>
>> Hi Ori,
>>
>> On 04/10/2021 14:37, Ori Kam wrote:
>>> Hi Ivan,
>>>
>>>> -----Original Message-----
>>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>>>> Sent: Monday, October 4, 2021 2:06 PM
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
>>>> API
>>>>
>>>> Hi Ori,
>>>>
>>>> On 04/10/2021 08:45, Ori Kam wrote:
>>>>> Hi Ivan,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>>>>>> Sent: Sunday, October 3, 2021 9:11 PM
>>>>>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
>>>>>> API
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 03/10/2021 15:40, Ori Kam wrote:
>>>>>>> Hi Andrew and Ivan,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>>> Sent: Friday, October 1, 2021 4:47 PM
>>>>>>>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow
>>>>>>>> API
>>>>>>>>
>>>>>>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>>>>>
>>>>>>>> For use with "transfer" flows. Supposed to match traffic entering
>>>>>>>> the e-switch from the external world (network, guests) via the
>>>>>>>> port which is logically connected with the given ethdev.
>>>>>>>>
>>>>>>>> Must not be combined with attributes "ingress" / "egress".
>>>>>>>>
>>>>>>>> This item is meant to use the same structure as ethdev item.
>>>>>>>>
>>>>>>>
>>>>>>> In case the app is not working with representors, meaning each
>>>>>>> switch port is mapped to ethdev.
>>>>>>> both items (ethdev and eswitch port ) have the same meaning?
>>>>>>
>>>>>> No. Ethdev means ethdev, and e-switch port is the point where this
>>>>>> ethdev is plugged to. For example, "transfer + ESWITCH_PORT" for a
>>>>>> regular PF ethdev typically means the network port (maybe you can
>>>>>> recall the idea that a PF ethdev "represents" the network port it's
>>>> associated with).
>>>>>>
>>>>>> I believe, that diagrams which these patches add to
>>>>>> "doc/guides/prog_guide/rte_flow.rst" may come in handy to
>>>>>> understand the meaning. Also, you can take a look at our larger
>>>>>> diagram from the Sep 14 gathering.
>>>>>>
>>>>>
>>>>> Lets look at the following system:
>>>>> E-Switch has 3 ports - PF, VF1, VF2
>>>>> The ports are distributed as follows:
>>>>> DPDK application:
>>>>> ethdev(0) pf,
>>>>> ethdev(1) representor to VF1
>>>>> ethdev(2) representor to VF2
>>>>> ethdev(3) VF1
>>>>>
>>>>> VM:
>>>>> VF2
>>>>>
>>>>> As we know all representors are realy connected to the PF(at least
>>>>> in this example)
>>>>
>>>> This example tries to say that the e-switch has 3 ports in total,
>>>> and, given your explanation, one may indeed agree that *in this
>>>> example* representors re-use e-switch port of ethdev=0 (with some
>>>> metadata to distinguish packets, etc.). But one can hardly assume
>>>> that *all* representors with any vendor's NIC are connected to the
>>>> e-switch the same way. It's vendor specific. Well, at least,
>>>> applications don't have this knowledge and don't need to.
>>>>
>>>>>
>>>>> So matching on ethdev(3)  means matching on traffic sent from DPDK
>>>>> port
>>>> 3 right?
>>>>
>>>> Item ETHDEV (ethdev_id=3) matches traffic sent by DPDK port 3. Looks
>>>> like we're on the same page here.
>>>>
>>>
>>> Good.
>>>
>>>>> And matching on eswitch_port(3) means matching in traffic that goes
>>>>> into VF1 which is the same traffic as ethdev(3) right?
>>>>
>>>> I didn't catch the thought about "the same traffic". Direction is not the
>> same.
>>>> Item ESWITCH_PORT (ethdev_id=3) matches traffic sent by DPDK port 1.
>>>>
>>> This is the critical part for my understanding.
>>> Matching on ethdev_id(3) means matching on traffic that is coming from
>> DPDK port3.
>>
>> Right.
>>
>>> So from E-Switch view point it is traffic that goes into VF1?
>>
>> No. Above you clearly say "coming from DPDK port3". That is, from the VF1.
>> *Not* going into it. Port 3 (ethdev_id=3) *is* VF1.
>>
> But taffic that goes from DPDK port 3 goes into VF1,
> what am I missing?

DPDK port 3 is a VF1 itself. So, is it loopback? I think no.

Let me repeat your example:

DPDK application:
ethdev(0) pf,
ethdev(1) representor to VF1
ethdev(2) representor to VF2
ethdev(3) VF1

VM:
VF2

Traffic that goes from DPDK port 3 (which is VF1 bound to DPDK)
goes to its representor by default, i.e. ethdev(1).

> 
>>> While matching on E-Switch_port(3) means matching on traffic coming
>> from VF1?
>>
>> No. It means matching on traffic coming from ethdev 1. From the VF1's
>> representor.
>>
>>>
>>> And by the same logic matching on ethdev_id(1) means matching on
>>> taffic that was sent from DPDK port 1 and matching on E-Switch_port(1)
>>> means matching on traffic coming from
>>> VF1
>>
>> In this case, you've got this right. But please see my above notes. By the
>> looks of it, you might have run into confusion over there.
>>
> That is the issue I'm not sure I understand, and I think that 
> if I don't underdand this, this miss underdanding will be shared by others.

In order to address the confusion and misunderstanding we
should find out the source of it. We always match on source
(inbound port) and the result is a destination (outgoing port).
So, if item is ETHDEV, the source is DPDK application via
corresponding ethdev port.  If ETHDEV is an action, the
destination is the DPDK application via specified ethdev port.
Above is regardless of the ethdev port type (representor or
not). Always, no exceptions.

>>>
>>> So in this case eswitch_port(3) is equal ot eswitch_port(1) right?
>>> While ethdev(1) is not equal to ethdev(3)
>>
>> No.
>>
>> Item ETHDEV (ethdev_id=1) equals item ESWITCH_PORT (ethdev_id=3).
>> Item ETHDEV (ethdev_id=3) equals item ESWITCH_PORT (ethdev_id=1).
>>
> I think this was my first undestaning, lets see if I can explain it using my words.
> ETHDEV - means that the source port that we are matching on is the closest one
> the dpdk application.

Yes, it sounds right.

> Example like above ETHDEV(ethdev_id=1) means matching
> on traffic coming from the PF with some metadata that marks this DPDK port.

No, no, no. It is the problem of too much knowledge and
diving too deep. Neither you nor API user should not
think about it. Representor ethdev(1) is a DPDK ethdev
port that's it. An application can send traffic via the
ethdev port and receive traffic from it. How the traffic
goes inside is vendor-specific implementation detail.
Application just know that if it sends traffic to VF1
representor ethdev(1), the traffic will be received
from VF1 by default. If it sends traffic from VF1,
the traffic will be received from VF1 representor
by default. It is the definition of the representor.

> ETHDEV(ethdev_id=3) means matching on traffic coming from VF1.

Yes.

> 
> ESWITCH_PORT meaning matching on the port that is connected to the DPDK port
> (the other side of the wire). Example ESWITCH_PORT(ethdev_id=1) means matching
> on traffic coming from VF1

Yes, since ethdev(1) is VF1 representor.

> While matching on ESWITCH_PORT(ethdev_id=3) means matching on PF with some
> metadata.

Again, yes and no. No, since you should not think about it this way. It
is just traffic sent via VF1 representor (since ethdev(3) is VF1 and its
other side of the logical wire is VF1
representor). No vendor-specific implementation details.

> 
> Everything assume that representors are on the PF and use some metadata.

No, it is a vendor-specific implementation details.
Flow API definitions should not mention it.

> 
> Did I get it right?

I think you get overall idea right, but explanations
are too complicated. It is simpler in fact.

>>>
>>> And just to complete the picture, matching on ethdev(2) will result in
>>> traffic coming from the dpdk port and matching on eswitch_port(2) will
>>> match on traffic coming from VF2
>>
>> Exactly.
>>
>>
>> But, Ori, let me draw your attention to the following issue. In order to
>> simplify understanding, I suggest that we refrain from saying "traffic that
>> GOES TO". Where it goes depends on default rules that are supposed to be
>> maintained by the PMD when ports get plugged / unplugged.
>>
>> The flow items ETHDEV and ESWITH_PORT define the SOURCE of traffic.
>> That's it. They define where the traffic "goes FROM".
>>
>> Say, the DPDK application sends a packet from ethdev 0. This packet enters
>> the e-switch. Match engine sits in the e-switch and intercepts the packet. It
>> doesn't care where the packet *would go* if it wasn't intercepted. It cares
>> about where the packet comes from. And it comes from ethdev 0. So, in the
>> focus, we have the SOURCE of the packet.
>>
> 
> Agree with you we should only look at coming from,
> but something in the patch made me thing otherwise (not sure what part)

I've reread the documentation and failed to find,
but it will be hard for me to do it, since I read
it too many times already.

Thanks,
Andrew.
  
Ivan Malov Oct. 5, 2021, 12:12 p.m. UTC | #9
Hi Ori, Andrew,

On 05/10/2021 12:19, Andrew Rybchenko wrote:
> Hi Ori,
> 
> On 10/5/21 9:20 AM, Ori Kam wrote:
>> Hi Ivan,
>>
>>> -----Original Message-----
>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>>> Cc: dev@dpdk.org
>>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
>>>
>>> Hi Ori,
>>>
>>> On 04/10/2021 14:37, Ori Kam wrote:
>>>> Hi Ivan,
>>>>
>>>>> -----Original Message-----
>>>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>>>>> Sent: Monday, October 4, 2021 2:06 PM
>>>>> Cc: dev@dpdk.org
>>>>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
>>>>> API
>>>>>
>>>>> Hi Ori,
>>>>>
>>>>> On 04/10/2021 08:45, Ori Kam wrote:
>>>>>> Hi Ivan,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>>>>>>> Sent: Sunday, October 3, 2021 9:11 PM
>>>>>>> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
>>>>>>> API
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 03/10/2021 15:40, Ori Kam wrote:
>>>>>>>> Hi Andrew and Ivan,
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>>>> Sent: Friday, October 1, 2021 4:47 PM
>>>>>>>>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow
>>>>>>>>> API
>>>>>>>>>
>>>>>>>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>>>>>>
>>>>>>>>> For use with "transfer" flows. Supposed to match traffic entering
>>>>>>>>> the e-switch from the external world (network, guests) via the
>>>>>>>>> port which is logically connected with the given ethdev.
>>>>>>>>>
>>>>>>>>> Must not be combined with attributes "ingress" / "egress".
>>>>>>>>>
>>>>>>>>> This item is meant to use the same structure as ethdev item.
>>>>>>>>>
>>>>>>>>
>>>>>>>> In case the app is not working with representors, meaning each
>>>>>>>> switch port is mapped to ethdev.
>>>>>>>> both items (ethdev and eswitch port ) have the same meaning?
>>>>>>>
>>>>>>> No. Ethdev means ethdev, and e-switch port is the point where this
>>>>>>> ethdev is plugged to. For example, "transfer + ESWITCH_PORT" for a
>>>>>>> regular PF ethdev typically means the network port (maybe you can
>>>>>>> recall the idea that a PF ethdev "represents" the network port it's
>>>>> associated with).
>>>>>>>
>>>>>>> I believe, that diagrams which these patches add to
>>>>>>> "doc/guides/prog_guide/rte_flow.rst" may come in handy to
>>>>>>> understand the meaning. Also, you can take a look at our larger
>>>>>>> diagram from the Sep 14 gathering.
>>>>>>>
>>>>>>
>>>>>> Lets look at the following system:
>>>>>> E-Switch has 3 ports - PF, VF1, VF2
>>>>>> The ports are distributed as follows:
>>>>>> DPDK application:
>>>>>> ethdev(0) pf,
>>>>>> ethdev(1) representor to VF1
>>>>>> ethdev(2) representor to VF2
>>>>>> ethdev(3) VF1
>>>>>>
>>>>>> VM:
>>>>>> VF2
>>>>>>
>>>>>> As we know all representors are realy connected to the PF(at least
>>>>>> in this example)
>>>>>
>>>>> This example tries to say that the e-switch has 3 ports in total,
>>>>> and, given your explanation, one may indeed agree that *in this
>>>>> example* representors re-use e-switch port of ethdev=0 (with some
>>>>> metadata to distinguish packets, etc.). But one can hardly assume
>>>>> that *all* representors with any vendor's NIC are connected to the
>>>>> e-switch the same way. It's vendor specific. Well, at least,
>>>>> applications don't have this knowledge and don't need to.
>>>>>
>>>>>>
>>>>>> So matching on ethdev(3)  means matching on traffic sent from DPDK
>>>>>> port
>>>>> 3 right?
>>>>>
>>>>> Item ETHDEV (ethdev_id=3) matches traffic sent by DPDK port 3. Looks
>>>>> like we're on the same page here.
>>>>>
>>>>
>>>> Good.
>>>>
>>>>>> And matching on eswitch_port(3) means matching in traffic that goes
>>>>>> into VF1 which is the same traffic as ethdev(3) right?
>>>>>
>>>>> I didn't catch the thought about "the same traffic". Direction is not the
>>> same.
>>>>> Item ESWITCH_PORT (ethdev_id=3) matches traffic sent by DPDK port 1.
>>>>>
>>>> This is the critical part for my understanding.
>>>> Matching on ethdev_id(3) means matching on traffic that is coming from
>>> DPDK port3.
>>>
>>> Right.
>>>
>>>> So from E-Switch view point it is traffic that goes into VF1?
>>>
>>> No. Above you clearly say "coming from DPDK port3". That is, from the VF1.
>>> *Not* going into it. Port 3 (ethdev_id=3) *is* VF1.
>>>
>> But taffic that goes from DPDK port 3 goes into VF1,
>> what am I missing?

Terms like "PF", "VF", "PHY_PORT" describe the nature of the e-switch 
ports. In your example, DPDK port 3 is just based on VF1. The 
application doesn't have such knowledge. To it, this is a regular ethdev.

In order to gain correct understanding, you should imagine an observer 
standing inside the e-switch. When that observer sees packets entering 
the e-switch FROM the VF1, we effectively talk about packets sent by the 
ethdev sitting on top of that VF1, that is, packets coming FROM DPDK 
port 3. And vice versa: if you say "traffic that goes into VF1", you 
mean traffic that LEAVES the e-switch via VF1, that is, traffic, going 
TO DPDK port 3.

"VF1" is just a name of a logical "window" between the e-switch and its 
surroundings. If this VF is passed to a guest machine, then this VF is a 
"window" between the e-switch and the guest machine. If you plug this VF 
to the DPDK application (create an ethdev on top of this VF), then this 
VF is a "window" between the e-switch and the PMD. That's it.

> 
> DPDK port 3 is a VF1 itself. So, is it loopback? I think no.
> 
> Let me repeat your example:
> 
> DPDK application:
> ethdev(0) pf,
> ethdev(1) representor to VF1
> ethdev(2) representor to VF2
> ethdev(3) VF1
> 
> VM:
> VF2
> 
> Traffic that goes from DPDK port 3 (which is VF1 bound to DPDK)
> goes to its representor by default, i.e. ethdev(1).

+1

> 
>>
>>>> While matching on E-Switch_port(3) means matching on traffic coming
>>> from VF1?
>>>
>>> No. It means matching on traffic coming from ethdev 1. From the VF1's
>>> representor.
>>>
>>>>
>>>> And by the same logic matching on ethdev_id(1) means matching on
>>>> taffic that was sent from DPDK port 1 and matching on E-Switch_port(1)
>>>> means matching on traffic coming from
>>>> VF1
>>>
>>> In this case, you've got this right. But please see my above notes. By the
>>> looks of it, you might have run into confusion over there.
>>>
>> That is the issue I'm not sure I understand, and I think that
>> if I don't underdand this, this miss underdanding will be shared by others.

Please see my diagram below.

> 
> In order to address the confusion and misunderstanding we
> should find out the source of it. We always match on source
> (inbound port) and the result is a destination (outgoing port).
> So, if item is ETHDEV, the source is DPDK application via
> corresponding ethdev port.  If ETHDEV is an action, the
> destination is the DPDK application via specified ethdev port.
> Above is regardless of the ethdev port type (representor or
> not). Always, no exceptions.
> 
>>>>
>>>> So in this case eswitch_port(3) is equal ot eswitch_port(1) right?
>>>> While ethdev(1) is not equal to ethdev(3)
>>>
>>> No.
>>>
>>> Item ETHDEV (ethdev_id=1) equals item ESWITCH_PORT (ethdev_id=3).
>>> Item ETHDEV (ethdev_id=3) equals item ESWITCH_PORT (ethdev_id=1).
>>>
>> I think this was my first undestaning, lets see if I can explain it using my words.
>> ETHDEV - means that the source port that we are matching on is the closest one
>> the dpdk application.
> 
> Yes, it sounds right.
> 
>> Example like above ETHDEV(ethdev_id=1) means matching
>> on traffic coming from the PF with some metadata that marks this DPDK port.

Some vendors support allocation of separate logical e-switch ports for 
representors. Other vendors can't support that, and, in this case, they 
indeed have to devise "PF + metadata" solution. But DPDK framework (I 
mean, vendor-agnostic code) can't assume any of these options as a 
"standard". Neither can applications do that. It's truly vendor-specific.

> 
> No, no, no. It is the problem of too much knowledge and
> diving too deep. Neither you nor API user should not
> think about it. Representor ethdev(1) is a DPDK ethdev
> port that's it. An application can send traffic via the
> ethdev port and receive traffic from it. How the traffic
> goes inside is vendor-specific implementation detail.
> Application just know that if it sends traffic to VF1
> representor ethdev(1), the traffic will be received
> from VF1 by default. If it sends traffic from VF1,
> the traffic will be received from VF1 representor
> by default. It is the definition of the representor.

+1

> 
>> ETHDEV(ethdev_id=3) means matching on traffic coming from VF1.
> 
> Yes.
> 
>>
>> ESWITCH_PORT meaning matching on the port that is connected to the DPDK port
>> (the other side of the wire). Example ESWITCH_PORT(ethdev_id=1) means matching
>> on traffic coming from VF1
> 
> Yes, since ethdev(1) is VF1 representor.
> 
>> While matching on ESWITCH_PORT(ethdev_id=3) means matching on PF with some
>> metadata.
> 
> Again, yes and no. No, since you should not think about it this way. It
> is just traffic sent via VF1 representor (since ethdev(3) is VF1 and its
> other side of the logical wire is VF1
> representor). No vendor-specific implementation details.
> 
>>
>> Everything assume that representors are on the PF and use some metadata.
> 
> No, it is a vendor-specific implementation details.
> Flow API definitions should not mention it.

+1

> 
>>
>> Did I get it right?

Let's try to look at the overall idea one more time. Just to get us on 
the same page.

In general, for any given ethdev you have:


ETHDEV (A)
|
|    (PMD area)
|
CLOSEST ESWITCH_PORT (B)
|
|    (e-switch area)
|
REMOTE ESWITCH_PORT (C)
|
X


The point "B" is located closest to the ethdev (A). Not to the 
application in general, but to the given ethdev.

The point "C" is the most remote point from the ethdev's perspective. 
Again, it's not the most remote from the application. It's the most 
remote from this specific ethdev.

 From the application perspective, the nature of (B) and (C) is unknown. 
But the application knows for sure that these points just exist. To it, 
these points are some *logical* e-switch ports.

Now.

When you say "item ETHDEV", you tell the PMD that you want to match 
packets entering the *PMD area* FROM point (A). But you also add 
"transfer" attribute. This attribute makes the PMD translate your 
request to the e-switch viewpoint by "transferring" point "A" one level 
below. What was "A" becomes "B". This way, an imaginary observer 
standing inside the *e-switch area* intercepts packets which enter this 
area FROM (B).
Summary: this way, you match packets going *DOWN* from (A).

When you say "item ESWITCH_PORT", you tell the PMD that you want to 
match packets entering the *PMD area* FROM point (B). Once again, 
"transfer" translates this to the e-switch viewpoint, that is, (B) 
becomes (C). So, the e-switch knows that it should intercept packets 
entering its area FROM point (C).
Summary: this way, you match packets going *UP* from (X).


Some use case examples:

- The ethdev (A) is a DPDK port based on a PF associated with a network 
port. In this case, (B) == PF and (C) == (X) == network port.

- The ethdev (A) is a representor of VF1 which is passed to a VM. In 
this case, (B) is just an e-switch logical port. It's nature is vendor 
specific. It can be a PF, it can be not. It depends. (C) == VF, (X) == VM.

- The ethdev (A) is a representor of VF1 which is plugged to the same 
DPDK application. In this case, (B) is, once again, some logical 
e-switch port. (C) == VF. But (X) here is another ethdev. So, this way, 
ethdev (A) is a representor's ethdev and ethdev (X) is a VF's ethdev.

In all cases the application doesn't care about the nature of the 
e-switch ports (B) and (C). It just knows that they exist.

> 
> I think you get overall idea right, but explanations
> are too complicated. It is simpler in fact.
> 
>>>>
>>>> And just to complete the picture, matching on ethdev(2) will result in
>>>> traffic coming from the dpdk port and matching on eswitch_port(2) will
>>>> match on traffic coming from VF2
>>>
>>> Exactly.
>>>
>>>
>>> But, Ori, let me draw your attention to the following issue. In order to
>>> simplify understanding, I suggest that we refrain from saying "traffic that
>>> GOES TO". Where it goes depends on default rules that are supposed to be
>>> maintained by the PMD when ports get plugged / unplugged.
>>>
>>> The flow items ETHDEV and ESWITH_PORT define the SOURCE of traffic.
>>> That's it. They define where the traffic "goes FROM".
>>>
>>> Say, the DPDK application sends a packet from ethdev 0. This packet enters
>>> the e-switch. Match engine sits in the e-switch and intercepts the packet. It
>>> doesn't care where the packet *would go* if it wasn't intercepted. It cares
>>> about where the packet comes from. And it comes from ethdev 0. So, in the
>>> focus, we have the SOURCE of the packet.
>>>
>>
>> Agree with you we should only look at coming from,
>> but something in the patch made me thing otherwise (not sure what part)
> 
> I've reread the documentation and failed to find,
> but it will be hard for me to do it, since I read
> it too many times already.
> 
> Thanks,
> Andrew.
>
  

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index e05b0d83d2..188d0ee39d 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -308,6 +308,8 @@  enum index {
 	ITEM_POL_POLICY,
 	ITEM_ETHDEV,
 	ITEM_ETHDEV_ID,
+	ITEM_ESWITCH_PORT,
+	ITEM_ESWITCH_PORT_ETHDEV_ID,
 
 	/* Validate/create actions. */
 	ACTIONS,
@@ -1003,6 +1005,7 @@  static const enum index next_item[] = {
 	ITEM_INTEGRITY,
 	ITEM_CONNTRACK,
 	ITEM_ETHDEV,
+	ITEM_ESWITCH_PORT,
 	END_SET,
 	ZERO,
 };
@@ -1377,6 +1380,12 @@  static const enum index item_ethdev[] = {
 	ZERO,
 };
 
+static const enum index item_eswitch_port[] = {
+	ITEM_ESWITCH_PORT_ETHDEV_ID,
+	ITEM_NEXT,
+	ZERO,
+};
+
 static const enum index next_action[] = {
 	ACTION_END,
 	ACTION_VOID,
@@ -3632,6 +3641,21 @@  static const struct token token_list[] = {
 			     item_param),
 		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev, id)),
 	},
+	[ITEM_ESWITCH_PORT] = {
+		.name = "eswitch_port",
+		.help = "match traffic at e-switch going from the external port associated with the given ethdev",
+		.priv = PRIV_ITEM(ESWITCH_PORT,
+				  sizeof(struct rte_flow_item_ethdev)),
+		.next = NEXT(item_eswitch_port),
+		.call = parse_vc,
+	},
+	[ITEM_ESWITCH_PORT_ETHDEV_ID] = {
+		.name = "ethdev_id",
+		.help = "ethdev ID",
+		.next = NEXT(item_eswitch_port, NEXT_ENTRY(COMMON_UNSIGNED),
+			     item_param),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev, id)),
+	},
 	/* Validate/create actions. */
 	[ACTIONS] = {
 		.name = "actions",
@@ -8370,6 +8394,9 @@  flow_item_default_mask(const struct rte_flow_item *item)
 	case RTE_FLOW_ITEM_TYPE_ETHDEV:
 		mask = &rte_flow_item_ethdev_mask;
 		break;
+	case RTE_FLOW_ITEM_TYPE_ESWITCH_PORT:
+		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 ab628d9139..292bb42410 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1460,6 +1460,28 @@  Use this with attribute **transfer**. Attributes **ingress** and
    | ``mask`` | ``id``   | zeroed for wildcard match |
    +----------+----------+---------------------------+
 
+Item: ``ESWITCH_PORT``
+^^^^^^^^^^^^^^^^^^^^^^
+
+Matches traffic at e-switch going from the external port associated
+with the given ethdev, for example, traffic from net. port or guest.
+
+::
+
+   *    (Ethdev) ~~~~~~~~~~~~ (Internal Port) ~~~~ [] <<<< (External Port)
+   *    :  SW                 :   Logical                    Net / Guest :
+   *    :                     :                                          :
+   *    | ---- PMD Layer ---- | ------------ E-Switch Layer ------------ |
+   *
+   *    [] shows the effective ("transfer") standpoint, the match engine;
+   *    << shows the traffic flow in question hitting the match engine;
+   *    ~~ shows logical interconnects between the endpoints.
+
+Use this with attribute **transfer**. Attributes **ingress** and
+**egress** (`Attribute: Traffic direction`_) must not be used.
+
+This item is meant to use the same structure as `Item: ETHDEV`_.
+
 Actions
 ~~~~~~~
 
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index 91631adb4e..b2b27de3f0 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -167,7 +167,7 @@  API Changes
    Also, make sure to start the actual text at the margin.
    =======================================================
 
-* ethdev: Added item ``ETHDEV`` to flow API.
+* ethdev: Added items ``ETHDEV``, ``ESWITCH_PORT`` to flow API.
 
 * cryptodev: The API rte_cryptodev_pmd_is_valid_dev is modified to
   rte_cryptodev_is_valid_dev as it can be used by the application as
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 6d5de5457c..9a5c2a2d82 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3824,6 +3824,10 @@  This section lists supported pattern items and their attributes, if any.
 
   - ``id {unsigned}``: ethdev ID
 
+- ``eswitch_port``: match traffic at e-switch going from the external port associated with the given ethdev
+
+  - ``ethdev_id {unsigned}``: ethdev ID
+
 Actions list
 ^^^^^^^^^^^^
 
diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
index 84eb61cb6c..c4aea5625f 100644
--- a/lib/ethdev/rte_flow.c
+++ b/lib/ethdev/rte_flow.c
@@ -101,6 +101,7 @@  static const struct rte_flow_desc_data rte_flow_desc_item[] = {
 	MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
 	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
 	MK_FLOW_ITEM(ETHDEV, sizeof(struct rte_flow_item_ethdev)),
+	MK_FLOW_ITEM(ESWITCH_PORT, 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 880502098e..1a7e4c2e3d 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -583,6 +583,16 @@  enum rte_flow_item_type {
 	 * @see struct rte_flow_item_ethdev
 	 */
 	RTE_FLOW_ITEM_TYPE_ETHDEV,
+
+	/**
+	 * [META]
+	 *
+	 * Matches traffic at e-switch going from the external port associated
+	 * with the given ethdev, for example, traffic from net. port or guest.
+	 *
+	 * @see struct rte_flow_item_ethdev
+	 */
+	RTE_FLOW_ITEM_TYPE_ESWITCH_PORT,
 };
 
 /**
@@ -1813,7 +1823,7 @@  static const struct rte_flow_item_conntrack rte_flow_item_conntrack_mask = {
  * @b EXPERIMENTAL: this structure may change without prior notice
  *
  * Provides an ethdev ID for use with items which are as follows:
- * RTE_FLOW_ITEM_TYPE_ETHDEV.
+ * RTE_FLOW_ITEM_TYPE_ETHDEV, RTE_FLOW_ITEM_TYPE_ESWITCH_PORT.
  */
 struct rte_flow_item_ethdev {
 	uint16_t id; /**< Ethdev ID */