Re: [dpdk-dev] [WARNING: UNSCANNABLE EXTRACTION FAILED][WARNING: UNSCANNABLE EXTRACTION FAILED] [PATCH v3] doc: mtr: add API walk through

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.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