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

[Ori Kam <orika@nvidia.com>]

Hi Andrew,

> -----Original Message-----
> From: Ori Kam
> Sent: Thursday, February 17, 2022 1:09 PM
> Subject: RE: [PATCH v5 03/10] ethdev: bring in async queue-based flow rules operations
> 
> Hi Andrew,
> 
> > -----Original Message-----
> > From: Andrew Rybchenko <andrew.rybchenko@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.rybchenko@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.rybchenko@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 <akozyrev@nvidia.com>
> > >>>>> Acked-by: Ori Kam <orika@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?
> 

Thinking about it again,
lets move it to a dedecated parameter.

Ori
> > Andrew.