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

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

[Ivan Malov]

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.

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

[Alexander Kozyrev]

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.

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

[Ajit Khaparde]

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.

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

[Alexander Kozyrev]

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.

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

[Ivan Malov]

Hi all,

On Thu, 27 Jan 2022, Alexander Kozyrev wrote:

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

Good.

>>> But it has too many drawbacks in our opinion:

Is that so?

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

Yes, all of these are to be handled differently, but this does not mean
that one cannot think of a unified handle format. The application can
remember which cookie corresponds to which operation type after all.

>>> Also, the unified function is bloated with various parameters not needed
>> for all operations.

That is far-fetched.

Non-unified set of APIs is also bloated. Takes long to read. Many
duplicating comments. When one has added a new API for a different
type of task, they will have to duplicate many lines one more time.

In the case of unified API, one has to add a new enum type (one line),
specific (and thus concise) description for it, and the corresponding
structure for the task data. That's it. The rest is up to the PMDs.

Also, it should be possible to make the task data IN-OUT, to return
its task-specific handle (to address your above concern).

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

The documentation for the method is always the same. Precise and concise.
The task data structures will have their own descriptions, yes. But that
does not make the documentation messy. Or am I missing something?

>>> 2. Performance consideration.
>>> We aimed the new API with the insertion/deletion rate in mind.

Good.

>>> Adding if conditions to distinguish between requested operation will cause
>> some degradation.

Some degradation.. - how serious would it be? What's for the "if"
conditions, well, I believe the compiler is smart enough to deal
with them efficiently. After all, the suggested approach is
a fixed set of operation (task) types. One can have a
static array of task-specific methods in the PMD.
And only one condition to check the value bounds.

>>> It is preferred to have separate small functions that do one job and make it
>> efficient.

A noble idea.

>> Interfaces are still the same.

That is the major point of confusion. The application developer has to
be super-careful to tell the queue version of "flow_create" from the
regular one. The two set of APIs are indeed counterparts, and that's
might be ambiguous. Whilst in the unified approach, readers will
understand that this new API is just a task-submitter for
the asynchronous type of operation.

> Glad I made it clearer. Ivan, what do you think about these considerations?

Well, I'm not pushing anyone to abandon the existing approach and switch
to the unified API design. But the above points might not be that
difficult to address. This deserves more discussions.

Any other opinions?

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

rte_flow_tunnel_decap_set(), rte_flow_tunnel_action_decap_release();
rte_flow_tunnel_match(), rte_flow_tunnel_item_release().

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

By the sound of it, "push" is just a shorter term for "doorbell".

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

That is far-fetched. Polling implies some periodic invocations,
that is correct. But it's up to the application whether to
make this a tight loop or not. Or am I missing something?

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

Pull is not that informative. The commonly-used term is "poll".
But let's hear some other opinions. Maybe someone has their
way with naming.

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

OK. Seems like I overestimated clarity of the term "cookie".

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

It will reduce the amount of generic (now just repeating) comments.
That's it. Task-specific comments will stay, and that is fine.

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

Is that so? It shouldn't be like that. It should say about enqueuing a
task, whilst task-specific comments belong in the corresponding enum
or structure definitions. Definitely not a single huge comment.

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

Very well.

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

Alright.

--
Ivan M

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

[Ori Kam]

Hi 

> -----Original Message-----
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> Sent: Tuesday, February 1, 2022 2:18 AM
> Subject: RE: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules
> 
> Hi all,
> 
> On Thu, 27 Jan 2022, Alexander Kozyrev wrote:
> 
> > 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.
> 
> Good.
> 
> >>> But it has too many drawbacks in our opinion:
> 
> Is that so?
> 
> >>> 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.
> 
> Yes, all of these are to be handled differently, but this does not mean
> that one cannot think of a unified handle format. The application can
> remember which cookie corresponds to which operation type after all.
> 

I agree with with Alexander, merging all of those functions to one will result in
extra parameters which will be unsued.
Basic programing apporch is that functions should do one thing,
it is easier to document / use / test and will run faster.

> >>> Also, the unified function is bloated with various parameters not needed
> >> for all operations.
> 
> That is far-fetched.
> 
Just thinking about extra parameter to return the handle only in case
of create flow.

