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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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 {