On Wednesday, January 26, 2022 13:54 Ajit Khaparde <ajit.khapa...@broadcom.com> wrote: > > On Tue, Jan 25, 2022 at 9:03 PM Alexander Kozyrev <akozy...@nvidia.com> > wrote: > > > > On Monday, January 24, 2022 19:00 Ivan Malov <ivan.ma...@oktetlabs.ru> > 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.
Glad I made it clearer. Ivan, what do you think about these considerations? > > > > > 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.
