Hi Ori, > -----Original Message----- > From: Ori Kam <or...@nvidia.com> > Sent: Monday, March 29, 2021 10:23 AM > To: Matan Azrad <ma...@nvidia.com>; Dumitrescu, Cristian > <cristian.dumitre...@intel.com>; Li Zhang <l...@nvidia.com>; Dekel Peled > <dek...@nvidia.com>; Slava Ovsiienko <viachesl...@nvidia.com>; Shahaf > Shuler <shah...@nvidia.com>; lir...@marvell.com; Singh, Jasvinder > <jasvinder.si...@intel.com>; NBU-Contact-Thomas Monjalon > <tho...@monjalon.net>; Yigit, Ferruh <ferruh.yi...@intel.com>; Andrew > Rybchenko <andrew.rybche...@oktetlabs.ru>; Jerin Jacob > <jerinjac...@gmail.com>; Hemant Agrawal <hemant.agra...@nxp.com>; > Ajit Khaparde <ajit.khapa...@broadcom.com> > Cc: dev@dpdk.org; Raslan Darawsheh <rasl...@nvidia.com>; Roni Bar Yanai > <ron...@nvidia.com> > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API > > 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?
We need this new proposed RTE_FLOW_ACTION_TYPE_COLOR action to set the packet color in the packet mbuf (i.e. in the mbuf::sched:color field) in order to tell the later stages of the pipeline what the packet color is. > if it is to save the color it should be done by using mark/metadata As stated in its description, the RTE_FLOW_ACTION_TYPE_MARK action Is setting the mbuf::hash.fdir.hi field, which is used for a different purpose that is unrelated to the packet color, which has its own field within the mbuf. > Or by the action of meter. The new proposed RTE_FLOW_ACTION_TYPE_COLOR action is indeed an action of the meter and meter only, right? For example you can see > RTE_FLOW_ACTION_TYPE_SECURITY > Which if exist saves the session id to a dedicated mbuf field. > The meter processing and action take place independently of the security API: it can be enabled when the security API is disabled and is not conditioned in any way by the security API. To be honest, I don't understand the connection with the security API that you are trying to make here. > > > > }; > > > > > > > > /** > > [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 > No, we need to save the packet color in the packet mbuf (mbuf::sched:color), and the RTE_FLOW_ACTION_TYPE_PASSTHRU action is not doing this. > > > > > + * It can be used without creating it by the rte_mtr_meter_policy_add > > > > function. > > > > + */ > > > Best, > Ori Regards, Cristian