From: Ori Kam <orika@nvidia.com>
To: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>,
Matan Azrad <matan@nvidia.com>, Li Zhang <lizh@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: Thu, 1 Apr 2021 14:22:26 +0000 [thread overview]
Message-ID: <DM6PR12MB4987F5006BBB05DCF0EE20D8D67B9@DM6PR12MB4987.namprd12.prod.outlook.com> (raw)
In-Reply-To: <DM6PR11MB279676B81A4BD71FF8ECC8BAEB7B9@DM6PR11MB2796.namprd11.prod.outlook.com>
Hi Cristian,
> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
>
> Hi Ori,
>
> > -----Original Message-----
> > From: Ori Kam <orika@nvidia.com>
> > Hi Cristian,
> >
> > > -----Original Message-----
> > > From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Hi Ori,
> > >
> > > > -----Original Message-----
> > > > From: Ori Kam <orika@nvidia.com>
> > > > Hi All,
> > > >
> > > > > -----Original Message-----
> > > > > From: Matan Azrad <matan@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(+), 74 deletions(-)
> > > > > > >
> > > > > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > > > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4
> > 100644
> > > > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > > > @@ -31,6 +31,7 @@
> > > > > > > #include <rte_ecpri.h>
> > > > > > > #include <rte_mbuf.h>
> > > > > > > #include <rte_mbuf_dyn.h>
> > > > > > > +#include <rte_meter.h>
> > > > > > >
> > > > > > > #ifdef __cplusplus
> > > > > > > extern "C" {
> > > > > > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > > > > > > * See struct rte_flow_action_modify_field.
> > > > > > > */
> > > > > > > RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > > > > > +
> > > > > > > + /**
> > > > > > > + * Color the packet to reflect the meter color result.
> > > > > > > + *
> > > > > > > + * See struct rte_flow_action_color.
> > > > > > > + */
> > > > > > > + RTE_FLOW_ACTION_TYPE_COlOR,
> > > > > >
> > > > > > Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> > > > > >
> > > >
> > > > Why do we need this action?
> > >
> > > We need this new proposed RTE_FLOW_ACTION_TYPE_COLOR action to
> > set the
> > > packet color in the packet mbuf (i.e. in the mbuf::sched:color field) in order
> > to
> > > tell the later stages of the pipeline what the packet color is.
> > >
> > > > if it is to save the color it should be done by using mark/metadata
> > >
> > > As stated in its description, the RTE_FLOW_ACTION_TYPE_MARK action Is
> > > setting the mbuf::hash.fdir.hi field, which is used for a different purpose
> > that is
> > > unrelated to the packet color, which has its own field within the mbuf.
> > >
> >
> > Agree,
> >
> > > > Or by the action of meter.
> > >
> > > The new proposed RTE_FLOW_ACTION_TYPE_COLOR action is indeed an
> > action
> > > of the meter and meter only, right?
> > >
> > > For example you can see
> > > > RTE_FLOW_ACTION_TYPE_SECURITY
> > > > Which if exist saves the session id to a dedicated mbuf field.
> > > >
> > >
> > > The meter processing and action take place independently of the security
> > API: it
> > > can be enabled when the security API is disabled and is not conditioned in
> > any
> > > way by the security API. To be honest, I don't understand the connection
> > with
> > > the security API that you are trying to make here.
> > >
> >
> > Sorry for not being clear,
> > I didn’t mean use the security action, what I meant is just like the security
> > action
> > which when given, it will saves the session and pass it to the SW in dedicated
> > member in the
> > mbuf, the same with meter if the meter action is present then
> > the PMD should know to save the color value and extract it to the correct
> > mbuf member.
> >
> > Does that make sense?
> >
>
> Yes, I does, I guess we are using the same principle for the proposed
> RTE_FLOW_ACTION_TYPE_COLOR action, right?
>
Just to make sure we are talking about the same thing.
I don't think we should add any new action for this.
My idea is that when the PMD will see the action RTE_FLOW_ACTION_TYPE_METER,
it will send the traffic to the meter, and will also in the of case the packet being
routed to the application will update the color member in the mbuf with color value.
> >
> > > > > > > };
> > > > > > >
> > > > > > > /**
> > > >
> > > > [Snip]
> > > >
> > > > > > 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 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 suggest we avoid the "no action" statement, as it might be
> > confusing.
> > > > >
> > > > > Maybe "do nothing" is better?
> > > > >
> > > > Maybe passthrough? Or in rte_flow passthru
> > > >
> > >
> > > No, we need to save the packet color in the packet mbuf
> > (mbuf::sched:color),
> > > and the RTE_FLOW_ACTION_TYPE_PASSTHRU action is not doing this.
> > >
> >
> > Please see my comment above.
> > The saving of color will be done automatically.
>
> The "automatically" part might be the problem for some apps, like the one
> Matan is describing.
>
> If for example all that user wants is to drop all the RED packets immediately
> with no need to consider the packet color later in the CPU app, then the user
> policy is simply: [GREEN => pass-through; YELLOW => pass-through; RED =>
> drop] with no need to use the RTE_FLOW_ACTION_TYPE_COLOR action to set
> the color in the mbuf::sched::color field, as the app does not need to know the
> color. This is the use-case described by Matan.
>
> If for example the user utilizes the packet color on the CPU app as the packet
> drop priority in case of congestion, then the user policy is simply: [GREEN =>
> color; YELLOW => color; RED => color], so the
> RTE_FLOW_ACTION_TYPE_COLOR action is used to set the color in the
> mbuf::sched:color field. This is the use-case I described in earlier emails.
>
> So it is probably the best option to have the user to explicitly set / not set the
> RTE_FLOW_ACTION_TYPE_COLOR action as opposed to have this action always
> executed automatically/implicitly.
>
> Makes sense?
>
I think I understand what you are saying but I don't agree,
First I assume that the meter color is dynamic field, if not the
answer will need some small change.
lets look at the use cases:
1. The first case of direct drop:
The PMD sees that the dynamic color was not registered so it knows
not to same or update the field.
2. Second case the app register the color field and the pmd
knows that it needs to save it when meter action was set.
In any case the worst that will happen is that the PMD will store a value that
will not be checked.
Also adding a color action makes sense only if I can match on it later on, and for that
we have registers tags,mark and metadata.
Another approach is to totally remove the policy and just create mtr_color_item,
Then when application issues the meter action the color will be set to the color_item
Which the application will be able to match on.
basically the application will create the policy using rte flow group with rules that matches
the color.
This is the way I really think it should be done, but we can move to this way later on.
> >
> > > >
> > > > > > > + * It can be used without creating it by the
> > rte_mtr_meter_policy_add
> > > > > > > function.
> > > > > > > + */
> > > >
> > > >
> > > > Best,
> > > > Ori
> > >
> > > Regards,
> > > Cristian
> >
> > Best,
> > Ori
>
> Regards,
> Cristian
Best,
Ori
next prev parent reply other threads:[~2021-04-01 14:22 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 [this message]
2021-03-29 16:08 ` Dumitrescu, Cristian
2021-03-29 20:43 ` Matan Azrad
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=DM6PR12MB4987F5006BBB05DCF0EE20D8D67B9@DM6PR12MB4987.namprd12.prod.outlook.com \
--to=orika@nvidia.com \
--cc=ajit.khaparde@broadcom.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=cristian.dumitrescu@intel.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=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).