[dpdk-dev] [PATCH v1 02/12] ethdev: add eswitch port item to flow API

[Ivan Malov]

Hi Ori,

On 04/10/2021 14:37, Ori Kam wrote:
> Hi Ivan,
> 
>> -----Original Message-----
>> From: Ivan Malov <Ivan.Malov at oktetlabs.ru>
>> Sent: Monday, October 4, 2021 2:06 PM
>> Cc: dev at 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 at 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 at 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 at 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 at oktetlabs.ru>
>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko at 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 M