From: Dumitrescu, Cristian > Hi Matan, > > > -----Original Message----- > > From: Matan Azrad <ma...@nvidia.com> > > Sent: Thursday, March 25, 2021 6:57 AM > > To: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; Li Zhang > > <l...@nvidia.com>; Dekel Peled <dek...@nvidia.com>; Ori Kam > > <or...@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 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( <snip> > > > > +/** > > > > + * Policy id 0 is default policy. > > > > > > 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 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". > > 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... > > > > > I suggest we avoid the "no action" statement, as it might be confusing. > > > > Maybe "do nothing" is better? > > > > Yes, makes sense to me. <snip>
