Re: [PATCH v8 02/11] ethdev: add flow item/action templates

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

On 2/21/22 16:12, Ori Kam wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, February 21, 2022 12:57 PM
>> Subject: Re: [PATCH v8 02/11] ethdev: add flow item/action templates
>>
>> On 2/20/22 06:44, 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]
>>
>>> +For example, to create an actions template with the same Mark ID
>>> +but different Queue Index for every rule:
>>> +
>>> +.. code-block:: c
>>> +
>>> +	rte_flow_actions_template_attr attr = {.ingress = 1};
>>> +	struct rte_flow_action act[] = {
>>> +		/* Mark ID is 4 for every rule, Queue Index is unique */
>>> +		[0] = {.type = RTE_FLOW_ACTION_TYPE_MARK,
>>> +		       .conf = &(struct rte_flow_action_mark){.id = 4}},
>>> +		[1] = {.type = RTE_FLOW_ACTION_TYPE_QUEUE},
>>> +		[2] = {.type = RTE_FLOW_ACTION_TYPE_END,},
>>> +	};
>>> +	struct rte_flow_action msk[] = {
>>> +		/* Assign to MARK mask any non-zero value to make it constant */
>>> +		[0] = {.type = RTE_FLOW_ACTION_TYPE_MARK,
>>> +		       .conf = &(struct rte_flow_action_mark){.id = 1}},
>>
>> 1 looks very strange. I can understand it in the case of
>> integer and boolean fields, but what to do in the case of
>> arrays? IMHO, it would be better to use all 0xff's in value.
>> Anyway, it must be defined very carefully and non-ambiguous.
>>
> There is some issues with all 0xff for example in case of pointers or
> enums this it will result in invalid value.
> So I vote for saving it as is.
> I fully agree that it should be defined very clearly.
> I think that for arrays with predefined size (I don't think we have such in rte_flow)
> it should be declared that that the first element should not be 0.

It is good that we agree that the aspect should be documented
very carefully. Let's do it.

> 
>>> +		[1] = {.type = RTE_FLOW_ACTION_TYPE_QUEUE},
>>> +		[2] = {.type = RTE_FLOW_ACTION_TYPE_END,},
>>> +	};
>>> +	struct rte_flow_error err;
>>> +
>>> +	struct rte_flow_actions_template *actions_template =
>>> +		rte_flow_actions_template_create(port, &attr, &act, &msk, &err);
>>> +
>>> +The concrete value for Queue Index will be provided at the rule creation.
>>
>> [snip]
>>
>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>>> index ffd48e40d5..e9f684eedb 100644
>>> --- a/lib/ethdev/rte_flow.c
>>> +++ b/lib/ethdev/rte_flow.c
>>> @@ -1461,3 +1461,255 @@ 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 (template_attr == NULL) {
>>> +		RTE_FLOW_LOG(ERR,
>>> +			     "Port %"PRIu16" template attr is NULL.\n",
>>> +			     port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_ATTR,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (pattern == NULL) {
>>> +		RTE_FLOW_LOG(ERR,
>>> +			     "Port %"PRIu16" pattern is NULL.\n",
>>> +			     port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_ATTR,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (dev->data->flow_configured == 0) {
>>> +		RTE_FLOW_LOG(INFO,
>>> +			"Flow engine on port_id=%"PRIu16" is not configured.\n",
>>> +			port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				RTE_FLOW_ERROR_TYPE_STATE,
>>> +				NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (unlikely(!ops))
>>> +		return NULL;
>>
>> See notes about order of checks in previous patch review notes.
>>
>>> +	if (likely(!!ops->pattern_template_create)) {
>>> +		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(pattern_template == NULL))
>>> +		return 0;
>>> +	if (unlikely(!ops))
>>> +		return -rte_errno;
>>
>> Same here. I'm afraid it is really important here as well,
>> since request should not return OK if port_id is invalid.
>>
>>
>>> +	if (likely(!!ops->pattern_template_destroy)) {
>>> +		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 (template_attr == NULL) {
>>> +		RTE_FLOW_LOG(ERR,
>>> +			     "Port %"PRIu16" template attr is NULL.\n",
>>> +			     port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_ATTR,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (actions == NULL) {
>>> +		RTE_FLOW_LOG(ERR,
>>> +			     "Port %"PRIu16" actions is NULL.\n",
>>> +			     port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_ATTR,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (masks == NULL) {
>>> +		RTE_FLOW_LOG(ERR,
>>> +			     "Port %"PRIu16" masks is NULL.\n",
>>> +			     port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_ATTR,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +
>>> +	}
>>> +	if (dev->data->flow_configured == 0) {
>>> +		RTE_FLOW_LOG(INFO,
>>> +			"Flow engine on port_id=%"PRIu16" is not configured.\n",
>>> +			port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_STATE,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (unlikely(!ops))
>>> +		return NULL;
>>
>> same here
>>
>>> +	if (likely(!!ops->actions_template_create)) {
>>> +		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(actions_template == NULL))
>>> +		return 0;
>>> +	if (unlikely(!ops))
>>> +		return -rte_errno;
>>
>> same here
>>
>>> +	if (likely(!!ops->actions_template_destroy)) {
>>> +		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 (table_attr == NULL) {
>>> +		RTE_FLOW_LOG(ERR,
>>> +			     "Port %"PRIu16" table attr is NULL.\n",
>>> +			     port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_ATTR,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (pattern_templates == NULL) {
>>> +		RTE_FLOW_LOG(ERR,
>>> +			     "Port %"PRIu16" pattern templates is NULL.\n",
>>> +			     port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_ATTR,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (actions_templates == NULL) {
>>> +		RTE_FLOW_LOG(ERR,
>>> +			     "Port %"PRIu16" actions templates is NULL.\n",
>>> +			     port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_ATTR,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (dev->data->flow_configured == 0) {
>>> +		RTE_FLOW_LOG(INFO,
>>> +			"Flow engine on port_id=%"PRIu16" is not configured.\n",
>>> +			port_id);
>>> +		rte_flow_error_set(error, EINVAL,
>>> +				   RTE_FLOW_ERROR_TYPE_STATE,
>>> +				   NULL, rte_strerror(EINVAL));
>>> +		return NULL;
>>> +	}
>>> +	if (unlikely(!ops))
>>> +		return NULL;
>>
>> Order of checks
>>
>>> +	if (likely(!!ops->template_table_create)) {
>>> +		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(template_table == NULL))
>>> +		return 0;
>>> +	if (unlikely(!ops))
>>> +		return -rte_errno;
>>> +	if (likely(!!ops->template_table_destroy)) {
>>> +		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 cdb7b2be68..776e8ccc11 100644
>>> --- a/lib/ethdev/rte_flow.h
>>> +++ b/lib/ethdev/rte_flow.h
>>> @@ -4983,6 +4983,280 @@ 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.
>>
>> Would it be useful to mentioned that at least one direction
>> bit must be set? Otherwise request does not make sense.
>>
> Agree one direction must be set.
> 
>>> + */
>>> +__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;
>>
>> I should notice this earlier, but it looks like a new feature
>> which sounds unrelated to templates. If so, it makes asymmetry
>> in sync and async flow rules capabilities.
>> Am I missing something?
>>
>> Anyway, the feature looks hidden in the patch.
>>
> No this is not hidden feature.
> In current API application must specify all the preciding items,
> For example application wants to match on udp source port.
> The rte flow will look something like eth / ipv4/ udp sport = xxx ..
> When PMD gets this pattern it must enforce the after the eth
> there will be IPv4 and then UDP and then add the match for the
> sport.
> This means that the PMD addes extra matching.
> If the application already validated that there is udp in the packet
> in group 0 and then jump to group 1  it can save the HW those extra matching
> by enabling this bit which means that the HW should only match on implicit
> masked fields.

Old API allows to insert rule to non-0 table as well.
So, similar logic could be applicable. Do we want to
have the same feature in old API?

> 
>>> +	/** Pattern valid for rules applied to ingress traffic. */
>>> +	uint32_t ingress:1;
>>> +	/** Pattern valid for rules applied to egress traffic. */
>>> +	uint32_t egress:1;
>>> +	/** Pattern valid for rules applied to transfer traffic. */
>>> +	uint32_t transfer:1;
>>> +};
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Create flow pattern template.
>>> + *
>>> + * 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.
>>> + * @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.
>>
>> Don't we want to be explicit about used negative error code?
>> The question is applicable to all functions.
>>
> Same answer as given in other patch.
> Since PMD may have different/extra error codes I don't think we should
> give them here.
> 
>> [snip]
>>
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Flow actions template attributes.
>>
>> Same question about no directions specified.
>>
>>> + */
>>> +__extension__
>>> +struct rte_flow_actions_template_attr {
>>> +	/** Action valid for rules applied to ingress traffic. */
>>> +	uint32_t ingress:1;
>>> +	/** Action valid for rules applied to egress traffic. */
>>> +	uint32_t egress:1;
>>> +	/** Action valid for rules applied to transfer traffic. */
>>> +	uint32_t transfer:1;
>>> +};
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Create flow actions template.
>>> + *
>>> + * 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.
>>> + *
>>> + * @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,
>>
>> Comparison with zero makes sense for integers only.
>>
> 
> Why? It can also be with pointers enums.

It should be NULL for pointers and enum-specific member of
enum.

> 
>>> + *   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]
> 
> Best,
> Ori
>