Re: [dpdk-dev] [RFC] add flow action context API

[Andrey Vesnovaty <andrey.vesnovaty@gmail.com>]

On Wed, Jun 3, 2020 at 1:02 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 20/05/2020 11:18, Andrey Vesnovaty:
> > This commit introduces extension of DPDK flow action API enabling
> > modification of single rte_flow_action.
> >
> > Motivation and example
> > ===
> > Adding or removing one or more queues to RSS actions cloned in multiple
> > flow rules imposes per rule toll for current DPDK flow API; the scenario
> > requires for each flow sharing cloned RSS action:
> > - call `rte_flow_destroy()`
> > - call `rte_flow_create()` with modified RSS action
> >
> > In order to prevent the overhead of multiple RSS flow rules
> reconfiguration
> > API for in-place flow action modification introduced in this commit.
>
> It seems there is an usability improvement with this new API.
> If I understand well, the main motivation is to improve the performance?
> The PMD implementation should try to keep a shared object
> to benefit of the performance improvement, right?
>
> Right, the goal is performance improvement.
Single API call modifies behaviour of multiple flows.

>
> The existing rte_flow API functions are:
>         rte_flow_validate()
>         rte_flow_create()
>         rte_flow_destroy()
>         rte_flow_flush()
>         rte_flow_query()
>         rte_flow_isolate()
>         rte_flow_get_aged_flows()
>
> > +     # added in 20.08
> > +     rte_flow_action_ctx_create;
> > +     rte_flow_action_ctx_destoy;
> > +     rte_flow_action_ctx_modify;
> > +     rte_flow_action_ctx_query;
>
> We had "create", "destroy", "query", but no "modify" capability.
> The new API is adding 2 things in my opinion:
>         - shared action object
>         - "modify" capability (is "update" a better wording?)
>

Naming is one of the most challenging parts of this RFC.
Some similarity I have found in existing code is
rte_mtr_policer_actions_update()
Is there any existing code having update/modify semantics?

>
> About the wording, do we need "ctx"?
> I feel rte_flow_action is a good enough prefix for this API,
> and should be documented as a shared action object.
> I think the word "object" is more meaningful than "context".
> Am I missing something?
>
> CTX comes for the fact that each flow_rule doesn't have an ownership for
the given action but operates inside some context (shared action context
actually).
As mentioned above, naming is one of the most challenging parts of this
RFC.

>
> > +     /**
> > +      * Describes action context.
> > +      *
> > +      * Enables multiple rules reference the same action by id/ctx.
> > +      *
> > +      * No action specific struct here (void*) since it can be any
> > +      * action type.
> > +      */
> > +     RTE_FLOW_ACTION_TYPE_CTX,
>
> Why do we need a new action type?
>
> Because it's not an action itself but a reference/handle to it.

>
> > @@ -101,6 +101,28 @@ struct rte_flow_ops {
> > +     /** See rte_flow_action_ctx_destoy() */
> > +     void *(*action_ctx_create)
> > +             (struct rte_eth_dev *dev,
> > +             const struct rte_flow_action *action,
> > +             struct rte_flow_error *error);
> > +     /** See rte_flow_action_ctx_create() */
> > +     int (*action_ctx_destroy)
> > +             (struct rte_eth_dev *dev,
> > +             void *ctx,
> > +             struct rte_flow_error *error);
> > +     /** See rte_flow_action_ctx_modify() */
> > +     int (*action_ctx_modify)
> > +             (struct rte_eth_dev *dev,
> > +             void *ctx,
> > +             const void *action_conf,
> > +             struct rte_flow_error *error);
> > +     /** See rte_flow_action_ctx_query() */
> > +     int (*action_ctx_query)
> > +             (struct rte_eth_dev *dev,
> > +             const void *ctx,
> > +             void *data,
> > +             struct rte_flow_error *error);
>
> API functions are directly linked to PMD ops, it looks simple and good.
>
>
>