DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: Jerin Jacob <jerinjacobk@gmail.com>
Cc: "jerinj@marvell.com" <jerinj@marvell.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"Andrew Rybchenko" <andrew.rybchenko@oktetlabs.ru>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"arybchenko@solarflare.com" <arybchenko@solarflare.com>,
	"lizh@nvidia.com" <lizh@nvidia.com>,
	"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
	 "Singh, Jasvinder" <jasvinder.singh@intel.com>,
	"matan@nvidia.com" <matan@nvidia.com>
Subject: Re: [dpdk-dev] [WARNING: UNSCANNABLE EXTRACTION FAILED][WARNING: UNSCANNABLE EXTRACTION FAILED] [PATCH v3] doc: mtr: add API walk through
Date: Thu, 5 Aug 2021 22:17:02 +0000	[thread overview]
Message-ID: <DM8PR11MB567059658C37CD3ECE62B283EBF29@DM8PR11MB5670.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CALBAE1OWym+qR3bt9Pn92Q9oyyNhoyMZk9x=8cVfn9_Hi4sYBg@mail.gmail.com>

Hi Jerin,
> 
> On Thu, Aug 5, 2021 at 4:36 PM Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com> wrote:
> >
> > HI Jerin,
> >
> > Thanks for your patch!
> >
> > Initially, it looked like an easy job to review it, but there is actually an
> elephant in the room on how to chain the meters, see below.
> 
> Yes. That's why I send this patch to finalize how to chain the meter.
> It was hard for me to follow that's why added a digram.
> 

OK, got it :)

<snip>

> > > +#. A meter object consists of a profile and a policy. Use above created
> > > objects to create
> > > +   meter object using ``rte_mtr_create()``. Application uses
> > > +   ``struct rte_mtr_params::meter_profile_id`` and ``struct
> > > rte_mtr_params::meter_policy_id``
> > > +   to specify the profile (created in step 2) and policy (created in step 3).
> >
> > It is mainly the meter object configuration that consists of a profile and a
> policy, but not exactly the meter object itself.
> >
> > How about:
> >         The application creates a meter object using the rte_mtr_create() API
> function. One of the previously created meter profile and meter policy are
> provided as arguments at this step.
> 
> Sure. I just though to add exact struct field so that it easy for end
> users to know.
> 
> How about your version + struct name.
> 
> The application creates a meter object using the rte_mtr_create() API
> function.
> One of the previously created meter profile (`struct
> rte_mtr_params::meter_profile_id``) and meter policy
> ``struct rte_mtr_params::meter_policy_id`` are provided as arguments
> at this step.
> 

Great, thanks!

<snip>

> > > +#. The API allows chaining the meter objects to create complex
> metering
> > > topology
> > > +   by specifying ``struct rte_mtr_meter_policy_params::actions`` action
> as
> > > +   ``RTE_FLOW_ACTION_TYPE_METER`` to the parent meter object
> encoded
> > > as
> > > +   ``struct rte_flow_action_meter::mtr_id``.
> >
> > Now this could be the elephant in the room:
> >
> > With the latest API changes that went in recently, the rte_mtr API now
> allows for a list of (any) rte_flow actions to be specified per color for each
> meter object, which opens up the door for a meter action to call one (or
> more) subsequent meter actions (on the same or different meter objects)
> conditional of a specific color. Which is another (new) way to chain meters,
> right? Let's refer to this as the Meter Chaining - Approach B.
> 
> Yes, my diagram is approach B.
> 
> >
> > Before these API changes, the only way to chain meters was by specifying
> multiple meter actions (action on the same or different meter object) for the
> same rte_flow. Let's refer to this as the Meter Chaining - Approach A.
> 
> Yes. Me too was assuming that way. But no one really implemented the
> chaining. With the MLX patch that got changed.
> 
> >
> > The Meter Chaining - Approach A is valid. After a bit of thinking, I don't see
> any reason to invalidate the approach you describe, so I agree that Meter
> Chaining - Approach B is also valid, with the (small) advantage that is
> conditional of a specific color.
> >
> > So, I think we should describe here both approaches. How about adding a
> new distinct section for Meter Chaining, where we describe both approaches
> as valid? It would be great if you could have a separate diagram for each
> approach :)
> 
> This is the exact reason to send the patch. Approach B is a superset.
> Should we allow two approaches ? It will complicate the driver as it
> needs to track two ways of changing.
> Can we keep only the new approach, Just to make everyone's life easy?
> We started implementing this driver and realized two paths are painful
> for the driver.
> 

I agree the latest API changes push more complexity into the driver, but both approaches logically make sense and are already part of the API, so I think it is better to implement them both, if possible, and also describe them both in the doc. Supporting both will avoid confusing the users.

Also this problem that you mention is true for any action, not just for the meter action: now there are two different paths to enable any action type, one is enablement as a flow action, and the other one is enablement as a meter action; allowing just one path for the meter action leads to allowing just one path for  all the other actions as well, which defeats the purpose of the API update, right?

<snip>

Regards,
Cristian

  reply	other threads:[~2021-08-05 22:17 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-18  9:44 [dpdk-dev] [PATCH] " jerinj
2021-07-27 11:21 ` Jerin Jacob
2021-08-04 11:34 ` [dpdk-dev] [PATCH v2] " jerinj
2021-08-04 16:46   ` Thomas Monjalon
2021-08-05 10:10   ` [dpdk-dev] [PATCH v3] " jerinj
2021-08-05 11:05     ` [dpdk-dev] [WARNING: UNSCANNABLE EXTRACTION FAILED][WARNING: UNSCANNABLE EXTRACTION FAILED] " Dumitrescu, Cristian
2021-08-05 12:32       ` Jerin Jacob
2021-08-05 22:17         ` Dumitrescu, Cristian [this message]
2021-08-06  8:49           ` Jerin Jacob
2021-08-06  9:43     ` [dpdk-dev] " jerinj
2021-08-06  9:45       ` [dpdk-dev] [PATCH v5] " jerinj
2021-08-06 17:46         ` [dpdk-dev] [WARNING: UNSCANNABLE EXTRACTION FAILED][WARNING: UNSCANNABLE EXTRACTION FAILED] " Dumitrescu, Cristian
2021-08-07  8:16           ` Jerin Jacob
2021-08-07  8:21         ` [dpdk-dev] [PATCH v6] " jerinj
2021-08-09  8:37           ` [dpdk-dev] [WARNING: UNSCANNABLE EXTRACTION FAILED][WARNING: UNSCANNABLE EXTRACTION FAILED] " Dumitrescu, Cristian
2021-11-26 13:45             ` David Marchand

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=DM8PR11MB567059658C37CD3ECE62B283EBF29@DM8PR11MB5670.namprd11.prod.outlook.com \
    --to=cristian.dumitrescu@intel.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jasvinder.singh@intel.com \
    --cc=jerinj@marvell.com \
    --cc=jerinjacobk@gmail.com \
    --cc=lizh@nvidia.com \
    --cc=matan@nvidia.com \
    --cc=thomas@monjalon.net \
    /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).