DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: Matan Azrad <matan@nvidia.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: Wed, 31 Mar 2021 15:46:01 +0000	[thread overview]
Message-ID: <DM6PR11MB279618A3F04CDFAAD0FDC744EB7C9@DM6PR11MB2796.namprd11.prod.outlook.com> (raw)
In-Reply-To: <BL0PR12MB248342AEB0764A343280C30CDF7E9@BL0PR12MB2483.namprd12.prod.outlook.com>

Hi Matan,

> -----Original Message-----
> From: Matan Azrad <matan@nvidia.com>
> Sent: Monday, March 29, 2021 9:44 PM
> 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
> 
> 
> 
> 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".
> 

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

  reply	other threads:[~2021-03-31 15:46 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
2021-03-31 15:46         ` Dumitrescu, Cristian [this message]
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=DM6PR11MB279618A3F04CDFAAD0FDC744EB7C9@DM6PR11MB2796.namprd11.prod.outlook.com \
    --to=cristian.dumitrescu@intel.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --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=matan@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).