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.
> > > 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.
