From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id EA3E2A0C4D; Mon, 4 Oct 2021 13:58:36 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6D6C04135C; Mon, 4 Oct 2021 13:58:36 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id B58124134B for ; Mon, 4 Oct 2021 13:58:34 +0200 (CEST) Received: from [100.65.5.102] (unknown [5.144.122.214]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 29E4C7F508; Mon, 4 Oct 2021 14:58:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 29E4C7F508 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633348714; bh=we+XVg1GLNv/CdqLpRjrZXX/fd8QspKiOVXhrT9vDAI=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=S6kYFAnGTdpUki3K+Qu87KtPh4IHO5Bu3QN/0sjwKWHpbhi5gWwNLPR2w5Po6buog 48ZKN4XT3SoCpdZosvSADxxOydkET1cMhzAH/MPrF189KYXdIzlqyOq+2Zp0e50QmB GKI8saKYrp6+HoYxuKAZ0O1arE5NdJsvWj0ahbp0= To: Ori Kam , Andrew Rybchenko , Xiaoyun Li , NBU-Contact-Thomas Monjalon , Ferruh Yigit Cc: "dev@dpdk.org" References: <20210907125157.3843-1-ivan.malov@oktetlabs.ru> <20211001134716.1608857-1-andrew.rybchenko@oktetlabs.ru> <20211001134716.1608857-3-andrew.rybchenko@oktetlabs.ru> From: Ivan Malov Message-ID: <98369ad5-f6d8-dadb-194a-fe4f87a34a11@oktetlabs.ru> Date: Mon, 4 Oct 2021 14:58:22 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1 02/12] ethdev: add eswitch port item to flow API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Ori, On 04/10/2021 14:37, Ori Kam wrote: > Hi Ivan, > >> -----Original Message----- >> From: Ivan Malov >> 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 >>>> 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 >>>>>> Sent: Friday, October 1, 2021 4:47 PM >>>>>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow API >>>>>> >>>>>> From: Ivan Malov >>>>>> >>>>>> 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 >>>>>> Signed-off-by: Andrew Rybchenko >>>>>> --- >>>>>> 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