Re: [dpdk-dev] [PATCH 2/3] ethdev: add flow item/action templates

[Alexander Kozyrev <akozyrev@nvidia.com>]

> From: Ivan Malov <Ivan.Malov@oktetlabs.ru> On Wednesday, October 6, 2021 13:25
> On 06/10/2021 07:48, 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 item template defines common matching fields (the item mask)
> without
> > values. The action 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 item and action 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 table creation time.
> >
> > The flow rule creation is done by selecting a table, an item template
> > and an action template (which are bound to the table), and setting unique
> > values for the items and actions.
> >
> > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > Suggested-by: Ori Kam <orika@nvidia.com>
> > ---
> >   lib/ethdev/rte_flow.h | 268
> ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 268 insertions(+)
> >
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index c69d503b90..ba3204b17e 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -4358,6 +4358,274 @@ int
> >   rte_flow_configure(uint16_t port_id,
> >   		   const struct rte_flow_port_attr *port_attr,
> >   		   struct rte_flow_error *error);
> > +
> > +/**
> > + * Opaque type returned after successfull creation of item template.
> 
> Typo: "successfull" --> "successful".
Thanks for noticing, will correct.

> > + * This handle can be used to manage the created item template.
> > + */
> > +struct rte_flow_item_template;
> > +
> > +__extension__
> > +struct rte_flow_item_template_attr {
> > +	/**
> > +	 * Version of the struct layout, should be 0.
> > +	 */
> > +	uint32_t version;
> > +	/* No attributes so far. */
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Create item template.
> > + * The item template defines common matching fields (item mask)
> 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 order of items in the template must be the same at rule insertion.
> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param[in] attr
> > + *   Item template attributes.
> 
> Please consider adding meaningful prefixes to "attr" here and below.
> This is needed to avoid confusion with "struct rte_flow_attr".
> 
> Example: "template_attr".
No problem.

> > + * @param[in] items
> > + *   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.
> > + */
> > +__rte_experimental
> > +struct rte_flow_item_template *
> > +rte_flow_item_template_create(uint16_t port_id,
> > +			      const struct rte_flow_item_template_attr *attr,
> > +			      const struct rte_flow_item items[],
> > +			      struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Destroy item template.
> > + * 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] template
> > + *   Handle to the template to be destroyed.
> 
> Perhaps "handle OF the template"?
You are right.

> > + * @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_item_template_destroy(uint16_t port_id,
> > +			       struct rte_flow_item_template *template,
> > +			       struct rte_flow_error *error);
> > +
> > +/**
> > + * Opaque type returned after successfull creation of action template.
> 
> Single "l" in "successful".
Ditto.

> > + * This handle can be used to manage the created action template.
> > + */
> > +struct rte_flow_action_template;
> > +
> > +__extension__
> > +struct rte_flow_action_template_attr {
> > +	/**
> > +	 * Version of the struct layout, should be 0.
> > +	 */
> > +	uint32_t version;
> > +	/* No attributes so far. */
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Create action template.
> > + * The action 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 order of the action in the template must be kept when inserting
> rules.
> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param[in] attr
> > + *   Template attributes.
> 
> Perhaps add a meaningful prefix to "attr".
Sure thing, will rename all the "attr" to "thing_attr".

> > + * @param[in] actions
> > + *   Associated actions (list terminated by the END action).
> > + *   The spec member is only used if the mask is 1.
> 
> Maybe "its mask is all ones"?
Not necessarily, just non-zero value would do. Will make it clearer.

> > + * @param[in] masks
> > + *   List of actions that marks which of the action's member is constant.
> 
> Consider the following action example:
> 
> struct rte_flow_action_vxlan_encap {
>          struct rte_flow_item *definition;
> };
> 
> So, if "definition" is not NULL, the whole header definition is supposed
> to be constant, right? Or am I missing something?
If definition has non-zero value then the action spec will be used in every rule created with this template.
In this particular example, yes, this definition is going to be a constant header for all the rules.


> > + *   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_action_template *
> > +rte_flow_action_template_create(uint16_t port_id,
> > +			const struct rte_flow_action_template_attr *attr,
> > +			const struct rte_flow_action actions[],
> > +			const struct rte_flow_action masks[],
> > +			struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Destroy action template.
> > + * 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] template
> > + *   Handle to 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_action_template_destroy(uint16_t port_id,
> > +			const struct rte_flow_action_template *template,
> > +			struct rte_flow_error *error);
> > +
> > +
> > +/**
> > + * Opaque type returned after successfull creation of table.
> 
> Redundant "l" in "successful".
Consider this fixed.

> > + * This handle can be used to manage the created table.
> > + */
> > +struct rte_flow_table;
> > +
> > +enum rte_flow_table_mode {
> > +	/**
> > +	 * Fixed size, the number of flow rules will be limited.
> > +	 * It is possible that some of the rules will not be inserted
> > +	 * due to conflicts/lack of space.
> > +	 * When rule insertion fails with try again error,
> > +	 * the application may use one of the following ways
> > +	 * to address this state:
> > +	 * 1. Keep this rule processing in the software.
> > +	 * 2. Try to offload this rule at a later time,
> > +	 *    after some rules have been removed from the hardware.
> > +	 * 3. Create a new table and add this rule to the new table.
> > +	 */
> > +	RTE_FLOW_TABLE_MODE_FIXED,
> > +	/**
> > +	 * Resizable, the PMD/HW will insert all rules.
> > +	 * No try again error will be received in this mode.
> > +	 */
> > +	RTE_FLOW_TABLE_MODE_RESIZABLE,
> > +};
> > +
> > +/**
> > + * Table attributes.
> > + */
> > +struct rte_flow_table_attr {
> > +	/**
> > +	 * Version of the struct layout, should be 0.
> > +	 */
> > +	uint32_t version;
> > +	/**
> > +	 * Flow attributes that will be used in the table.
> > +	 */
> > +	struct rte_flow_attr attr;
> 
> Perhaps, "flow_attr" then?
As we agreed.

> > +	/**
> > +	 * Maximum number of flow rules that this table holds.
> > +	 * It can be hard or soft limit depending on the mode.
> > +	 */
> > +	uint32_t max_rules;
> 
> How about "nb_flows_max"?
Just nb_flows maybe?