Re: [dpdk-dev] [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API

[Ori Kam <orika@nvidia.com>]

Hi Cristian,

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Hi Ori,
> 
> > -----Original Message-----
> > From: Ori Kam <orika@nvidia.com>
> > Hi All,
> >
> > > -----Original Message-----
> > > From: Matan Azrad <matan@nvidia.com>
> > > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> > >
> > > Hi Cristian
> > >
> > > Thank you for your important review!
> > > I agree with all your comments except one, please see inline.
> > >
> > > From: Dumitrescu, Cristian
> > > > Hi Li and Matan,
> > > >
> > > > Thank you for your proposal, some comments below.
> > > >
> > > > I am also adding Jerin and Hemant to this thread, as they also
> participated
> > in
> > > > the definition of the rte_mtr API in 2017. Also Ajit expressed some
> > interest in
> > > a
> > > > previous email.
> > > >
> > > > > -----Original Message-----
> > > > > From: Li Zhang <lizh@nvidia.com>
> > > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > > > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> > > > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > Andrew
> > > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> > > > > <cristian.dumitrescu@intel.com>
> > > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> > > > >
> > > > > Currently, the flow meter policy does not support multiple actions per
> > > > > color; also the allowed action types per color are very limited.
> > > > > In addition, the policy cannot be pre-defined.
> > > > >
> > > > > Due to the growing in flow actions offload abilities there is a
> > > > > potential for the user to use variety of actions per color differently.
> > > > > This new meter policy API comes to allow this potential in the most
> > > > > ethdev common way using rte_flow action definition.
> > > > > A list of rte_flow actions will be provided by the user per color in
> > > > > order to create a meter policy.
> > > > > In addition, the API forces to pre-define the policy before the meters
> > > > > creation in order to allow sharing of single policy with multiple
> > > > > meters efficiently.
> > > > >
> > > > > meter_policy_id is added into struct rte_mtr_params.
> > > > > So that it can get the policy during the meters creation.
> > > > >
> > > > > Policy id 0 is default policy. Action per color as below:
> > > > > green - no action, yellow - no action, red - drop
> > > > >
> > > > > Allow coloring the packet using a new rte_flow_action_color as could
> > > > > be done by the old policy API,
> > > > >
> > > >
> > > > The proposal essentially is to define the meter policy based on rte_flow
> > > actions
> > > > rather than a reduced action set defined specifically just for meter object.
> > > This
> > > > makes sense to me.
> > > >
> > > > > The next API function were added:
> > > > > - rte_mtr_meter_policy_add
> > > > > - rte_mtr_meter_policy_delete
> > > > > - rte_mtr_meter_policy_update
> > > > > - rte_mtr_meter_policy_validate
> > > > > The next struct was changed:
> > > > > - rte_mtr_params
> > > > > - rte_mtr_capabilities
> > > > > The next API was deleted:
> > > > > - rte_mtr_policer_actions_update
> > > > >
> > > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > > ---
> > > > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > > > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > > > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> > > > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > > > >  4 files changed, 210 insertions(+), 74 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4 100644
> > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > @@ -31,6 +31,7 @@
> > > > >  #include <rte_ecpri.h>
> > > > >  #include <rte_mbuf.h>
> > > > >  #include <rte_mbuf_dyn.h>
> > > > > +#include <rte_meter.h>
> > > > >
> > > > >  #ifdef __cplusplus
> > > > >  extern "C" {
> > > > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > > > >        * See struct rte_flow_action_modify_field.
> > > > >        */
> > > > >       RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > > > +
> > > > > +     /**
> > > > > +      * Color the packet to reflect the meter color result.
> > > > > +      *
> > > > > +      * See struct rte_flow_action_color.
> > > > > +      */
> > > > > +     RTE_FLOW_ACTION_TYPE_COlOR,
> > > >
> > > > Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> > > >
> >
> > Why do we need this action?
> 
> We need this new proposed RTE_FLOW_ACTION_TYPE_COLOR action to set the
> packet color in the packet mbuf (i.e. in the mbuf::sched:color field) in order to
> tell the later stages of the pipeline what the packet color is.
> 
> > if it is to save the color it should be done by using mark/metadata
> 
> As stated in its description, the  RTE_FLOW_ACTION_TYPE_MARK action Is
> setting the mbuf::hash.fdir.hi field, which is used for a different purpose that is
> unrelated to the packet color, which has its own field within the mbuf.
> 

Agree,

> > Or by the action of meter.
> 
> The new proposed RTE_FLOW_ACTION_TYPE_COLOR action is indeed an action
> of the meter and meter only, right?
> 
> For example you can see
> > RTE_FLOW_ACTION_TYPE_SECURITY
> > Which if exist saves the session id to a dedicated mbuf field.
> >
> 
> The meter processing and action take place independently of the security API: it
> can be enabled when the security API is disabled and is not conditioned in any
> way by the security API. To be honest, I don't understand the connection with
> the security API that you are trying to make here.
> 

Sorry for not being clear,
I didn’t mean use the security action, what I meant is just like the security action
which when given, it will saves the session and pass it to the SW in dedicated member in the 
mbuf, the same with meter if the meter action is present then
the PMD should know to save the color value and extract it to the correct mbuf member.

Does that make sense?


> > > > >  };
> > > > >
> > > > >  /**
> >
> > [Snip]
> >
> > > > I suggest you do not redundantly specify the value of the default policy ID
> > in
> > > the
> > > > comment. Replace by "Default policy ID."
> > > >
> > > > > + * Action per color as below:
> > > > > + * green - no action, yellow - no action, red - drop
> > > >
> > > > This does not make sense to me as the default policy. The default policy
> > > should
> > > > be "no change", i.e. green -> green (no change), yellow -> yellow (no
> > change),
> > > > red -> red (no change).
> > >
> > > Can you explain why it doesn't make sense to you?
> > >
> > > Meter with "no change" for all colors has no effect on the packets so it is
> > > redundant action which just costs performance and resources - probably
> > never
> > > be used.
> > >
> > > The most common usage for meter is to drop all the packets come above
> > the
> > > defined rate limit - so it makes sense to take this behavior as default.
> > >
> > >
> > > > I suggest we avoid the "no action" statement, as it might be confusing.
> > >
> > > Maybe "do nothing" is better?
> > >
> > Maybe passthrough? Or in rte_flow passthru
> >
> 
> No, we need to save the packet color in the packet mbuf (mbuf::sched:color),
> and the RTE_FLOW_ACTION_TYPE_PASSTHRU action is not doing this.
> 

Please see my comment above.
The saving of color will be done automatically.

> >
> > > > > + * It can be used without creating it by the rte_mtr_meter_policy_add
> > > > > function.
> > > > > + */
> >
> >
> > Best,
> > Ori
> 
> Regards,
> Cristian

Best,
Ori