[dpdk-dev] [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API

Matan Azrad matan at nvidia.com
Sun Apr 4 15:48:28 CEST 2021


Hi Cristian

From: Dumitrescu, Cristian
> Hi Matan,
> 
> > -----Original Message-----
> > From: Matan Azrad <matan at nvidia.com>
> > Sent: Monday, March 29, 2021 9:44 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>; Li Zhang
> > <lizh at nvidia.com>; Dekel Peled <dekelp at nvidia.com>; Ori Kam
> > <orika at nvidia.com>; Slava Ovsiienko <viacheslavo at nvidia.com>; Shahaf
> > Shuler <shahafs at nvidia.com>; lironh at marvell.com; Singh, Jasvinder
> > <jasvinder.singh at intel.com>; NBU-Contact-Thomas Monjalon
> > <thomas at monjalon.net>; Yigit, Ferruh <ferruh.yigit at intel.com>; Andrew
> > Rybchenko <andrew.rybchenko at oktetlabs.ru>; Jerin Jacob
> > <jerinjacobk at gmail.com>; Hemant Agrawal <hemant.agrawal at nxp.com>;
> Ajit
> > Khaparde <ajit.khaparde at broadcom.com>
> > Cc: dev at dpdk.org; Raslan Darawsheh <rasland at nvidia.com>; Roni Bar
> > Yanai <roniba at 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 <matan at nvidia.com>
> > > > Sent: Thursday, March 25, 2021 6:57 AM
> > > > To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>; Li Zhang
> > > > <lizh at nvidia.com>; Dekel Peled <dekelp at nvidia.com>; Ori Kam
> > > > <orika at nvidia.com>; Slava Ovsiienko <viacheslavo at nvidia.com>;
> > > > Shahaf Shuler <shahafs at nvidia.com>; lironh at marvell.com; Singh,
> > > > Jasvinder <jasvinder.singh at intel.com>; NBU-Contact-Thomas Monjalon
> > > > <thomas at monjalon.net>; Yigit, Ferruh <ferruh.yigit at intel.com>;
> > Andrew
> > > > Rybchenko <andrew.rybchenko at oktetlabs.ru>; Jerin Jacob
> > > > <jerinjacobk at gmail.com>; Hemant Agrawal
> > <hemant.agrawal at nxp.com>;
> > > Ajit
> > > > Khaparde <ajit.khaparde at broadcom.com>
> > > > Cc: dev at dpdk.org; Raslan Darawsheh <rasland at nvidia.com>; Roni Bar
> > > > Yanai <roniba at 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 <lizh at nvidia.com>
> > > > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > > > To: dekelp at nvidia.com; orika at nvidia.com;
> > > > > > viacheslavo at nvidia.com; matan at nvidia.com; shahafs at nvidia.com;
> > > > > > lironh at marvell.com; Singh, Jasvinder
> > > > > > <jasvinder.singh at intel.com>; Thomas Monjalon
> > > > > > <thomas at monjalon.net>; Yigit, Ferruh <ferruh.yigit at intel.com>;
> > > > Andrew
> > > > > > Rybchenko <andrew.rybchenko at oktetlabs.ru>; Dumitrescu,
> > > > > > Cristian <cristian.dumitrescu at intel.com>
> > > > > > Cc: dev at dpdk.org; rasland at nvidia.com; roniba at 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 <lizh at 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.
> 

I said that the direction is to do "full offload", for sure "partial offload" is exists too, using MARK, METADATA, etc.. actions.
Example for "full offload" is all the hairpin features, added lastly.


> 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?

Yes.

> >
> > > > 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?
> 

OK, for that.

> 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?

Good direction
I think it is better to give these 2 examples in the policy create\validate description or in example application,
Then the user just can copy it to create a policy....

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


More information about the dev mailing list