Also in future I guess we will want to add modify rule and indirect actions
I assume that in both case dedicated functions will be much better
for example for the indirect use the same names we are using now
only adding q in the name and extra queue parameter.

> Non-unified set of APIs is also bloated. Takes long to read. Many
> duplicating comments. When one has added a new API for a different
> type of task, they will have to duplicate many lines one more time.
> 
> In the case of unified API, one has to add a new enum type (one line),
> specific (and thus concise) description for it, and the corresponding
> structure for the task data. That's it. The rest is up to the PMDs.
> 
> Also, it should be possible to make the task data IN-OUT, to return
> its task-specific handle (to address your above concern).
> 
> >>> 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.
> 

I disagree with you I think that having unused and misleading parameters
is much more messy from user view point.

> The documentation for the method is always the same. Precise and concise.
> The task data structures will have their own descriptions, yes. But that
> does not make the documentation messy. Or am I missing something?
> 
> >>> 2. Performance consideration.
> >>> We aimed the new API with the insertion/deletion rate in mind.
> 
> Good.
> 
> >>> Adding if conditions to distinguish between requested operation will cause
> >> some degradation.
> 
> Some degradation.. - how serious would it be? What's for the "if"
> conditions, well, I believe the compiler is smart enough to deal
> with them efficiently. After all, the suggested approach is
> a fixed set of operation (task) types. One can have a
> static array of task-specific methods in the PMD.
> And only one condition to check the value bounds.
> 
yes from our testing just an if will result in degradation.
Even just the indirect ref to the PMD function results in degradation.
I guess if you try to add an if in the datapath (rx_burst/tx_burst) you will 
Need to explain the reason.

> >>> It is preferred to have separate small functions that do one job and make it
> >> efficient.
> 
> A noble idea.
> 
Noble idea is good so lets do it small functions.

> >> Interfaces are still the same.
> 
> That is the major point of confusion. The application developer has to
> be super-careful to tell the queue version of "flow_create" from the
> regular one. The two set of APIs are indeed counterparts, and that's
> might be ambiguous. Whilst in the unified approach, readers will
> understand that this new API is just a task-submitter for
> the asynchronous type of operation.
> 
> > Glad I made it clearer. Ivan, what do you think about these considerations?
> 
> Well, I'm not pushing anyone to abandon the existing approach and switch
> to the unified API design. But the above points might not be that
> difficult to address. This deserves more discussions.
> 
> Any other opinions?
> 
I vote for the suggested API lets try and see.

> >
> >>>
> >>>> 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?
> 
> rte_flow_tunnel_decap_set(), rte_flow_tunnel_action_decap_release();
> rte_flow_tunnel_match(), rte_flow_tunnel_item_release().
> 
> >>>
> >>>> 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.
> >>>
> 
> By the sound of it, "push" is just a shorter term for "doorbell".
> 


I don't think the name should be doorbell, since it may not just be doorbell
for example PMD may organize the rules in a better way.

> >>>> 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.
> 
> That is far-fetched. Polling implies some periodic invocations,
> that is correct. But it's up to the application whether to
> make this a tight loop or not. Or am I missing something?
> 
I like the dequeue since it also tells the developer that this is how he 
remove rules from the queue.

> >>> 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.
> 
> Pull is not that informative. The commonly-used term is "poll".
> But let's hear some other opinions. Maybe someone has their
> way with naming.
> 
> >>>
> >>>> 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.
> 
> OK. Seems like I overestimated clarity of the term "cookie".
> 
> >>>
> >>>> 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.
> 
> It will reduce the amount of generic (now just repeating) comments.
> That's it. Task-specific comments will stay, and that is fine.
> 
Like above API should be as direct as possible.

> >>> 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.
> 
> Is that so? It shouldn't be like that. It should say about enqueuing a
> task, whilst task-specific comments belong in the corresponding enum
> or structure definitions. Definitely not a single huge comment.
> 
> >>>
> >>>> 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.
> 
> Very well.
> 
> >>>
> >>>> 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.
> >
> 
> Alright.
> 
> --
> Ivan M

Best,
Ori

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

[Ivan Malov]

Hi Ori, Alexander,

