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 650B2A04A6; Tue, 25 Jan 2022 01:00:15 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CF3F74115F; Tue, 25 Jan 2022 01:00:14 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id C2AC04069F for ; Tue, 25 Jan 2022 01:00:13 +0100 (CET) Received: by shelob.oktetlabs.ru (Postfix, from userid 115) id 10ECB41; Tue, 25 Jan 2022 03:00:13 +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 6305438; Tue, 25 Jan 2022 03:00:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 6305438 Authentication-Results: shelob.oktetlabs.ru/6305438; dkim=none; dkim-atps=neutral Date: Tue, 25 Jan 2022 03:00:12 +0300 (MSK) From: Ivan Malov To: Alexander Kozyrev cc: dpdk-dev , Ori Kam , Thomas Monjalon , 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 Message-ID: <2fb135e6-f492-bb8-1554-4df1f089fc79@oktetlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset=US-ASCII 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 Alexander, 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. Also, I suggest that the attribute "drain" be replaced by "postpone" (invert the meaning). 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. 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. 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"? 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. 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. 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 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. -- Ivan M.