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

[Alexander Kozyrev <akozyrev@nvidia.com>]

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.

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