On Tue, 1 Feb 2022, Ori Kam wrote:

> Hi
>
>> -----Original Message-----
>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Sent: Tuesday, February 1, 2022 2:18 AM
>> Subject: RE: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules
>>
>> Hi all,
>>
>> On Thu, 27 Jan 2022, Alexander Kozyrev wrote:
>>
>>> 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.
>>
>> Good.
>>
>>>>> But it has too many drawbacks in our opinion:
>>
>> Is that so?
>>
>>>>> 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.
>>
>> Yes, all of these are to be handled differently, but this does not mean
>> that one cannot think of a unified handle format. The application can
>> remember which cookie corresponds to which operation type after all.
>>
>
> I agree with with Alexander, merging all of those functions to one will result in
> extra parameters which will be unsued.

It is not clear which parameters are going to be unused in which cases.
Task type? Always used. Task data? Always used. Cookie? Presumably,
always used. But, as I said, you can either change the name to
your liking or prefer a task data-internal parameter to pass
task handles back to the caller instead of the "cookie".

> Basic programing apporch is that functions should do one thing,
> it is easier to document / use / test and will run faster.

By the sound of it, we are starting to run into disagreement in what comes
to being "easier to document". A unified API by its very definition should
be easier to document, extend and maintain.

However, I see your point. This design spares no expense to make
the code super-fast. I have no strong opinion on whether it is
good to abandon other basic programming points like keeping
things generic / unified and extensible, which are as
important as the point of having simpler functions.

Perhaps caring about performance indeed prevails here.
And maybe I should quit being that sceptical.

>
>>>>> Also, the unified function is bloated with various parameters not needed
>>>> for all operations.
>>
>> That is far-fetched.
>>
> Just thinking about extra parameter to return the handle only in case
> of create flow.

One can make the task data argument "in-out" and keep the handle field
over there, union-style. And no handle for tasks which do not need one.

>
> Also in future I guess we will want to add modify rule and indirect actions
> I assume that in both case dedicated functions will be much better
> for example for the indirect use the same names we are using now
> only adding q in the name and extra queue parameter.
>
>> Non-unified set of APIs is also bloated. Takes long to read. Many
>> duplicating comments. When one has added a new API for a different
>> type of task, they will have to duplicate many lines one more time.
>>
>> In the case of unified API, one has to add a new enum type (one line),
>> specific (and thus concise) description for it, and the corresponding
>> structure for the task data. That's it. The rest is up to the PMDs.
>>
>> Also, it should be possible to make the task data IN-OUT, to return
>> its task-specific handle (to address your above concern).
>>
>>>>> 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.
>>
>
> I disagree with you I think that having unused and misleading parameters
> is much more messy from user view point.

Yes, but currently it is not clear which parameters
one deems unused or misleading and in which cases.

>
>> The documentation for the method is always the same. Precise and concise.
>> The task data structures will have their own descriptions, yes. But that
>> does not make the documentation messy. Or am I missing something?
>>
>>>>> 2. Performance consideration.
>>>>> We aimed the new API with the insertion/deletion rate in mind.
>>
>> Good.
>>
>>>>> Adding if conditions to distinguish between requested operation will cause
>>>> some degradation.
>>
>> Some degradation.. - how serious would it be? What's for the "if"
>> conditions, well, I believe the compiler is smart enough to deal
>> with them efficiently. After all, the suggested approach is
>> a fixed set of operation (task) types. One can have a
>> static array of task-specific methods in the PMD.
>> And only one condition to check the value bounds.
>>
> yes from our testing just an if will result in degradation.
> Even just the indirect ref to the PMD function results in degradation.
> I guess if you try to add an if in the datapath (rx_burst/tx_burst) you will
> Need to explain the reason.

I see your point. But, in my understanding, this new design, first of all,
does not belong in *traffic* path (albeit it is meant for *fast path*,
in general), so it is not quite correct to compare it with Rx burst
functionality. Secondly, the idea is to make things non-blocking
and avoid tedious parsing (hence all the templates and tables),
and that goal is achieved in any case. So, removing *all* "if"
checks at any cost might not be that reasonable.

BTW, keeping things robust is another basic programming point...

