Hi All,
> -----Original Message-----
> From: Matan Azrad <ma...@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 <l...@nvidia.com>
> > > Sent: Thursday, March 18, 2021 8:58 AM
> > > To: dek...@nvidia.com; or...@nvidia.com; viachesl...@nvidia.com;
> > > ma...@nvidia.com; shah...@nvidia.com; lir...@marvell.com; Singh,
> > > Jasvinder <jasvinder.si...@intel.com>; Thomas Monjalon
> > > <tho...@monjalon.net>; Yigit, Ferruh <ferruh.yi...@intel.com>; Andrew
> > > Rybchenko <andrew.rybche...@oktetlabs.ru>; Dumitrescu, Cristian
> > > <cristian.dumitre...@intel.com>
> > > Cc: dev@dpdk.org; rasl...@nvidia.com; ron...@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 <l...@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
