From: Dumitrescu, Cristian

Hi Matan,

quoted

-----Original Message-----
From: Matan Azrad <redacted>
Sent: Thursday, March 25, 2021 6:57 AM
To: Dumitrescu, Cristian <redacted>; Li Zhang
[off-list ref]; Dekel Peled [off-list ref]; Ori Kam
[off-list ref]; Slava Ovsiienko [off-list ref]; Shahaf
Shuler [off-list ref]; lironh@marvell.com; Singh, Jasvinder
[off-list ref]; NBU-Contact-Thomas Monjalon
[off-list ref]; Yigit, Ferruh [off-list ref]; Andrew
Rybchenko [off-list ref]; Jerin Jacob
[off-list ref]; Hemant Agrawal [off-list ref];

Ajit

quoted

Khaparde [off-list ref]
Cc: dev@dpdk.org; Raslan Darawsheh <redacted>; Roni Bar
Yanai [off-list ref]
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

quoted

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

quoted

quoted

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(

<snip>

quoted

quoted

quoted

+/**
+ * Policy id 0 is default policy.

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),

quoted

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 mbuf::sched::color needs to be set for the packet, and the only way to do
this is by applying the RTE_FLOW_ACTION_TYPE_COLOR Action, right? It would
make sense that the default policy is to simply apply to the packet the color
that the meter just computed for the current packet with no change, right?

I don't think so.
When we are working with HW offloads (this is the main goal of rte_flow and this meter API) the motivation is to do the actions directly in the NIC HW.
Moving the color information to the SW is like doing "partial offload".

quoted

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 don't agree with this assertion either. One typical usage of the color is to
accept all input packets from the user, either green, yellow or red in the
absence of any congestion, and charge the user for this traffic; in case of
congestion, as typically detected later (typically on scheduling and maybe on a
different network node, depending on the application), the packet color is used
to prioritize between packets, i.e. drop red packets first before dropping any
yellow or green packets. In this case, there is no pre-defined "drop all red
packets straight away" policy.

I familiar with a lot of meter users(at least 5 applications) in the industry, no one use the color actions. 
All of them drop red packets and continue to the next flow actions(after meter) otherwise.


If you insist, we can define 2 default IDs...

quoted

quoted

I suggest we avoid the "no action" statement, as it might be confusing.

Maybe "do nothing" is better?

Yes, makes sense to me.

<snip>