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

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

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.