>
>>>>> It is preferred to have separate small functions that do one job and make it
>>>> efficient.
>>
>> A noble idea.
>>
> Noble idea is good so lets do it small functions.

Well, I have no strong objections. You can try and see how it goes.

>
>>>> Interfaces are still the same.
>>
>> That is the major point of confusion. The application developer has to
>> be super-careful to tell the queue version of "flow_create" from the
>> regular one. The two set of APIs are indeed counterparts, and that's
>> might be ambiguous. Whilst in the unified approach, readers will
>> understand that this new API is just a task-submitter for
>> the asynchronous type of operation.
>>
>>> Glad I made it clearer. Ivan, what do you think about these considerations?
>>
>> Well, I'm not pushing anyone to abandon the existing approach and switch
>> to the unified API design. But the above points might not be that
>> difficult to address. This deserves more discussions.
>>
>> Any other opinions?
>>
> I vote for the suggested API lets try and see.

Agreed. Sometimes it pays to try and see the results.
This API has experimental status anyway...

>
>>>
>>>>>
>>>>>> 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?
>>
>> rte_flow_tunnel_decap_set(), rte_flow_tunnel_action_decap_release();
>> rte_flow_tunnel_match(), rte_flow_tunnel_item_release().
>>
>>>>>
>>>>>> 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.
>>>>>
>>
>> By the sound of it, "push" is just a shorter term for "doorbell".
>>
>
>
> I don't think the name should be doorbell, since it may not just be doorbell
> for example PMD may organize the rules in a better way.
>
>>>>>> 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.
>>
>> That is far-fetched. Polling implies some periodic invocations,
>> that is correct. But it's up to the application whether to
>> make this a tight loop or not. Or am I missing something?
>>
> I like the dequeue since it also tells the developer that this is how he
> remove rules from the queue.

Look. The major point here is that, according to the description,
task completions may be reordered. Hence, technically, this is
not a queue, not a ring, but rather a "postponed task list".
That being said, I believe that the term "queue" (like "q"
in the API names) can be retained - it's fine to call a
completion list a "queue". But in what comes to the
other ring-specific terms like "dequeue", I would
go for some other alternatives like "poll".

Please correct me if I've got that wrong.

>
>>>>> 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.
>>
>> Pull is not that informative. The commonly-used term is "poll".
>> But let's hear some other opinions. Maybe someone has their
>> way with naming.
>>
>>>>>
>>>>>> 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.
>>
>> OK. Seems like I overestimated clarity of the term "cookie".
>>
>>>>>
>>>>>> 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.
>>
>> It will reduce the amount of generic (now just repeating) comments.
>> That's it. Task-specific comments will stay, and that is fine.
>>
> Like above API should be as direct as possible.

OK. Your point is very clear. I do not object.
But maybe more opinions exist regarding that.

>
>>>>> 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.
>>
>> Is that so? It shouldn't be like that. It should say about enqueuing a
>> task, whilst task-specific comments belong in the corresponding enum
>> or structure definitions. Definitely not a single huge comment.
>>
>>>>>
>>>>>> 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.
>>
>> Very well.
>>
>>>>>
>>>>>> 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.
>>>
>>
>> Alright.
>>
>> --
>> Ivan M
>
> Best,
> Ori
>

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

[Ori Kam]

Hi Ivan,

> -----Original Message-----
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> Sent: Wednesday, February 2, 2022 1:24 AM
> Subject: RE: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules
> 
> Hi Ori, Alexander,
> 
> On Tue, 1 Feb 2022, Ori Kam wrote:
> 
> > Hi
> >
> >> -----Original Message-----
> >> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> >> Sent: Tuesday, February 1, 2022 2:18 AM
> >> Subject: RE: [PATCH v2 03/10] ethdev: bring in async queue-based flow rules
> >>
> >> Hi all,
> >>
> >> On Thu, 27 Jan 2022, Alexander Kozyrev wrote:
> >>
> >>> 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.
> >>
> >> Good.
> >>
> >>>>> But it has too many drawbacks in our opinion:
> >>
> >> Is that so?
> >>
> >>>>> 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.
> >>
> >> Yes, all of these are to be handled differently, but this does not mean
> >> that one cannot think of a unified handle format. The application can
> >> remember which cookie corresponds to which operation type after all.
> >>
> >
> > I agree with with Alexander, merging all of those functions to one will result in
> > extra parameters which will be unsued.
> 
> It is not clear which parameters are going to be unused in which cases.
> Task type? Always used. Task data? Always used. Cookie? Presumably,
> always used. But, as I said, you can either change the name to
> your liking or prefer a task data-internal parameter to pass
> task handles back to the caller instead of the "cookie".
> 
Example for unsused parameter will be the returned handle to the flow.

