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

[Thomas Monjalon <thomas@monjalon.net>]

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?

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

> > > +/**
> > > + * @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.