Hi All,

-----Original Message-----
From: Matan Azrad <redacted>
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

quoted

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

quoted

previous email.

quoted

-----Original Message-----
From: Li Zhang <redacted>
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 [off-list ref]; Thomas Monjalon
[off-list ref]; Yigit, Ferruh [off-list ref]; Andrew
Rybchenko [off-list ref]; Dumitrescu, Cristian
[off-list ref]
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

quoted

rather than a reduced action set defined specifically just for meter object.

This

quoted

makes sense to me.

quoted

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

quoted

quoted

 };

 /**

[Snip]

quoted

I suggest you do not redundantly specify the value of the default policy ID in

the

quoted

comment. Replace by "Default policy ID."

quoted

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

quoted

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.

quoted

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
 

quoted

quoted

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

Best,
Ori