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

[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.

>>> +				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]