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

[Ori Kam <orika@nvidia.com>]

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.

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

> > +	/** 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.

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