Re: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules

[Ajit Khaparde <ajit.khaparde@broadcom.com>]

On Tue, Jan 25, 2022 at 9:03 PM Alexander Kozyrev <akozyrev@nvidia.com> wrote:
>
> On Monday, January 24, 2022 19:00 Ivan Malov <ivan.malov@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.