[RFC] ethdev: datapath-focused meter actions

[RFC] ethdev: datapath-focused meter actions

[Alexander Kozyrev]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

Agenda: to discuss https://patchwork.dpdk.org/project/dpdk/patch/20220408024658.2004918-1-akozyrev@nvidia.com/
Moving the meeting in order to address Cristian's concerns about initial RFC, will prepare new version and slides to address them.

[-- Attachment #2: Type: text/calendar, Size: 4875 bytes --]

BEGIN:VCALENDAR
METHOD:REQUEST
PRODID:Microsoft Exchange Server 2010
VERSION:2.0
BEGIN:VTIMEZONE
TZID:Eastern Standard Time
BEGIN:STANDARD
DTSTART:16010101T020000
TZOFFSETFROM:-0400
TZOFFSETTO:-0500
RRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=1SU;BYMONTH=11
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:16010101T020000
TZOFFSETFROM:-0500
TZOFFSETTO:-0400
RRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=2SU;BYMONTH=3
END:DAYLIGHT
END:VTIMEZONE
BEGIN:VEVENT
ORGANIZER;CN=Alexander Kozyrev:mailto:akozyrev@nvidia.com
ATTENDEE;ROLE=REQ-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Ori Kam:ma
 ilto:orika@nvidia.com
ATTENDEE;ROLE=REQ-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Jerin Jaco
 b:mailto:jerinj@marvell.com
ATTENDEE;ROLE=REQ-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Cristian D
 umitrescu:mailto:cristian.dumitrescu@intel.com
ATTENDEE;ROLE=REQ-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=NBU-Contac
 t-Thomas Monjalon (EXTERNAL):mailto:thomas@monjalon.net
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Ray Kinsel
 la:mailto:mdr@ashroe.eu
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Sunil Kuma
 r Kori:mailto:skori@marvell.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Andrew Ryb
 chenko:mailto:andrew.rybchenko@oktetlabs.ru
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Vipin.Varg
 hese@amd.com:mailto:Vipin.Varghese@amd.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Ivan Malov
 :mailto:ivan.malov@oktetlabs.ru
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN="Awal, Moha
 mmad Abdul":mailto:mohammad.abdul.awal@intel.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN="Zhang, Qi 
 Z":mailto:qi.z.zhang@intel.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Ajit Khapa
 rde:mailto:ajit.khaparde@broadcom.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN="Richardson
 , Bruce":mailto:bruce.richardson@intel.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Konstantin
  Ananyev:mailto:konstantin.ananyev@intel.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN="Singh, Jas
 vinder":mailto:jasvinder.singh@intel.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=Ferruh Yig
 it:mailto:ferruh.yigit@xilinx.com
ATTENDEE;ROLE=OPT-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN=dev@dpdk.o
 rg:mailto:dev@dpdk.org
DESCRIPTION;LANGUAGE=en-US:Agenda: to discuss https://patchwork.dpdk.org/pr
 oject/dpdk/patch/20220408024658.2004918-1-akozyrev@nvidia.com/\nMoving the
  meeting in order to address Cristian's concerns about initial RFC\, will 
 prepare new version and slides to address them.\n
UID:040000008200E00074C5B7101A82E00800000000C095FF317C59D801000000000000000
 0100000000A269881A79BA24EB83F9C1E7770D105
SUMMARY;LANGUAGE=en-US:[RFC] ethdev: datapath-focused meter actions
DTSTART;TZID=Eastern Standard Time:20220511T110000
DTEND;TZID=Eastern Standard Time:20220511T120000
CLASS:PUBLIC
PRIORITY:5
DTSTAMP:20220427T192242Z
TRANSP:OPAQUE
STATUS:CONFIRMED
SEQUENCE:2
LOCATION;LANGUAGE=en-US:https://meet.jit.si/DPDK
X-MICROSOFT-CDO-APPT-SEQUENCE:2
X-MICROSOFT-CDO-OWNERAPPTID:-1162197018
X-MICROSOFT-CDO-BUSYSTATUS:TENTATIVE
X-MICROSOFT-CDO-INTENDEDSTATUS:BUSY
X-MICROSOFT-CDO-ALLDAYEVENT:FALSE
X-MICROSOFT-CDO-IMPORTANCE:1
X-MICROSOFT-CDO-INSTTYPE:0
X-MICROSOFT-ONLINEMEETINGINFORMATION:{"OnlineMeetingChannelId":null\,"Onlin
 eMeetingProvider":3}
X-MICROSOFT-SKYPETEAMSMEETINGURL:https://teams.microsoft.com/l/meetup-join/
 19%3ameeting_ODJmNmM2YzMtNzhkNS00MWU4LTgzOWQtNjViOGZjMjcwYWUx%40thread.v2/
 0?context=%7b%22Tid%22%3a%2243083d15-7273-40c1-b7db-39efd9ccc17a%22%2c%22O
 id%22%3a%22ba2ecf61-0c29-4a84-a635-fac1420713ed%22%7d
X-MICROSOFT-SCHEDULINGSERVICEUPDATEURL:https://api.scheduler.teams.microsof
 t.com/teams/43083d15-7273-40c1-b7db-39efd9ccc17a/ba2ecf61-0c29-4a84-a635-f
 ac1420713ed/19_meeting_ODJmNmM2YzMtNzhkNS00MWU4LTgzOWQtNjViOGZjMjcwYWUx@th
 read.v2/0
X-MICROSOFT-SKYPETEAMSPROPERTIES:{"cid":"19:meeting_ODJmNmM2YzMtNzhkNS00MWU
 4LTgzOWQtNjViOGZjMjcwYWUx@thread.v2"\,"private":true\,"type":0\,"mid":0\,"
 rid":0\,"uid":null}
X-MICROSOFT-ONLINEMEETINGCONFLINK:conf:sip:akozyrev@nvidia.com\;gruu\;opaqu
 e=app:conf:focus:id:teams:2:0!19:meeting_ODJmNmM2YzMtNzhkNS00MWU4LTgzOWQtN
 jViOGZjMjcwYWUx-thread.v2!ba2ecf610c294a84a635fac1420713ed!43083d15727340c
 1b7db39efd9ccc17a
X-MICROSOFT-DONOTFORWARDMEETING:FALSE
X-MICROSOFT-DISALLOW-COUNTER:FALSE
X-MICROSOFT-LOCATIONS:[ { "DisplayName" : "https://meet.jit.si/DPDK"\, "Loc
 ationAnnotation" : ""\, "LocationSource" : 0\, "Unresolved" : true\, "Loca
 tionUri" : "" } ]
BEGIN:VALARM
DESCRIPTION:REMINDER
TRIGGER;RELATED=START:-PT15M
ACTION:DISPLAY
END:VALARM
END:VEVENT
END:VCALENDAR

[RFC] ethdev: datapath-focused meter actions

[Alexander Kozyrev]

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.

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).
3. rte_mtr_create() is then needed to search (with locks) for previously
created profile and policy in order to create the meter object.
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:
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.

2. Preparing a meter object should be the part of flow rule creation
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(). 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,
 };
 
 /**
@@ -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. */
+};
+
 /**
  * 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

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

[Jerin Jacob]

+ @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.
>
> 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).
> 3. rte_mtr_create() is then needed to search (with locks) for previously
> created profile and policy in order to create the meter object.
> 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:
> 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.
>
> 2. Preparing a meter object should be the part of flow rule creation
> 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(). 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,
>  };
>
>  /**
> @@ -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. */
> +};
> +
>  /**
>   * 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
>

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

[Dumitrescu, Cristian]

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

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

[Dumitrescu, Cristian]

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

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

[Alexander Kozyrev]

On Tuesday, April 26, 2022 9:44 Dumitrescu, Cristian <cristian.dumitrescu@intel.com>:
> 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!

Completely agree, I scheduled a discussion on May 10.

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

Unfortunately we cannot create all meter objects at the initialization stage
since an application may not know the split between the meters, profiles and policies.
Any particular profile and/or policy may be created based on 5-tuple, for example.
In this case additional delays are introduced in order to find the proper profile/policy
and to create meter object if they were not created in the hardware before.

> Please explain what problem are you trying to fix here. I suggest you provide the
> sequence diagram and tell us where the problem is.

We are trying to remove any unnecessary latencies in case where flow rules are inserted
as part of the packet processing. An application parses a packet, and, based on its content,
inserts some flow rule to the hardware. This action should be as fast as possible in order
not to slow down the packet processing. Any locks to find meter objects or any resource
allocations should be optimized out if possible.

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

This is true for the software implementation of the metering library.
But NIC has to create some real objects at some point in case of hardware implementation.
In case of a policy there will be some flow rules offloaded to the NIC eventually.
Offloading those rules will cause additional delays during the first usage of the policy.

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

It may be created right before the flow is inserted, based on the packet content.
An application may select different profile/policy for different 5-tuple values.

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

Again, the decision on the which meter to use may be taken only when an app sees the packets.
That is the main focus of this RFC, speed up these decisions with all possible means.

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

Agree, the logic on how to create the profile stays in the meter API.
We are merely proposing additional flexibility on how and when we can use it.
 
> 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.

There are two stages for meter creation: meter allocation and meter configuration.
Meter allocation can be done at the application startup with rte_flow_configure().
And meter configuration that will be done during traffic and we need them to be as fast as possible.
Since new RTE flow API is multi-threaded without locks, and asynchronous while the meter one is not
this means that the application will be blocked during its datapath processing.

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

Not true, while the configuration is done per flow, we can still give the same meter id.
And (profile == null) in the meter_ext action means that the meter will be shared.

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

Since the profile and policy are dependent on the traffic.

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

Again, allocation should be done at startup, while configuration is done  during 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?

We can have modify filed (set color) to update the color item. I'll include this change in v2.

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

We supply the profile and this action will simply set the proper color based on profile, nothing else.
What to do with assigned packet colors will be decided later via ITEM_TYPE_METER_COLOR.

> > >  /**
> > >   * 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
> > >