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

Hi Jerin,

On Thu, Aug 5, 2021 at 4:36 PM Dumitrescu, Cristian
[off-list ref] wrote:

quoted

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>

quoted

quoted

+#. 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.

quoted

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>

quoted

quoted

+#. The API allows chaining the meter objects to create complex

metering

quoted

quoted

topology
+   by specifying ``struct rte_mtr_meter_policy_params::actions`` action

as

quoted

quoted

+   ``RTE_FLOW_ACTION_TYPE_METER`` to the parent meter object

encoded

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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