RE: [RFC] ethdev: datapath-focused meter actions

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

I forgot to mention: besides the my statement at the top of my reply, there are many comments inline below :)

> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Tuesday, April 26, 2022 2:44 PM
> To: Jerin Jacob <jerinjacobk@gmail.com>; Alexander Kozyrev
> <akozyrev@nvidia.com>
> Cc: dpdk-dev <dev@dpdk.org>; Ori Kam <orika@nvidia.com>; Thomas
> Monjalon <thomas@monjalon.net>; Ivan Malov <ivan.malov@oktetlabs.ru>;
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Awal, Mohammad Abdul
> <mohammad.abdul.awal@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Jerin Jacob <jerinj@marvell.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: RE: [RFC] ethdev: datapath-focused meter actions
> 
> Hi Alexander,
> 
> After reviewing this RFC, I have to say that your proposal is very unclear to me.
> I don't understand what is the problem you're trying to solve and what exactly
> is that you cannot do with the current meter and flow APIs.
> 
> I suggest we get together for a community call with all the interested folks
> invited in order to get more clarity on your proposal, thank you!
> 
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Friday, April 8, 2022 9:21 AM
> > To: Alexander Kozyrev <akozyrev@nvidia.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>
> > Cc: dpdk-dev <dev@dpdk.org>; Ori Kam <orika@nvidia.com>; Thomas
> > Monjalon <thomas@monjalon.net>; Ivan Malov <ivan.malov@oktetlabs.ru>;
> > Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Awal, Mohammad Abdul
> > <mohammad.abdul.awal@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > Jerin Jacob <jerinj@marvell.com>; Ajit Khaparde
> > <ajit.khaparde@broadcom.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>
> > Subject: Re: [RFC] ethdev: datapath-focused meter actions
> >
> > + @Cristian Dumitrescu meter maintainer.
> >
> >
> > On Fri, Apr 8, 2022 at 8:17 AM Alexander Kozyrev <akozyrev@nvidia.com>
> > wrote:
> > >
> > > The introduction of asynchronous flow rules operations allowed users
> > > to create/destroy flow rules as part of the datapath without blocking
> > > on Flow API and slowing the packet processing down.
> > >
> > > That applies to every possible action that has no preparation steps.
> > > Unfortunately, one notable exception is the meter action.
> > > There is a separate API to prepare a meter profile and a meter policy
> > > before any meter object can be used as a flow rule action.
> 
> I disagree. Creation of meter policies and meter objects is decoupled from the
> flow creation. Meter policies and meter objects can all be created at
> initialization or on-the-fly, and their creation does not directly require the data
> plane to be stopped.
> 
> Please explain what problem are you trying to fix here. I suggest you provide
> the sequence diagram and tell us where the problem is.
> 
> > >
> > > The application logic is the following:
> > > 1. rte_mtr_meter_profile_add() is called to create the meter profile
> > > first to define how to classify incoming packets and to assign an
> > > appropriate color to them.
> > > 2. rte_mtr_meter_policy_add() is invoked to define the fate of a packet,
> > > based on its color (practically creating flow rules, matching colors).
> 
> Nope, the policy add does not create any flows. In fact, it does not create any
> meter objects either. It simply defines a configuration pattern that can be
> reused many times when meter objects are created afterwards.
> 
> > > 3. rte_mtr_create() is then needed to search (with locks) for previously
> > > created profile and policy in order to create the meter object.
> 
> The rte_mtr_create() is not created at the time the flow is created, but at a
> prior decoupled moment. I don't see any issue here.
> 
> > > 4. rte_flow_create() is now finally can be used to specify the created
> > > meter as an action.
> > >
> > > This approach doesn't fit into the asynchronous rule creation model
> > > and can be improved with the following proposal:
> 
> Again, the creation of meter policies and objects is decoupled from the flow
> creation; in fact, the meter policies and objects must be created before the
> flows using them are created.
> 
> > > 1. Creating a policy may be replaced with the creation of a group with
> > > up to 3 different rules for every color using asynchronous Flow API.
> > > That requires the introduction of a new pattern item - meter color.
> > > Then creation a flow rule with the meter means a simple jump to a group:
> > > rte_flow_async_create(group=1, pattern=color, actions=...);
> > > rte_flow_async_create(group=0, pattern=5-tuple,
> > >                       actions=meter,jump group 1);
> > > This allows to classify packets and act upon their color classifications.
> > > The Meter action assigns a color to a packet and an appropriate action
> > > is selected based on the Meter color in group 1.
> > >
> 
> The meter objects requires a relatively complex configuration procedure. This
> is one of the reasons meters have their own API, so we can keep that
> complexity away from the flow API.
> 
> You seem to indicate that your desired behavior is to create the meter objects
> when the flow is created rather than in advance. Did I get it correctly? This is
> possible with the current API as well by simply creating the meter object
> immediately before the flow gets created.
> 
> Stitching the creation of new meter object to the flow creation (if I understand
> your approach right) doe not allow for some important features, such as:
> -reusing meter objects that were previously created by reassigning them to a
> different flow
> -having multiple flows use the same shared meter.
> 
> > > 2. Preparing a meter object should be the part of flow rule creation
> 
> Why?? Please take some time to clearly explain this, your entire proposal
> seems to be predicated on this assertion being true.
> 
> > > and use the same flow queue to benefit from asynchronous operations:
> > > rte_flow_async_create(group=0, pattern=5-tuple,
> > >                       actions=meter id 1 profile rfc2697, jump group 1);
> > > Creation of the meter object takes time and flow creation must wait
> > > until it is ready before inserting the rule. Using the same queue allows
> > > ensuring that. There is no need to create a meter object outside of the
> > > Flow API, but this approach won't affect the old Meter API in any way.
> > >
> > > 3. Another point of optimization is to prepare all the resources needed
> > > in advance in rte_flow_configure().
> 
> This seems to directly contradict you previous statement that meter objects
> need to be created at the same time when the flow is created (exact quote of
> your statement from above: " Preparing a meter object should be the part of
> flow rule creation").
> 
> All the policy rules can be created
> > > during the initialization stage easily and put into several groups.
> > > These groups can be used by many meter objects by simple jump action to
> > > an appropriate group. Meter objects can be preallocated as well and
> > > configured with required profile parameters later at the flow rule
> > > creation stage. The number of pre-allocated profiles/policies is
> > > specified in the Flow engine resources settings.
> > >
> > > These optimizations alongside already existing pattern/actions templates
> > > can improve the insertion rate significantly and allow meter usage as
> > > part of the datapath. The introduction of the new API is intended to be
> > > used with the asynchronous Flow API. Deprecation of the old Meter API
> > > is not planned at this point.
> > >
> > > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > > ---
> > >  lib/ethdev/rte_flow.h | 71
> > ++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 70 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > > index d8827dd184..aec36a9f0a 100644
> > > --- a/lib/ethdev/rte_flow.h
> > > +++ b/lib/ethdev/rte_flow.h
> > > @@ -33,6 +33,7 @@
> > >  #include <rte_bitops.h>
> > >  #include <rte_mbuf.h>
> > >  #include <rte_mbuf_dyn.h>
> > > +#include <rte_mtr.h>
> > >  #include <rte_meter.h>
> > >  #include <rte_gtp.h>
> > >  #include <rte_l2tpv2.h>
> > > @@ -671,6 +672,13 @@ enum rte_flow_item_type {
> > >          * See struct rte_flow_item_gre_opt.
> > >          */
> > >         RTE_FLOW_ITEM_TYPE_GRE_OPTION,
> > > +
> > > +       /**
> > > +        * Matches Meter Color.
> > > +        *
> > > +        * See struct rte_flow_item_meter_color.
> > > +        */
> > > +       RTE_FLOW_ITEM_TYPE_METER_COLOR,
> 
> As discussed in the previous community call on meters, it makes perfect sense
> to me to be able to use the meter color as one of the flow match fields.
> 
> We just need to make sure that when this is needed, it is guaranteed that the
> packet has a color, i.e. there is a meter action previously in this chain that got
> executed, or there is a default packet color if not. How do we make sure of
> this?
> 
> > >  };
> > >
> > >  /**
> > > @@ -1990,6 +1998,26 @@ static const struct rte_flow_item_ppp
> > rte_flow_item_ppp_mask = {
> > >  };
> > >  #endif
> > >
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > + *
> > > + * RTE_FLOW_ITEM_TYPE_METER_COLOR
> > > + *
> > > + * Matches a meter color set in the packet meta-data
> > > + * (i.e. struct rte_mbuf::sched::color).
> > > + */
> > > +struct rte_flow_item_meter_color {
> > > +       enum rte_color color; /**< Packet color. */
> > > +};
> > > +
> > > +/** Default mask for RTE_FLOW_ITEM_TYPE_METER_COLOR. */
> > > +#ifndef __cplusplus
> > > +static const struct rte_flow_item_meter_color
> > rte_flow_item_meter_color_mask = {
> > > +       .color = 0x3,
> > > +};
> > > +#endif
> > > +
> > >  /**
> > >   * Matching pattern item definition.
> > >   *
> > > @@ -2376,6 +2404,14 @@ enum rte_flow_action_type {
> > >          */
> > >         RTE_FLOW_ACTION_TYPE_METER,
> > >
> > > +       /**
> > > +        * Extended Traffic metering and policing (MTR).
> > > +        *
> > > +        * See struct rte_flow_action_meter_ext.
> > > +        * See file rte_mtr.h for MTR object configuration.
> > > +        */
> > > +       RTE_FLOW_ACTION_TYPE_METER_EXT,
> > > +
> > >         /**
> > >          * Redirects packets to security engine of current device for security
> > >          * processing as specified by security session.
> > > @@ -3128,6 +3164,19 @@ struct rte_flow_action_meter {
> > >         uint32_t mtr_id; /**< MTR object ID created with rte_mtr_create(). */
> > >  };
> > >
> > > +/**
> > > + * RTE_FLOW_ACTION_TYPE_METER_EXT
> > > + *
> > > + * Extended Traffic metering and policing (MTR).
> > > + *
> > > + * Packets matched by items of this type can be either dropped or passed
> to
> > the
> > > + * next item with their color set by the MTR object.
> > > + */
> > > +struct rte_flow_action_meter_ext {
> > > +       uint32_t mtr_id; /**< MTR object ID. */
> > > +       struct rte_meter_profile *profile; /**< MTR profile. */
> > > +};
> > > +
> 
> How is this proposed meter extended action different from the existing meter
> action? This is not explained at all here, please explain.
> 
> The comment seems to indicate a copy & paste error, as "packets matched by
> items of this type ..." indicates a match item, and this is an action item.
> 
> > >  /**
> > >   * RTE_FLOW_ACTION_TYPE_SECURITY
> > >   *
> > > @@ -4899,10 +4948,20 @@ struct rte_flow_port_info {
> > >          */
> > >         uint32_t max_nb_aging_objects;
> > >         /**
> > > -        * Maximum number traffic meters.
> > > +        * Maximum number of traffic meters.
> > >          * @see RTE_FLOW_ACTION_TYPE_METER
> > >          */
> > >         uint32_t max_nb_meters;
> > > +       /**
> > > +        * Maximum number of traffic meter profiles.
> > > +        * @see RTE_FLOW_ACTION_TYPE_METER
> > > +        */
> > > +       uint32_t max_nb_meter_profiles;
> > > +       /**
> > > +        * Maximum number of traffic meters policices.
> > > +        * @see RTE_FLOW_ACTION_TYPE_METER
> > > +        */
> > > +       uint32_t max_nb_meter_policies;
> > >  };
> > >
> > >  /**
> > > @@ -4972,6 +5031,16 @@ struct rte_flow_port_attr {
> > >          * @see RTE_FLOW_ACTION_TYPE_METER
> > >          */
> > >         uint32_t nb_meters;
> > > +       /**
> > > +        * Number of meter profiles to configure.
> > > +        * @see RTE_FLOW_ACTION_TYPE_METER
> > > +        */
> > > +       uint32_t nb_meter_profiles;
> > > +       /**
> > > +        * Number of meter policies to configure.
> > > +        * @see RTE_FLOW_ACTION_TYPE_METER
> > > +        */
> > > +       uint32_t nb_meter_policies;
> > >  };
> > >
> > >  /**
> > > --
> > > 2.18.2
> > >
> 
> Regards,
> Cristian