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

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

Hi,

On 13/10/2021 04:25, Alexander Kozyrev wrote:
>> 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?
> 

Probabably OK, too, but I don't have a strong opinion.
One more option: "table_size".

-- 
Ivan M