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

[Alexander Kozyrev <akozyrev@nvidia.com>]

On Monday, February 21, 2022 9:49 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>:
> [snip]
> 
> > @@ -3777,6 +3782,125 @@ and pattern and actions templates are created.
> >   				&actions_templates, nb_actions_templ,
> >   				&error);
> >
> > +Asynchronous operations
> > +-----------------------
> > +
> > +Flow rules management can be done via special lockless flow
> management queues.
> > +- Queue operations are asynchronous and not thread-safe.
> > +
> > +- Operations can thus be invoked by the app's datapath,
> > +  packet processing can continue while queue operations are processed by
> NIC.
> > +
> > +- Number of flow queues is configured at initialization stage.
> > +
> > +- Available operation types: rule creation, rule destruction,
> > +  indirect rule creation, indirect rule destruction, indirect rule update.
> > +
> > +- Operations may be reordered within a queue.
> > +
> > +- Operations can be postponed and pushed to NIC in batches.
> > +
> > +- Results pulling must be done on time to avoid queue overflows.
> 
> I guess the documenation is for applications, but IMHO it is a
> driver responsiblity. Application should not care  about it.
> Yes, applicatoin should do pulling, but it should not think
> about overflow. Request should be rejected if there is no space
> in queue.

It is rejected in case of queue overflow and -EAGAIN is returned.

> > +
> > +- User data is returned as part of the result to identify an operation.
> 
> Also "User data should uniquelly identify request (may be except corner
> case when only one request is enqueued at most)."

It is up to application what to put into the user data and how it differentiates
between the operations. I don't want to restrict this in any way.

> > +
> > +- Flow handle is valid once the creation operation is enqueued and must
> be
> > +  destroyed even if the operation is not successful and the rule is not
> inserted.
> > +
> > +- Application must wait for the creation operation result before
> enqueueing
> > +  the deletion operation to make sure the creation is processed by NIC.
> > +
> 
> [snip]
> 
> > +The asynchronous flow rule insertion logic can be broken into two phases.
> > +
> > +1. Initialization stage as shown here:
> > +
> > +.. _figure_rte_flow_async_init:
> > +
> > +.. figure:: img/rte_flow_async_init.*
> > +
> > +2. Main loop as presented on a datapath application example:
> > +
> > +.. _figure_rte_flow_async_usage:
> > +
> > +.. figure:: img/rte_flow_async_usage.*
> > +
> > +Enqueue creation operation
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Enqueueing a flow rule creation operation is similar to simple creation.
> > +
> > +.. code-block:: c
> > +
> > +	struct rte_flow *
> > +	rte_flow_async_create(uint16_t port_id,
> > +			      uint32_t queue_id,
> > +			      const struct rte_flow_q_ops_attr *q_ops_attr,
> 
> May be rte_flow_async_ops_attr *attr?

It is still operation on a queue, but I'll rename it to rte_flow_ops_attr to
simplify the description: it is operations attributes.

> > +			      struct rte_flow_template_table *template_table,
> > +			      const struct rte_flow_item pattern[],
> > +			      uint8_t pattern_template_index,
> > +			      const struct rte_flow_action actions[],
> > +			      uint8_t actions_template_index,
> > +			      void *user_data,
> > +			      struct rte_flow_error *error);
> > +
> > +A valid handle in case of success is returned. It must be destroyed later
> > +by calling ``rte_flow_async_destroy()`` even if the rule is rejected by HW.
> 
> [snip]
> 
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > index e9f684eedb..4e7b202522 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -1396,6 +1396,7 @@ rte_flow_flex_item_release(uint16_t port_id,
> >   int
> >   rte_flow_info_get(uint16_t port_id,
> >   		  struct rte_flow_port_info *port_info,
> > +		  struct rte_flow_queue_info *queue_info,
> 
> It should be either optional (update description) or sanity
> checked vs NULL below (similar to port_info).

Ok.

> >   		  struct rte_flow_error *error)
> >   {
> >   	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > @@ -1415,7 +1416,7 @@ rte_flow_info_get(uint16_t port_id,
> >   		return -rte_errno;
> >   	if (likely(!!ops->info_get)) {
> >   		return flow_err(port_id,
> > -				ops->info_get(dev, port_info, error),
> > +				ops->info_get(dev, port_info, queue_info,
> error),
> >   				error);
> >   	}
> >   	return rte_flow_error_set(error, ENOTSUP,
> > @@ -1426,6 +1427,8 @@ rte_flow_info_get(uint16_t port_id,
> >   int
> >   rte_flow_configure(uint16_t port_id,
> >   		   const struct rte_flow_port_attr *port_attr,
> > +		   uint16_t nb_queue,
> > +		   const struct rte_flow_queue_attr *queue_attr[],
> 
> Is it really an array of pointers? If yes, why?

Yes, it is. Different queue may have different attributes (sizes...).

> >   		   struct rte_flow_error *error)
> >   {
> >   	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > @@ -1433,7 +1436,7 @@ rte_flow_configure(uint16_t port_id,
> >   	int ret;
> >
> >   	dev->data->flow_configured = 0;
> > -	if (port_attr == NULL) {
> > +	if (port_attr == NULL || queue_attr == NULL) {
> >   		RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n",
> port_id);
> 
> Log message becomes misleading

Will fix this.

> [snip]
> 
> >   		return -EINVAL;
> >   	}
> > @@ -1452,7 +1455,7 @@ rte_flow_configure(uint16_t port_id,
> >   	if (unlikely(!ops))
> >   		return -rte_errno;
> >   	if (likely(!!ops->configure)) {
> > -		ret = ops->configure(dev, port_attr, error);
> > +		ret = ops->configure(dev, port_attr, nb_queue, queue_attr,
> error);
> >   		if (ret == 0)
> >   			dev->data->flow_configured = 1;
> >   		return flow_err(port_id, ret, error);
> > @@ -1713,3 +1716,104 @@ rte_flow_template_table_destroy(uint16_t
> port_id,
> >   				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >   				  NULL, rte_strerror(ENOTSUP));
> >   }
> > +
> > +struct rte_flow *
> > +rte_flow_async_create(uint16_t port_id,
> > +		      uint32_t queue_id,
> > +		      const struct rte_flow_q_ops_attr *q_ops_attr,
> > +		      struct rte_flow_template_table *template_table,
> > +		      const struct rte_flow_item pattern[],
> > +		      uint8_t pattern_template_index,
> > +		      const struct rte_flow_action actions[],
> > +		      uint8_t actions_template_index,
> > +		      void *user_data,
> > +		      struct rte_flow_error *error)
> > +{
> > +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > +	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> > +	struct rte_flow *flow;
> > +
> > +	if (unlikely(!ops))
> > +		return NULL;
> > +	if (likely(!!ops->async_create)) {
> 
> Hm, we should make a consistent decision. If it is super-
> critical fast path - we should have no sanity checks at all.
> If no, we should have all simple sanity checks. Otherwise,
> I don't understand why we do some checks and ignore another.

Agree. Will remove every check in this async API.

> [snip]
> 
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index 776e8ccc11..9e71a576f6 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -4884,6 +4884,10 @@ rte_flow_flex_item_release(uint16_t port_id,
> >    *
> >    */
> >   struct rte_flow_port_info {
> > +	/**
> > +	 * Maximum umber of queues for asynchronous operations.
> 
> umber -> number

Will fix typo, thanks.