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

[Ivan Malov <ivan.malov@oktetlabs.ru>]

Hi,

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

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

For sure, "const struct rte_flow_item items[]" would look better
when renamed to "const struct rte_flow_item pattern[]".

The same goes for "rte_flow_action_template_create()" and "at_attr".

Perhaps, "rte_flow_action_list_shape_create()" then?

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

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

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

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

> +                   struct rte_flow_item_template *item_templates[],
Perhaps, "const struct"? The name could be "pattern_templates".

> +                   uint8_t nb_item_templates,
Why not "unsigned int"? The name could be "nb_pattern_templates".

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

--
Ivan M.