Re: [PATCH v5 02/10] ethdev: add flow item/action templates

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

On 2/12/22 01:25, Alexander Kozyrev wrote:
> On Fri, Feb 11, 2022 6:27 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
>> On 2/11/22 05:26, Alexander Kozyrev wrote:
>>> Treating every single flow rule as a completely independent and separate
>>> entity negatively impacts the flow rules insertion rate. Oftentimes in an
>>> application, many flow rules share a common structure (the same item mask
>>> and/or action list) so they can be grouped and classified together.
>>> This knowledge may be used as a source of optimization by a PMD/HW.
>>>
>>> The pattern template defines common matching fields (the item mask) without
>>> values. The actions template holds a list of action types that will be used
>>> together in the same rule. The specific values for items and actions will
>>> be given only during the rule creation.
>>>
>>> A table combines pattern and actions templates along with shared flow rule
>>> attributes (group ID, priority and traffic direction). This way a PMD/HW
>>> can prepare all the resources needed for efficient flow rules creation in
>>> the datapath. To avoid any hiccups due to memory reallocation, the maximum
>>> number of flow rules is defined at the table creation time.
>>>
>>> The flow rule creation is done by selecting a table, a pattern template
>>> and an actions template (which are bound to the table), and setting unique
>>> values for the items and actions.
>>>
>>> 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 66614ae29b..b53f8c9b89 100644
>>> --- a/lib/ethdev/rte_flow.c
>>> +++ b/lib/ethdev/rte_flow.c
>>> @@ -1431,3 +1431,150 @@ rte_flow_configure(uint16_t port_id,
>>>    				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>>    				  NULL, rte_strerror(ENOTSUP));
>>>    }
>>> +
>>> +struct rte_flow_pattern_template *
>>> +rte_flow_pattern_template_create(uint16_t port_id,
>>> +		const struct rte_flow_pattern_template_attr *template_attr,
>>> +		const struct rte_flow_item pattern[],
>>> +		struct rte_flow_error *error)
>>> +{
>>> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>> +	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
>>> +	struct rte_flow_pattern_template *template;
>>> +
>>> +	if (unlikely(!ops))
>>> +		return NULL;
>>> +	if (likely(!!ops->pattern_template_create)) {
>>
>> Don't we need any state checks?
>>
>> Check pattern vs NULL?
> 
> Still the same situation, no NULL checks elsewhere in rte flow API.

I still think that it is wrong as explained.
Same is applicable to many review notes below.

> 
>>
>>> +		template = ops->pattern_template_create(dev, template_attr,
>>> +						     pattern, error);
>>> +		if (template == NULL)
>>> +			flow_err(port_id, -rte_errno, error);
>>> +		return template;
>>> +	}
>>> +	rte_flow_error_set(error, ENOTSUP,
>>> +			   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> +			   NULL, rte_strerror(ENOTSUP));
>>> +	return NULL;
>>> +}
>>> +
>>> +int
>>> +rte_flow_pattern_template_destroy(uint16_t port_id,
>>> +		struct rte_flow_pattern_template *pattern_template,
>>> +		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->pattern_template_destroy)) {
>>
>> IMHO we should return success here if pattern_template is NULL
> 
> Just like in rte_flow_destroy() it is up to PMD driver to decide.

Why? We must define behaviour in the case of NULL and guarantee
it.

>>> +		return flow_err(port_id,
>>> +				ops->pattern_template_destroy(dev,
>>> +							      pattern_template,
>>> +							      error),
>>> +				error);
>>> +	}
>>> +	return rte_flow_error_set(error, ENOTSUP,
>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> +				  NULL, rte_strerror(ENOTSUP));
>>> +}
>>> +
>>> +struct rte_flow_actions_template *
>>> +rte_flow_actions_template_create(uint16_t port_id,
>>> +			const struct rte_flow_actions_template_attr
>> *template_attr,
>>> +			const struct rte_flow_action actions[],
>>> +			const struct rte_flow_action masks[],
>>> +			struct rte_flow_error *error)
>>> +{
>>> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>> +	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
>>> +	struct rte_flow_actions_template *template;
>>> +
>>> +	if (unlikely(!ops))
>>> +		return NULL;
>>> +	if (likely(!!ops->actions_template_create)) {
>>
>> State checks?
>>
>> Check actions and masks vs NULL?
> 
> No, sorry.
> 
>>
>>> +		template = ops->actions_template_create(dev, template_attr,
>>> +							actions, masks, error);
>>> +		if (template == NULL)
>>> +			flow_err(port_id, -rte_errno, error);
>>> +		return template;
>>> +	}
>>> +	rte_flow_error_set(error, ENOTSUP,
>>> +			   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> +			   NULL, rte_strerror(ENOTSUP));
>>> +	return NULL;
>>> +}
>>> +
>>> +int
>>> +rte_flow_actions_template_destroy(uint16_t port_id,
>>> +			struct rte_flow_actions_template *actions_template,
>>> +			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->actions_template_destroy)) {
>>
>> IMHO we should return success here if actions_template is NULL
> 
> Just like in rte_flow_destroy() it is up to PMD driver to decide.

Same

>>
>>> +		return flow_err(port_id,
>>> +				ops->actions_template_destroy(dev,
>>> +							      actions_template,
>>> +							      error),
>>> +				error);
>>> +	}
>>> +	return rte_flow_error_set(error, ENOTSUP,
>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> +				  NULL, rte_strerror(ENOTSUP));
>>> +}
>>> +
>>> +struct rte_flow_template_table *
>>> +rte_flow_template_table_create(uint16_t port_id,
>>> +			const struct rte_flow_template_table_attr *table_attr,
>>> +			struct rte_flow_pattern_template
>> *pattern_templates[],
>>> +			uint8_t nb_pattern_templates,
>>> +			struct rte_flow_actions_template
>> *actions_templates[],
>>> +			uint8_t nb_actions_templates,
>>> +			struct rte_flow_error *error)
>>> +{
>>> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>> +	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
>>> +	struct rte_flow_template_table *table;
>>> +
>>> +	if (unlikely(!ops))
>>> +		return NULL;
>>> +	if (likely(!!ops->template_table_create)) {
>>
>> Argument sanity checks here. array NULL when size is not 0.
> 
> Hate to say no so many times, but I cannot help it.
> 
>>
>>> +		table = ops->template_table_create(dev, table_attr,
>>> +					pattern_templates,
>> nb_pattern_templates,
>>> +					actions_templates,
>> nb_actions_templates,
>>> +					error);
>>> +		if (table == NULL)
>>> +			flow_err(port_id, -rte_errno, error);
>>> +		return table;
>>> +	}
>>> +	rte_flow_error_set(error, ENOTSUP,
>>> +			   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> +			   NULL, rte_strerror(ENOTSUP));
>>> +	return NULL;
>>> +}
>>> +
>>> +int
>>> +rte_flow_template_table_destroy(uint16_t port_id,
>>> +				struct rte_flow_template_table
>> *template_table,
>>> +				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->template_table_destroy)) {
>>
>> Return success if template_table is NULL
> 
> Just like in rte_flow_destroy() it is up to PMD driver to decide.

Same

>   
>>> +		return flow_err(port_id,
>>> +				ops->template_table_destroy(dev,
>>> +							    template_table,
>>> +							    error),
>>> +				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 92be2a9a89..e87db5a540 100644
>>> --- a/lib/ethdev/rte_flow.h
>>> +++ b/lib/ethdev/rte_flow.h
>>> @@ -4961,6 +4961,266 @@ rte_flow_configure(uint16_t port_id,
>>>    		   const struct rte_flow_port_attr *port_attr,
>>>    		   struct rte_flow_error *error);
>>>
>>> +/**
>>> + * Opaque type returned after successful creation of pattern template.
>>> + * This handle can be used to manage the created pattern template.
>>> + */
>>> +struct rte_flow_pattern_template;
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Flow pattern template attributes.
>>> + */
>>> +__extension__
>>> +struct rte_flow_pattern_template_attr {
>>> +	/**
>>> +	 * Relaxed matching policy.
>>> +	 * - PMD may match only on items with mask member set and skip
>>> +	 * matching on protocol layers specified without any masks.
>>> +	 * - If not set, PMD will match on protocol layers
>>> +	 * specified without any masks as well.
>>> +	 * - Packet data must be stacked in the same order as the
>>> +	 * protocol layers to match inside packets, starting from the lowest.
>>> +	 */
>>> +	uint32_t relaxed_matching:1;
>>> +};
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Create pattern template.
>>
>> Create flow pattern template.
> 
> Ok.
> 
>>> + *
>>> + * The pattern template defines common matching fields without values.
>>> + * For example, matching on 5 tuple TCP flow, the template will be
>>> + * eth(null) + IPv4(source + dest) + TCP(s_port + d_port),
>>> + * while values for each rule will be set during the flow rule creation.
>>> + * The number and order of items in the template must be the same
>>> + * at the rule creation.
>>> + *
>>> + * @param port_id
>>> + *   Port identifier of Ethernet device.
>>> + * @param[in] template_attr
>>> + *   Pattern template attributes.
>>> + * @param[in] pattern
>>> + *   Pattern specification (list terminated by the END pattern item).
>>> + *   The spec member of an item is not used unless the end member is used.
>>
>> Interpretation of the pattern may depend on transfer vs non-transfer
>> rule to be used. It is essential information and we should provide it
>> when pattern template is created.
>>
>> The information is provided on table stage, but it is too late.
> 
> Why is it too late? Application knows which template goes to which table.
> And the pattern is generic to accommodate anything, user just need to put it
> into the right table.

Because it is more convenient to handle it when individual
template is processed. Otherwise error reporting will be
complicated since it could be just one template which is
wrong.

Otherwise, I see no point to have driver callbacks
template creation API. I can do nothing here since
I have no enough context. What's the problem to add
the context?

> 
>>
>>> + * @param[out] error
>>> + *   Perform verbose error reporting if not NULL.
>>> + *   PMDs initialize this structure in case of error only.
>>> + *
>>> + * @return
>>> + *   Handle on success, NULL otherwise and rte_errno is set.
>>> + */
>>> +__rte_experimental
>>> +struct rte_flow_pattern_template *
>>> +rte_flow_pattern_template_create(uint16_t port_id,
>>> +		const struct rte_flow_pattern_template_attr *template_attr,
>>> +		const struct rte_flow_item pattern[],
>>> +		struct rte_flow_error *error);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Destroy pattern template.
>>
>> Destroy flow pattern template.
> 
> Ok.
> 
>>> + *
>>> + * This function may be called only when
>>> + * there are no more tables referencing this template.
>>> + *
>>> + * @param port_id
>>> + *   Port identifier of Ethernet device.
>>> + * @param[in] pattern_template
>>> + *   Handle of the template to be destroyed.
>>> + * @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_pattern_template_destroy(uint16_t port_id,
>>> +		struct rte_flow_pattern_template *pattern_template,
>>> +		struct rte_flow_error *error);
>>> +
>>> +/**
>>> + * Opaque type returned after successful creation of actions template.
>>> + * This handle can be used to manage the created actions template.
>>> + */
>>> +struct rte_flow_actions_template;
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Flow actions template attributes.
>>> + */
>>> +struct rte_flow_actions_template_attr;
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Create actions template.
>>
>> Create flow rule actions template.
> 
> Yes, finally compensating for multiple no's.
> 
>>> + *
>>> + * The actions template holds a list of action types without values.
>>> + * For example, the template to change TCP ports is TCP(s_port + d_port),
>>> + * while values for each rule will be set during the flow rule creation.
>>> + * The number and order of actions in the template must be the same
>>> + * at the rule creation.
>>
>> Again, it highly depends on transfer vs non-transfer. Moreover,
>> application definitely know it. So, it should say if the action
>> is intended for transfer or non-transfer flow rule.
> 
> It is up to application to define which pattern it is going to use in different tables.

Same as above.

> 
>>> + *
>>> + * @param port_id
>>> + *   Port identifier of Ethernet device.
>>> + * @param[in] template_attr
>>> + *   Template attributes.
>>> + * @param[in] actions
>>> + *   Associated actions (list terminated by the END action).
>>> + *   The spec member is only used if @p masks spec is non-zero.
>>> + * @param[in] masks
>>> + *   List of actions that marks which of the action's member is constant.
>>> + *   A mask has the same format as the corresponding action.
>>> + *   If the action field in @p masks is not 0,
>>> + *   the corresponding value in an action from @p actions will be the part
>>> + *   of the template and used in all flow rules.
>>> + *   The order of actions in @p masks is the same as in @p actions.
>>> + *   In case of indirect actions present in @p actions,
>>> + *   the actual action type should be present in @p mask.
>>> + * @param[out] error
>>> + *   Perform verbose error reporting if not NULL.
>>> + *   PMDs initialize this structure in case of error only.
>>> + *
>>> + * @return
>>> + *   Handle on success, NULL otherwise and rte_errno is set.
>>> + */
>>> +__rte_experimental
>>> +struct rte_flow_actions_template *
>>> +rte_flow_actions_template_create(uint16_t port_id,
>>> +		const struct rte_flow_actions_template_attr *template_attr,
>>> +		const struct rte_flow_action actions[],
>>> +		const struct rte_flow_action masks[],
>>> +		struct rte_flow_error *error);

[snip]