Hi Matan, > -----Original Message----- > From: Matan Azrad <ma...@nvidia.com> > Sent: Monday, March 29, 2021 9:44 PM > 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 > > > > 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". >
Sorry, Matan, but as the bulk of the packets are passed by the NIC to the CPU as opposed to being returned to the network without the CPU ever seeing them, the rte_flow API is a partial offload API, not a full offload API, not sure why we disagree here. Just to make sure we are on the same page and not getting round in circles: I think you agree that we should have this action RTE_FLOW_ACTION_TYPE_COLOR to setup the color for the CPU to see in mbuf::sched:color, but you don't agree this action should be part of the default policy, did I get your position correctly? > > > > 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. > I politely but firmly disagree. None of the apps that I have seen is dropping the red packets straight away, they simply use the color as an indication of the packet drop priority at a later stage in the pipeline when congestion is detected. > > If you insist, we can define 2 default IDs... > Maybe we should not have any pre-registered policies at all? For the user's convenience, we could provide the configuration parameters for some of the common policies, like the ones mentioned here, in the API and let the users decide which one(s), if any, they want to register? > > > > > > > I suggest we avoid the "no action" statement, as it might be confusing. > > > > > > Maybe "do nothing" is better? > > > > > > > Yes, makes sense to me. > > <snip> Regards, Cristian
