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

[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?
if it is to save the color it should be done by using mark/metadata
Or by the action of meter. For example you can see RTE_FLOW_ACTION_TYPE_SECURITY
Which if exist saves the session id to a dedicated mbuf field.

> > >  };
> > >
> > >  /**

[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
 

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


Best,
Ori