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 E6EF7A00C5; Thu, 15 Sep 2022 10:48:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 944644021D; Thu, 15 Sep 2022 10:48:57 +0200 (CEST) Received: from NAM11-BN8-obe.outbound.protection.outlook.com (mail-bn8nam11on2040.outbound.protection.outlook.com [40.107.236.40]) by mails.dpdk.org (Postfix) with ESMTP id 98A7140156 for ; Thu, 15 Sep 2022 10:48:56 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YmmxQG72eodpNvxgpW26wSI6Q9HPe9gum71bPiz5hnAwhORklcBbat2viRZaCIaaKsewyfxNfK1hvwksPhyTPPe7J/JuUB2VURlCSizLbgE7FXxLKNkWaYx1ke2K4ba08tsSUtyK6ybDT7TSr+miWUepUnaVS8ntUq6sojoPVfdLWxRqe6QiYQ2j5YDwPu/JfoIWWlNDv6boLOMeWOCbZ+C/oBGGz4a+JUB2S7hLRZGTO7y44MzSACYhrnosEJezONHeMhj3ZsmmFCo4Zn4Q3Qm03RplzgCn5tIBQ99HaZanJ1worZTFHTR0cQ9F6R2XCUxAZwrievZp57Ip0FTkPQ== 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=uM/tHlTT3u6b272YAZJSPaf9+dinF7dbAAHUGpkLM6s=; b=VYV2PU2Ns2D+UkuBpgWGeOYQSKKtl9LN+jL0r0msTyv4PIvBudE0ex3JoVqxD6jBipqtygNWS23xbpi6l7KzrIxMRATzY5hrD5ZAt5W6VWQvwpLdg79OE4fARIBRuKEdf7cgbVpXlYz9QnCFWJm0w8Z/OXYKIO/bY8DOhhIcKX6cVvdfEHAjvt+z9t2WhbCy06bbTJToTraMItnGyvgOvHyWNN3VCbfVdRsRjzFTEPBskb5j6QarjRwjUXMRaJ1pWt8UCRSG/phRNMxKa/D2kmcyQ+W9Dh9yQDkYTZAp70pH/Fvc2Us4WWiaoD82Wr6Bxs4nY+ELQV+KjowjjVOFew== 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=uM/tHlTT3u6b272YAZJSPaf9+dinF7dbAAHUGpkLM6s=; b=k5L03p5IaQgOX/IZzV2rimCCxElwrCCdKHlLuBNNbyd8V+Cd2hd6DuXM7uVXJzGsJkG9vzNVBmroU0DSs8pplbHQ6X6h8uv1+JO4BfjBDyDU2t7RpNx09VR1j7Uz3Gca5InXKNyq4LXPIS/jqO6Ihd7gTwjYJfHQqvn0GM9dsfAREgYP73v2NIEMDoJyIcBrE9MOtXOAk/RwsDXN8144nB9PlVaAyNqBzL5MAWozxwsQpzfJHoG8hIsF1XspqUXnOwdHrV2mNwVyrTKqz/HjVHTn/RSTdHV3bePrE8qYsNSrd/tZr39ZXPmjy2xcksvNMdguA50oHMfF8Hf2uUiHvw== Received: from BN9PR12MB5273.namprd12.prod.outlook.com (2603:10b6:408:11e::22) by DM6PR12MB4074.namprd12.prod.outlook.com (2603:10b6:5:218::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5632.14; Thu, 15 Sep 2022 08:48:53 +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; Thu, 15 Sep 2022 08:48:53 +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/RnGnq3cDLKAgAFR3ACAABgfgIAA1YvQgABHMQCAABGTUIAAcJcAgACeIvCAAHY1AIAABrng Date: Thu, 15 Sep 2022 08:48:53 +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> <6164993a-ba4e-c1ea-aaf5-5cc7c35d3724@oktetlabs.ru> In-Reply-To: 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_|DM6PR12MB4074:EE_ x-ms-office365-filtering-correlation-id: 520b7b03-6016-4d06-7bd8-08da96f71e47 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: oUOhr1+emQVOkKGqzTtnu2XlKXvkYElia1ZLEwza08lTGFdX2N7TJrT3Cm+nlgkM+Aqbnu5IEtnIoxOJlvdpy9ir6S+p1WM10PlAcqimiMYUdNMtm3gFKpbJTFB16hK5xx1m8lVpIEcToPrdZqjXEt/7FARnaDiC/RM9kAS851Zq0SkmAmFeN8Pu6y4b0WHV4iZ/W7aruCYA5J7yJtEU6LBRLto5lmyBdDtu/wEz7Fj+3+fnHn8HAwaf9wNWDZhrKkeS0ETys4uAq+ZgYQpPXarXKjX89I213gYrRN/EkfnM9di04Cs5qBZg0Af9T48OMec61gStWx8fYz3n2Jyfsxugu+XazveOrpLDnt2bcr7pVOddtJ4Scit7w1Xw1keBaoOHvvzQow4I7QdeJuXq2zulrQ0SMsGXsnEDdt43hdwfzlUo1u6E6ehs4EYksSI+4XrYRcaxqYMPcA4VfJTixKcvm3jvxXyVxMGtLPn6/Xz2FDGuyxksLxUj4vBt24YrANFUTtsYkoLVxzToSvaezmFwcaeoW+mOkuirS0iSYzV5de7IgrO5IjPoqw6P7R5a7E8AxQFEfyq++Qn5oMFP5SsBZ6avxgYsmpUituA0nRPNsQdMLq7K6L8dFjlgrDba/EuL7GdfVeFjNlTpg8EabrWNfPjnC8BWxrDSUpEmpvE8cVLS5qC5fbrnuKTJPqKNPIvm9Q1nkccLZNHZVCgsr2myd9iOkszCZUPcObN1C1wE/Gh/z+lyjSUS3zEHNV1Jz+VmRAARZSb/GhMvdm0/7g== 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)(376002)(39860400002)(366004)(396003)(136003)(346002)(451199015)(66574015)(55016003)(30864003)(186003)(38100700002)(122000001)(83380400001)(316002)(64756008)(5660300002)(66446008)(52536014)(66556008)(76116006)(8936002)(8676002)(4326008)(41300700001)(107886003)(86362001)(2906002)(478600001)(9686003)(38070700005)(26005)(6506007)(7696005)(66946007)(53546011)(71200400001)(66476007)(33656002)(54906003)(6916009)(579004)(559001); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ESpDeiiQsxUs5WMZekhxPlNjcfaPJ4wWt2oybb5U2x0gFWPTVMpL0wvga6d4?= =?us-ascii?Q?7cl6tEu3Z+45NDJPc+hEtYNp/N65rAFgzTgfBR/TMr47rNq4r5inB/NXEQ0r?= =?us-ascii?Q?w4LVksYIRnr5IFFj/w71eUBwFFZrOquAJETtyt07rHfKTcD+yTCATJZY3ZQ3?= =?us-ascii?Q?KXd1c0Q+oF78mDN6+PAoMSrttj0lQy18zhq1aUk1EAZB+wIBV0CmdqaaLeJu?= =?us-ascii?Q?m8nOA1TLdTSZVNB36or4g/ws5d2fcqC6nqaq7lGU2gwL0F1rM+stz/js0KF1?= =?us-ascii?Q?S6T2Q6ovsw5WH/W5z1wpgJ8U82e+MNJL+0qp/K6TsymfrhRXtKey0hFdrglf?= =?us-ascii?Q?3XJVWoD3dLkKcBcKQCf63/6U8xiXuyPoyGI/q+agT5Q46+T1d6kdmvmpxs/z?= =?us-ascii?Q?1lf923Fkrw3xwaBdmPGQbOpbn5Fzy/BCYu4jKVSCKDwMmvVVYrrtIS+uWhzv?= =?us-ascii?Q?rHtIOr/Igt1NwudfNparPklRc22l7KzDw7wPbgtOeLCD1ZeEKEwsXrzXnCWV?= =?us-ascii?Q?kqG5/iT+ad/jUuqCOBKITQ4mRa0BLQC1dcGuz1bq9KA01pyw8viGqYT62nss?= =?us-ascii?Q?AxUweEIChxQuvzhc0WAo4BKFnwz8UBEZIRp6SMxv58YRWkTDXUrakmH6gxc1?= =?us-ascii?Q?Th3rlnExSWwiERjJkSYBiMzjySWwAtgXsysliKB9CkzYyGyk8tW1FtSHeoIz?= =?us-ascii?Q?UitZ+cG0Jz5rLVrIjtA1vaWCobjzjThW5Nwm39MJFLhQeN4W5iddMxfdloJu?= =?us-ascii?Q?uTh3ctT3qg3kcK14Ga7SZyhUVlQPHxRVbTJRR1LOA0R3w6Eg3+my0L18/g5s?= =?us-ascii?Q?h2GgS9Pfl8WGJ6MvactyV26A3ju2gwJA1yo54F2AfCb3UjTOve5LJq17a07a?= =?us-ascii?Q?FaoxjvTc48g4r+khR6ff3Rz2pvfgFvO9iMJM8Og2umZDhfhL0RX8Rp397m2J?= =?us-ascii?Q?DKlSf7g1apaFcGcV1ju3/reBY/kVj0yQO8ymdCbLj1KxqtkaLcv+6lSfx5Lw?= =?us-ascii?Q?gLpBw5Ov0+y+Ugxcb+9cxQTZhOIEyaJvV8FSuxCU+q4QbbNURSr5M9kQhEcQ?= =?us-ascii?Q?pWSMEcsaOdUlNfhLGrp16hHpNFzXMjURpOQS0PwHcSieK1Sf67KMbBu7j0ZH?= =?us-ascii?Q?soOgXUDoj68HmylpJw++dIhNUVn5Nco/zKyn1h9U3w5ae8chwd8f4ICyJ2O1?= =?us-ascii?Q?Z7muRhpkiR9gN+CMV+lqsChxE9zLXVcDzZWzGBXj3yVU07eURMSxKurnGEpT?= =?us-ascii?Q?I2cUqxiuqrVA/7pSL9dS5e198kPZ5O6CgLL0q7i3ummuJky5TSHXflAnHt0h?= =?us-ascii?Q?VV3TtVj5ubKcdkQhhz8lp9qDptM+HulbYvi4MXcqy63/NvzzbTIx/mAOenjZ?= =?us-ascii?Q?APg5sivUQJf42h+h0dSWwNouY5dT1Ys4dDoJu0zYfSG0Fy4CCdirlXtVkTCP?= =?us-ascii?Q?IqzOulcZ119rWafZ39tJnptc/sGk0tYsy7eHC4H/Su/RWlg0xjWIiCDvZLhg?= =?us-ascii?Q?vuAK7hgQrAJs1CzSaUec9Lxd9Ezgx0qPftS3Z0TPZcF0cFRITCbsbreOXI24?= =?us-ascii?Q?4/Dea09wbDiV8KUs1KigcTiKNHi++OL9mucsSGc3?= 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: 520b7b03-6016-4d06-7bd8-08da96f71e47 X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Sep 2022 08:48:53.5279 (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: VqeO732nPxl8LfJ7Wfbd86Tqs5NEgFo0QjlW8VPjPtq4119Qo2LjrF7r9Bb/hnj7V4e0x+clHtady1i70Diqsw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4074 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 Ivan: BR Rongwei -----Original Message----- From: Ivan Malov =20 Sent: Thursday, September 15, 2022 15:47 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 transf= er table External email: Use caution opening links or attachments Hi Rongwei, On Thu, 15 Sep 2022, Rongwei Liu wrote: > HI Ivan: > > BR > Rongwei > >> -----Original Message----- >> From: Ivan Malov >> Sent: Wednesday, September 14, 2022 23:18 >> 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 tra= nsfer >> table >> >> External email: Use caution opening links or attachments >> >> >> Hi Rongwei, >> >> On Wed, 14 Sep 2022, Rongwei Liu wrote: >> >>> 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 >>>> transfer table >>>> >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> Hi, >>>> >>>> On Wed, 14 Sep 2022, Rongwei Liu wrote: >>>> >>>>> 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 >>>>>>> specific >>>> 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? >>>> >>>> ^^ >>>> >>>> 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 s= outh >> band per you concept. >> >> Transfer proxy has nothing to do with physical ports. And I should stres= s out >> that "south band" and the likes are NOT my concepts. Instead, I think th= at >> direction designations like "south" or "north" aren't applicable when ta= lking >> about the embedded switch and its flow (transfer) rules. >> >>> Traffic from vport (not transfer_proxy) or north band per your concept = won't >> hit even if same packets. >> >> Please see above. Transfer proxy is a completely different concept. >> And I never used "north band" concept. >> >>>> >>>> -- >>>> >>>>>> >>>>>> 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. >>>> >>>> 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 next time. >>> Ack. Will add VF as an example. >>>> >>>>>> 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 >>>>>> that >>>> 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. >>>> >>>> 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. >>>> >>>> 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. >>>> >>>> But when the user provides, in example, item REPRESENTED_PORT to >>>> point to the physical (wire) port, the embedded switch knows exactly >>>> which port the packets should enter it from. >>>> In this case, it is supposed to match only packets coming from that >>>> physical 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. >>>> >>> There is traffic arriving or leaving the switch, so there is always dir= ection, >> implicit or explicit. >> >> This does not contradict my thoughts above. "Direction" is *defined* by = two >> points (like in geometry): an initial point (the switch port through whi= ch a >> packet enters the switch) and the terminal point (the match engine insid= e the >> switch). If one knows these two points, no extra hints are required to s= pecify >> some "direction". Because direction is already represented by this "vect= or" of >> sorts. That's why presence of the port match item in the pattern is abso= lutely >> sufficient. > Good to see this. Thank for the information. You're very welcome. > This update leverages the concept exactly defined by you: "an initial poi= nt (the switch port through which a > packet enters the switch)" No, it doesn't seem so. Based on your explanations, it appears that this update tries to refer to a "super set" of ports which have something in common. For example, with attribute "wire_orig" you seem to be trying to request that the rule match packets arriving from wire through ANY of the phys.ports. So my point is: why express an obvious match item as an attrbiute? Let me explain more based on your point and sentences: "Direction" is *defined* by two points (like in geometry): an initial point= (the switch port through which a packet enters the switch) and the terminal point (the match engine inside t= he switch). Wire_orig: an initial port is from uplink or internet wire, terminal port i= s switch (physical or logic), switch will handle the packets eventually=20 Vport_orig: an initial port is from virtual terminal port is switch (physic= al or logic) Looks they match perfectly. I think you have some misunderstanding on matching item, it should contain = matching filed(rte_item->mask), matching value(rte_item->spec) What you proposed "ANY_GUEST_PORT/ANY_PHY_PORT" seemed to mix both together= . ANY_GUEST_PORT: port belongs to specific switch domain and it's virtual ANY_PHY_PORT: port belongs to specific switch domain and it's physical. For example, nobody tries to replace match item IPv4 with an attribute "is_ipv4". That would be strange, to say the least. Why should the "vf_orig" case be an exception then? It's a good point. There is already "port_id/represented_port", why do you = want to add "IS_***_PORTS" matching item? Like IPv4, it matches rx_only, tx_only, rx_tx for INGRESS EGRESS TRANSFER d= omain, eventually it will follow domain principle. Matching item should be generic. It stands for what the users care and what= they want.=20 "vf_orig"/"wire_orig" is resource sensitive and beyond matching items and m= atching item should follow it always. By using this, there is no possibility to match the cut-off path. It' very = advance feature > If you think direction not good, we can change to other words like "initi= al port"/"origin port" etc. As I explained multiple times, "direction" is rather obscure from the viewpoint located inside the embedded switch. Yes, on non-transfer (VNIC) level, there are *exactly* two directions: ingress and egress. But, inside of the embedded switch (transfer rules), there can be *multiple* various "directions", which are not even directions, =3D they're traffic PATHs in fact. Renaming to "intitial port" and "origin port" won't be helpful either because, for users, it will be hard to figure out the difference between the attribute and items PORT_REPRESENTOR / REPRESENTED_PORT. If, however, you add new items instead of the attribute, the user will likely see that the new items and the existing ones are just alternative options =3D representor-based items help to address exact ports (one rule - one port), whilst your new items help to address super sets of ports like "all wire ports" or "all guest ports". You forgot rte_item->mask here.=20 So, the short of it: 1) these "wire_orig" / "vf_orig" are in fact yet another match criteria; 2) because of that, they should go to match items and not to attributes. >> >> However, based on your later explanations, the use of precise port item = is >> simply inconvenient in your use case because you are trying to match tra= ffic >> from *multiple* ports that have something in common (i.e. all VFs or all= wire >> ports). >> >> And, instead of adding a new item type which would serve exactly your ne= eds, >> you for some reason try to add an attribute, which has multiple drawback= s >> which I described in my previous letter. >> >>> For transfer rules, there is a concept transfer_proxy. >>> It takes the switch ownership; all switch rules should be configured vi= a >> transfer_proxy. >> >> Yes, such concept exists, but it's a don't care with regard to the probl= em that >> we're discussing, sorry. >> Furthermore, unlike "switch domain ID" (which is the same for all ethdev= s >> belonging to a given physical NIC board), nobody guarantees that it's on= ly one >> transfer proxy port. Some NIC vendors allows transfer rules to be added = via >> any ethdev port. >> > Does any flow rule leverage switchid already. Is it too obscure for end-u= ser? No, I'm not saying about flow rules. I'm explaining the logic which application may use to identify which ethdevs are on which NICs. Imagine a DPDK application which has two ethdevs instantiated: one ethdev sits on top of the admin. PF (ethdev 0), the other one sits on top of a low-privilege PF (ethdev 1). In the latter case, it can also be a VF. Both ethdev 0 and ethdev 1 belong to the same physical NIC board. Now, what I'm trying to explain is the fact that "proxy" behaviour may differ between various vendors: - some vendors say that they can support managing "transfer" rules via any PFs / VFs. They do not require that some specific PF ethdev be used to do that. With such vendors, if the application makes a query "What's the proxy port ID for the ethdev 1?", it will get "The proxy port ID for ethdev 1 is 1" response. - but other vendors cannot support the above workflow and they require that "transfer" rules be managed using some specific (admin) ethdev. If the application makes the same query here, it will get the following response: "The proxy port ID for ethdev 1 is 0". So, given these explanations, it is incorrect to assume that the proxy port ID for all ethdevs belonging to the same NIC board will be the same. They simply may not be like this. However, *regardless* of the two above scenarious and regardless of vendor, for NICs which have embedded switch feature, when the user tries to check the "switch domain ID" for ethdev 0 and ethdev 1, they will get the same value. So, this should be the right criterion for the application (not for flow rules themselves) to decide which ethdev belongs to which physical NIC board. Why you said user is good to check switch domain id and know port belonging= s. But not good to know basic dpdk rte_flow api usage? There are too many assumptions. Using VF as example, they are different from beginnings, see sriov commands= : echo $num > /sysfs/ .... /PF_BDF/sriov_num echo VF_BDF > /sysfs/.../bind or unbind >>> >>> Image a logic switch with one PF and two VFs. >>> PF is the transfer proxy and VF belongs to the PF logically. >>> When receiving traffic from PF, we can say it comes into the logic swit= ch. >> >> That's correct. >> >>> When packet sent from VF (VF belongs to PF), so we can say traffic leav= es >> the switch. >> >> That's not correct. Traffic sent from VF (for example, a guest VM is sen= ding >> packets) also *enters* the switch. PFs and VFs are in fact *separate* lo= gical >> ports of the embedded switch. >> >>> >>> Item REPRESENTED_PORT indicates switch to match traffic sent from which >> port, comes into, or leave switch. >> >> That is not correct either. Item REPRESENTED_PORT tells the switch to ma= tch >> packets which come into the switch FROM the logical port which is >> represented by the given DPDK ethdev. >> >> For example, if ethdev=3D"E" is the *main* PF which is bound to physical= port "P", >> then item REPRESENTED_PORT with ethdev ID being set to "E" tells the swi= tch >> that only packet coming to NIC from *wire* via physical port "E" should = match. >> >>> We can say it as one kind of packet metadata. >> >> Kind of yes, but might be vendor-specific. No need to delve into this. >> >>> Like you said, DPDK always treat transfer to match any PORTs traffic. >> >> Slight correction: it treats it this way until it sees an exact port ite= m. >> If the user provides REPRESENTED_PORT (or PORT_REPRESENTOR), it's no >> longer *any* ports traffic, it's an exact port traffic. That's it. >> >>> When REPRESENTED_PORT is specified, the rules are limited to some >> dedicated PORTs. >> >> These rules match only packets arriving TO the embedded switch FROM the >> said dedicated ports. >> >>> Other PORTs are ignored because metadata mismatching. >> >> Kind of yes, correct. >> >>> Rules still have the capability to match ANY PORTS if metadata matched. >> >> This statement is only correct for the cases when the user does NOT use >> neither item REPRESENTED_PORT nor item PORT_REPRESENTOR. >> >>> >>> This update will allow user to cut the other PORTs matching capabilitie= s. >> >> As I explained, this is exactly what items PORT_REPRESENTOR and >> REPRESENTED_PORT do. No need to have an extra attribute. >> >> If the user adds item REPRESENTED_PORT with ethdev_id=3D"E", like in the >> above example, to match packets entering NIC via the physical port "P", = then >> this rule will NOT match packets entering NIC from other points. For exa= mple, >> packets transmitted by a virtual machine via a VF will not match in this= case. >> >>>>> Port id depends on the attach sequence. >>>> >>>> Unfortunately, this is hardly a good argument because flow rules are >>>> supposed to be inserted based on the run-time packet learning. Attach >>>> sequence is a don't care here. >>>> >>>>>> 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 elsewhe= re. >>>>>> >>>>> We have explained the high possibility of single-direction matching, = right? >>>> >>>> Not quite. As I said, it is not correct to assume any "direction", >>>> like in 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 physica= l >> ports. >>>> >>>> The user adds an appropriate item to the pattern (REPRESENTED_PORT), >>>> and doing so specifies the packet path which it enters the switch. >>>> >>>>> It' hard to list all the possibilities of traffic matching preference= s. >>>> >>>> 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.). >>>> >>> As far as I know, DPDK changes "transfer ingress" to "transfer", so it'= more >> clear that transfer can match both directions (both ingress and egress). >> >> Not quite. DPDK has abandoned the use of "ingress / egress" in "transfer= " >> rules because "ingress" and "egress" are only applicable on the VNIC lev= el. For >> example, there is a PF attached to DPDK application: >> packets that the application receives through this ethdev, are ingress, = and >> packets that it transmits (tx_burst) are egress. >> >> I can explain in other words. Imagine yourself standing *inside* a room = which >> only has one door. When someone enters the room, it's "ingress", when >> someone leaves, it's "egress". It's relative to your viewpoint. >> In this example, such a room represents a VNIC / ethdev. >> >> And now imagine yourself standing *outside* of another room / auditorium >> which has multiple doors / exits. You're standing near some particular e= xit "A" >> (VNIC / ethdev), but people may enter this room via another door "B" and= then >> leave it via yet another door "C". In this case, from your viewpoint, th= is traffic >> cannot be considered neither ingress nor egress. Because these people do= not >> approach you. >> >> Like in this example, embedded switch is like a large auditorium with ma= ny- >> many doors / exits. And there can be many-many >> directions: packet can enter the switch via phys. port "P1" >> and then leave it via another phys. port "P2". Or it can enter the switc= h via >> phys. port and the leave it via VF's logical port (to be delivered to a = guest >> machine), or a packet can travel from one VF to another one. >> >> There's no PRE-DEFINED direction like "north to south" or "east to west"= . >> And this explains why it's very undesirable to use term "direction". >> >>> REPRESENTED_PORT is the evolution of "port_id", I think, it' only one k= ind of >> matching items. >> >> Yes. But nobody prevents you from defining yet another match item which = will >> be able to refer to a *group* of ports which have something in common (i= .e. >> "all guest ports of this switch" >> pointing to all logical ports currently attached to virtual machines / g= uests, or >> "all wire ports of this swtich"). >> >>> >>> For large scale deployment like 10M rules, if we can save resources >> significantly by introducing direction, why not? >> >> I do not deny the fact that you have a use case where resources can be s= aved >> significantly if you give the PMD some extra knowledge when creating a f= low >> table / pattern template. That's totally OK. What I object is the very >> implementation and the use of term "direction". If you add new item type= s >> (like above), then, when you create an async table 1 pattern template, y= ou will >> have item ANY_WIRE_PORTS, and, for table 2 pattern template, you'll have >> item ANY_GUEST_PORTS. >> As you see, the two pattern templates now differ because the match crite= ria >> use different items. >> >>> >>> 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 pa= ttern >> template A or action template B or table C. >>> Resources may be allocated early at step 3 since table' rule_nums prope= rty. >> >> No, item REPRESENTED_PORT *can* be provided inside pattern template A, >> but, as you pointed out earlier, the problem is that you can't distingui= sh >> different pattern templates which have this item, because pattern templa= tes >> know nothing about *exact* port IDs and only know item MASKS. Yes, I agr= ee >> that in your case such problem exists, but, as I say above, it can be so= lved by >> adding new item types: one for referring to all phys. ports of a given N= IC and >> another one for pointing to a group of current guest users (VFs). >> >>>>> 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, ignore= s non- >> table rules. >>>>>>>> >>>>>>>>> >>>>>>> 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 "vpor= t 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_vxlan". They >>>> are same. >>>>> Only differs with values like vni 100 or 200 vice versa. >>>> >>>> Not quite. Look closer: you use *different* port IDs for (1) and (2). >>>> The value of "ethdev_id" field in item REPRESENTED_PORT differs. >>>> >>>>>>> >>>>>>> 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 DP= DK. >>>>>> >>>>>> 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 / s= ource. >>>>>> >>>>> Could you check the async API guidance? I think pattern template >>>>> focusing >>>> 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 .... >>>> >>>> OK, so, based on this explanation, it appears that you might be >>>> looking to refer >>>> to: >>>> a) a *set* of any physical (wire) ports >>>> b) a *set* of any guest ports (VFs) >>>> >>> Great, looks we are more and more closer to the agreement. >> >> Looks so. >> >>>> You chose to achieve this using an attribute, but: >>>> >>>> 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"; >>>> >>> Do you have any better naming proposal? >> >> As I said, what you are trying to achieve using a new attribute would be= way >> better to achieve using new pattern items which can be easily told one f= rom >> another in PMD when pre-allocaing resources for different async flow tab= les. >> >> So, I don't have any proposal for *attribute* naming. >> What I propose is to consider new items instead. >> >>>> 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; >>>> >>> Like you said, vport may contain VF, SF etc. vport_orgin is on the logi= c switch >> perspective. >>> Any proposal is welcome. >> >> The problem is, vport can be easily confused with a slightly more generi= c >> "lport" (embedded switch's "logical port"), and, logical ports, in turn,= are not >> confined to just VFs or PFs. For example, physical (wire) ports are ALSO= logical >> ports of the switch. >> >>>> 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; >>>> >>> Not matter how many NICs has been probed by the DPDK, there is always >> switch/PF/VF/SF.. concept. >> >> Correct. >> >>> Each switch must have an owner identified by transfer_proxy(). Vport (V= F/SF) >> can't cross switch in normal case. >> >> No. That is not correct. This is tricky, but please hear me out: an indi= vidual NIC >> board (that is, a given *switch*) is identified only by its switch domai= n ID. As I >> explained above, "transfer proxy" is just a technical hint for the applc= ation to >> indicate an ethdev through which "transfer" rules must be managed. Not a= ll >> vendors support this concept (and they are not obliged to support it). >> >>> The traffic comes from one NIC can't be offloaded by other NICs unless >> forwarded by the application. >> >> Right, but forwarding in software (inside DPDK application) is out of sc= ope with >> regard to the problem that we're discussing. >> >>> If user use new attribute to cut one side resource, I think user is sma= rt >> enough to management the rules in different NICs. >> >> As I explained above, I do not deny the existence of the problem that yo= ur >> patch is trying to solve. Now it looks like we're on the same page with = regard >> to understanding the fact that what you're trying to do is to introduce = a match >> criterion that would refer to a GROUP of similar ports. In my opinion, t= his is >> not an *attribute*, it's a *match criterion*, and it should be implement= ed as >> two new items. >> >> Having two different item types would perfectly fit the need to know the >> difference between such "directions" (as per your terminology) early eno= ugh, >> when parsing templates. >> >>> 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? >> >> No-no-no. Imagine a PMD which does not support "transfer" rules. >> In such PMD, in the flow parsing function one would have: >> >> if (!!attr->transfer) { >> print_error("Transfer is not supported"); >> return EINVAL; >> } >> >> If you add a new attribute, then PMDs which are NOT going to support it = need >> to be updated to add similar check. >> Otherwise, they will simply ignore presence / absence of the attribute i= n the >> rule, and validation result will be unreliable. >> >> Yes, if this attribute is 0x0, then indeed behaviour does nto change. Bu= t what if >> it's 0x1 or 0x2? >> PMDs that do not support these values must somehow reject such rules on >> parsing. >> >> However, this problem does not manifest itself when parsing items. Typia= lly, in >> a PMD, one would have: >> >> switch (item->type) { >> case RTE_FLOW_ITEM_TYPE_VOID: >> break; >> >> case RTE_FLOW_ITEM_TYPE_ETH: >> /* blah-blah-blah */ >> break; >> >> default: >> return ENOTSUP; >> } > Are you assuming all PMDs will be implemented in the upper style? One may take a look at the existing PMDs. It's open source after all. When one has an array of items of unknown count which is END-terminated, then, obviously, the PMD has to traverse it one way or another. If it stubles upon an unknown item, it will have nothing to do but to throw an error. > This new field targets async API which was added recently. No impact on s= ync API. Rongwei, I see your point. The problem with it, however, is that even if you describe it in comments, the code won't prevent non-sync API from seeing this attribute in "struct rte_flow_attr". As I say, "struct rte_flow_attr" has been here for ages. When one adds a flow rule in a sync way, they fill out the very same structure. And the user may set this new argument to non-zero by mistake. Yes, you may argue that the app developer should be smart enough to read your comment before the struct member which says that this field is for a-sync only. Right. But that's not the only scenario. The field may become non-zero because of some other mistake in the program which, for example, leads to the struct memory being corrupted in one way or another. That's why the PMD has to validate flow rules... I am very confusing.=20 If the memory is corrupted or set mistakenly, we should fix it. Memory corruption will lead unpredictable mistake especially under multi-th= read. So, the PMD must detect this inconsistency somehow and throw an error. With your approach (attribute), the PMDs have to be updated to have these checks. With the item approach that I suggest, updating the PMDs is obviously not needed. Am I missing something? Let's discuss. =20 I am afraid pmd still needs to check pattern conflicts between "IS_ANY_***P= ORTS" with "port_id"/"represented_port" to avoid conflicts. May I know if the attributes fully occupied at your side for some special p= urpose? =20 > I don't predict any effort on the existing PMD behavior. I see your point. But how is this expressed in code? As I explain above, consistency checks are what flow validate API is for. New argument means new checks. That's it. Like commit log and what I mentioned multiple time. If user choose to use advanced feature, they should read manual carefully a= nd take the responsibility. > But agree with you: we should emphasize it' only for async mode. It's better to express this in code. So that the problem (if any) can be detected programmatically and not just from reading comments. >From my point of view, the easiest way to have this done is to add items instead of attributes, =3D no need to update PMDs. We have readme, code snippet already.=20 Even if user set the attribute in the sync API, nothing should happen since= no underlayer support.(still behave like current TRANSFER domain) Unless PMD has bug, but it is always good to fix bugs, right? > >> >> So, if you introduce two new item types to solve your problem, then you = won't >> have to update existing PMDs. If the vendor wants to support the new ite= ms >> (say, MLX or SFC), they'll update their code to accept the items. But ot= her >> vendors will not do anything. If the user tries to pass such an item to = a vendor >> which doesn't support the feature, the "default" case will just throw an= error. >> >> This is what I mean when pointing out such difference between adding an >> attribute VS adding new item types. >> >>>> 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 to suggest adding new items: >>>> >>>> (the names are just sketch, for sure, it should be discussed) >>>> >>>> ANY_PHY_PORTS { switch_domain_id } >>>> =3D match packets entering the embedded switch from *whatever* >>>> physical ports belonging to the given switch domain >>>> >>> How many PHY_PORTS can one switch have, per your thought? Can I treat >> the PHY_PORTS as the { switch_domain_id } owner as transfer_proxy()? >> >> A single physical NIC board is supposed to have a single embedded switch >> engine. Hence, if the NIC board has, in example, two or four physical po= rts, >> these will be the physical ports of the switch. That's it. >> >> As for the transfer proxy, please see my explanations above. >> It's not *always* reliable to tell whether two given ethdevs belong to t= he same >> physical NIC board or not. >> >> Switch domain ID is the right criterion (for applications). >> >>>> 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 >>>> >>>> The field "switch_domain_id" is required to tell one physical board / >>>> vendor from another (as I explained in point (3)). >>>> The application can query this parameter from ethdev's switch info: >>>> please see "struct rte_eth_switch_info". >>>> >>>> What's your opinion? >>>> >>> How can we handle ANY_PHY_PORTS/ ANY_GUEST_PORTS ' relationship >> with REPRESENTED_PORT if conflicts? >>> Need future tuning. >> >> And if you carry on with "vf_orig" / "wire_orig" approach, you will inev= itably >> have the very same problem: possible conflict with items like >> REPRESENTED_PORT. So does it matter? Yes, checks need to be done by PMDs >> when parsing patterns. >> >>> Like I said before, offloaded rules can't cross different NIC vendor' >> "switch_domain_id". >>> If user probes multiple NICs in one application, application should tak= e care >> of packet forwarding. >>> Also application should be aware which ports belong to which NICs. >> >> Yes, perhaps, domain ID is not needed in the new items. >> But the application still must keep track of switch domain IDs itself so= it knows >> which rules to manage via which ethdevs. >> >> Any other opinions? > ANY_PHY_PORTS/ ANY_GUEST_PORTS looks like a super set of ports. So does the new attribute, doesn't it? > This will come another challenge: "why can't we use REPRESENTED_PORT wit= h mask" or "combine several REPRESENTED_PORT together"? This problem has been here for many other items, including now deprecated items PF, VF and PHY_PORT. Yes, theoretically, when the PMD looks through the pattern, it has to check that its items do not overlap / contradict. That's kind of OK, isn't it? The PMD has to check things after all... For example, no one prevents user from submitting a pattern with several adjacent items ETH in it. The PMD is supposed to turn such request down. >> >>>>> >>>>>>> >>>>>>>> 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. >>>> >>>> So I understand. But there's one slight problem: in your patch, you >>>> add the 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 provide accessors for it in all sync-related >>>> commands in testpmd, but your patch does not do that. >>>> >>> Like the title said, "creating transfer table" is the ASYNC operation. >>> 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. >> >> No-no-no. There might be slight misunderstanding. I understand that you = are >> limiting the scope of your patch by saying this and this. >> That's OK. What I'm trying to point out is the fact that your patch neve= rtheless >> touches the COMMON part of the flow API which is shared between two >> approaches (sync and async). > Yeah, you are right, we should emphasize it for async API not sync in the= code and comments. >> >> Imagine a reader that does not know anything about the async approach. >> He just opens the file in vim and goes directly to struct rte_flow_attr. >> And, over there, he sees the new attribute "wire_orig". He then immediat= ely >> assumes that these attributes can be used in testpmd. Now the reader ope= ns >> testpmd and tries to insert a flow rule using the sync approach: >> >> flow create priority 0 transfer vf_orig pattern / ... / end actions drop >> > > This is wrong statement. > If user has no idea with cmdline usage, he should rely on "tab indication= ' not something by guessing. > > The command prefix "flow" bifurcated now to sync and async now, user may = use any keyword combinations. > He will get "argument error" if it's not good unless he knows what' he is= doing. > Again: we should emphasize it's only for async API only. OK, even if this example is not good enough, I still believe that it is not right to introduce new match criteria in the form of rule attributes. Match criteria belong in the pattern. > >> And doing so will be a failure, because your patch does not add the new >> attribute keyword to sync flow rule syntax parser. That's it. >> >> Once again, I should ephasize: the reader MAY know nothing about the asy= nc >> approach. But if the attribute is present in "struct rte_flow_attr", it >> immediately means that it is available everywhere. Both sync and async. >> >> So, with this in mind, your attempt to limit the scope of the patch to a= sync-only >> rules looks a little bit artificial. It's not correct from the *formal* = standpoint. >> >>> >>>> In other words, it is wrong to assume that "struct rte_flow_attr" >>>> only applies to async approach. It had been introduced long before >>>> the async flow design was added to DPDK. That's it. >>>> >>>>>> >>>>>> 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 >>>>>> from >>>> 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. >>>> >>>> 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- >> direction traffic no matter what ports. >>>> b) how it is observed in customer deployments; >>> Customer have the requirements to save resources and their offloaded ru= les >> is direction aware. >>>> c) why the proposed patch is a good solution. >>> New attributes provide the way to remove one direction and save underla= yer >> resource. >>> All of the above can be found in the commit log. >> >> I understand all of that, but my point is, the existing commit message i= s way >> too brief. Yes, it mentions that SOME customers have SOME deployments, b= ut >> it does not shed light on which specifics these deployments have. For ex= ample, >> back in the day, when items PORT_REPRESENTOR and REPRESENTED_PORT >> were added, the cover letter for that patch series provided details of >> deployment specifics (application: OvS, scenario: full offload rules). >> >> So, it's always better to expand on such specifics so that the reader ha= s full >> picture in their head and doesn't need to look elsewhere. >> Not all readers of the commit message will be happy to delve into our >> discussions on the mailing list to get the gist. >> > It' approach diverse. Pattern item approach will attract another discussi= on thread, right? As I said, match criteria belong in flow pattern. I recognise the importance of the problem that you're looking to solve. It's very good that you care to address it, but what this patch tries to do is to add more match criteria in the form of new attributes with rather questionable names... There's a room for improvement. When I say that new features should not confuse readers, I mean a very basic thing: readers know that match criteria all sit in the pattern. And they refer to the pattern item enum in the code and in documentation to learn about criteria, while "struct rte_flow_attr" is an unusual place from which to learn about match criteria. > We should get a conclusion and reflect in the commit changes&logs, and it= 's easy for others to absorb. Yes, but before we get to that, perhaps it pays to hear more feedback from other reviewers. Thomas? Ori? Andrew? >>> >>>> >>> >>>>>>> >>>>>>>>> By default, the transfer domain is bi-direction, and no behavior >> changes. >>>>>>>>> >>>>>>>>> 1. Match wire origin only >>>>>>>>> flow template_table 0 create group 0 priority 0 transfer wire_or= ig... >>>>>>>>> 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_pro= xy(). >>>>>>>>> */ >>>>>>>>> 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. >>>> >>>> I believe this comment should be reworked, in case the idea of having >>>> an extra attribute persists. >>>> >>>>>>>> >>>>>>>>> + * 0x2 origin vport, >>>>>>>> >>>>>>>> What does "origin vport" mean? Hardly a good term as well. >>>> >>>> I still believe this explanation is way too brief and needs to be >>>> reworked to provide more details, to define the use case for the attri= bute >> more specifically. >>>> >>>>>>>> >>>>>>>>> + * N/A both set. >>>>>>>> >>>>>>>> What's this? >>>> >>>> The question stands. >>>> >>>>>>>> >>>>>>>>> + */ >>>>>>>>> + 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 >>>>> >>> >> >> Thank you. > Thanks, Ivan