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

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Monday, March 29, 2021 10:23 AM
> To: Matan Azrad <matan@nvidia.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Li Zhang <lizh@nvidia.com>; Dekel Peled
> <dekelp@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> <jerinjacobk@gmail.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> Ajit Khaparde <ajit.khaparde@broadcom.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar Yanai
> <roniba@nvidia.com>
> Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> 
> 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.

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

> > > >  };
> > > >
> > > >  /**
> 
> [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.

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

Regards,
Cristian