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

[Alexander Kozyrev <akozyrev@nvidia.com>]

On Wednesday, January 19, 2022 10:16 Ivan Malov <ivan.malov@oktetlabs.ru> wrote:

> > +Oftentimes in an application, many flow rules share a common structure
> > +(the same pattern 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 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. This API is not thread-safe.
> 
> Consider:
> 
> +Typically, flow rules generated by a given application conform to a small
> +group of "shapes". What defines a "shape" is a set of specific item masks
> +and action types. This knowledge facilitates optimisations in PMDs / HW.
> +
> +With such "shapes" (templates) being grouped in tables, a flow rule can
> +be created by selecting a template (pattern, action list) within a given
> +table and filling out specific match / action properties.
I don't like the idea of introducing the new term here.
Shaping is associated with bandwidth throttling in the network.
But I like your wording better, let me adopt this style.

> > +     struct rte_flow_item_template *
> > +     rte_flow_item_template_create(uint16_t port_id,
> > +                             const struct rte_flow_item_template_attr
> *it_attr,
> > +                             const struct rte_flow_item items[],
> > +                             struct rte_flow_error *error);
> 
> I'm afraid "it_attr" is hardly readable. Also, the API name can
> trick users into thinking that it's all about creating a single
> item template rather than a flow pattern template.
> Perhaps rename to "rte_flow_pattern_template_create()"?
> Use "tmpl" instead of "template"? Or "shape" maybe?
Maybe rte_flow_pattern_template_create() is better. Will rename.

> For sure, "const struct rte_flow_item items[]" would look better
> when renamed to "const struct rte_flow_item pattern[]".
Agree, will rename to pattern as in the rte_flow_create() function.

> The same goes for "rte_flow_action_template_create()" and "at_attr".
Ok, will rename it_attr and at_attr to {pattern/actions}_template_attr.

> Perhaps, "rte_flow_action_list_shape_create()" then?
Definitely not shape, let's stick to what we already have in the rte_flow_create().
Will keep actions[] to match the existing API. actions_list is something different.

> > +A table combines a number of item and action templates along with
> shared flow
> > +rule attributes (group ID, priority and traffic direction). This way a
> PMD/HW
> 
> Please consider:
> 
> +A template table consists of multiple pattern templates and action list
> +templates associated with a single set of rule attributes (group ID,
> +priority, etc).
Good wording, thanks.

> Perhaps rename "item_templates[]" and "action_templates[]"
> to "pattern_templates[]" and "action_list_templates[]".
> Maybe make use of the term "shape" here as well...
Will use "pattern_templates[]" and "action_templates[]".

> > +     /**
> > +      * 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;
> 
> Consider rewording this to a bullet-formatted set of statements.
> For brevity. For improved clarity.
Ok.

> > +      * Flow attributes that will be used in the table.
> 
> Perhaps: "Flow attributes to be used in each rule generated from this
> table". Something like that.
No problem.
> 
> > +                   struct rte_flow_item_template *item_templates[],
> Perhaps, "const struct"? The name could be "pattern_templates".
Cannot make them constant because we modify reference counter for them inside.
That allows us to track which templates are still in use by tables.

> > +                   uint8_t nb_item_templates,
> Why not "unsigned int"? The name could be "nb_pattern_templates".
The reason for using only 256 is that we can use an array.
It is to wasteful to allocate larger array for very few templates.
If you have more templates you should consider creating different tables.

> > +                   struct rte_flow_action_template *action_templates[],
> > +                   uint8_t nb_action_templates,
> Same questions here.
Same answers as above.