[v5,01/10] ethdev: introduce flow pre-configuration hints

  The flow rules creation/destruction at a large scale incurs a performance
penalty and may negatively impact the packet processing when used
as part of the datapath logic. This is mainly because software/hardware
resources are allocated and prepared during the flow rule creation.

In order to optimize the insertion rate, PMD may use some hints provided
by the application at the initialization phase. The rte_flow_configure()
function allows to pre-allocate all the needed resources beforehand.
These resources can be used at a later stage without costly allocations.
Every PMD may use only the subset of hints and ignore unused ones or
fail in case the requested configuration is not supported.

The rte_flow_info_get() is available to retrieve the information about
supported pre-configurable resources. Both these functions must be called
before any other usage of the flow API engine.

Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst     |  37 +++++++++
 doc/guides/rel_notes/release_22_03.rst |   6 ++
 lib/ethdev/rte_flow.c                  |  40 +++++++++
 lib/ethdev/rte_flow.h                  | 108 +++++++++++++++++++++++++
 lib/ethdev/rte_flow_driver.h           |  10 +++
 lib/ethdev/version.map                 |   2 +
 6 files changed, 203 insertions(+)
  

On 2/11/22 05:26, Alexander Kozyrev wrote:
> The flow rules creation/destruction at a large scale incurs a performance
> penalty and may negatively impact the packet processing when used
> as part of the datapath logic. This is mainly because software/hardware
> resources are allocated and prepared during the flow rule creation.
> 
> In order to optimize the insertion rate, PMD may use some hints provided
> by the application at the initialization phase. The rte_flow_configure()
> function allows to pre-allocate all the needed resources beforehand.
> These resources can be used at a later stage without costly allocations.
> Every PMD may use only the subset of hints and ignore unused ones or
> fail in case the requested configuration is not supported.
> 
> The rte_flow_info_get() is available to retrieve the information about
> supported pre-configurable resources. Both these functions must be called
> before any other usage of the flow API engine.
> 
> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>
> ---
>   doc/guides/prog_guide/rte_flow.rst     |  37 +++++++++
>   doc/guides/rel_notes/release_22_03.rst |   6 ++
>   lib/ethdev/rte_flow.c                  |  40 +++++++++
>   lib/ethdev/rte_flow.h                  | 108 +++++++++++++++++++++++++
>   lib/ethdev/rte_flow_driver.h           |  10 +++
>   lib/ethdev/version.map                 |   2 +
>   6 files changed, 203 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index b4aa9c47c2..72fb1132ac 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -3589,6 +3589,43 @@ Return values:
>   
>   - 0 on success, a negative errno value otherwise and ``rte_errno`` is set.
>   
> +Flow engine configuration
> +-------------------------
> +
> +Configure flow API management.
> +
> +An application may provide some parameters at the initialization phase about
> +rules engine configuration and/or expected flow rules characteristics.
> +These parameters may be used by PMD to preallocate resources and configure NIC.
> +
> +Configuration
> +~~~~~~~~~~~~~
> +
> +This function performs the flow API management configuration and
> +pre-allocates needed resources beforehand to avoid costly allocations later.
> +Expected number of counters or meters in an application, for example,
> +allow PMD to prepare and optimize NIC memory layout in advance.
> +``rte_flow_configure()`` must be called before any flow rule is created,
> +but after an Ethernet device is configured.
> +
> +.. code-block:: c
> +
> +   int
> +   rte_flow_configure(uint16_t port_id,
> +                     const struct rte_flow_port_attr *port_attr,
> +                     struct rte_flow_error *error);
> +
> +Information about resources that can benefit from pre-allocation can be
> +retrieved via ``rte_flow_info_get()`` API. It returns the maximum number
> +of pre-configurable resources for a given port on a system.
> +
> +.. code-block:: c
> +
> +   int
> +   rte_flow_info_get(uint16_t port_id,
> +                     struct rte_flow_port_info *port_info,
> +                     struct rte_flow_error *error);
> +
>   .. _flow_isolated_mode:
>   
>   Flow isolated mode
> diff --git a/doc/guides/rel_notes/release_22_03.rst b/doc/guides/rel_notes/release_22_03.rst
> index f03183ee86..2a47a37f0a 100644
> --- a/doc/guides/rel_notes/release_22_03.rst
> +++ b/doc/guides/rel_notes/release_22_03.rst
> @@ -69,6 +69,12 @@ New Features
>     New APIs, ``rte_eth_dev_priority_flow_ctrl_queue_info_get()`` and
>     ``rte_eth_dev_priority_flow_ctrl_queue_configure()``, was added.
>   
> +* ** Added functions to configure Flow API engine
> +
> +  * ethdev: Added ``rte_flow_configure`` API to configure Flow Management
> +    engine, allowing to pre-allocate some resources for better performance.
> +    Added ``rte_flow_info_get`` API to retrieve pre-configurable resources.
> +
>   * **Updated AF_XDP PMD**
>   
>     * Added support for libxdp >=v1.2.2.
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index a93f68abbc..66614ae29b 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
>   	ret = ops->flex_item_release(dev, handle, error);
>   	return flow_err(port_id, ret, error);
>   }
> +
> +int
> +rte_flow_info_get(uint16_t port_id,
> +		  struct rte_flow_port_info *port_info,
> +		  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);
> +
> +	if (unlikely(!ops))
> +		return -rte_errno;
> +	if (likely(!!ops->info_get)) {

expected ethdev state must be validated. Just configured?

> +		return flow_err(port_id,
> +				ops->info_get(dev, port_info, error),

port_info must be checked vs NULL

> +				error);
> +	}
> +	return rte_flow_error_set(error, ENOTSUP,
> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +				  NULL, rte_strerror(ENOTSUP));
> +}
> +
> +int
> +rte_flow_configure(uint16_t port_id,
> +		   const struct rte_flow_port_attr *port_attr,
> +		   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);
> +
> +	if (unlikely(!ops))
> +		return -rte_errno;
> +	if (likely(!!ops->configure)) {

The API must validate ethdev state. configured and not started?

> +		return flow_err(port_id,
> +				ops->configure(dev, port_attr, error),

port_attr must be checked vs NULL

> +				error);
> +	}
> +	return rte_flow_error_set(error, ENOTSUP,
> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +				  NULL, rte_strerror(ENOTSUP));
> +}
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 1031fb246b..92be2a9a89 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t port_id,
>   			   const struct rte_flow_item_flex_handle *handle,
>   			   struct rte_flow_error *error);
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Information about available pre-configurable resources.
> + * The zero value means a resource cannot be pre-allocated.
> + *
> + */
> +struct rte_flow_port_info {
> +	/**
> +	 * Number of pre-configurable counter actions.
> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
> +	 */
> +	uint32_t nb_counters;

Name says that it is a number of counters, but description
says that it is about actions.
Also I don't understand what does "pre-configurable" mean.
Isn't it a maximum number of available counters?
If no, how can I find a maximum?

> +	/**
> +	 * Number of pre-configurable aging flows actions.
> +	 * @see RTE_FLOW_ACTION_TYPE_AGE
> +	 */
> +	uint32_t nb_aging_flows;

Same

> +	/**
> +	 * Number of pre-configurable traffic metering actions.
> +	 * @see RTE_FLOW_ACTION_TYPE_METER
> +	 */
> +	uint32_t nb_meters;

Same

> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Retrieve configuration attributes supported by the port.

Description should be a bit more flow API aware.
Right now it sounds too generic.

> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param[out] port_info
> + *   A pointer to a structure of type *rte_flow_port_info*
> + *   to be filled with the contextual information of the port.
> + * @param[out] error
> + *   Perform verbose error reporting if not NULL.
> + *   PMDs initialize this structure in case of error only.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +__rte_experimental
> +int
> +rte_flow_info_get(uint16_t port_id,
> +		  struct rte_flow_port_info *port_info,
> +		  struct rte_flow_error *error);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Resource pre-allocation and pre-configuration settings.

What is the difference between pre-allocation and pre-configuration?
Why are both mentioned above, but just pre-configured actions are
mentioned below?

> + * The zero value means on demand resource allocations only.
> + *
> + */
> +struct rte_flow_port_attr {
> +	/**
> +	 * Number of counter actions pre-configured.
> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
> +	 */
> +	uint32_t nb_counters;
> +	/**
> +	 * Number of aging flows actions pre-configured.
> +	 * @see RTE_FLOW_ACTION_TYPE_AGE
> +	 */
> +	uint32_t nb_aging_flows;
> +	/**
> +	 * Number of traffic metering actions pre-configured.
> +	 * @see RTE_FLOW_ACTION_TYPE_METER
> +	 */
> +	uint32_t nb_meters;
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Configure the port's flow API engine.
> + *
> + * This API can only be invoked before the application
> + * starts using the rest of the flow library functions.
> + *
> + * The API can be invoked multiple times to change the
> + * settings. The port, however, may reject the changes.
> + *
> + * Parameters in configuration attributes must not exceed
> + * numbers of resources returned by the rte_flow_info_get API.
> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param[in] port_attr
> + *   Port configuration attributes.
> + * @param[out] error
> + *   Perform verbose error reporting if not NULL.
> + *   PMDs initialize this structure in case of error only.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +__rte_experimental
> +int
> +rte_flow_configure(uint16_t port_id,
> +		   const struct rte_flow_port_attr *port_attr,
> +		   struct rte_flow_error *error);
> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/ethdev/rte_flow_driver.h b/lib/ethdev/rte_flow_driver.h
> index f691b04af4..7c29930d0f 100644
> --- a/lib/ethdev/rte_flow_driver.h
> +++ b/lib/ethdev/rte_flow_driver.h
> @@ -152,6 +152,16 @@ struct rte_flow_ops {
>   		(struct rte_eth_dev *dev,
>   		 const struct rte_flow_item_flex_handle *handle,
>   		 struct rte_flow_error *error);
> +	/** See rte_flow_info_get() */
> +	int (*info_get)
> +		(struct rte_eth_dev *dev,
> +		 struct rte_flow_port_info *port_info,
> +		 struct rte_flow_error *err);
> +	/** See rte_flow_configure() */
> +	int (*configure)
> +		(struct rte_eth_dev *dev,
> +		 const struct rte_flow_port_attr *port_attr,
> +		 struct rte_flow_error *err);
>   };
>   
>   /**
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index cd0c4c428d..f1235aa913 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -260,6 +260,8 @@ EXPERIMENTAL {
>   	# added in 22.03
>   	rte_eth_dev_priority_flow_ctrl_queue_configure;
>   	rte_eth_dev_priority_flow_ctrl_queue_info_get;
> +	rte_flow_info_get;
> +	rte_flow_configure;
>   };
>   
>   INTERNAL {
  

On Friday, February 11, 2022 5:17 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
> Sent: Friday, February 11, 2022 5:17
> To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
> Cc: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; ivan.malov@oktetlabs.ru; ferruh.yigit@intel.com;
> mohammad.abdul.awal@intel.com; qi.z.zhang@intel.com; jerinj@marvell.com;
> ajit.khaparde@broadcom.com; bruce.richardson@intel.com
> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration hints
> 
> On 2/11/22 05:26, Alexander Kozyrev wrote:
> > The flow rules creation/destruction at a large scale incurs a performance
> > penalty and may negatively impact the packet processing when used
> > as part of the datapath logic. This is mainly because software/hardware
> > resources are allocated and prepared during the flow rule creation.
> >
> > In order to optimize the insertion rate, PMD may use some hints provided
> > by the application at the initialization phase. The rte_flow_configure()
> > function allows to pre-allocate all the needed resources beforehand.
> > These resources can be used at a later stage without costly allocations.
> > Every PMD may use only the subset of hints and ignore unused ones or
> > fail in case the requested configuration is not supported.
> >
> > The rte_flow_info_get() is available to retrieve the information about
> > supported pre-configurable resources. Both these functions must be called
> > before any other usage of the flow API engine.
> >
> > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > Acked-by: Ori Kam <orika@nvidia.com>
> > ---
> >   doc/guides/prog_guide/rte_flow.rst     |  37 +++++++++
> >   doc/guides/rel_notes/release_22_03.rst |   6 ++
> >   lib/ethdev/rte_flow.c                  |  40 +++++++++
> >   lib/ethdev/rte_flow.h                  | 108 +++++++++++++++++++++++++
> >   lib/ethdev/rte_flow_driver.h           |  10 +++
> >   lib/ethdev/version.map                 |   2 +
> >   6 files changed, 203 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> > index b4aa9c47c2..72fb1132ac 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -3589,6 +3589,43 @@ Return values:
> >
> >   - 0 on success, a negative errno value otherwise and ``rte_errno`` is set.
> >
> > +Flow engine configuration
> > +-------------------------
> > +
> > +Configure flow API management.
> > +
> > +An application may provide some parameters at the initialization phase about
> > +rules engine configuration and/or expected flow rules characteristics.
> > +These parameters may be used by PMD to preallocate resources and
> configure NIC.
> > +
> > +Configuration
> > +~~~~~~~~~~~~~
> > +
> > +This function performs the flow API management configuration and
> > +pre-allocates needed resources beforehand to avoid costly allocations later.
> > +Expected number of counters or meters in an application, for example,
> > +allow PMD to prepare and optimize NIC memory layout in advance.
> > +``rte_flow_configure()`` must be called before any flow rule is created,
> > +but after an Ethernet device is configured.
> > +
> > +.. code-block:: c
> > +
> > +   int
> > +   rte_flow_configure(uint16_t port_id,
> > +                     const struct rte_flow_port_attr *port_attr,
> > +                     struct rte_flow_error *error);
> > +
> > +Information about resources that can benefit from pre-allocation can be
> > +retrieved via ``rte_flow_info_get()`` API. It returns the maximum number
> > +of pre-configurable resources for a given port on a system.
> > +
> > +.. code-block:: c
> > +
> > +   int
> > +   rte_flow_info_get(uint16_t port_id,
> > +                     struct rte_flow_port_info *port_info,
> > +                     struct rte_flow_error *error);
> > +
> >   .. _flow_isolated_mode:
> >
> >   Flow isolated mode
> > diff --git a/doc/guides/rel_notes/release_22_03.rst
> b/doc/guides/rel_notes/release_22_03.rst
> > index f03183ee86..2a47a37f0a 100644
> > --- a/doc/guides/rel_notes/release_22_03.rst
> > +++ b/doc/guides/rel_notes/release_22_03.rst
> > @@ -69,6 +69,12 @@ New Features
> >     New APIs, ``rte_eth_dev_priority_flow_ctrl_queue_info_get()`` and
> >     ``rte_eth_dev_priority_flow_ctrl_queue_configure()``, was added.
> >
> > +* ** Added functions to configure Flow API engine
> > +
> > +  * ethdev: Added ``rte_flow_configure`` API to configure Flow Management
> > +    engine, allowing to pre-allocate some resources for better performance.
> > +    Added ``rte_flow_info_get`` API to retrieve pre-configurable resources.
> > +
> >   * **Updated AF_XDP PMD**
> >
> >     * Added support for libxdp >=v1.2.2.
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > index a93f68abbc..66614ae29b 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
> >   	ret = ops->flex_item_release(dev, handle, error);
> >   	return flow_err(port_id, ret, error);
> >   }
> > +
> > +int
> > +rte_flow_info_get(uint16_t port_id,
> > +		  struct rte_flow_port_info *port_info,
> > +		  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);
> > +
> > +	if (unlikely(!ops))
> > +		return -rte_errno;
> > +	if (likely(!!ops->info_get)) {
> 
> expected ethdev state must be validated. Just configured?
> 
> > +		return flow_err(port_id,
> > +				ops->info_get(dev, port_info, error),
> 
> port_info must be checked vs NULL

We don’t have any NULL checks for parameters in the whole ret flow API library.
See rte_flow_create() for example. attributes, pattern and actions are passed to PMD unchecked.

> > +				error);
> > +	}
> > +	return rte_flow_error_set(error, ENOTSUP,
> > +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +				  NULL, rte_strerror(ENOTSUP));
> > +}
> > +
> > +int
> > +rte_flow_configure(uint16_t port_id,
> > +		   const struct rte_flow_port_attr *port_attr,
> > +		   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);
> > +
> > +	if (unlikely(!ops))
> > +		return -rte_errno;
> > +	if (likely(!!ops->configure)) {
> 
> The API must validate ethdev state. configured and not started?
Again, we have no such validation for any rte flow API today.

> 
> > +		return flow_err(port_id,
> > +				ops->configure(dev, port_attr, error),
> 
> port_attr must be checked vs NULL
Same.

> > +				error);
> > +	}
> > +	return rte_flow_error_set(error, ENOTSUP,
> > +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +				  NULL, rte_strerror(ENOTSUP));
> > +}
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index 1031fb246b..92be2a9a89 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t port_id,
> >   			   const struct rte_flow_item_flex_handle *handle,
> >   			   struct rte_flow_error *error);
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Information about available pre-configurable resources.
> > + * The zero value means a resource cannot be pre-allocated.
> > + *
> > + */
> > +struct rte_flow_port_info {
> > +	/**
> > +	 * Number of pre-configurable counter actions.
> > +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
> > +	 */
> > +	uint32_t nb_counters;
> 
> Name says that it is a number of counters, but description
> says that it is about actions.
> Also I don't understand what does "pre-configurable" mean.
> Isn't it a maximum number of available counters?
> If no, how can I find a maximum?
It is number of pre-allocated and pre-configured actions.
How are they pr-configured is up to PDM driver.
But let's change to "pre-configured" everywhere.
Configuration includes some memory allocation anyway. 

> 
> > +	/**
> > +	 * Number of pre-configurable aging flows actions.
> > +	 * @see RTE_FLOW_ACTION_TYPE_AGE
> > +	 */
> > +	uint32_t nb_aging_flows;
> 
> Same
Ditto.
 
> > +	/**
> > +	 * Number of pre-configurable traffic metering actions.
> > +	 * @see RTE_FLOW_ACTION_TYPE_METER
> > +	 */
> > +	uint32_t nb_meters;
> 
> Same
Ditto.

> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Retrieve configuration attributes supported by the port.
> 
> Description should be a bit more flow API aware.
> Right now it sounds too generic.
Ok, how about
"Get information about flow engine pre-configurable resources."
 
> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param[out] port_info
> > + *   A pointer to a structure of type *rte_flow_port_info*
> > + *   to be filled with the contextual information of the port.
> > + * @param[out] error
> > + *   Perform verbose error reporting if not NULL.
> > + *   PMDs initialize this structure in case of error only.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_info_get(uint16_t port_id,
> > +		  struct rte_flow_port_info *port_info,
> > +		  struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Resource pre-allocation and pre-configuration settings.
> 
> What is the difference between pre-allocation and pre-configuration?
> Why are both mentioned above, but just pre-configured actions are
> mentioned below?
Please see answer to this question above.
 
> > + * The zero value means on demand resource allocations only.
> > + *
> > + */
> > +struct rte_flow_port_attr {
> > +	/**
> > +	 * Number of counter actions pre-configured.
> > +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
> > +	 */
> > +	uint32_t nb_counters;
> > +	/**
> > +	 * Number of aging flows actions pre-configured.
> > +	 * @see RTE_FLOW_ACTION_TYPE_AGE
> > +	 */
> > +	uint32_t nb_aging_flows;
> > +	/**
> > +	 * Number of traffic metering actions pre-configured.
> > +	 * @see RTE_FLOW_ACTION_TYPE_METER
> > +	 */
> > +	uint32_t nb_meters;
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Configure the port's flow API engine.
> > + *
> > + * This API can only be invoked before the application
> > + * starts using the rest of the flow library functions.
> > + *
> > + * The API can be invoked multiple times to change the
> > + * settings. The port, however, may reject the changes.
> > + *
> > + * Parameters in configuration attributes must not exceed
> > + * numbers of resources returned by the rte_flow_info_get API.
> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param[in] port_attr
> > + *   Port configuration attributes.
> > + * @param[out] error
> > + *   Perform verbose error reporting if not NULL.
> > + *   PMDs initialize this structure in case of error only.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_configure(uint16_t port_id,
> > +		   const struct rte_flow_port_attr *port_attr,
> > +		   struct rte_flow_error *error);
> > +
> >   #ifdef __cplusplus
> >   }
> >   #endif
> > diff --git a/lib/ethdev/rte_flow_driver.h b/lib/ethdev/rte_flow_driver.h
> > index f691b04af4..7c29930d0f 100644
> > --- a/lib/ethdev/rte_flow_driver.h
> > +++ b/lib/ethdev/rte_flow_driver.h
> > @@ -152,6 +152,16 @@ struct rte_flow_ops {
> >   		(struct rte_eth_dev *dev,
> >   		 const struct rte_flow_item_flex_handle *handle,
> >   		 struct rte_flow_error *error);
> > +	/** See rte_flow_info_get() */
> > +	int (*info_get)
> > +		(struct rte_eth_dev *dev,
> > +		 struct rte_flow_port_info *port_info,
> > +		 struct rte_flow_error *err);
> > +	/** See rte_flow_configure() */
> > +	int (*configure)
> > +		(struct rte_eth_dev *dev,
> > +		 const struct rte_flow_port_attr *port_attr,
> > +		 struct rte_flow_error *err);
> >   };
> >
> >   /**
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> > index cd0c4c428d..f1235aa913 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -260,6 +260,8 @@ EXPERIMENTAL {
> >   	# added in 22.03
> >   	rte_eth_dev_priority_flow_ctrl_queue_configure;
> >   	rte_eth_dev_priority_flow_ctrl_queue_info_get;
> > +	rte_flow_info_get;
> > +	rte_flow_configure;
> >   };
> >
> >   INTERNAL {
  

On 2/11/22 21:47, Alexander Kozyrev wrote:
> On Friday, February 11, 2022 5:17 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
>> Sent: Friday, February 11, 2022 5:17
>> To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
>> Cc: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
>> <thomas@monjalon.net>; ivan.malov@oktetlabs.ru; ferruh.yigit@intel.com;
>> mohammad.abdul.awal@intel.com; qi.z.zhang@intel.com; jerinj@marvell.com;
>> ajit.khaparde@broadcom.com; bruce.richardson@intel.com
>> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration hints
>>
>> On 2/11/22 05:26, Alexander Kozyrev wrote:
>>> The flow rules creation/destruction at a large scale incurs a performance
>>> penalty and may negatively impact the packet processing when used
>>> as part of the datapath logic. This is mainly because software/hardware
>>> resources are allocated and prepared during the flow rule creation.
>>>
>>> In order to optimize the insertion rate, PMD may use some hints provided
>>> by the application at the initialization phase. The rte_flow_configure()
>>> function allows to pre-allocate all the needed resources beforehand.
>>> These resources can be used at a later stage without costly allocations.
>>> Every PMD may use only the subset of hints and ignore unused ones or
>>> fail in case the requested configuration is not supported.
>>>
>>> The rte_flow_info_get() is available to retrieve the information about
>>> supported pre-configurable resources. Both these functions must be called
>>> before any other usage of the flow API engine.
>>>
>>> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
>>> Acked-by: Ori Kam <orika@nvidia.com>

[snip]

>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>>> index a93f68abbc..66614ae29b 100644
>>> --- a/lib/ethdev/rte_flow.c
>>> +++ b/lib/ethdev/rte_flow.c
>>> @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
>>>    	ret = ops->flex_item_release(dev, handle, error);
>>>    	return flow_err(port_id, ret, error);
>>>    }
>>> +
>>> +int
>>> +rte_flow_info_get(uint16_t port_id,
>>> +		  struct rte_flow_port_info *port_info,
>>> +		  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);
>>> +
>>> +	if (unlikely(!ops))
>>> +		return -rte_errno;
>>> +	if (likely(!!ops->info_get)) {
>>
>> expected ethdev state must be validated. Just configured?
>>
>>> +		return flow_err(port_id,
>>> +				ops->info_get(dev, port_info, error),
>>
>> port_info must be checked vs NULL
> 
> We don’t have any NULL checks for parameters in the whole ret flow API library.
> See rte_flow_create() for example. attributes, pattern and actions are passed to PMD unchecked.

IMHO it is hardly a good reason to have no such check here.
The API is pure control path. So, it must validate all input
arguments and it is better to do it in a generic place.

>>> +				error);
>>> +	}
>>> +	return rte_flow_error_set(error, ENOTSUP,
>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> +				  NULL, rte_strerror(ENOTSUP));
>>> +}
>>> +
>>> +int
>>> +rte_flow_configure(uint16_t port_id,
>>> +		   const struct rte_flow_port_attr *port_attr,
>>> +		   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);
>>> +
>>> +	if (unlikely(!ops))
>>> +		return -rte_errno;
>>> +	if (likely(!!ops->configure)) {
>>
>> The API must validate ethdev state. configured and not started?
> Again, we have no such validation for any rte flow API today.

Same here. If documentation defines in which state the API
should be called, generic code must guarantee it.

>>
>>> +		return flow_err(port_id,
>>> +				ops->configure(dev, port_attr, error),
>>
>> port_attr must be checked vs NULL
> Same.
> 
>>> +				error);
>>> +	}
>>> +	return rte_flow_error_set(error, ENOTSUP,
>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> +				  NULL, rte_strerror(ENOTSUP));
>>> +}
>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>>> index 1031fb246b..92be2a9a89 100644
>>> --- a/lib/ethdev/rte_flow.h
>>> +++ b/lib/ethdev/rte_flow.h
>>> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t port_id,
>>>    			   const struct rte_flow_item_flex_handle *handle,
>>>    			   struct rte_flow_error *error);
>>>
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Information about available pre-configurable resources.
>>> + * The zero value means a resource cannot be pre-allocated.
>>> + *
>>> + */
>>> +struct rte_flow_port_info {
>>> +	/**
>>> +	 * Number of pre-configurable counter actions.
>>> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
>>> +	 */
>>> +	uint32_t nb_counters;
>>
>> Name says that it is a number of counters, but description
>> says that it is about actions.
>> Also I don't understand what does "pre-configurable" mean.
>> Isn't it a maximum number of available counters?
>> If no, how can I find a maximum?
> It is number of pre-allocated and pre-configured actions.
> How are they pr-configured is up to PDM driver.
> But let's change to "pre-configured" everywhere.
> Configuration includes some memory allocation anyway.

Sorry, but I still don't understand. I guess HW has
a hard limit on a number of counters. How can I get
the information?

>>
>>> +	/**
>>> +	 * Number of pre-configurable aging flows actions.
>>> +	 * @see RTE_FLOW_ACTION_TYPE_AGE
>>> +	 */
>>> +	uint32_t nb_aging_flows;
>>
>> Same
> Ditto.
>   
>>> +	/**
>>> +	 * Number of pre-configurable traffic metering actions.
>>> +	 * @see RTE_FLOW_ACTION_TYPE_METER
>>> +	 */
>>> +	uint32_t nb_meters;
>>
>> Same
> Ditto.
> 
>>> +};
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Retrieve configuration attributes supported by the port.
>>
>> Description should be a bit more flow API aware.
>> Right now it sounds too generic.
> Ok, how about
> "Get information about flow engine pre-configurable resources."
>   
>>> + *
>>> + * @param port_id
>>> + *   Port identifier of Ethernet device.
>>> + * @param[out] port_info
>>> + *   A pointer to a structure of type *rte_flow_port_info*
>>> + *   to be filled with the contextual information of the port.
>>> + * @param[out] error
>>> + *   Perform verbose error reporting if not NULL.
>>> + *   PMDs initialize this structure in case of error only.
>>> + *
>>> + * @return
>>> + *   0 on success, a negative errno value otherwise and rte_errno is set.
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_flow_info_get(uint16_t port_id,
>>> +		  struct rte_flow_port_info *port_info,
>>> +		  struct rte_flow_error *error);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Resource pre-allocation and pre-configuration settings.
>>
>> What is the difference between pre-allocation and pre-configuration?
>> Why are both mentioned above, but just pre-configured actions are
>> mentioned below?
> Please see answer to this question above.
>   
>>> + * The zero value means on demand resource allocations only.
>>> + *
>>> + */
>>> +struct rte_flow_port_attr {
>>> +	/**
>>> +	 * Number of counter actions pre-configured.
>>> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
>>> +	 */
>>> +	uint32_t nb_counters;
>>> +	/**
>>> +	 * Number of aging flows actions pre-configured.
>>> +	 * @see RTE_FLOW_ACTION_TYPE_AGE
>>> +	 */
>>> +	uint32_t nb_aging_flows;
>>> +	/**
>>> +	 * Number of traffic metering actions pre-configured.
>>> +	 * @see RTE_FLOW_ACTION_TYPE_METER
>>> +	 */
>>> +	uint32_t nb_meters;
>>> +};
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Configure the port's flow API engine.
>>> + *
>>> + * This API can only be invoked before the application
>>> + * starts using the rest of the flow library functions.
>>> + *
>>> + * The API can be invoked multiple times to change the
>>> + * settings. The port, however, may reject the changes.
>>> + *
>>> + * Parameters in configuration attributes must not exceed
>>> + * numbers of resources returned by the rte_flow_info_get API.
>>> + *
>>> + * @param port_id
>>> + *   Port identifier of Ethernet device.
>>> + * @param[in] port_attr
>>> + *   Port configuration attributes.
>>> + * @param[out] error
>>> + *   Perform verbose error reporting if not NULL.
>>> + *   PMDs initialize this structure in case of error only.
>>> + *
>>> + * @return
>>> + *   0 on success, a negative errno value otherwise and rte_errno is set.
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_flow_configure(uint16_t port_id,
>>> +		   const struct rte_flow_port_attr *port_attr,
>>> +		   struct rte_flow_error *error);
>>> +
>>>    #ifdef __cplusplus
>>>    }
>>>    #endif

[snip]
  

On Wed, Feb 16, 2022 8:03 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>:
> On 2/11/22 21:47, Alexander Kozyrev wrote:
> > On Friday, February 11, 2022 5:17 Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
> >> Sent: Friday, February 11, 2022 5:17
> >> To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
> >> Cc: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon
> (EXTERNAL)
> >> <thomas@monjalon.net>; ivan.malov@oktetlabs.ru;
> ferruh.yigit@intel.com;
> >> mohammad.abdul.awal@intel.com; qi.z.zhang@intel.com;
> jerinj@marvell.com;
> >> ajit.khaparde@broadcom.com; bruce.richardson@intel.com
> >> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration
> hints
> >>
> >> On 2/11/22 05:26, Alexander Kozyrev wrote:
> >>> The flow rules creation/destruction at a large scale incurs a performance
> >>> penalty and may negatively impact the packet processing when used
> >>> as part of the datapath logic. This is mainly because software/hardware
> >>> resources are allocated and prepared during the flow rule creation.
> >>>
> >>> In order to optimize the insertion rate, PMD may use some hints
> provided
> >>> by the application at the initialization phase. The rte_flow_configure()
> >>> function allows to pre-allocate all the needed resources beforehand.
> >>> These resources can be used at a later stage without costly allocations.
> >>> Every PMD may use only the subset of hints and ignore unused ones or
> >>> fail in case the requested configuration is not supported.
> >>>
> >>> The rte_flow_info_get() is available to retrieve the information about
> >>> supported pre-configurable resources. Both these functions must be
> called
> >>> before any other usage of the flow API engine.
> >>>
> >>> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> >>> Acked-by: Ori Kam <orika@nvidia.com>
> 
> [snip]
> 
> >>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> >>> index a93f68abbc..66614ae29b 100644
> >>> --- a/lib/ethdev/rte_flow.c
> >>> +++ b/lib/ethdev/rte_flow.c
> >>> @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
> >>>    	ret = ops->flex_item_release(dev, handle, error);
> >>>    	return flow_err(port_id, ret, error);
> >>>    }
> >>> +
> >>> +int
> >>> +rte_flow_info_get(uint16_t port_id,
> >>> +		  struct rte_flow_port_info *port_info,
> >>> +		  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);
> >>> +
> >>> +	if (unlikely(!ops))
> >>> +		return -rte_errno;
> >>> +	if (likely(!!ops->info_get)) {
> >>
> >> expected ethdev state must be validated. Just configured?
> >>
> >>> +		return flow_err(port_id,
> >>> +				ops->info_get(dev, port_info, error),
> >>
> >> port_info must be checked vs NULL
> >
> > We don’t have any NULL checks for parameters in the whole ret flow API
> library.
> > See rte_flow_create() for example. attributes, pattern and actions are
> passed to PMD unchecked.
> 
> IMHO it is hardly a good reason to have no such check here.
> The API is pure control path. So, it must validate all input
> arguments and it is better to do it in a generic place.

Agree, I have no objections to introduce these validation checks on control path.
My only concern is the data-path performance, so I'm reluctant to add them to
rte_flow_q_create/destroy functions. But let's add NULL checks to configuration
routines, ok?

> >>> +				error);
> >>> +	}
> >>> +	return rte_flow_error_set(error, ENOTSUP,
> >>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >>> +				  NULL, rte_strerror(ENOTSUP));
> >>> +}
> >>> +
> >>> +int
> >>> +rte_flow_configure(uint16_t port_id,
> >>> +		   const struct rte_flow_port_attr *port_attr,
> >>> +		   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);
> >>> +
> >>> +	if (unlikely(!ops))
> >>> +		return -rte_errno;
> >>> +	if (likely(!!ops->configure)) {
> >>
> >> The API must validate ethdev state. configured and not started?
> > Again, we have no such validation for any rte flow API today.
> 
> Same here. If documentation defines in which state the API
> should be called, generic code must guarantee it.

Ok, as long as it stays in the configuration phase only.

> >>
> >>> +		return flow_err(port_id,
> >>> +				ops->configure(dev, port_attr, error),
> >>
> >> port_attr must be checked vs NULL
> > Same.
> >
> >>> +				error);
> >>> +	}
> >>> +	return rte_flow_error_set(error, ENOTSUP,
> >>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >>> +				  NULL, rte_strerror(ENOTSUP));
> >>> +}
> >>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> >>> index 1031fb246b..92be2a9a89 100644
> >>> --- a/lib/ethdev/rte_flow.h
> >>> +++ b/lib/ethdev/rte_flow.h
> >>> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t
> port_id,
> >>>    			   const struct rte_flow_item_flex_handle *handle,
> >>>    			   struct rte_flow_error *error);
> >>>
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Information about available pre-configurable resources.
> >>> + * The zero value means a resource cannot be pre-allocated.
> >>> + *
> >>> + */
> >>> +struct rte_flow_port_info {
> >>> +	/**
> >>> +	 * Number of pre-configurable counter actions.
> >>> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
> >>> +	 */
> >>> +	uint32_t nb_counters;
> >>
> >> Name says that it is a number of counters, but description
> >> says that it is about actions.
> >> Also I don't understand what does "pre-configurable" mean.
> >> Isn't it a maximum number of available counters?
> >> If no, how can I find a maximum?
> > It is number of pre-allocated and pre-configured actions.
> > How are they pr-configured is up to PDM driver.
> > But let's change to "pre-configured" everywhere.
> > Configuration includes some memory allocation anyway.
> 
> Sorry, but I still don't understand. I guess HW has
> a hard limit on a number of counters. How can I get
> the information?

Sorry for not being clear. These are resources/objects limitation.
It may be the hard HW limit on number of counter objects, for example.
Or the system has a little of memory and NIC is constrained in memory
in its attempt to create these counter objects as another example.
In any case, the info_get() API should return the limit to a user.

> >>
> >>> +	/**
> >>> +	 * Number of pre-configurable aging flows actions.
> >>> +	 * @see RTE_FLOW_ACTION_TYPE_AGE
> >>> +	 */
> >>> +	uint32_t nb_aging_flows;
> >>
> >> Same
> > Ditto.
> >
> >>> +	/**
> >>> +	 * Number of pre-configurable traffic metering actions.
> >>> +	 * @see RTE_FLOW_ACTION_TYPE_METER
> >>> +	 */
> >>> +	uint32_t nb_meters;
> >>
> >> Same
> > Ditto.
> >
> >>> +};
> >>> +
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Retrieve configuration attributes supported by the port.
> >>
> >> Description should be a bit more flow API aware.
> >> Right now it sounds too generic.
> > Ok, how about
> > "Get information about flow engine pre-configurable resources."
> >
> >>> + *
> >>> + * @param port_id
> >>> + *   Port identifier of Ethernet device.
> >>> + * @param[out] port_info
> >>> + *   A pointer to a structure of type *rte_flow_port_info*
> >>> + *   to be filled with the contextual information of the port.
> >>> + * @param[out] error
> >>> + *   Perform verbose error reporting if not NULL.
> >>> + *   PMDs initialize this structure in case of error only.
> >>> + *
> >>> + * @return
> >>> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> >>> + */
> >>> +__rte_experimental
> >>> +int
> >>> +rte_flow_info_get(uint16_t port_id,
> >>> +		  struct rte_flow_port_info *port_info,
> >>> +		  struct rte_flow_error *error);
> >>> +
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Resource pre-allocation and pre-configuration settings.
> >>
> >> What is the difference between pre-allocation and pre-configuration?
> >> Why are both mentioned above, but just pre-configured actions are
> >> mentioned below?
> > Please see answer to this question above.
> >
> >>> + * The zero value means on demand resource allocations only.
> >>> + *
> >>> + */
> >>> +struct rte_flow_port_attr {
> >>> +	/**
> >>> +	 * Number of counter actions pre-configured.
> >>> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
> >>> +	 */
> >>> +	uint32_t nb_counters;
> >>> +	/**
> >>> +	 * Number of aging flows actions pre-configured.
> >>> +	 * @see RTE_FLOW_ACTION_TYPE_AGE
> >>> +	 */
> >>> +	uint32_t nb_aging_flows;
> >>> +	/**
> >>> +	 * Number of traffic metering actions pre-configured.
> >>> +	 * @see RTE_FLOW_ACTION_TYPE_METER
> >>> +	 */
> >>> +	uint32_t nb_meters;
> >>> +};
> >>> +
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Configure the port's flow API engine.
> >>> + *
> >>> + * This API can only be invoked before the application
> >>> + * starts using the rest of the flow library functions.
> >>> + *
> >>> + * The API can be invoked multiple times to change the
> >>> + * settings. The port, however, may reject the changes.
> >>> + *
> >>> + * Parameters in configuration attributes must not exceed
> >>> + * numbers of resources returned by the rte_flow_info_get API.
> >>> + *
> >>> + * @param port_id
> >>> + *   Port identifier of Ethernet device.
> >>> + * @param[in] port_attr
> >>> + *   Port configuration attributes.
> >>> + * @param[out] error
> >>> + *   Perform verbose error reporting if not NULL.
> >>> + *   PMDs initialize this structure in case of error only.
> >>> + *
> >>> + * @return
> >>> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> >>> + */
> >>> +__rte_experimental
> >>> +int
> >>> +rte_flow_configure(uint16_t port_id,
> >>> +		   const struct rte_flow_port_attr *port_attr,
> >>> +		   struct rte_flow_error *error);
> >>> +
> >>>    #ifdef __cplusplus
> >>>    }
> >>>    #endif
> 
> [snip]
  

