Hi Andrew,
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> Sent: Thursday, February 17, 2022 12:53 PM
> Subject: Re: [PATCH v5 03/10] ethdev: bring in async queue-based flow rules
> operations
>
> Hi Ori,
>
> On 2/16/22 17:53, Ori Kam wrote:
> > Hi Andew,
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> >> Sent: Wednesday, February 16, 2022 3:34 PM
> >> Subject: Re: [PATCH v5 03/10] ethdev: bring in async queue-based flow
> >> rules operations
> >>
> >> On 2/12/22 05:19, Alexander Kozyrev wrote:
> >>> On Fri, Feb 11, 2022 7:42 Andrew Rybchenko
> >>> <andrew.rybche...@oktetlabs.ru>:
> >>>> On 2/11/22 05:26, Alexander Kozyrev wrote:
> >>>>> A new, faster, queue-based flow rules management mechanism is needed
> >>>> for
> >>>>> applications offloading rules inside the datapath. This asynchronous
> >>>>> and lockless mechanism frees the CPU for further packet processing and
> >>>>> reduces the performance impact of the flow rules creation/destruction
> >>>>> on the datapath. Note that queues are not thread-safe and the queue
> >>>>> should be accessed from the same thread for all queue operations.
> >>>>> It is the responsibility of the app to sync the queue functions in case
> >>>>> of multi-threaded access to the same queue.
> >>>>>
> >>>>> The rte_flow_q_flow_create() function enqueues a flow creation to the
> >>>>> requested queue. It benefits from already configured resources and sets
> >>>>> unique values on top of item and action templates. A flow rule is
> >>>>> enqueued
> >>>>> on the specified flow queue and offloaded asynchronously to the
> >>>> hardware.
> >>>>> The function returns immediately to spare CPU for further packet
> >>>>> processing. The application must invoke the rte_flow_q_pull() function
> >>>>> to complete the flow rule operation offloading, to clear the queue, and
> >>>>> to
> >>>>> receive the operation status. The rte_flow_q_flow_destroy() function
> >>>>> enqueues a flow destruction to the requested queue.
> >>>>>
> >>>>> Signed-off-by: Alexander Kozyrev <akozy...@nvidia.com>
> >>>>> Acked-by: Ori Kam <or...@nvidia.com>
> >>
> >> [snip]
> >>
> >>>>> +
> >>>>> +- Available operation types: rule creation, rule destruction,
> >>>>> + indirect rule creation, indirect rule destruction, indirect rule
> >>>>> update.
> >>>>> +
> >>>>> +- Operations may be reordered within a queue.
> >>>>
> >>>> Do we want to have barriers?
> >>>> E.g. create rule, destroy the same rule -> reoder -> destroy fails, rule
> >>>> lives forever.
> >>>
> >>> API design is crafter with the throughput as the main goal in mind.
> >>> We allow user to enforce any ordering outside these functions.
> >>> Another point that not all PMDs/NIC will have this out-of-order execution.
> >>
> >> Throughput is nice, but there more important requirements
> >> which must be satistied before talking about performance.
> >> Could you explain me what I should do based on which
> >> information from NIC in order to solve above problem?
> >>
> >
> > The idea is that if application has dependency between the rules/ rules
> > operations.
> > It should wait for the completion of the operation before sending the
> > dependent operation.
> > In the example you provided above, according to the documeation application
> > should wait
> > for the completion of the flow creation before destroying it.
>
> I see, thanks. May be I read documentation not that attentive.
> I'll reread on the next version review cycle.
>
> >>>>> +
> >>>>> +- Operations can be postponed and pushed to NIC in batches.
> >>>>> +
> >>>>> +- Results pulling must be done on time to avoid queue overflows.
> >>>>
> >>>> polling? (as libc poll() which checks status of file descriptors)
> >>>> it is not pulling the door to open it :)
> >>>
> >>> poll waits for some event on a file descriptor as it title says.
> >>> And then user has to invoke read() to actually get any info from the fd.
> >>> The point of our function is to return the result immediately, thus
> >>> pulling.
> >>> We had many names appearing in the thread for these functions.
> >>> As we know, naming variables is the second hardest thing in programming.
> >>> I wanted this pull for results pulling be a counterpart for the push for
> >>> pushing the operations to a NIC. Another idea is pop/push pair, but they
> >>> are
> >>> more like for operations only, not for results.
> >>> Having said that I'm at the point of accepting any name here.
> >>
> >> I agree that it is hard to choose good naming.
> >> Just want to say that polling is not alway waiting.
> >>
> >> poll - check the status of (a device), especially as part of a repeated
> >> cycle.
> >>
> >> Here we're checking status of flow engine requests and yes,
> >> finally in a repeated cycle.
> >>
> >> [snip]
> >>
> >>>>> +/**
> >>>>> + * @warning
> >>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>>>> + *
> >>>>> + * Queue operation attributes.
> >>>>> + */
> >>>>> +struct rte_flow_q_ops_attr {
> >>>>> + /**
> >>>>> + * The user data that will be returned on the completion events.
> >>>>> + */
> >>>>> + void *user_data;
> >>>>
> >>>> IMHO it must not be hiddne in attrs. It is a key information
> >>>> which is used to understand the opration result. It should
> >>>> be passed separately.
> >>>
> >>> Maybe, on the other hand it is optional and may not be needed by an
> >>> application.
> >>
> >> I don't understand how it is possible. Without it application
> >> don't know fate of its requests.
> >>
> > IMHO since user_data should be in all related operations API
> > along with the attr, splitting the user_data will just add extra parameter
> > to each function call. Since we have number of functions and will add
> > more in future I think it will be best to keep it in this location.
>
> My problem with hiding user_data inside attr is that
> 'user_data' is not an auxiliary attribute defining extra
> properties of the request. It is a key information.
> May be attr is not an ideal name for such grouping
> of parameters. Unfortunately I have no better ideas right now.
>
I understand your point, if you don't have objections lets keep the current one
and if needed we will modify.
Is that O.K?
> Andrew.
