DPDK patches and discussions
 help / color / mirror / Atom feed
From: Matan Azrad <matan@nvidia.com>
To: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>,
	Li Zhang <lizh@nvidia.com>, Dekel Peled <dekelp@nvidia.com>,
	Ori Kam <orika@nvidia.com>,
	Slava Ovsiienko <viacheslavo@nvidia.com>,
	Shahaf Shuler <shahafs@nvidia.com>,
	"lironh@marvell.com" <lironh@marvell.com>,
	"Singh, Jasvinder" <jasvinder.singh@intel.com>,
	NBU-Contact-Thomas Monjalon <thomas@monjalon.net>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
	Jerin Jacob <jerinjacobk@gmail.com>,
	Hemant Agrawal <hemant.agrawal@nxp.com>,
	Ajit Khaparde <ajit.khaparde@broadcom.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Raslan Darawsheh <rasland@nvidia.com>,
	Roni Bar Yanai <roniba@nvidia.com>
Subject: Re: [dpdk-dev] [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
Date: Mon, 29 Mar 2021 20:43:45 +0000	[thread overview]
Message-ID: <BL0PR12MB248342AEB0764A343280C30CDF7E9@BL0PR12MB2483.namprd12.prod.outlook.com> (raw)
In-Reply-To: <DM6PR11MB2796D0A7E1AA9CF51D365651EB7E9@DM6PR11MB2796.namprd11.prod.outlook.com>



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

  reply	other threads:[~2021-03-29 20:43 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-18  8:58 Li Zhang
2021-03-18  8:58 ` [dpdk-dev] [PATCH 2/2] [RFC]: ethdev: manage meter API object handles by the drivers Li Zhang
2021-03-23 21:33   ` Dumitrescu, Cristian
2021-03-25  8:21     ` Matan Azrad
2021-03-25 23:16       ` Ajit Khaparde
2021-03-29 19:56         ` Matan Azrad
2021-03-27 13:15       ` Jerin Jacob
2021-03-29 20:10         ` Matan Azrad
2021-03-31 10:22           ` Jerin Jacob
2021-03-23 21:02 ` [dpdk-dev] [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API Dumitrescu, Cristian
2021-03-25  6:56   ` Matan Azrad
2021-03-29  9:23     ` Ori Kam
2021-03-29 16:24       ` Dumitrescu, Cristian
2021-04-01 13:13         ` Ori Kam
2021-04-01 13:35           ` Dumitrescu, Cristian
2021-04-01 14:22             ` Ori Kam
2021-03-29 16:08     ` Dumitrescu, Cristian
2021-03-29 20:43       ` Matan Azrad [this message]
2021-03-31 15:46         ` Dumitrescu, Cristian
2021-04-04 13:48           ` Matan Azrad
2021-03-29 10:38 ` Jerin Jacob
2021-03-29 20:31   ` Matan Azrad
2021-03-31 10:50     ` Jerin Jacob
2021-04-13  0:14 ` [dpdk-dev] [PATCH v3 0/2] Support " Li Zhang
2021-04-13  0:14   ` [dpdk-dev] [PATCH v3 1/2] ethdev: add pre-defined " Li Zhang
2021-04-13 14:19     ` Dumitrescu, Cristian
2021-04-14  3:23       ` Li Zhang
2021-04-13 14:59     ` Dumitrescu, Cristian
2021-04-14  4:55       ` Li Zhang
2021-04-14  8:02         ` Thomas Monjalon
2021-04-14  8:31           ` Matan Azrad
2021-04-14  8:47           ` Asaf Penso
2021-04-14  8:59             ` Li Zhang
2021-04-14  9:04             ` Thomas Monjalon
2021-04-14 14:00             ` Dumitrescu, Cristian
2021-04-14 16:21               ` Li Zhang
2021-04-13 16:25     ` Kinsella, Ray
2021-04-13  0:14   ` [dpdk-dev] [PATCH v3 2/2] app/testpmd: support policy actions per color Li Zhang
2021-04-14  3:12 ` [dpdk-dev] [PATCH v4 0/2] Support meter policy API Li Zhang
2021-04-14  3:12   ` [dpdk-dev] [PATCH v4 1/2] ethdev: add pre-defined " Li Zhang
2021-04-14  3:12   ` [dpdk-dev] [PATCH v4 2/2] app/testpmd: support policy actions per color Li Zhang
2021-04-14  6:32 ` [dpdk-dev] [PATCH v5 0/2] Support meter policy API Li Zhang
2021-04-14  6:32   ` [dpdk-dev] [PATCH v5 1/2] ethdev: add pre-defined " Li Zhang
2021-04-14  6:32   ` [dpdk-dev] [PATCH v5 2/2] app/testpmd: support policy actions per color Li Zhang
2021-04-14  8:57 ` [dpdk-dev] [PATCH v6 0/2] Support meter policy API Li Zhang
2021-04-14  8:57   ` [dpdk-dev] [PATCH v6 1/2] ethdev: add pre-defined " Li Zhang
2021-04-14 16:16     ` Dumitrescu, Cristian
2021-04-15  1:59       ` Li Zhang
2021-04-14 22:21     ` Singh, Jasvinder
2021-04-15  2:00       ` Li Zhang
2021-04-14  8:58   ` [dpdk-dev] [PATCH v6 2/2] app/testpmd: support policy actions per color Li Zhang
2021-04-15  4:54 ` [dpdk-dev] [PATCH v7 0/2] Support meter policy API Li Zhang
2021-04-15  4:54   ` [dpdk-dev] [PATCH v7 1/2] ethdev: add pre-defined " Li Zhang
2021-04-15  4:54   ` [dpdk-dev] [PATCH v7 2/2] app/testpmd: support policy actions per color Li Zhang
2021-04-15  9:20 ` [dpdk-dev] [PATCH v8 0/2] Support meter policy API Li Zhang
2021-04-15  9:20   ` [dpdk-dev] [PATCH v8 1/2] ethdev: add pre-defined " Li Zhang
2021-04-15 15:13     ` Ori Kam
2021-04-19 12:34     ` Singh, Jasvinder
2021-04-19 16:13       ` Jiawei(Jonny) Wang
2021-04-15  9:20   ` [dpdk-dev] [PATCH v8 2/2] app/testpmd: support policy actions per color Li Zhang
2021-04-19 16:08   ` [dpdk-dev] [PATCH v9 0/2] Support meter policy API Jiawei Wang
2021-04-19 16:08     ` [dpdk-dev] [PATCH v9 1/2] ethdev: add pre-defined " Jiawei Wang
2021-04-20 11:18       ` Dumitrescu, Cristian
2021-04-20 12:55         ` Asaf Penso
2021-04-20 21:01           ` Dumitrescu, Cristian
2021-04-19 16:08     ` [dpdk-dev] [PATCH v9 2/2] app/testpmd: support policy actions per color Jiawei Wang
2021-04-20 11:36     ` [dpdk-dev] [PATCH v9 0/2] Support meter policy API Ferruh Yigit
2021-04-20 14:08       ` Jiawei(Jonny) Wang
2021-04-20 14:04     ` [dpdk-dev] [PATCH v10 " Jiawei Wang
2021-04-20 14:04       ` [dpdk-dev] [PATCH v10 1/2] ethdev: add pre-defined " Jiawei Wang
2021-04-20 17:12         ` Ajit Khaparde
2021-04-21 19:43         ` Thomas Monjalon
2021-04-22  1:29           ` Li Zhang
2021-04-20 14:04       ` [dpdk-dev] [PATCH v10 2/2] app/testpmd: support policy actions per color Jiawei Wang
2021-04-20 17:14         ` Ajit Khaparde
2021-04-21 10:23       ` [dpdk-dev] [PATCH v10 0/2] Support meter policy API Ferruh Yigit
2021-04-20 17:56 ` [dpdk-dev] [PATCH 1/2] [RFC]: ethdev: add pre-defined " Stephen Hemminger
2021-04-21  2:49   ` Li Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BL0PR12MB248342AEB0764A343280C30CDF7E9@BL0PR12MB2483.namprd12.prod.outlook.com \
    --to=matan@nvidia.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=cristian.dumitrescu@intel.com \
    --cc=dekelp@nvidia.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=jasvinder.singh@intel.com \
    --cc=jerinjacobk@gmail.com \
    --cc=lironh@marvell.com \
    --cc=lizh@nvidia.com \
    --cc=orika@nvidia.com \
    --cc=rasland@nvidia.com \
    --cc=roniba@nvidia.com \
    --cc=shahafs@nvidia.com \
    --cc=thomas@monjalon.net \
    --cc=viacheslavo@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).