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

[Ori Kam]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> Subject: Re: [PATCH v5 02/10] ethdev: add flow item/action templates
> 
> On 2/12/22 01:25, Alexander Kozyrev wrote:
> > On Fri, Feb 11, 2022 6:27 Andrew Rybchenko <andrew.rybchenko at 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 at nvidia.com>
> >>> Acked-by: Ori Kam <orika at nvidia.com>
> 
> [snip]
> 

[Snip]

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

The idea is that the same template can be used in different
domains (ingress/egress and transfer)
May be we can add on which domains this template is expected to be used.
What do you think?

> >
> >>
> >>> + * @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.
> 
Same comment as above, what do you think?

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

Best,
Ori