> > Basic programing apporch is that functions should do one thing,
> > it is easier to document / use / test and will run faster.
> 
> By the sound of it, we are starting to run into disagreement in what comes
> to being "easier to document". A unified API by its very definition should
> be easier to document, extend and maintain.
> 
> However, I see your point. This design spares no expense to make
> the code super-fast. I have no strong opinion on whether it is
> good to abandon other basic programming points like keeping
> things generic / unified and extensible, which are as
> important as the point of having simpler functions.
> 
> Perhaps caring about performance indeed prevails here.
> And maybe I should quit being that sceptical.
> 

From my view point performance is everything in this case
since any slow down will mean that application packet processing
will take more time.


> >
> >>>>> Also, the unified function is bloated with various parameters not needed
> >>>> for all operations.
> >>
> >> That is far-fetched.
> >>
> > Just thinking about extra parameter to return the handle only in case
> > of create flow.
> 
> One can make the task data argument "in-out" and keep the handle field
> over there, union-style. And no handle for tasks which do not need one.
> 
I had bad experience with this approach in the RegEx.
I got number of comments why did we merged the in/out to the same
structure.
I perefer to keep it sperated.

> >
> > Also in future I guess we will want to add modify rule and indirect actions
> > I assume that in both case dedicated functions will be much better
> > for example for the indirect use the same names we are using now
> > only adding q in the name and extra queue parameter.
> >
> >> Non-unified set of APIs is also bloated. Takes long to read. Many
> >> duplicating comments. When one has added a new API for a different
> >> type of task, they will have to duplicate many lines one more time.
> >>
> >> In the case of unified API, one has to add a new enum type (one line),
> >> specific (and thus concise) description for it, and the corresponding
> >> structure for the task data. That's it. The rest is up to the PMDs.
> >>
> >> Also, it should be possible to make the task data IN-OUT, to return
> >> its task-specific handle (to address your above concern).
> >>
> >>>>> 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.
> >>
> >
> > I disagree with you I think that having unused and misleading parameters
> > is much more messy from user view point.
> 
> Yes, but currently it is not clear which parameters
> one deems unused or misleading and in which cases.
> 
> >
> >> The documentation for the method is always the same. Precise and concise.
> >> The task data structures will have their own descriptions, yes. But that
> >> does not make the documentation messy. Or am I missing something?
> >>
> >>>>> 2. Performance consideration.
> >>>>> We aimed the new API with the insertion/deletion rate in mind.
> >>
> >> Good.
> >>
> >>>>> Adding if conditions to distinguish between requested operation will cause
> >>>> some degradation.
> >>
> >> Some degradation.. - how serious would it be? What's for the "if"
> >> conditions, well, I believe the compiler is smart enough to deal
> >> with them efficiently. After all, the suggested approach is
> >> a fixed set of operation (task) types. One can have a
> >> static array of task-specific methods in the PMD.
> >> And only one condition to check the value bounds.
> >>
> > yes from our testing just an if will result in degradation.
> > Even just the indirect ref to the PMD function results in degradation.
> > I guess if you try to add an if in the datapath (rx_burst/tx_burst) you will
> > Need to explain the reason.
> 
> I see your point. But, in my understanding, this new design, first of all,
> does not belong in *traffic* path (albeit it is meant for *fast path*,
> in general), so it is not quite correct to compare it with Rx burst
> functionality. Secondly, the idea is to make things non-blocking
> and avoid tedious parsing (hence all the templates and tables),
> and that goal is achieved in any case. So, removing *all* "if"
> checks at any cost might not be that reasonable.
> 
From my point of view it is part of the traffic path.
think about this way, all application needs to do is get the
first packet and offload the rule.
after the offload the application should not see packets from this flow.
This means that any delay in insetion will mean 2 things,
One that packet processing will take more time for the application which
Will result in lower connections per second.
Second more packets will arrive to the application from the same flow.
for example UDP traffic, until the rule is in the HW more packets of this rule
will be routed to the application. So as fast as we insert the rules fewer packets
will arrive to the application.

