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

[Alexander Kozyrev <akozyrev@nvidia.com>]

On Wednesday, January 26, 2022 13:54 Ajit Khaparde <ajit.khaparde@broadcom.com> wrote:
>
> 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.

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.