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 9477FA04A3; Wed, 26 Jan 2022 19:54:35 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2624042750; Wed, 26 Jan 2022 19:54:35 +0100 (CET) Received: from mail-io1-f53.google.com (mail-io1-f53.google.com [209.85.166.53]) by mails.dpdk.org (Postfix) with ESMTP id C04344274B for ; Wed, 26 Jan 2022 19:54:33 +0100 (CET) Received: by mail-io1-f53.google.com with SMTP id e79so694160iof.13 for ; Wed, 26 Jan 2022 10:54:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=t9kMuXG8SVR0TdvB3Vyevldja8IztCFEXcpz/HZneBk=; b=UOQUK7vvQXmd5dX/rIF9MZyAVoj1u5lbNWubPIZvax9rBqDwFPfE/0flTUxfsWpRuH iQ0lsz1JUeWR+Oh+i7dL0HozQeqP0TZErw4SqSJYpysHlVMMHhqAYmf6E/ShQYNKRFIz djsuB3G3P9ZVPrGNSSIVje6M02LmxmR0OE+64= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=t9kMuXG8SVR0TdvB3Vyevldja8IztCFEXcpz/HZneBk=; b=j8Lfo1cPCx/Pf5vq2fEBvZDebejv/LIUArt0cWrnQJpJ3AYB3Yah6nXPJ5UyrwWYaP Ob4NGdRYOuzjItZJs4cWYHOv0xusTlAjwacLue8d4XphhMZ0WTCPA/l+RmBKYQzTnCKh y9up+j2E7Z3JsKiqxRRSJjhYv14/qUEaiF8kA2l/Rbl+2U32iBtBTdeIPzwU620coXo7 BzGfij8rKWFOL3TfMlix2Ovsv8uqpOQ/8tsrwnw33G4YFRgDmQyKfep99/HmIPPuywjp +wE60NLbmIe9JNwvjJ7gIpzJV/TFyaxoFtqXDY4MoabMz9iK0/ctCQ3B2J8+VZsWNPr8 WuTA== X-Gm-Message-State: AOAM533DlIe7t1S0jXPJ2AHdVZOQuZGEddgJxXGIs/2DbFP5ZdRlqzlD VY0a85wpFHRUc5h0voW4WsedbwvLxhGdgjxAzPPKuQ== X-Google-Smtp-Source: ABdhPJwki7fobdn4hqAy5WdaG+xDC0obHPUBfwBMib1BsdLoGEpenAAK5OaqZSKybtajgSLN4CrDrDEz+YEFjYgQabQ= X-Received: by 2002:a02:7a13:: with SMTP id a19mr4488398jac.163.1643223271925; Wed, 26 Jan 2022 10:54:31 -0800 (PST) MIME-Version: 1.0 References: <2fb135e6-f492-bb8-1554-4df1f089fc79@oktetlabs.ru> In-Reply-To: From: Ajit Khaparde Date: Wed, 26 Jan 2022 10:54:15 -0800 Message-ID: Subject: Re: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules To: Alexander Kozyrev Cc: Ivan Malov , dpdk-dev , Ori Kam , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , Andrew Rybchenko , Ferruh Yigit , "mohammad.abdul.awal@intel.com" , Qi Zhang , Jerin Jacob Content-Type: text/plain; charset="UTF-8" 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 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. > 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 differently. > Also, the unified function is bloated with various parameters not needed for all operations. > 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. > 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 it efficient. > 3. Conforming to the current API. > The idea is to have the same API as we had before and extend it with asynchronous 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. Interfaces are still the same. Alexander, I think you have made some good points here. Dedicated API is better compared to the unified function. > > > 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(). > > > > 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. > > > 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. > > > > 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 messy. > > > 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. > > > 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.