> BTW, keeping things robust is another basic programming point...
>

Fully agree with that, that is why I think simple functions are much more robust.

 
> >
> >>>>> It is preferred to have separate small functions that do one job and make it
> >>>> efficient.
> >>
> >> A noble idea.
> >>
> > Noble idea is good so lets do it small functions.
> 
> Well, I have no strong objections. You can try and see how it goes.
> 
+1
> >
> >>>> Interfaces are still the same.
> >>
> >> That is the major point of confusion. The application developer has to
> >> be super-careful to tell the queue version of "flow_create" from the
> >> regular one. The two set of APIs are indeed counterparts, and that's
> >> might be ambiguous. Whilst in the unified approach, readers will
> >> understand that this new API is just a task-submitter for
> >> the asynchronous type of operation.
> >>
> >>> Glad I made it clearer. Ivan, what do you think about these considerations?
> >>
> >> Well, I'm not pushing anyone to abandon the existing approach and switch
> >> to the unified API design. But the above points might not be that
> >> difficult to address. This deserves more discussions.
> >>
> >> Any other opinions?
> >>
> > I vote for the suggested API lets try and see.
> 
> Agreed. Sometimes it pays to try and see the results.
> This API has experimental status anyway...
> 
Thanks,
+1
> >
> >>>
> >>>>>
> >>>>>> 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?
> >>
> >> rte_flow_tunnel_decap_set(), rte_flow_tunnel_action_decap_release();
> >> rte_flow_tunnel_match(), rte_flow_tunnel_item_release().
> >>
> >>>>>
> >>>>>> 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.
> >>>>>
> >>
> >> By the sound of it, "push" is just a shorter term for "doorbell".
> >>
> >
> >
> > I don't think the name should be doorbell, since it may not just be doorbell
> > for example PMD may organize the rules in a better way.
> >
> >>>>>> 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.
> >>
> >> That is far-fetched. Polling implies some periodic invocations,
> >> that is correct. But it's up to the application whether to
> >> make this a tight loop or not. Or am I missing something?
> >>
> > I like the dequeue since it also tells the developer that this is how he
> > remove rules from the queue.
> 
> Look. The major point here is that, according to the description,
> task completions may be reordered. Hence, technically, this is
> not a queue, not a ring, but rather a "postponed task list".
> That being said, I believe that the term "queue" (like "q"
> in the API names) can be retained - it's fine to call a
> completion list a "queue". But in what comes to the
> other ring-specific terms like "dequeue", I would
> go for some other alternatives like "poll".
> 
> Please correct me if I've got that wrong.
> 

Maybe in future we will wish to add that the queue can be ordered
also depending on other vendors caps. 

> >
> >>>>> 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.
> >>
> >> Pull is not that informative. The commonly-used term is "poll".
> >> But let's hear some other opinions. Maybe someone has their
> >> way with naming.
> >>
> >>>>>
> >>>>>> 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.
> >>
> >> OK. Seems like I overestimated clarity of the term "cookie".
> >>
> >>>>>
> >>>>>> 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.
> >>
> >> It will reduce the amount of generic (now just repeating) comments.
> >> That's it. Task-specific comments will stay, and that is fine.
> >>
> > Like above API should be as direct as possible.
> 
> OK. Your point is very clear. I do not object.
> But maybe more opinions exist regarding that.
> 
> >
> >>>>> 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.
> >>
> >> Is that so? It shouldn't be like that. It should say about enqueuing a
> >> task, whilst task-specific comments belong in the corresponding enum
> >> or structure definitions. Definitely not a single huge comment.
> >>
> >>>>>
> >>>>>> 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.
> >>
> >> Very well.
> >>
> >>>>>
> >>>>>> 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.
> >>>
> >>
> >> Alright.
> >>
> >> --
> >> Ivan M
> >
> > Best,
> > Ori
> >

Ori