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 F135BA00C3; Wed, 26 Jan 2022 06:03:50 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A8ACA411A7; Wed, 26 Jan 2022 06:03:50 +0100 (CET) Received: from NAM10-DM6-obe.outbound.protection.outlook.com (mail-dm6nam10on2082.outbound.protection.outlook.com [40.107.93.82]) by mails.dpdk.org (Postfix) with ESMTP id 28BBB4069D for ; Wed, 26 Jan 2022 06:03:49 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=DssaQ7vi1Faov67JA9dIBsZnxPXtOtU4KJ0p+qhIuwUWDtVt1+uJqM85xAKF8xeWePuYMrcKVCwl91HGtiZXFnZrMknta9GHtnatmDSbsQV6BmcCKABJAk5yDCnfovgiBgJaFshCNblaN9eEzK46z5hpUqb7tE4lJ7YeVFqtI5OY8LihWWuyxzNXHnvaKjEnBqCVo8FU4qescuysX5tSCfgvsyFpdHv1cYiTftlsPfNVYGSBGI922XY8Pn8e46DCtIWZsNrIAVUaPzcpzNJsYmONDwuX920I6ZCpbsazZTNNbtw2GGlv3SffgSrZudYZ7pvFs/rCdk0NzUUB5KKqug== 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=SXLR6Q+4NmyZGJtoV6vbiwuiE4wGXFEMICxUOpQhh0A=; b=YyMrkkZrTA6WGvxs9E7oleAO9C0AvzkYz5fjspLzwHrJgxjDD1feNU32YcwKX0RUA952QAIBFzqWeIOFEBfNPeBf6QH7qecJ4XJQ3pUW7to4B81oMKNJaog19pAdn3wG3UUYebCpN8uXmge2+2Lz/PkG/X75LKms72GfOpjnOyumi9TT9fNL2ziUkloYQI9Coc8RUq9EdZuezjcfVx1FyVJG5JHgUbiQGmYLT37V7vYT7XY2DAnZJC8btHpO+aHTzaYifdMnUBE8bpg6RQAnKcrAZ/MADqNtEwfIlnhi9RibwFR/9N821OyKqNfEUbTpuKcUKqrFdbml9bEhjx+Czw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; 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=SXLR6Q+4NmyZGJtoV6vbiwuiE4wGXFEMICxUOpQhh0A=; b=A92zdAF92lCxy0f46rSQg6ZUFvtVlOUicJ+qBGIsBcrlf47sZ8nxhsbeIeUMnJ9UHY5X2szuU5AaD43QdPkgb6L6vTcA0rU6RWG6icuKaabU2aEj7vBlEdkH2t7IWn4LEdNIvONwBwelc430O0dLVbFOwZAAHRPerZPqF+rTbFF9htJPhrdpw5DTNxglYqAMPB5Tjvb4p4TVXhPSXl/C1SIrhh/Qn5jxtJB+GvFBlQWw+lzvWozV3d6EdGYMDf/cyYKb+eDVaLtLPXgImpRR9w4vETiVvMckPswzS9VpZfhGLzaZIniPaExw+9PnbncHcR9Kj3wx//peanFYdST7zw== Received: from MWHPR12MB1232.namprd12.prod.outlook.com (2603:10b6:300:10::20) by DM4PR12MB5327.namprd12.prod.outlook.com (2603:10b6:5:39e::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4888.11; Wed, 26 Jan 2022 05:03:47 +0000 Received: from DM5PR12MB2405.namprd12.prod.outlook.com (2603:10b6:4:b2::20) by MWHPR12MB1232.namprd12.prod.outlook.com (2603:10b6:300:10::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4909.12; Wed, 26 Jan 2022 05:03:46 +0000 Received: from DM5PR12MB2405.namprd12.prod.outlook.com ([fe80::cd5b:cd8d:cd38:8c31]) by DM5PR12MB2405.namprd12.prod.outlook.com ([fe80::cd5b:cd8d:cd38:8c31%6]) with mapi id 15.20.4909.019; Wed, 26 Jan 2022 05:03:46 +0000 From: Alexander Kozyrev To: Ivan Malov CC: dpdk-dev , Ori Kam , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , Andrew Rybchenko , Ferruh Yigit , "mohammad.abdul.awal@intel.com" , Qi Zhang , Jerin Jacob , Ajit Khaparde Subject: RE: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules Thread-Topic: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules Thread-Index: AQHYEX6OUrQfghgPnkWQlkoO8pTDdqxz0yew Date: Wed, 26 Jan 2022 05:03:46 +0000 Message-ID: References: <2fb135e6-f492-bb8-1554-4df1f089fc79@oktetlabs.ru> In-Reply-To: <2fb135e6-f492-bb8-1554-4df1f089fc79@oktetlabs.ru> Accept-Language: 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-office365-filtering-correlation-id: 58efb76c-70c9-47d2-b800-08d9e0893bbf x-ms-traffictypediagnostic: MWHPR12MB1232:EE_|DM4PR12MB5327:EE_ x-ld-processed: 43083d15-7273-40c1-b7db-39efd9ccc17a,ExtAddr,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: VsuDXCjU1V9TOGrRV0v/lXsBfHFnc1xg3kwQu5eGy1AkD4dalJ41V5r/BhFZ9CrNA7raRFEITBDqBiVdPPai9baqNhXjTg/bWfhsBMX9jB0NCXPh9PeyoS4vkd4h2gyaiw3BWnJsERVCGkj3PPSlpZxd5TfmkJFJEkkabc4yzbZV60RciskhUXdawsJXQ7lJondLg2MtCyNn+82VvzZVpgadQROFVQv+niH04JoR+dSeBfubfcCcERW0simkHgATnaTkUiWVz3MFBvtS+QTnJ/NUkug+ABOl5D5C5e0e/1LelQD8bjc4PNZwNHNxwoM6QySreQA8jzDldNEbEBeW87NbzEEp7Jxh1ZJcX4+HiHA7muZESp6dHaNLBkYegTy2uW14tF4k+31sp1S62tbT6Xf99oQyNvlA6KEe6znjxD8FTPtM+l7Ku4pmPSiCb6rrAXcKm/4WqnIc8Dq4MER7exMWPseL21QBWcBigr6Yi2D8HNxtiRpODo5qupUFzwKKcjZu8MP4/18Jq9tKftRRqt3vH60P42nxv4MQ3uzGXbdJetQoW87drWk0Sj/Za8nH+3x4krSDtrqOAQh2mXS0WjRaVjQ21YY3kt5ntbDYTo/QXnEuZOqB1kkE3YX/M7WpKu8KuW4J0SNTTXsyDBIthL0H1V/7xquLPv7qaySnI0h3iLAid2z5osrf7B10ta+wZ6uQz9Ljz2W26eAHmsg4/A== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:MWHPR12MB1232.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(186003)(66556008)(66476007)(9686003)(2906002)(83380400001)(66946007)(26005)(316002)(54906003)(64756008)(6916009)(7696005)(8936002)(5660300002)(122000001)(8676002)(52536014)(76116006)(38070700005)(53546011)(71200400001)(508600001)(38100700002)(66446008)(4326008)(86362001)(55016003)(33656002)(6506007); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?sOqPxXR3r5IKWohvwKIZfoRocQaDc5eXxm10LRPCLn7o1vNzhCxkv+toq/gL?= =?us-ascii?Q?rade8TuXAI8pKnv+dyHgIG/3u5WX4HbdWS09qv8f7CJ8OM0Ab+iCjLNQ1HRZ?= =?us-ascii?Q?J2YwnYx4cYjuKalb9SmM+xQVR2bVoghW28xDjDjhzptb0vZPtoR0jSaOiiSI?= =?us-ascii?Q?FKz1R7cfQSlyU7zGKxZ21DaX812tis11oYV9N0MnzTPYf6F/NYFxRc6QdgJs?= =?us-ascii?Q?1iS25JA+QbvUld48y+eNfEgG5jO4Qb/hDg6peuj2S7cafHMwGFZQ0AORD/DR?= =?us-ascii?Q?NybQWDvrkNc2mJekwjwaqYo+tDIrtFULGtHc9hPM3iSppNxPMdL3CM4fdlYe?= =?us-ascii?Q?sDEa/ElhFTWbtprQUbgzxFR7p+SvU33zCRxpj3ILvfqTOS0T4k4JZvYWGH2c?= =?us-ascii?Q?Alp/N3HvgSkkvz67ilcoR8/yDAVupLDv9MPSEkxBnZkzpmrZfJs81/xXYt41?= =?us-ascii?Q?4wSXQEabQDhVZP6/CFT3cTbAHbyQ9L+nx29jkdz8JKNajzLnp3IQj96+I/O5?= =?us-ascii?Q?PavGV/5TvPqYVdDqCJMxtT3BjNectmnlTUc0Va5pXMcWlIpD+D9rzzM0EtgS?= =?us-ascii?Q?09I2B7QG03ytZSf1uwlu0HHj8U4tw0Y3wzMyy+hgJzAqwU+VNRrNMsKHs7G3?= =?us-ascii?Q?x3LWq0viLRGhG5nzsMFFwiTEiR6xToO4lCvvvB9SW+1txsmpau7P/ylr/QUF?= =?us-ascii?Q?+wdaZQ1+ESuwOzQYE2EjdSdbytJI5I60jbtFjHp/OvronUqL6qlqpyHShxfb?= =?us-ascii?Q?L50ABDsMk51wDqmvI6RynvVELYv2LmkRi3Z7P5MBPX9ZEuHfbx4WykpKUAFY?= =?us-ascii?Q?+RqXcguLMXDDBBhVdx1EiND7x8BdffDoDet/uO4IDsMF5S+n9FLA1JIpNMuQ?= =?us-ascii?Q?TM7uoYKmb3J04oop6uHqAbibL9W1Y3L4m+4k1oJizCLm1Wv2FY2soUNVcYa0?= =?us-ascii?Q?OCZ+KYNN8QJt9YLj2eblGqk7PdS1WNN6pUJLvfHNyW4TxsgAfXTY4/EvXQST?= =?us-ascii?Q?3LGWBJYDsprWY58HKOQcNnfGPPQJePomw1il3p0NydKgkwkyycsPzaT+8OC1?= =?us-ascii?Q?O/RPf+WXp0lbpEOzUBSUdbcNnH8JutibmB9fFgNJEzS5g7FQzTMIoa1QQ1Z7?= =?us-ascii?Q?mi48vetHT2yMXg9sjlc0FF28DOcC2Ua5DX/0GTeSJ6rRW00m4ef0CrSvknWQ?= =?us-ascii?Q?AICkrTYxw6Y59WytiB4LQAcWZ43pSkIKi0sNEIr3E53dGOU24aRw72vc844x?= =?us-ascii?Q?HxuH7/my6kFk8mZAIsvDRuX2cf4Giyy3wy0z2nFxLo/votiqH3r4wAaCZCuL?= =?us-ascii?Q?QrAJFN9NpmwwN7c5oWDOz5NCFthKreFm/FHv4nD1zUc7D3LRWDMrAixOEe5X?= =?us-ascii?Q?+G7iR+iP+0V8NkngmDj2jOsfBT797RVGiJYDZdG6trSbXJnsDIyi64GbSYpJ?= =?us-ascii?Q?9hzYJkBCzmHnScafFN9aUGbLqdQE3HrNGVlpFdzKTIp/x4Jr+B8yequUPYh1?= =?us-ascii?Q?cH7kvMIaRrYyck1fUdujigUkUBmYJdl3th1/FYue/WXz1pcEEvu5zbBesk0H?= =?us-ascii?Q?+Ep6IxDcAdW+zr2HLQn+woMx1tM0mXn86HFgYgTHO/tE6klMUst+a1oInMYj?= =?us-ascii?Q?fg=3D=3D?= 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: DM5PR12MB2405.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 58efb76c-70c9-47d2-b800-08d9e0893bbf X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jan 2022 05:03:46.6103 (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: VzZf9hivh0PQjDWGFlnmVrsO8/wSXbxWUthIubtEvhYMnr14n7Wr125jCjj6M2OxDE1t7fKWRcRzYEneNScFZA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5327 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 On Monday, January 24, 2022 19:00 Ivan Malov wrot= e: > This series is very helpful as it draws attention to > the problem of making flow API efficient. That said, > there is much room for improvement, especially in > what comes to keeping things clear and concise. >=20 > In example, the following APIs >=20 > - rte_flow_q_flow_create() > - rte_flow_q_flow_destroy() > - rte_flow_q_action_handle_create() > - rte_flow_q_action_handle_destroy() > - rte_flow_q_action_handle_update() >=20 > should probably be transformed into a unified one >=20 > int > rte_flow_q_submit_task(uint16_t port_id, > uint32_t queue_id, > const struct rte_flow_q_ops_attr *q_ops_attr, > enum rte_flow_q_task_type task_type, > const void *task_data, > rte_flow_q_task_cookie_t cookie, > struct rte_flow_error *error); >=20 > with a handful of corresponding enum defines and data structures > for these 5 operations. We were thinking about the unified function for all queue operations. But it has too many drawbacks in our opinion: 1. Different operation return different results and unneeded parameters. q_flow_create gives a flow handle, q_action_handle returns indirect action = handle. destroy functions return the status. All these cases needs to be handled di= fferently. Also, the unified function is bloated with various parameters not needed fo= r all operations. Both of these point results in hard to understand API and messy documentati= on with various structures on how to use it in every particular case. 2. Performance consideration. We aimed the new API with the insertion/deletion rate in mind. Adding if conditions to distinguish between requested operation will cause = some degradation. It is preferred to have separate small functions that do one job and make i= t efficient. 3. Conforming to the current API. The idea is to have the same API as we had before and extend it with asynch= ronous counterparts. That is why we took the familiar functions and added queue-based version s = for them. It is easier for application to switch to new API with this approach. Inter= faces are still the same. > By the way, shouldn't this variety of operation types cover such > from the tunnel offload model? Getting PMD's opaque "tunnel > match" items and "tunnel set" actions - things like that. Don't quite get the idea. Could you please elaborate more on this? > Also, I suggest that the attribute "drain" > be replaced by "postpone" (invert the meaning). > rte_flow_q_drain() should then be renamed to > rte_flow_q_push_postponed(). >=20 > The rationale behind my suggestion is that "drain" tricks readers into > thinking that the enqueued operations are going to be completely purged, > whilst the true intention of the API is to "push" them to the hardware. I don't have a strong opinion on this naming, if you think "postpone" is be= tter. Or we can name it as "doorbell" to signal a NIC about some work to be done and "rte_flow_q_doorbell" to do this explicitly after a few operations. > rte_flow_q_dequeue() also needs to be revisited. The name suggests that > some "tasks" be cancelled, whereas in reality this API implies "poll" > semantics. So why not name it "rte_flow_q_poll"? The polling implies an active busy-waiting of the result. Which is not the = case here. What we do is only getting results for already processed operations, hence = "dequeue" as opposite to "queue". What do you think? Or we can have push for drain and pull for dequeue as an= alternative. > I believe this function should return an array of completions, just > like it does in the current version, but provide a "cookie" (can be > represented by a uintptr_t value) for each completion entry. >=20 > The rationale behind choosing "cookie" over "user_data" is clarity. > Term "user_data" sounds like "flow action conf" or "counter data", > whilst in reality it likely stands for something normally called > a cookie. Please correct me if I've got that wrong. I haven't heard about cookies in context not related to web browsing. I think that user data is more than a simple cookie, it can contain anything that application wants to associate with this flow rule, i.e. connection ID, timestamp, anything. It is more general term here. > Well, the short of it: submit_task(), push_postponed() and poll(). > Having a unified "submit_task()" API should allow to reduce the > amount of comment duplication. I'm afraid it won't reduce the amount of comments needed anyway. Instead of 5 descriptions of purpose-specific function we will end up with a huge blob of text trying to explain how to use a single function with 5 different input structures and 3 different output types. That is really m= essy. > In what comes to RST commentary, please consider a bullet-formatted > description for improved readability: >=20 > - Flow rule management is done via special flow management queues > - The queue operations are asynchronous and not thread-safe > - The operations can thus be invoked by the app's datapath > - The queue count is configured at initialisation stage > - Available operation types: submit_task, push_postponed and poll > - Polling provides completions for submitted tasks > - The completions can be reordered withing a queue > - Polling must be done on time to avoid overflows Agree, it does seem nicer and cleaner, will adopt this style in v3. > Please note that the above review notes are just a quick primer, > nothing more. I may be mistaken with regard to some aspects. >=20 > I humbly request that we agree on the API sketch and naming > before going to the next review cycle. >=20 > Thank you. Thanks for your suggestions, let's see if we can find a common round here.