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,

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.

> -----Original Message-----
> From: jerinj@marvell.com <jerinj@marvell.com>
> Sent: Thursday, August 5, 2021 11:11 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org; arybchenko@solarflare.com; lizh@nvidia.com;
> ajit.khaparde@broadcom.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; matan@nvidia.com; Jerin Jacob
> <jerinj@marvell.com>
> Subject: [WARNING: UNSCANNABLE EXTRACTION FAILED][WARNING:
> UNSCANNABLE EXTRACTION FAILED][dpdk-dev] [PATCH v3] doc: mtr: add
> API walk through
> 
> From: Jerin Jacob <jerinj@marvell.com>
> 
> Added a diagram to document meter library components
> and added text for steps performed by the application to
> configure the traffic meter and policing library.
> 
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> ---
> v2: Fix long lines in svg file to avoid patch apply issue
> v3: Fix all Thomas's comment at
> http://patches.dpdk.org/project/dpdk/patch/20210804113410.3604616-1-
> jerinj@marvell.com/
> 
>  doc/guides/prog_guide/img/meter.svg           | 1600 +++++++++++++++++
>  .../traffic_metering_and_policing.rst         |   29 +
>  lib/ethdev/rte_mtr.h                          |    4 +-
>  3 files changed, 1631 insertions(+), 2 deletions(-)
>  create mode 100644 doc/guides/prog_guide/img/meter.svg
> 
> diff --git a/doc/guides/prog_guide/img/meter.svg
> b/doc/guides/prog_guide/img/meter.svg
> new file mode 100644
> index 0000000000..7214c5dc2b
> --- /dev/null
> +++ b/doc/guides/prog_guide/img/meter.svg

<snip>

Just to avoid confusion with the rte_meter API (from the lib/meter library), may change the name to rte_mtr_meter_chaining.svg ?


> diff --git a/doc/guides/prog_guide/traffic_metering_and_policing.rst
> b/doc/guides/prog_guide/traffic_metering_and_policing.rst
> index c0537e653c..0fe013522f 100644
> --- a/doc/guides/prog_guide/traffic_metering_and_policing.rst
> +++ b/doc/guides/prog_guide/traffic_metering_and_policing.rst
> @@ -20,6 +20,7 @@ The main features are:
>    and RFC 4115 Two Rate Three Color Marker (trTCM)
>  * Policer actions (per meter output color): recolor, drop
>  * Statistics (per policer output color)
> +* Chaining the meter objects

How about:
	Chaining multiple meter objects.

> 
>  Configuration steps
>  -------------------
> @@ -64,3 +65,31 @@ The processing done for each input packet hitting an
> MTR object is:
>  * Statistics: The set of counters maintained for each MTR object is
>    configurable and subject to the implementation support. This set includes
>    the number of packets and bytes dropped or passed for each output color.
> +
> +API Walk-through
> +----------------
> +
> +.. figure:: img/meter.*
> +
> +   Meter components
> +
> +This section will introduce the reader to the critical APIs to use
> +the traffic meter and policing library.
> +
> +In general, the following steps are performed by the application to
> configure
> +the traffic meter and policing library.
> +
> +#. Application gets the meter driver capabilities using
> ``rte_mtr_capabilities_get()``.
> +#. Application identifies the profile(s) needed for metering and creates it
> with
> +   ``rte_mtr_meter_profile_add()``.

How about:
	The application creates the required meter profiles by using the rte_mtr_meter_profile_add() API function.

> +#. Application identifies the policies needed and creates it with
> ``rte_mtr_meter_policy_add()``.

How about:
	The application creates the required meter policies by using the rte_mtr_meter_policy_add() API function.

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

> +#. Once the meter object is created, the application shall use
> ``rte_flow_create()`` API to
> +   instantiate the meter object using ``RTE_FLOW_ACTION_TYPE_METER``
> action.

The instantiate word might not be the best word here, as it typically indicates creating an object as opposed linking it to some other entity.

How about:
	The application enables the meter object execution as part of the flow action processing by calling the rte_flow_create() API function with one of the flow action set to ``RTE_FLOW_ACTION_TYPE_METER`` and the associated meter object ID set to this meter object.
	

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

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.

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 :)


> diff --git a/lib/ethdev/rte_mtr.h b/lib/ethdev/rte_mtr.h
> index dc246dd7af..40df0888c8 100644
> --- a/lib/ethdev/rte_mtr.h
> +++ b/lib/ethdev/rte_mtr.h
> @@ -219,7 +219,7 @@ struct rte_mtr_meter_policy_params {
>   * @see enum rte_mtr_stats_type
>   */
>  struct rte_mtr_params {
> -	/** Meter profile ID. */
> +	/** Meter profile ID. @see rte_mtr_meter_profile_add() */
>  	uint32_t meter_profile_id;
> 
>  	/** Meter input color in case of MTR object chaining. When non-
> zero: if
> @@ -259,7 +259,7 @@ struct rte_mtr_params {
>  	 */
>  	uint64_t stats_mask;
> 
> -	/** Meter policy ID. */
> +	/** Meter policy ID. @see rte_mtr_meter_policy_add() */
>  	uint32_t meter_policy_id;
>  };
> 
> --
> 2.32.0

Thanks again for your contribution!

Regards,
Cristian