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 C8DA2A0032; Wed, 14 Sep 2022 12:17:31 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 674DB4021D; Wed, 14 Sep 2022 12:17:31 +0200 (CEST) Received: from NAM11-CO1-obe.outbound.protection.outlook.com (mail-co1nam11on2041.outbound.protection.outlook.com [40.107.220.41]) by mails.dpdk.org (Postfix) with ESMTP id 4254840156 for ; Wed, 14 Sep 2022 12:17:29 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Yw4iAkeu5Ud6rEPRYSsbcWeQwGxdCojiNhCkW/uCGC4kb6VQ1Rk7Xuvr4kKUXXPOWnQiCXmp2+fLXyZ3bwR41SBd/nVke7wiOsz3ePF5kYdEVlxcynxH1sKs8VFh/LZxTdnqb4o+promu4W776oL4G3Ij6ZgXMKtdnMu2YbEtAvZ8xWAmbZFF3WBROHUen2M0MRLEy8HyqZjrHH6mZiKzjiYpxdy1Os++7yPAj6fCQxEMhTD6YAy9vx/qd3LEW40IPI9tyITF2lf2MFy5fJamyQEYkt/eJNYpDPiEXQn//APhi7LUEO/SvB/YXpBRClaOEZjtOnLQVWELLOPwysRqA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=FQ7mB7QBeWKI8bJDBXy+leajUptnDJrFrjWnl/NJMu0=; b=Bwq/1Xidewf9D1re0HKXvuFx3fST8bZ/7ELYSNvmdNgJdTg9xJv0/XTF9Gt7M6B9XyJSviFcRWCtUwkpgQVyPLk9OJJYj+u4qe4IligRRMtXyoq0zLXBAlG82ByEaWpUiuy5SYaPsLEh1qJCigiwb4s/t4p9Zf6Dk2Jq64UPgQf/clKxFUW4jSgqKu+ySeORhRM/UZQpRLOo0HWy0amPQNWJZoBCQcDyutw08VcOkwIThyrOI9oYgN9kz1sOd9XOD0VL7qWtiPunBsNOm0fy3k5prWwOixO70ch7OPe9TAEcsDHuI3xO1Ck2u+aIb/VpmaLLtexEwiIAIjD4gx4/bA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nvidia.com; dmarc=pass action=none header.from=nvidia.com; dkim=pass header.d=nvidia.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=FQ7mB7QBeWKI8bJDBXy+leajUptnDJrFrjWnl/NJMu0=; b=HRUAcXyLJzpmcRFyUq2OHw8cSCSnSNs73rFHw/nywSRkdJDFp28u5uZaxwKB+XrZHehMoYs2Dp1hQf9oq9tK5HHX/yM8Kh/cSKcQzPYMuCUTGz6kc49wRV1oDBNKQpnTHqN/1N+yGpo0GSuxF1FwPnbuI/Z8oL41ZOD7hZlxkQZVUilleNRfVsMI7pq5ZIICMKLEDerqRVlfVPchwnaZw7vRP5nawlsEF/n+VZF2FiOBtUt0R/sM3DvvcZnpXyRxN5z+EI14vfX/AN3G5spruXW5+rygjEXgLZ9lSdJehvxiowVu0i2gpjjtNc2KFmN+vRBIZln5nwK5ffKPd1C2hQ== Received: from BN9PR12MB5273.namprd12.prod.outlook.com (2603:10b6:408:11e::22) by IA1PR12MB6235.namprd12.prod.outlook.com (2603:10b6:208:3e5::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5588.11; Wed, 14 Sep 2022 10:17:26 +0000 Received: from BN9PR12MB5273.namprd12.prod.outlook.com ([fe80::a426:af46:a459:d88b]) by BN9PR12MB5273.namprd12.prod.outlook.com ([fe80::a426:af46:a459:d88b%5]) with mapi id 15.20.5612.022; Wed, 14 Sep 2022 10:17:26 +0000 From: Rongwei Liu To: Ivan Malov CC: Matan Azrad , Slava Ovsiienko , Ori Kam , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , Aman Singh , Yuying Zhang , Andrew Rybchenko , "dev@dpdk.org" , Raslan Darawsheh Subject: RE: [PATCH v1] ethdev: add direction info when creating the transfer table Thread-Topic: [PATCH v1] ethdev: add direction info when creating the transfer table Thread-Index: AQHYwmNUlJ87k3rsrk6Ble4p/RnGnq3cDLKAgAFR3ACAABgfgIAA1YvQgABHMQCAABGTUA== Date: Wed, 14 Sep 2022 10:17:26 +0000 Message-ID: References: <20220907024020.2474860-1-rongweil@nvidia.com> <1be72d6-be5b-88b2-f15-16fd2c6d0c0@oktetlabs.ru> <5d8d42b2-7011-cb46-7f2c-1b1019c4151e@oktetlabs.ru> <46841a9-37f1-29a8-ba86-ac5410723e2f@oktetlabs.ru> In-Reply-To: <46841a9-37f1-29a8-ba86-ac5410723e2f@oktetlabs.ru> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=nvidia.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: BN9PR12MB5273:EE_|IA1PR12MB6235:EE_ x-ms-office365-filtering-correlation-id: aa0f5084-ceb9-4846-209e-08da963a526f x-ld-processed: 43083d15-7273-40c1-b7db-39efd9ccc17a,ExtAddr x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: XN8AbAb+KIUldhgFG3KTonBgN9KyQ7xrnopgOAmncLltlFGPAqpaoJHgGcR7qQN5QYZhwsv/B5HAic1juvCt09wDTYQCTXGm9POPihIQipMx1Vnn+Ytdz5eY3AsuylSyzZ1JvUvouHmHqFolCZTu8ZBWFEe+cupgewH+hyjaYqd7y9D51ugk8n5kVmaPqg6a9tTdytT6wCbO6lYHFJzkVev3XtTfQ2liRhoohKBwHN5/BOCvZ/ESMZPc61BOBWOJVQh5n/HwyDWY0jTLfYXu/Fyxn3CP+RN0k768Dn2S7f+d7fV5tarTYFEz4cWfNX9RFK93G0m+pkfluLben1FisOd5+bsFocscei15ONRtHDcD2DYz/ey9LOkI6565djQoXRSQG+zCnUrwxBYJwBLWeYFpoGyT9cEVDPEkzf4efrSgz58efa8h++3eDhzKTtUDI7Bf9b+ItoQ5vrrrnF9GaDiSB5oSpOpKmHajgXY0U0Pt+SLIjVFGgkbz4laDoepnq1pn4HAq6TB2j1yf2lsL5Xu3Af7OsPk/72FyZBwbw3mPQAGuM91dgbcMb7njzPouxDB1P0QgGVP8Ijsqe6XKuIsmLdGFmR5YxQiyaxZW6jZQaRqWkeEM4qm+inKRl5Xbqa+oZJffVtzMSjw3dNu2ArTm/tutisEk044Z4Imahfl5N99wP2y8TftVmvXJ72qEL9MbYD3EEmrECzS8edeuWaS0+/J8fNfVQu0M+62sDmQ9ubjdl89a8NcVJUwAxeJuVc2Ic8CDyqRL3yBykBooIw== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BN9PR12MB5273.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(4636009)(396003)(39860400002)(346002)(136003)(366004)(376002)(451199015)(83380400001)(71200400001)(478600001)(5660300002)(107886003)(6506007)(52536014)(30864003)(66446008)(66556008)(122000001)(9686003)(186003)(2906002)(76116006)(38100700002)(8936002)(66946007)(7696005)(4326008)(66476007)(54906003)(41300700001)(8676002)(38070700005)(64756008)(316002)(33656002)(55016003)(86362001)(53546011)(26005)(6916009)(579004)(559001); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?a1fFvgviPX7fgHSpIuWnTFAME9Rxrhf8pPKZGNZfFzWsK+4Pjl8Xai41QWRT?= =?us-ascii?Q?TLqqki78fcACAt4QlCAjm5sz2Gd0gjn0Hw5HqdCniZxmgNToQPpFHC9SlqA/?= =?us-ascii?Q?lXkrF8til+8qB0z1pyXIfezzJ7KLksGRu6XssQ/wrYY8HF2aTCaLo1VfuHiB?= =?us-ascii?Q?F40mLslpbS3VPMM3SN1FjWv4ACSu6Qd9Aq9+QiAYUJDyImKrSo9vdboRik+M?= =?us-ascii?Q?v+Uz3/qs3gnrQPgt9h8Gun3FUo80bQOGw5pNV82EIdGYJY7rYOA+aDX6Z/dT?= =?us-ascii?Q?cHX4/Qg9UZ4elic1zkP5fBjti/v3JZV/4cqhvzxB10fyC0Zy5NorhBGiViXW?= =?us-ascii?Q?ZloTcPPCQfucfdLeAZ/Dwoq3ZlgpFbKMbxgY1Wk0a2wUosXuWsvwNXcvtfmG?= =?us-ascii?Q?r2ax7w1GUU6cBKmaoSl0BIzrpRxE2B5mx17cL7GCRLxE1cnaIzfEiurg/tFe?= =?us-ascii?Q?Y8SFRX3b4OsZV6mteYqgd9BeU2qWNC/P/cFb25liQW/BHYXpBl4l4rZrUr/o?= =?us-ascii?Q?Rm19aDjy2zr9+AaRIy86iEB5SGRU5m9WzK0L4R5nQS5KH6thsIfIt1RGhA0Z?= =?us-ascii?Q?rGwUF4ItnayPAEiAd1aaBTyQ3vA8cPeewiTjHlRb7WvnHcchCymXwnU57JH0?= =?us-ascii?Q?Uv9SkgzCiHAEZ4gBlkKV0lldyPHrnjwFaMSQjDCn4uIUvIwDRtZJt3sAFeRA?= =?us-ascii?Q?3lPIn1kv1vGZWB36MJ3NRtqsWu/y0E7+o4aPkjthV4icZlwJi1fSIRddetT3?= =?us-ascii?Q?DhBXvNcT5CxshpRrCK5+mHOs5Sqwe3bNLPWd5+AS8+9foPSvr6IocLsmSVWb?= =?us-ascii?Q?AWfi+kGLGY7b5QbRbNQWhq/vNbNMn8ymjmCzGkORPsJOyo60ivHi3PA5AEVI?= =?us-ascii?Q?hrQUeg6H6l/4+TIxHX/pxwQJfBIpoqgBrsDzk3DnGrqGepzV6tVTIJqDf1G6?= =?us-ascii?Q?rnNaFPuUhRF0xmuN2Kn3utV8l+xlwQvpzELny3s3YWXSedcLSnvCsIkc7zLB?= =?us-ascii?Q?d5lAHacHFIw87OV9pA4OfbjbZFHvP0i/sKw27qe7ACILapvMug7kfvp6i3cx?= =?us-ascii?Q?abpV29ER14WBFu7zesQLJTRXA+uYvc4AMkVEJikc8vl48wAYcYgNVH3YQLOM?= =?us-ascii?Q?MeFA7dTUu6G1F09skmDZRNEqEzAa060bo/p0QlXwRrbxXB9cMoXL2mxoPfrF?= =?us-ascii?Q?4RqlJgAw4FlcZtmZWbTWgJKHSTYgHEUJ420/BBFYl87dWcy9UHkGaKvF8a0D?= =?us-ascii?Q?v9ncIpgfEkOhigYL/m7FRGkZK+hEhnpMME0yAneqR9xNNJqLpepwuitrNY7s?= =?us-ascii?Q?H03YfhNijfUZ8RoEwZPe+PyYK+/DYlY+N63t99e8GkaIXgqjs62yDf7N1Ict?= =?us-ascii?Q?K6I/GfNFP7aYd2lhfYUqEuMqUxlDPhCAgH3z+n56WSIQ9juj6n+mp5UrD5fJ?= =?us-ascii?Q?LNQNQko7MHP0niZBaITfMgVlUgF1wsTST2VZb8WXL6inkeCj8Ww+lrkBZeJM?= =?us-ascii?Q?GbNdDM+YHaddI8hkkTwJuEGcmdi/P0InR1rr3jUVnZU6txnSF744L4xg1r1M?= =?us-ascii?Q?SIghBONpTGg93+TznGF1n0vtt4t6XeNYbKhrMfav?= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5273.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: aa0f5084-ceb9-4846-209e-08da963a526f X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Sep 2022 10:17:26.1474 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: 1qLcj6XE1TN8D4WqG/v04Z3NcrW+6nlz0oiq0olIsxtFkJUk6krBsnxB5cULRWxaj8rH2gKg8JODNy7ASBp4ug== X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6235 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 HI BR Rongwei > -----Original Message----- > From: Ivan Malov > Sent: Wednesday, September 14, 2022 15:32 > To: Rongwei Liu > Cc: Matan Azrad ; Slava Ovsiienko > ; Ori Kam ; NBU-Contact- > Thomas Monjalon (EXTERNAL) ; Aman Singh > ; Yuying Zhang ; > Andrew Rybchenko ; dev@dpdk.org; Raslan > Darawsheh > Subject: RE: [PATCH v1] ethdev: add direction info when creating the tran= sfer > table >=20 > External email: Use caution opening links or attachments >=20 >=20 > Hi, >=20 > On Wed, 14 Sep 2022, Rongwei Liu wrote: >=20 > > HI > > > > BR > > Rongwei > > > >> -----Original Message----- > >> From: Ivan Malov > >> Sent: Tuesday, September 13, 2022 22:33 > >> To: Rongwei Liu > >> Cc: Matan Azrad ; Slava Ovsiienko > >> ; Ori Kam ; NBU-Contact- > >> Thomas Monjalon (EXTERNAL) ; Aman Singh > >> ; Yuying Zhang ; > >> Andrew Rybchenko ; dev@dpdk.org; > >> Raslan Darawsheh > >> Subject: RE: [PATCH v1] ethdev: add direction info when creating the > >> transfer table > >> > >> External email: Use caution opening links or attachments > >> > >> > >> Hi Rongwei, > >> > >> PSB > >> > >> On Tue, 13 Sep 2022, Rongwei Liu wrote: > >> > >>> Hi > >>> > >>> BR > >>> Rongwei > >>> > >>>> -----Original Message----- > >>>> From: Ivan Malov > >>>> Sent: Tuesday, September 13, 2022 00:57 > >>>> To: Rongwei Liu > >>>> Cc: Matan Azrad ; Slava Ovsiienko > >>>> ; Ori Kam ; NBU-Contact- > >>>> Thomas Monjalon (EXTERNAL) ; Aman Singh > >>>> ; Yuying Zhang ; > >>>> Andrew Rybchenko ; dev@dpdk.org; > >>>> Raslan Darawsheh > >>>> Subject: Re: [PATCH v1] ethdev: add direction info when creating > >>>> the transfer table > >>>> > >>>> External email: Use caution opening links or attachments > >>>> > >>>> > >>>> Hi, > >>>> > >>>> On Wed, 7 Sep 2022, Rongwei Liu wrote: > >>>> > >>>>> The transfer domain rule is able to match traffic wire/vf origin > >>>>> and it means two directions' underlayer resource. > >>>> > >>>> The point of fact is that matching traffic coming from some entity > >>>> like wire / VF has been long generalised in the form of representors= . > >>>> So, a flow rule with attribute "transfer" is able to match traffic > >>>> coming from either a REPRESENTED_PORT or from a > PORT_REPRESENTOR > >> (please find these items). > >>>> > >>>>> > >>>>> In customer deployments, they usually match only one direction > >>>>> traffic in single flow table: either from wire or from vf. > >>>> > >>>> Which customer deployments? Could you please provide detailed > examples? > >>>> > >>>>> > >>> > >>> We saw a lot of customers' deployment like: > >>> 1. Match overlay traffic from wire and do decap, then send to specifi= c > vport. > >>> 2. Match specific 5-tuples and do encap, then send to wire. > >>> The matching criteria has obvious direction preference. > >> > >> Thank you. My questions are as follows: > >> > >> In (1), when you say "from wire", do you mean the need to match > >> packets arriving via whatever physical ports rather then matching > >> packets arriving from some specific phys. port? >=20 > ^^ >=20 > Could you please find my question above? Based on your understanding of > templates in async flow approach, an answer to this question may help us = find > the common ground. It means traffic arrived from physical ports (transfer_proxy role) or south= band per you concept. Traffic from vport (not transfer_proxy) or north band per your concept won'= t hit even if same packets. >=20 > -- >=20 > >> > >> If, however, matching traffic "from wire" in fact means matching > >> packets arriving from a *specific* physical port, then for sure item > >> REPRESENTED_PORT should perfectly do the job, and the proposed > >> attribute is unneeded. > >> > >> (BTW, in DPDK, it is customary to use term "physical port", not > >> "wire") > >> > >> In (1), what are "vport"s? Please explain. Once again, I should > >> remind that, in DPDK, folks prefer terms "represented entity" / > "representor" > >> over vendor-specific terms like "vport", etc. > >> > > Vport is virtual port for short such as VF. >=20 > Thanks. As I say, term "vport" might be confusing to some readers, so it'= d be > better to provide this explanation (about VF) in the commit description n= ext > time. Ack. Will add VF as an example. >=20 > >> As for (2), imagine matching 5-tuple traffic emitted by a VF / guest. > >> Could you please explain, why not just add a match item > >> REPRESENTED_PORT pointing to that VF via its representor? Doing so > >> should perfectly define the exact direction / traffic source. Isn't th= at > sufficient? > >> > > Per my view, there is matching field and matching value difference. > > Like IPv4 src_addr 1.1.1.1, 1.1.1.2. 1.1.1.3, will you treat it as same= or > different matching criteria? > > I would like to call them same since it can be summarized like > > 1.1.1.0/30 REPRESENTED_PORT is just another matching item, no essential > differences and it can't stand for direction info. >=20 > It looks like we're starting to run into disagreement here. > There's no "direction" at all. There's an embedded switch inside the NIC,= and > there're (logical) switch ports that packets enter the switch from. >=20 > When the user submits a "transfer" rule and does not provide neither > REPRESENTED_PORT nor PORT_REPRESENTOR in the pattern, the embedded > switch is supposed to match packets coming from ANY ports, be it VFs or > physical (wire) ports. >=20 > But when the user provides, in example, item REPRESENTED_PORT to point to > the physical (wire) port, the embedded switch knows exactly which port th= e > packets should enter it from. > In this case, it is supposed to match only packets coming from that physi= cal > port. And this should be sufficient. > This in fact replaces the need to know a "direction". > It's just an exact specification of packet's origin. >=20 There is traffic arriving or leaving the switch, so there is always directi= on, implicit or explicit.=20 For transfer rules, there is a concept transfer_proxy.=20 It takes the switch ownership; all switch rules should be configured via tr= ansfer_proxy. Image a logic switch with one PF and two VFs. PF is the transfer proxy and VF belongs to the PF logically.=20 When receiving traffic from PF, we can say it comes into the logic switch.= =20 When packet sent from VF (VF belongs to PF), so we can say traffic leaves t= he switch. =20 Item REPRESENTED_PORT indicates switch to match traffic sent from which por= t, comes into, or leave switch. We can say it as one kind of packet metadata. Like you said, DPDK always treat transfer to match any PORTs traffic.=20 When REPRESENTED_PORT is specified, the rules are limited to some dedicated= PORTs.=20 Other PORTs are ignored because metadata mismatching. Rules still have the capability to match ANY PORTS if metadata matched.=20 This update will allow user to cut the other PORTs matching capabilities. > > Port id depends on the attach sequence. >=20 > Unfortunately, this is hardly a good argument because flow rules are supp= osed > to be inserted based on the run-time packet learning. Attach sequence is = a > don't care here. >=20 > >> Also please mind that, although I appreciate your explanations here, > >> on the mailing list, they should finally be added to the commit > >> message, so that readers do not have to look for them elsewhere. > >> > > We have explained the high possibility of single-direction matching, ri= ght? >=20 > Not quite. As I said, it is not correct to assume any "direction", like i= n > geographical sense ("north", "south", etc.). Application has ethdevs, and= they > are representors of some "virtual ports" (in your terminology) belonging = to the > switch, for example, VFs, SFs or physical ports. >=20 > The user adds an appropriate item to the pattern (REPRESENTED_PORT), and > doing so specifies the packet path which it enters the switch. >=20 > > It' hard to list all the possibilities of traffic matching preferences. >=20 > And let's say more: one need never do this. That's exactly the reason why > DPDK has abandoned the concept of "direction" in *transfer* rules and > switched to the use of precise criteria (REPRESENTED_PORT, etc.). >=20 As far as I know, DPDK changes "transfer ingress" to "transfer", so it' mor= e clear that transfer can match both directions (both ingress and egress). REPRESENTED_PORT is the evolution of "port_id", I think, it' only one kind = of matching items. For large scale deployment like 10M rules, if we can save resources signifi= cantly by introducing direction, why not? Again, async API: 1. pattern template A 2. action template B 3. table C with pattern template A + action template B. 4. rule D, E, F... The specified REPRESENTED_PORT is provided in rules (D, E, F...) not patter= n template A or action template B or table C. Resources may be allocated early at step 3 since table' rule_nums property. > > The underlay is the one we have met for now. > >>> > >>>>> Introduce one new member transfer_mode into rte_flow_attr to > >>>>> indicate the flow table direction property: from wire, from vf or > >>>>> bi-direction(default). > >>>> > >>>> AFAIK, 'rte_flow_attr' serves both traditional flow rule insertion > >>>> and asynchronous (table) approach. The patch adds the attributes to > >>>> generic 'rte_flow_attr' but, for some reason, ignores non-table rule= s. > >>>> > >>>>> > >>> Sync API uses one rule to contain everything. It' hard for PMD to > >>> determine > >> if this rule has direction preference or not. > >>> Image a situation, just for an example: > >>> 1. Vport 1 VxLAN do decap send to vport 2. 1 million scale > >>> 2. Vport 0 (wire) VxLAN do decap send to vport 3. 1 hundred scale. > >>> 1 and 2 share the same matching conditions (eth / ipv4 / udp / vxlan > >>> /...), so > >> sync API consider them share matching determination logic. > >>> It means "2" have 1M scale capability too. Obviously, it wastes a > >>> lot of > >> resources. > >> > >> Strictly speaking, they do not share the same match pattern. > >> Your example clearly shows that, in (1), the pattern should request > >> packets coming from "vport 1" and, in (2), packets coming from "vport = 0". > >> > >> My point is simple: the "vport" from which packets enter the embedded > >> switch is ALSO a match criterion. If you accept this, you'll see: the > >> matching conditions differ. > >> > > See above. > > In this case, I think the matching fields are both "port_id + ipv4_vxla= n". They > are same. > > Only differs with values like vni 100 or 200 vice versa. >=20 > Not quite. Look closer: you use *different* port IDs for (1) and (2). > The value of "ethdev_id" field in item REPRESENTED_PORT differs. >=20 > >>> > >>> In async API, there is pattern_template introduced. We can mark "1" > >>> to use > >> pattern_tempate id 1 and "2" to use pattern_template 2. > >>> They will be separated from each other, don't share anymore. > >> > >> Consider an example. "Wire" is a physical port represented by PF0 > >> which, in turn, is attached to DPDK via ethdev 0. "VF" (vport?) is > >> attached to guest and is represented by a representor ethdev 1 in DPDK= . > >> > >> So, some rules (template 1) are needed to deliver packets from "wire" > >> to "VF" and also decapsulate them. And some rules (template 2) are > >> needed to deliver packets in the opposite direction, from "VF" > >> to "wire" and also encapsulate them. > >> > >> My question is, what prevents you from adding match item > >> REPRESENTED_PORT[ethdev_id=3D0] to the pattern template 1 and > >> REPRESENTED_PORT[ethdev_id=3D1] to the pattern template 2? > >> > >> As I said previously, if you insert such item before eth / ipv4 / etc > >> to your match pattern, doing so defines an *exact* direction / source. > >> > > Could you check the async API guidance? I think pattern template focusi= ng > on the matching field (mask). > > "REPRESENTED_PORT[ethdev_id=3D0] " and > "REPRESENTED_PORT[ethdev_id=3D1] "are the same. > > 1. pattern template: REPRESENTED_PORT mask 0xffff ... > > 2. action template: action1 / actions2. / 3. table create with > > pattern_template plus action template.. > > REPRESENTED_PORT[ethdev_id=3D0] will be rule1: rule create > REPRESENTED_PORT port_id is 0 / actions .... > > REPRESENTED_PORT[ethdev_id=3D1] will be rule2: rule create > REPRESENTED_PORT port_id is 1 / actions .... >=20 > OK, so, based on this explanation, it appears that you might be looking t= o refer > to: > a) a *set* of any physical (wire) ports > b) a *set* of any guest ports (VFs) >=20 Great, looks we are more and more closer to the agreement. > You chose to achieve this using an attribute, but: >=20 > 1) as I explained above, the use of term "direction" is wrong; > please hear me out: I'm not saying that your use case and > your optimisation is wrong: I'm saying that naming for it > is wrong: it has nothing to do with "direction"; >=20 Do you have any better naming proposal? > 2) while naming a *set* of wire ports as "wire_orig" might be OK, > sticking with term "vf_orig" for a *set* of guest ports is > clearly not, simply because the user may pass another PF > to a guest instead of passing a VF; in other words, > a better term is needed here; >=20 Like you said, vport may contain VF, SF etc. vport_orgin is on the logic sw= itch perspective. Any proposal is welcome. > 3) since it is possible to plug multiple NICs to a DPDK application, > even from different vendors, the user may end up having multiple > physical ports belonging to different physical NICs attached to > the application; if this is the case, then referring to a *set* > of wire ports using the new attribute is ambiguous in the > sense that it's unclear whether this applies only to > wire ports of some specific physical NIC or to the > physical ports of *all* NICs managed by the app; >=20 Not matter how many NICs has been probed by the DPDK, there is always switc= h/PF/VF/SF.. concept. Each switch must have an owner identified by transfer_proxy(). Vport (VF/SF= ) can't cross switch in normal case. The traffic comes from one NIC can't be offloaded by other NICs unless forw= arded by the application.=20 If user use new attribute to cut one side resource, I think user is smart e= nough to management the rules in different NICs. No default behavior changed with this update. > 4) adding an attribute instead of yet another pattern item type > is not quite good because PMDs need to be updated separately > to detect this attribute and throw an error if it's not > supported, whilst with a new item type, the PMDs do not > need to be updated =3D if a PMD sees an unsupported item > while traversing the item with switch () { case }, it > will anyway throw an error; > PMD also need to check if it supports new matching item or not, right? We can't assume NIC vendor' PMD implementation, right? > 5) as in (4), a new attribute is not good from documentation > standpoint; plase search for "represented_port =3D Y" in > documentation =3D this way, all supported items are > easily defined for various NIC vendors, but the > same isn't true for attributes =3D there is no > way to indicate supported attributes in doc. > > If points (1 - 5) make sense to you, then, if I may be so bold, I'd like = to suggest > that the idea of adding a new attribute be abandoned. Instead, I'd like t= o > suggest adding new items: >=20 > (the names are just sketch, for sure, it should be discussed) >=20 > ANY_PHY_PORTS { switch_domain_id } > =3D match packets entering the embedded switch from *whatever* > physical ports belonging to the given switch domain >=20 How many PHY_PORTS can one switch have, per your thought? Can I treat the P= HY_PORTS as the { switch_domain_id } owner as transfer_proxy()? > ANY_GUEST_PORTS { switch_domain_id } > =3D match packets entering the embedded switch from *whatever* > guest ports (VFs, PFs, etc.) belonging to the given > switch domain >=20 > The field "switch_domain_id" is required to tell one physical board / ven= dor > from another (as I explained in point (3)). > The application can query this parameter from ethdev's switch info: pleas= e see > "struct rte_eth_switch_info". >=20 > What's your opinion? >=20 How can we handle ANY_PHY_PORTS/ ANY_GUEST_PORTS ' relationship with REPRES= ENTED_PORT if conflicts? Need future tuning. Like I said before, offloaded rules can't cross different NIC vendor' "swi= tch_domain_id". If user probes multiple NICs in one application, application should take ca= re of packet forwarding.=20 Also application should be aware which ports belong to which NICs.=20 > > > >>> > >>>> For example, the diff below adds the attributes to "table" commands > >>>> in testpmd but does not add them to regular (non-table) commands > >>>> like "flow create". Why? > >>>> > >>>>> > >>> > >>> "table" command limits pattern_template to single direction or > >>> bidirection > >> per user specified attribute. > >> > >> As I say above, the same effect can be achieved by adding item > >> REPRESENTED_PORT to the corresponding pattern template. > > See above. > >> > >>> "rule" command must tight with one "table_id", so the rule will > >>> inherit the > >> "table" direction property, no need to specify again. > >> > >> You migh've misunderstood. I do not talk about "rule" command coupled > >> with some "table". What I talk about is regular, NON-async flow > >> insertion commands. > >> > >> Please take a look at section "/* Validate/create attributes. */" in > >> file "app/test-pmd/cmdline_flow.c". When one adds a new flow > >> attribute, they should reflect it the same way as VC_INGRESS, > VC_TRANSFER, etc. > >> > >> That's it. > > We don't intend to pass this to sync API. The above code example is for= sync > API. >=20 > So I understand. But there's one slight problem: in your patch, you add t= he new > attributes to the structure which is *shared* between sync and async use = case > scenarios. If one adds an attribute to this structure, they have to provi= de > accessors for it in all sync-related commands in testpmd, but your patch = does > not do that. >=20 Like the title said, "creating transfer table" is the ASYNC operation.=20 We have limited the scope of this patch. Sync API will be another story. Maybe we can add one more sentence to emphasize async API again. > In other words, it is wrong to assume that "struct rte_flow_attr" only ap= plies to > async approach. It had been introduced long before the async flow design = was > added to DPDK. That's it. >=20 > >> > >> But, as I say, I still believe that the new attributes aren't needed. > > I think we are not at the same page for now. Can we reach agreement on > > the same matching criteria first? > >>> > >>>>> It helps to save underlayer memory also on insertion rate. > >>>> > >>>> Which memory? Host memory? NIC memory? Term "underlayer" is > vague. > >>>> I suggest that the commit message be revised to first explain how > >>>> such memory is spent currently, then explain why this is not > >>>> optimal and, finally, which way the patch is supposed to improve > >>>> that. I.e. be more > >> specific. > >>>> > >>>>> > >>> > >>> For large scalable rules, HW (depends on implementation) always > >>> needs > >> memory to hold the rules' patterns and actions, either from NIC or fro= m > host. > >>> The memory footprint highly depends on "user rules' complexity", > >>> also diff > >> between NICs. > >>> ~50% memory saving is expected if one-direction is cut. > >> > >> Regardless of this talk, this explanation should probably be present > >> in the commit description. > >> > > This number may differ with different NICs or implementation. We can't = say > it for sure. >=20 > Not an exact number, of course, but a brief explanation of: > a) what is wrong / not optimal in the current design; Please check the commit log, transfer have the capability to match bi-direc= tion traffic no matter what ports. > b) how it is observed in customer deployments; Customer have the requirements to save resources and their offloaded rules = is direction aware. > c) why the proposed patch is a good solution. New attributes provide the way to remove one direction and save underlayer = resource. All of the above can be found in the commit log. >=20 > >>> > >>>>> By default, the transfer domain is bi-direction, and no behavior ch= anges. > >>>>> > >>>>> 1. Match wire origin only > >>>>> flow template_table 0 create group 0 priority 0 transfer wire_orig= ... > >>>>> 2. Match vf origin only > >>>>> flow template_table 0 create group 0 priority 0 transfer vf_orig..= . > >>>>> > >>>>> Signed-off-by: Rongwei Liu > >>>>> --- > >>>>> app/test-pmd/cmdline_flow.c | 26 ++++++++++++++++++= +++ > >>>>> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > >>>>> lib/ethdev/rte_flow.h | 9 ++++++- > >>>>> 3 files changed, 36 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/app/test-pmd/cmdline_flow.c > >>>>> b/app/test-pmd/cmdline_flow.c index 7f50028eb7..b25b595e82 100644 > >>>>> --- a/app/test-pmd/cmdline_flow.c > >>>>> +++ b/app/test-pmd/cmdline_flow.c > >>>>> @@ -177,6 +177,8 @@ enum index { > >>>>> TABLE_INGRESS, > >>>>> TABLE_EGRESS, > >>>>> TABLE_TRANSFER, > >>>>> + TABLE_TRANSFER_WIRE_ORIG, > >>>>> + TABLE_TRANSFER_VF_ORIG, > >>>>> TABLE_RULES_NUMBER, > >>>>> TABLE_PATTERN_TEMPLATE, > >>>>> TABLE_ACTIONS_TEMPLATE, > >>>>> @@ -1141,6 +1143,8 @@ static const enum index next_table_attr[] =3D= { > >>>>> TABLE_INGRESS, > >>>>> TABLE_EGRESS, > >>>>> TABLE_TRANSFER, > >>>>> + TABLE_TRANSFER_WIRE_ORIG, > >>>>> + TABLE_TRANSFER_VF_ORIG, > >>>>> TABLE_RULES_NUMBER, > >>>>> TABLE_PATTERN_TEMPLATE, > >>>>> TABLE_ACTIONS_TEMPLATE, > >>>>> @@ -2881,6 +2885,18 @@ static const struct token token_list[] =3D { > >>>>> .next =3D NEXT(next_table_attr), > >>>>> .call =3D parse_table, > >>>>> }, > >>>>> + [TABLE_TRANSFER_WIRE_ORIG] =3D { > >>>>> + .name =3D "wire_orig", > >>>>> + .help =3D "affect rule direction to transfer", > >>>> > >>>> This does not explain the "wire" aspect. It's too broad. > >>>> > >>>>> + .next =3D NEXT(next_table_attr), > >>>>> + .call =3D parse_table, > >>>>> + }, > >>>>> + [TABLE_TRANSFER_VF_ORIG] =3D { > >>>>> + .name =3D "vf_orig", > >>>>> + .help =3D "affect rule direction to transfer", > >>>> > >>>> This explanation simply duplicates such of the "wire_orig". > >>>> It does not explain the "vf" part. Should be more specific. > >>>> > >>>>> + .next =3D NEXT(next_table_attr), > >>>>> + .call =3D parse_table, > >>>>> + }, > >>>>> [TABLE_RULES_NUMBER] =3D { > >>>>> .name =3D "rules_number", > >>>>> .help =3D "number of rules in table", @@ -8894,6 > >>>>> +8910,16 @@ parse_table(struct context *ctx, const struct token > >>>>> +*token, > >>>>> case TABLE_TRANSFER: > >>>>> out->args.table.attr.flow_attr.transfer =3D 1; > >>>>> return len; > >>>>> + case TABLE_TRANSFER_WIRE_ORIG: > >>>>> + if (!out->args.table.attr.flow_attr.transfer) > >>>>> + return -1; > >>>>> + out->args.table.attr.flow_attr.transfer_mode =3D 1; > >>>>> + return len; > >>>>> + case TABLE_TRANSFER_VF_ORIG: > >>>>> + if (!out->args.table.attr.flow_attr.transfer) > >>>>> + return -1; > >>>>> + out->args.table.attr.flow_attr.transfer_mode =3D 2; > >>>>> + return len; > >>>>> default: > >>>>> return -1; > >>>>> } > >>>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > >>>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > >>>>> index 330e34427d..603b7988dd 100644 > >>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > >>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > >>>>> @@ -3332,7 +3332,8 @@ It is bound to > >>>> ``rte_flow_template_table_create()``:: > >>>>> > >>>>> flow template_table {port_id} create > >>>>> [table_id {id}] [group {group_id}] > >>>>> - [priority {level}] [ingress] [egress] [transfer] > >>>>> + [priority {level}] [ingress] [egress] > >>>>> + [transfer [vf_orig] [wire_orig]] > >>>> > >>>> Is it correct? Shouldn't it rather be [transfer] [vf_orig] > >>>> [wire_orig] ? > >>>> > >>>>> rules_number {number} > >>>>> pattern_template {pattern_template_id} > >>>>> actions_template {actions_template_id} diff --git > >>>>> a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index > >>>>> a79f1e7ef0..512b08d817 100644 > >>>>> --- a/lib/ethdev/rte_flow.h > >>>>> +++ b/lib/ethdev/rte_flow.h > >>>>> @@ -130,7 +130,14 @@ struct rte_flow_attr { > >>>>> * through a suitable port. @see rte_flow_pick_transfer_proxy= (). > >>>>> */ > >>>>> uint32_t transfer:1; > >>>>> - uint32_t reserved:29; /**< Reserved, must be zero. */ > >>>>> + /** > >>>>> + * 0 means bidirection, > >>>>> + * 0x1 origin uplink, > >>>> > >>>> What does "uplink" mean? It's too vague. Hardly a good term. >=20 > I believe this comment should be reworked, in case the idea of having an = extra > attribute persists. >=20 > >>>> > >>>>> + * 0x2 origin vport, > >>>> > >>>> What does "origin vport" mean? Hardly a good term as well. >=20 > I still believe this explanation is way too brief and needs to be reworke= d to > provide more details, to define the use case for the attribute more speci= fically. >=20 > >>>> > >>>>> + * N/A both set. > >>>> > >>>> What's this? >=20 > The question stands. >=20 > >>>> > >>>>> + */ > >>>>> + uint32_t transfer_mode:2; > >>>>> + uint32_t reserved:27; /**< Reserved, must be zero. */ > >>>>> }; > >>>>> > >>>>> /** > >>>>> -- > >>>>> 2.27.0 > >>>>> > >>>> > >>>> Since the attributes are added to generic 'struct rte_flow_attr', > >>>> non-table > >>>> (synchronous) flow rules are supposed to support them, too. If that > >>>> is indeed the case, then I'm afraid such proposal does not agree > >>>> with the existing items PORT_REPRESENTOR and REPRESENTED_PORT. > They > >>>> do exactly the same thing, but they are designed to be way more > >>>> generic. Why > >> not use them? > >> > >> The question stands. > >> > >>>> > >>>> Ivan > >>> > >> > >> Ivan > >