On 2/17/22 01:17, Alexander Kozyrev wrote:
> On Wed, Feb 16, 2022 8:03 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>:
>> On 2/11/22 21:47, Alexander Kozyrev wrote:
>>> On Friday, February 11, 2022 5:17 Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>> Sent: Friday, February 11, 2022 5:17
>>>> To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
>>>> Cc: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon
>> (EXTERNAL)
>>>> <thomas@monjalon.net>; ivan.malov@oktetlabs.ru;
>> ferruh.yigit@intel.com;
>>>> mohammad.abdul.awal@intel.com; qi.z.zhang@intel.com;
>> jerinj@marvell.com;
>>>> ajit.khaparde@broadcom.com; bruce.richardson@intel.com
>>>> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration
>> hints
>>>>
>>>> On 2/11/22 05:26, Alexander Kozyrev wrote:
>>>>> The flow rules creation/destruction at a large scale incurs a performance
>>>>> penalty and may negatively impact the packet processing when used
>>>>> as part of the datapath logic. This is mainly because software/hardware
>>>>> resources are allocated and prepared during the flow rule creation.
>>>>>
>>>>> In order to optimize the insertion rate, PMD may use some hints
>> provided
>>>>> by the application at the initialization phase. The rte_flow_configure()
>>>>> function allows to pre-allocate all the needed resources beforehand.
>>>>> These resources can be used at a later stage without costly allocations.
>>>>> Every PMD may use only the subset of hints and ignore unused ones or
>>>>> fail in case the requested configuration is not supported.
>>>>>
>>>>> The rte_flow_info_get() is available to retrieve the information about
>>>>> supported pre-configurable resources. Both these functions must be
>> called
>>>>> before any other usage of the flow API engine.
>>>>>
>>>>> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
>>>>> Acked-by: Ori Kam <orika@nvidia.com>
>>
>> [snip]
>>
>>>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>>>>> index a93f68abbc..66614ae29b 100644
>>>>> --- a/lib/ethdev/rte_flow.c
>>>>> +++ b/lib/ethdev/rte_flow.c
>>>>> @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
>>>>>     	ret = ops->flex_item_release(dev, handle, error);
>>>>>     	return flow_err(port_id, ret, error);
>>>>>     }
>>>>> +
>>>>> +int
>>>>> +rte_flow_info_get(uint16_t port_id,
>>>>> +		  struct rte_flow_port_info *port_info,
>>>>> +		  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);
>>>>> +
>>>>> +	if (unlikely(!ops))
>>>>> +		return -rte_errno;
>>>>> +	if (likely(!!ops->info_get)) {
>>>>
>>>> expected ethdev state must be validated. Just configured?
>>>>
>>>>> +		return flow_err(port_id,
>>>>> +				ops->info_get(dev, port_info, error),
>>>>
>>>> port_info must be checked vs NULL
>>>
>>> We don’t have any NULL checks for parameters in the whole ret flow API
>> library.
>>> See rte_flow_create() for example. attributes, pattern and actions are
>> passed to PMD unchecked.
>>
>> IMHO it is hardly a good reason to have no such check here.
>> The API is pure control path. So, it must validate all input
>> arguments and it is better to do it in a generic place.
> 
> Agree, I have no objections to introduce these validation checks on control path.

Good, we have a progress.

> My only concern is the data-path performance, so I'm reluctant to add them to
> rte_flow_q_create/destroy functions. But let's add NULL checks to configuration
> routines, ok?

My opinion is not that strong on the aspect, but, personally,
I'd have sanity checks in the case of flow create/destroy as
well. First of all it is not a true datapath. Second, these
checks are very lightweight.

Anyway, if nobody supports me, I'm OK to go without these
checks in generic functions, but it would be very useful to
highlight it in the parameters description.

>>>>> +				error);
>>>>> +	}
>>>>> +	return rte_flow_error_set(error, ENOTSUP,
>>>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>>>> +				  NULL, rte_strerror(ENOTSUP));
>>>>> +}
>>>>> +
>>>>> +int
>>>>> +rte_flow_configure(uint16_t port_id,
>>>>> +		   const struct rte_flow_port_attr *port_attr,
>>>>> +		   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);
>>>>> +
>>>>> +	if (unlikely(!ops))
>>>>> +		return -rte_errno;
>>>>> +	if (likely(!!ops->configure)) {
>>>>
>>>> The API must validate ethdev state. configured and not started?
>>> Again, we have no such validation for any rte flow API today.
>>
>> Same here. If documentation defines in which state the API
>> should be called, generic code must guarantee it.
> 
> Ok, as long as it stays in the configuration phase only.
> 
>>>>
>>>>> +		return flow_err(port_id,
>>>>> +				ops->configure(dev, port_attr, error),
>>>>
>>>> port_attr must be checked vs NULL
>>> Same.
>>>
>>>>> +				error);
>>>>> +	}
>>>>> +	return rte_flow_error_set(error, ENOTSUP,
>>>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>>>> +				  NULL, rte_strerror(ENOTSUP));
>>>>> +}
>>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>>>>> index 1031fb246b..92be2a9a89 100644
>>>>> --- a/lib/ethdev/rte_flow.h
>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t
>> port_id,
>>>>>     			   const struct rte_flow_item_flex_handle *handle,
>>>>>     			   struct rte_flow_error *error);
>>>>>
>>>>> +/**
>>>>> + * @warning
>>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>>>> + *
>>>>> + * Information about available pre-configurable resources.
>>>>> + * The zero value means a resource cannot be pre-allocated.
>>>>> + *
>>>>> + */
>>>>> +struct rte_flow_port_info {
>>>>> +	/**
>>>>> +	 * Number of pre-configurable counter actions.
>>>>> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
>>>>> +	 */
>>>>> +	uint32_t nb_counters;
>>>>
>>>> Name says that it is a number of counters, but description
>>>> says that it is about actions.
>>>> Also I don't understand what does "pre-configurable" mean.
>>>> Isn't it a maximum number of available counters?
>>>> If no, how can I find a maximum?
>>> It is number of pre-allocated and pre-configured actions.
>>> How are they pr-configured is up to PDM driver.
>>> But let's change to "pre-configured" everywhere.
>>> Configuration includes some memory allocation anyway.
>>
>> Sorry, but I still don't understand. I guess HW has
>> a hard limit on a number of counters. How can I get
>> the information?
> 
> Sorry for not being clear. These are resources/objects limitation.
> It may be the hard HW limit on number of counter objects, for example.
> Or the system has a little of memory and NIC is constrained in memory
> in its attempt to create these counter objects as another example.
> In any case, the info_get() API should return the limit to a user.

Look. First of all it is confusing that description says
"counter actions". I remember that we have no shared
counters now (just shared actions), but it does not matter
a lot. IMHO it is a bit more clear to say that it is
a limit on a number of flow counters. I guess it better
express the nature of the limitation. May be I'm missing
something. If so, I'd like to understand what.

Second, "per-configurable" is confusing. May be it is better
just to drop it? I.e. "Information about available resources."
Otherwise it is necessary to explain who and when
pre-configures these resources. Is it really pre-configured?

"The zero value means a resource cannot be pre-allocated."
Does it mean that the action cannot be used at all?
I think it must be explicitly clarified in the case of any
answer.
  

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Thursday, February 17, 2022 12:35 PM
> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration hints
> 
> On 2/17/22 01:17, Alexander Kozyrev wrote:
> > On Wed, Feb 16, 2022 8:03 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>:
> >> On 2/11/22 21:47, Alexander Kozyrev wrote:
> >>> On Friday, February 11, 2022 5:17 Andrew Rybchenko
> >> <andrew.rybchenko@oktetlabs.ru> wrote:
> >>>> Sent: Friday, February 11, 2022 5:17
> >>>> To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
> >>>> Cc: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon
> >> (EXTERNAL)
> >>>> <thomas@monjalon.net>; ivan.malov@oktetlabs.ru;
> >> ferruh.yigit@intel.com;
> >>>> mohammad.abdul.awal@intel.com; qi.z.zhang@intel.com;
> >> jerinj@marvell.com;
> >>>> ajit.khaparde@broadcom.com; bruce.richardson@intel.com
> >>>> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration
> >> hints
> >>>>
> >>>> On 2/11/22 05:26, Alexander Kozyrev wrote:
> >>>>> The flow rules creation/destruction at a large scale incurs a performance
> >>>>> penalty and may negatively impact the packet processing when used
> >>>>> as part of the datapath logic. This is mainly because software/hardware
> >>>>> resources are allocated and prepared during the flow rule creation.
> >>>>>
> >>>>> In order to optimize the insertion rate, PMD may use some hints
> >> provided
> >>>>> by the application at the initialization phase. The rte_flow_configure()
> >>>>> function allows to pre-allocate all the needed resources beforehand.
> >>>>> These resources can be used at a later stage without costly allocations.
> >>>>> Every PMD may use only the subset of hints and ignore unused ones or
> >>>>> fail in case the requested configuration is not supported.
> >>>>>
> >>>>> The rte_flow_info_get() is available to retrieve the information about
> >>>>> supported pre-configurable resources. Both these functions must be
> >> called
> >>>>> before any other usage of the flow API engine.
> >>>>>
> >>>>> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> >>>>> Acked-by: Ori Kam <orika@nvidia.com>
> >>
> >> [snip]
> >>
> >>>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> >>>>> index a93f68abbc..66614ae29b 100644
> >>>>> --- a/lib/ethdev/rte_flow.c
> >>>>> +++ b/lib/ethdev/rte_flow.c
> >>>>> @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
> >>>>>     	ret = ops->flex_item_release(dev, handle, error);
> >>>>>     	return flow_err(port_id, ret, error);
> >>>>>     }
> >>>>> +
> >>>>> +int
> >>>>> +rte_flow_info_get(uint16_t port_id,
> >>>>> +		  struct rte_flow_port_info *port_info,
> >>>>> +		  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);
> >>>>> +
> >>>>> +	if (unlikely(!ops))
> >>>>> +		return -rte_errno;
> >>>>> +	if (likely(!!ops->info_get)) {
> >>>>
> >>>> expected ethdev state must be validated. Just configured?
> >>>>
> >>>>> +		return flow_err(port_id,
> >>>>> +				ops->info_get(dev, port_info, error),
> >>>>
> >>>> port_info must be checked vs NULL
> >>>
> >>> We don’t have any NULL checks for parameters in the whole ret flow API
> >> library.
> >>> See rte_flow_create() for example. attributes, pattern and actions are
> >> passed to PMD unchecked.
> >>
> >> IMHO it is hardly a good reason to have no such check here.
> >> The API is pure control path. So, it must validate all input
> >> arguments and it is better to do it in a generic place.
> >
> > Agree, I have no objections to introduce these validation checks on control path.
> 
> Good, we have a progress.
> 
> > My only concern is the data-path performance, so I'm reluctant to add them to
> > rte_flow_q_create/destroy functions. But let's add NULL checks to configuration
> > routines, ok?
> 
> My opinion is not that strong on the aspect, but, personally,
> I'd have sanity checks in the case of flow create/destroy as
> well. First of all it is not a true datapath. Second, these
> checks are very lightweight.
> 
> Anyway, if nobody supports me, I'm OK to go without these
> checks in generic functions, but it would be very useful to
> highlight it in the parameters description.
> 
I vote for adding the checks only on configuration function.
From my point of view the rule functions are part of the data path
and should be treated this way.

> >>>>> +				error);
> >>>>> +	}
> >>>>> +	return rte_flow_error_set(error, ENOTSUP,
> >>>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >>>>> +				  NULL, rte_strerror(ENOTSUP));
> >>>>> +}
> >>>>> +
> >>>>> +int
> >>>>> +rte_flow_configure(uint16_t port_id,
> >>>>> +		   const struct rte_flow_port_attr *port_attr,
> >>>>> +		   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);
> >>>>> +
> >>>>> +	if (unlikely(!ops))
> >>>>> +		return -rte_errno;
> >>>>> +	if (likely(!!ops->configure)) {
> >>>>
> >>>> The API must validate ethdev state. configured and not started?
> >>> Again, we have no such validation for any rte flow API today.
> >>
> >> Same here. If documentation defines in which state the API
> >> should be called, generic code must guarantee it.
> >
> > Ok, as long as it stays in the configuration phase only.
> >
> >>>>
> >>>>> +		return flow_err(port_id,
> >>>>> +				ops->configure(dev, port_attr, error),
> >>>>
> >>>> port_attr must be checked vs NULL
> >>> Same.
> >>>
> >>>>> +				error);
> >>>>> +	}
> >>>>> +	return rte_flow_error_set(error, ENOTSUP,
> >>>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >>>>> +				  NULL, rte_strerror(ENOTSUP));
> >>>>> +}
> >>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> >>>>> index 1031fb246b..92be2a9a89 100644
> >>>>> --- a/lib/ethdev/rte_flow.h
> >>>>> +++ b/lib/ethdev/rte_flow.h
> >>>>> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t
> >> port_id,
> >>>>>     			   const struct rte_flow_item_flex_handle *handle,
> >>>>>     			   struct rte_flow_error *error);
> >>>>>
> >>>>> +/**
> >>>>> + * @warning
> >>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>>>> + *
> >>>>> + * Information about available pre-configurable resources.
> >>>>> + * The zero value means a resource cannot be pre-allocated.
> >>>>> + *
> >>>>> + */
> >>>>> +struct rte_flow_port_info {
> >>>>> +	/**
> >>>>> +	 * Number of pre-configurable counter actions.
> >>>>> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
> >>>>> +	 */
> >>>>> +	uint32_t nb_counters;
> >>>>
> >>>> Name says that it is a number of counters, but description
> >>>> says that it is about actions.
> >>>> Also I don't understand what does "pre-configurable" mean.
> >>>> Isn't it a maximum number of available counters?
> >>>> If no, how can I find a maximum?
> >>> It is number of pre-allocated and pre-configured actions.
> >>> How are they pr-configured is up to PDM driver.
> >>> But let's change to "pre-configured" everywhere.
> >>> Configuration includes some memory allocation anyway.
> >>
> >> Sorry, but I still don't understand. I guess HW has
> >> a hard limit on a number of counters. How can I get
> >> the information?
> >
> > Sorry for not being clear. These are resources/objects limitation.
> > It may be the hard HW limit on number of counter objects, for example.
> > Or the system has a little of memory and NIC is constrained in memory
> > in its attempt to create these counter objects as another example.
> > In any case, the info_get() API should return the limit to a user.
> 
> Look. First of all it is confusing that description says
> "counter actions". I remember that we have no shared
> counters now (just shared actions), but it does not matter
> a lot. IMHO it is a bit more clear to say that it is
> a limit on a number of flow counters. I guess it better
> express the nature of the limitation. May be I'm missing
> something. If so, I'd like to understand what.
> 
From my view point, this should be the number of resource/objects that
the HW can allocate (in an ideal system).
For example if the HW can allocate 1M counters but due to limited memory
on the system the actual number can be less.

Like you said we also have the handle action, this means that
the same object can be shared between any number of rules.
as a result the limitation is not on the number of rules but on the number of
resources allocated.

In addition and even more importantly during this stage there is no knowlge on the
number of rules that will be inserted.

So can we agree to say resources?




> Second, "per-configurable" is confusing. May be it is better
> just to drop it? I.e. "Information about available resources."
> Otherwise it is necessary to explain who and when
> pre-configures these resources. Is it really pre-configured?
> 
I'm O.K with dropping the configuration part
It should just say number of counter objects

> "The zero value means a resource cannot be pre-allocated."
> Does it mean that the action cannot be used at all?
> I think it must be explicitly clarified in the case of any
> answer.

Agree, it should state that if the PMD report 0 in means that
it doesn’t support such an object.

Best,
Ori
  

On 2/17/22 13:57, Ori Kam wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Thursday, February 17, 2022 12:35 PM
>> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration hints
>>
>> On 2/17/22 01:17, Alexander Kozyrev wrote:
>>> On Wed, Feb 16, 2022 8:03 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>:
>>>> On 2/11/22 21:47, Alexander Kozyrev wrote:
>>>>> On Friday, February 11, 2022 5:17 Andrew Rybchenko
>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>> Sent: Friday, February 11, 2022 5:17
>>>>>> To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
>>>>>> Cc: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon
>>>> (EXTERNAL)
>>>>>> <thomas@monjalon.net>; ivan.malov@oktetlabs.ru;
>>>> ferruh.yigit@intel.com;
>>>>>> mohammad.abdul.awal@intel.com; qi.z.zhang@intel.com;
>>>> jerinj@marvell.com;
>>>>>> ajit.khaparde@broadcom.com; bruce.richardson@intel.com
>>>>>> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration
>>>> hints
>>>>>>
>>>>>> On 2/11/22 05:26, Alexander Kozyrev wrote:
>>>>>>> The flow rules creation/destruction at a large scale incurs a performance
>>>>>>> penalty and may negatively impact the packet processing when used
>>>>>>> as part of the datapath logic. This is mainly because software/hardware
>>>>>>> resources are allocated and prepared during the flow rule creation.
>>>>>>>
>>>>>>> In order to optimize the insertion rate, PMD may use some hints
>>>> provided
>>>>>>> by the application at the initialization phase. The rte_flow_configure()
>>>>>>> function allows to pre-allocate all the needed resources beforehand.
>>>>>>> These resources can be used at a later stage without costly allocations.
>>>>>>> Every PMD may use only the subset of hints and ignore unused ones or
>>>>>>> fail in case the requested configuration is not supported.
>>>>>>>
>>>>>>> The rte_flow_info_get() is available to retrieve the information about
>>>>>>> supported pre-configurable resources. Both these functions must be
>>>> called
>>>>>>> before any other usage of the flow API engine.
>>>>>>>
>>>>>>> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
>>>>>>> Acked-by: Ori Kam <orika@nvidia.com>
>>>>
>>>> [snip]
>>>>
>>>>>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>>>>>>> index a93f68abbc..66614ae29b 100644
>>>>>>> --- a/lib/ethdev/rte_flow.c
>>>>>>> +++ b/lib/ethdev/rte_flow.c
>>>>>>> @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
>>>>>>>      	ret = ops->flex_item_release(dev, handle, error);
>>>>>>>      	return flow_err(port_id, ret, error);
>>>>>>>      }
>>>>>>> +
>>>>>>> +int
>>>>>>> +rte_flow_info_get(uint16_t port_id,
>>>>>>> +		  struct rte_flow_port_info *port_info,
>>>>>>> +		  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);
>>>>>>> +
>>>>>>> +	if (unlikely(!ops))
>>>>>>> +		return -rte_errno;
>>>>>>> +	if (likely(!!ops->info_get)) {
>>>>>>
>>>>>> expected ethdev state must be validated. Just configured?
>>>>>>
>>>>>>> +		return flow_err(port_id,
>>>>>>> +				ops->info_get(dev, port_info, error),
>>>>>>
>>>>>> port_info must be checked vs NULL
>>>>>
>>>>> We don’t have any NULL checks for parameters in the whole ret flow API
>>>> library.
>>>>> See rte_flow_create() for example. attributes, pattern and actions are
>>>> passed to PMD unchecked.
>>>>
>>>> IMHO it is hardly a good reason to have no such check here.
>>>> The API is pure control path. So, it must validate all input
>>>> arguments and it is better to do it in a generic place.
>>>
>>> Agree, I have no objections to introduce these validation checks on control path.
>>
>> Good, we have a progress.
>>
>>> My only concern is the data-path performance, so I'm reluctant to add them to
>>> rte_flow_q_create/destroy functions. But let's add NULL checks to configuration
>>> routines, ok?
>>
>> My opinion is not that strong on the aspect, but, personally,
>> I'd have sanity checks in the case of flow create/destroy as
>> well. First of all it is not a true datapath. Second, these
>> checks are very lightweight.
>>
>> Anyway, if nobody supports me, I'm OK to go without these
>> checks in generic functions, but it would be very useful to
>> highlight it in the parameters description.
>>
> I vote for adding the checks only on configuration function.
>  From my point of view the rule functions are part of the data path
> and should be treated this way.
> 
>>>>>>> +				error);
>>>>>>> +	}
>>>>>>> +	return rte_flow_error_set(error, ENOTSUP,
>>>>>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>>>>>> +				  NULL, rte_strerror(ENOTSUP));
>>>>>>> +}
>>>>>>> +
>>>>>>> +int
>>>>>>> +rte_flow_configure(uint16_t port_id,
>>>>>>> +		   const struct rte_flow_port_attr *port_attr,
>>>>>>> +		   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);
>>>>>>> +
>>>>>>> +	if (unlikely(!ops))
>>>>>>> +		return -rte_errno;
>>>>>>> +	if (likely(!!ops->configure)) {
>>>>>>
>>>>>> The API must validate ethdev state. configured and not started?
>>>>> Again, we have no such validation for any rte flow API today.
>>>>
>>>> Same here. If documentation defines in which state the API
>>>> should be called, generic code must guarantee it.
>>>
>>> Ok, as long as it stays in the configuration phase only.
>>>
>>>>>>
>>>>>>> +		return flow_err(port_id,
>>>>>>> +				ops->configure(dev, port_attr, error),
>>>>>>
>>>>>> port_attr must be checked vs NULL
>>>>> Same.
>>>>>
>>>>>>> +				error);
>>>>>>> +	}
>>>>>>> +	return rte_flow_error_set(error, ENOTSUP,
>>>>>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>>>>>> +				  NULL, rte_strerror(ENOTSUP));
>>>>>>> +}
>>>>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>>>>>>> index 1031fb246b..92be2a9a89 100644
>>>>>>> --- a/lib/ethdev/rte_flow.h
>>>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>>>> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t
>>>> port_id,
>>>>>>>      			   const struct rte_flow_item_flex_handle *handle,
>>>>>>>      			   struct rte_flow_error *error);
>>>>>>>
>>>>>>> +/**
>>>>>>> + * @warning
>>>>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>>>>>> + *
>>>>>>> + * Information about available pre-configurable resources.
>>>>>>> + * The zero value means a resource cannot be pre-allocated.
>>>>>>> + *
>>>>>>> + */
>>>>>>> +struct rte_flow_port_info {
>>>>>>> +	/**
>>>>>>> +	 * Number of pre-configurable counter actions.
>>>>>>> +	 * @see RTE_FLOW_ACTION_TYPE_COUNT
>>>>>>> +	 */
>>>>>>> +	uint32_t nb_counters;
>>>>>>
>>>>>> Name says that it is a number of counters, but description
>>>>>> says that it is about actions.
>>>>>> Also I don't understand what does "pre-configurable" mean.
>>>>>> Isn't it a maximum number of available counters?
>>>>>> If no, how can I find a maximum?
>>>>> It is number of pre-allocated and pre-configured actions.
>>>>> How are they pr-configured is up to PDM driver.
>>>>> But let's change to "pre-configured" everywhere.
>>>>> Configuration includes some memory allocation anyway.
>>>>
>>>> Sorry, but I still don't understand. I guess HW has
>>>> a hard limit on a number of counters. How can I get
>>>> the information?
>>>
>>> Sorry for not being clear. These are resources/objects limitation.
>>> It may be the hard HW limit on number of counter objects, for example.
>>> Or the system has a little of memory and NIC is constrained in memory
>>> in its attempt to create these counter objects as another example.
>>> In any case, the info_get() API should return the limit to a user.
>>
>> Look. First of all it is confusing that description says
>> "counter actions". I remember that we have no shared
>> counters now (just shared actions), but it does not matter
>> a lot. IMHO it is a bit more clear to say that it is
>> a limit on a number of flow counters. I guess it better
>> express the nature of the limitation. May be I'm missing
>> something. If so, I'd like to understand what.
>>
>  From my view point, this should be the number of resource/objects that
> the HW can allocate (in an ideal system).
> For example if the HW can allocate 1M counters but due to limited memory
> on the system the actual number can be less.
> 
> Like you said we also have the handle action, this means that
> the same object can be shared between any number of rules.
> as a result the limitation is not on the number of rules but on the number of
> resources allocated.
> 
> In addition and even more importantly during this stage there is no knowlge on the
> number of rules that will be inserted.
> 
> So can we agree to say resources?

Yes

>> Second, "per-configurable" is confusing. May be it is better
>> just to drop it? I.e. "Information about available resources."
>> Otherwise it is necessary to explain who and when
>> pre-configures these resources. Is it really pre-configured?
>>
> I'm O.K with dropping the configuration part
> It should just say number of counter objects
> 
>> "The zero value means a resource cannot be pre-allocated."
>> Does it mean that the action cannot be used at all?
>> I think it must be explicitly clarified in the case of any
>> answer.
> 
> Agree, it should state that if the PMD report 0 in means that
> it doesn’t support such an object.

Good.