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

[Alexander Kozyrev <akozyrev@nvidia.com>]

On Sat, Feb 12, 2022 4:25 Thomas Monjalon <thomas@monjalon.net> wrote:
> 12/02/2022 03:19, Alexander Kozyrev:
> > On Fri, Feb 11, 2022 7:42 Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>:
> > > On 2/11/22 05:26, Alexander Kozyrev wrote:
> > > > +__rte_experimental
> > > > +struct rte_flow *
> > > > +rte_flow_q_flow_create(uint16_t port_id,
> > >
> > > flow_q_flow does not sound like a good nameing, consider:
> > > rte_flow_q_rule_create() is
> <subsystem>_<subtype>_<object>_<action>
> >
> > More like:
> > <subsystem>_<subtype>_<object>_<action>
> >  <rte>_<flow>_<rule_create_operation>_<queue>
> > Which is pretty lengthy name as for me.
> 
> Naming :)
> This one may be improved I think.
> What is the problem with replacing "flow" with "rule"?
> Is it the right meaning?

I've got a better naming for all the functions. What do you think about this?
Asynchronous rte_flow_async_create and rte_flow_async_destroy functions
as an extension of synchronous rte_flow_create/ rte_flow_destroy API.
The same is true for asynchronous API for indirect actions:
	rte_flow_async_action_handle_create;
	rte_flow_async_action_handle_destroy;
	rte_flow_async_action_handle_update;
And rte_flow_push/rte_flow_pull without "_q_" part to make them clearer.
And yes, I'm still thinking pull is better than poll since we are actually retrieving
something, not just checking if it has something we can retrieve.
Let me know if we can agree on this scheme? Look pretty close to existing one.

> > > > +__rte_experimental
> > > > +struct rte_flow_action_handle *
> > > > +rte_flow_q_action_handle_create(uint16_t port_id,
> > > > +		uint32_t queue_id,
> > > > +		const struct rte_flow_q_ops_attr *q_ops_attr,
> > > > +		const struct rte_flow_indir_action_conf *indir_action_conf,
> > > > +		const struct rte_flow_action *action,
> > >
> > > I don't understand why it differs so much from rule creation.
> > > Why is action template not used?
> > > IMHO indirect actions should be dropped from the patch
> > > and added separately since it is a separate feature.
> >
> > I agree, they deserve a sperate patch since they are rather resource
> creations.
> > But, I'm afraid it is too late for RC1.
> 
> I think it could be done for RC2.

No problem, I'll create a separate commit for indirect actions.

> 
> > > > +/**
> > > > + * @warning
> > > > + * @b EXPERIMENTAL: this API may change without prior notice.
> > > > + *
> > > > + * Pull a rte flow operation.
> > > > + * The application must invoke this function in order to complete
> > > > + * the flow rule offloading and to retrieve the flow rule operation
> status.
> > > > + *
> > > > + * @param port_id
> > > > + *   Port identifier of Ethernet device.
> > > > + * @param queue_id
> > > > + *   Flow queue which is used to pull the operation.
> > > > + * @param[out] res
> > > > + *   Array of results that will be set.
> > > > + * @param[in] n_res
> > > > + *   Maximum number of results that can be returned.
> > > > + *   This value is equal to the size of the res array.
> > > > + * @param[out] error
> > > > + *   Perform verbose error reporting if not NULL.
> > > > + *   PMDs initialize this structure in case of error only.
> > > > + *
> > > > + * @return
> > > > + *   Number of results that were pulled,
> > > > + *   a negative errno value otherwise and rte_errno is set.
> > >
> > > Don't we want to define negative error code meaning?
> >
> > They are all standard, don't think we need another copy-paste here.
> 
> That's an API, it needs to be all explicit.
> I missed it before, we should add the error codes here.

I'll add if you want to see them listed.