RE: [PATCH v8 02/11] ethdev: add flow item/action templates

[Ori Kam <orika@nvidia.com>]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, February 21, 2022 5:06 PM
> Subject: Re: [PATCH v8 02/11] ethdev: add flow item/action templates
> 
> On 2/21/22 16:12, Ori Kam wrote:
> > Hi Andrew,
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Monday, February 21, 2022 12:57 PM
> >> Subject: Re: [PATCH v8 02/11] ethdev: add flow item/action templates
> >>
> >> On 2/20/22 06:44, 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

[Snip]

> >> Anyway, the feature looks hidden in the patch.
> >>
> > No this is not hidden feature.
> > In current API application must specify all the preciding items,
> > For example application wants to match on udp source port.
> > The rte flow will look something like eth / ipv4/ udp sport = xxx ..
> > When PMD gets this pattern it must enforce the after the eth
> > there will be IPv4 and then UDP and then add the match for the
> > sport.
> > This means that the PMD addes extra matching.
> > If the application already validated that there is udp in the packet
> > in group 0 and then jump to group 1  it can save the HW those extra matching
> > by enabling this bit which means that the HW should only match on implicit
> > masked fields.
> 
> Old API allows to insert rule to non-0 table as well.
> So, similar logic could be applicable. Do we want to
> have the same feature in old API?
> 
Maybe but in any case this should be done when we can break API.
In general I'm not sure that any new capabilities that will be added to this
API will be implemented in the current one.

> >
> >>> +	/** Pattern valid for rules applied to ingress traffic. */
> >>> +	uint32_t ingress:1;
> >>> +	/** Pattern valid for rules applied to egress traffic. */
> >>> +	uint32_t egress:1;
> >>> +	/** Pattern valid for rules applied to transfer traffic. */
> >>> +	uint32_t transfer:1;
> >>> +};
> >>> +

[Snip]

> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Create flow actions template.
> >>> + *
> >>> + * 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.
> >>> + *
> >>> + * @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,
> >>
> >> Comparison with zero makes sense for integers only.
> >>
> >
> > Why? It can also be with pointers enums.
> 
> It should be NULL for pointers and enum-specific member of
> enum.
> 

Since NULL is zero, I think it is much better to have the same logic
and compare to 0 in all cases.
Adding dedicated enum member will break current API, in addition
if we will have more complex structures like arrays it is much easier
to compare the first element with 0.
You can look at it as true/false for each field.

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

Best,
Ori