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

[Ivan Malov <ivan.malov@oktetlabs.ru>]

Hi Alexander,

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. Also, I suggest that the attribute "drain"
be replaced by "postpone" (invert the meaning).

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.

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.

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"?

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.


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.


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


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.

--
Ivan M.