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 50D18A0C4C; Mon, 4 Oct 2021 13:06:00 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CED8941300; Mon, 4 Oct 2021 13:05:59 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 08B6D40E3C for ; Mon, 4 Oct 2021 13:05:58 +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 6F02A7F4FD; Mon, 4 Oct 2021 14:05:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 6F02A7F4FD DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633345557; bh=WRNhjDIYywzN1bHoN8uMHqZrGTMqKyd6qL9keZF2B4Q=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=MJGF9/hkwnVy9GMRaxifAVRyHZ6eYgUj+KnBl+PjfgpuDLF3YPCRZFhCOhIkrhuWX HdwRNdPvfZOg85U+YHsbcYaTyuUQ6vePpGgnhndwXQ2B+C1hLNtTdrH16LAkaNhmvi 4w8/FLNdkRcX5uhAaa6x+oTqVzJKEeNj9O8W7v3U= 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: Date: Mon, 4 Oct 2021 14:05:45 +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 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. > 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 >>>> 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. > >>> >>>> + .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