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 0C0D0A00C5; Tue, 1 Feb 2022 01:18:04 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8CC1440DDA; Tue, 1 Feb 2022 01:18:03 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E2FDF40150 for ; Tue, 1 Feb 2022 01:18:01 +0100 (CET) Received: by shelob.oktetlabs.ru (Postfix, from userid 115) id EA9DA42; Tue, 1 Feb 2022 03:18:00 +0300 (MSK) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mail1.oktetlabs.ru X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=ALL_TRUSTED, DKIM_ADSP_DISCARD autolearn=no autolearn_force=no version=3.4.6 Received: from bree.oktetlabs.ru (bree.oktetlabs.ru [192.168.34.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPS id 0477038; Tue, 1 Feb 2022 03:18:00 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 0477038 Authentication-Results: shelob.oktetlabs.ru/0477038; dkim=none; dkim-atps=neutral Date: Tue, 1 Feb 2022 03:17:59 +0300 (MSK) From: Ivan Malov To: Alexander Kozyrev cc: Ajit Khaparde , dpdk-dev , Ori Kam , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , Andrew Rybchenko , Ferruh Yigit , "mohammad.abdul.awal@intel.com" , Qi Zhang , Jerin Jacob Subject: RE: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules In-Reply-To: Message-ID: References: <2fb135e6-f492-bb8-1554-4df1f089fc79@oktetlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed 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 all, On Thu, 27 Jan 2022, Alexander Kozyrev wrote: > On Wednesday, January 26, 2022 13:54 Ajit Khaparde wrote: >> >> On Tue, Jan 25, 2022 at 9:03 PM Alexander Kozyrev >> wrote: >>> >>> On Monday, January 24, 2022 19:00 Ivan Malov >> wrote: >>>> 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. >>>> >>>> In example, the following APIs >>>> >>>> - 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() >>>> >>>> should probably be transformed into a unified one >>>> >>>> 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); >>>> >>>> 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. Good. >>> But it has too many drawbacks in our opinion: Is that so? >>> 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 >> differently. Yes, all of these are to be handled differently, but this does not mean that one cannot think of a unified handle format. The application can remember which cookie corresponds to which operation type after all. >>> Also, the unified function is bloated with various parameters not needed >> for all operations. That is far-fetched. Non-unified set of APIs is also bloated. Takes long to read. Many duplicating comments. When one has added a new API for a different type of task, they will have to duplicate many lines one more time. In the case of unified API, one has to add a new enum type (one line), specific (and thus concise) description for it, and the corresponding structure for the task data. That's it. The rest is up to the PMDs. Also, it should be possible to make the task data IN-OUT, to return its task-specific handle (to address your above concern). >>> Both of these point results in hard to understand API and messy >> documentation with >>> various structures on how to use it in every particular case. The documentation for the method is always the same. Precise and concise. The task data structures will have their own descriptions, yes. But that does not make the documentation messy. Or am I missing something? >>> 2. Performance consideration. >>> We aimed the new API with the insertion/deletion rate in mind. Good. >>> Adding if conditions to distinguish between requested operation will cause >> some degradation. Some degradation.. - how serious would it be? What's for the "if" conditions, well, I believe the compiler is smart enough to deal with them efficiently. After all, the suggested approach is a fixed set of operation (task) types. One can have a static array of task-specific methods in the PMD. And only one condition to check the value bounds. >>> It is preferred to have separate small functions that do one job and make it >> efficient. A noble idea. >> Interfaces are still the same. That is the major point of confusion. The application developer has to be super-careful to tell the queue version of "flow_create" from the regular one. The two set of APIs are indeed counterparts, and that's might be ambiguous. Whilst in the unified approach, readers will understand that this new API is just a task-submitter for the asynchronous type of operation. > Glad I made it clearer. Ivan, what do you think about these considerations? Well, I'm not pushing anyone to abandon the existing approach and switch to the unified API design. But the above points might not be that difficult to address. This deserves more discussions. Any other opinions? > >>> >>>> 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? rte_flow_tunnel_decap_set(), rte_flow_tunnel_action_decap_release(); rte_flow_tunnel_match(), rte_flow_tunnel_item_release(). >>> >>>> 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(). >>>> >>>> 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 >> better. >>> 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. >>> By the sound of it, "push" is just a shorter term for "doorbell". >>>> 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. That is far-fetched. Polling implies some periodic invocations, that is correct. But it's up to the application whether to make this a tight loop or not. Or am I missing something? >>> 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. Pull is not that informative. The commonly-used term is "poll". But let's hear some other opinions. Maybe someone has their way with naming. >>> >>>> 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. >>>> >>>> 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. OK. Seems like I overestimated clarity of the term "cookie". >>> >>>> 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. It will reduce the amount of generic (now just repeating) comments. That's it. Task-specific comments will stay, and that is fine. >>> 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 >> messy. Is that so? It shouldn't be like that. It should say about enqueuing a task, whilst task-specific comments belong in the corresponding enum or structure definitions. Definitely not a single huge comment. >>> >>>> In what comes to RST commentary, please consider a bullet-formatted >>>> description for improved readability: >>>> >>>> - 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. Very well. >>> >>>> Please note that the above review notes are just a quick primer, >>>> nothing more. I may be mistaken with regard to some aspects. >>>> >>>> I humbly request that we agree on the API sketch and naming >>>> before going to the next review cycle. >>>> >>>> Thank you. >>> Thanks for your suggestions, let's see if we can find a common round here. > Alright. -- Ivan M