From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3CF96A0C40; Thu, 5 Aug 2021 14:33:03 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F3E7E40143; Thu, 5 Aug 2021 14:33:02 +0200 (CEST) Received: from mail-io1-f49.google.com (mail-io1-f49.google.com [209.85.166.49]) by mails.dpdk.org (Postfix) with ESMTP id BBD2640040 for ; Thu, 5 Aug 2021 14:33:01 +0200 (CEST) Received: by mail-io1-f49.google.com with SMTP id s184so6500066ios.2 for ; Thu, 05 Aug 2021 05:33:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=79YjZk5M4BGue4Pclmt5zLcEht0f7CrPtCUyUmRrG/w=; b=sgsR8hgiEY6ChBjKmPvV34O9h6Ill/rPH7l3i5W0KHcfWKjaaCAwTRbOn12DR/ght/ U8Roo3XfAoMtppyEyWxiXaml6X6BgcbHJ7N8NAQQblPev5W5J0ozmDWvvJMFlgmzo2+Y NqJETv0AH8wOctO3TLWP0epAAI+3FunTOWvT7FkZDWKWsQu+d0SMxyTvpK413qvfFv9l GnHc+sy+zcFDZo7icMtstHeaDaikV8R6VitW1NIanHfeSK4dMgUzjsfHEmxiAOQWDfFc LZPqdqcmboAAHJSUwgExtMHjaYZ63mUrvY7iXjEF355K68rfutIby5eKuRVyoCavOL03 4ErQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=79YjZk5M4BGue4Pclmt5zLcEht0f7CrPtCUyUmRrG/w=; b=nXBp3WSaAhz5Duu+GOYC7+xy3iqRNIMdXKbhSKEjVrlMs0yELYEbdaVu5IBoibrcXR RqSULF/0vmIbPY8pjzJ42bcqXkW7pvq28dCwrUAFkH2gVaAdypmBTP7079V3QkEnbYf/ 4hSqQli+8vMx7YMw5hB4MQbmKgFf1ayili5fep8lufUdaPBl4vVgMI+dUJ6HIJl/mS+C 0x0/jcBUMJkDh114sl92TIwMriAQUraIYs0oLrZQkTBjq+zQgpWXlWZU5NMePyXi9E3v fU6/N/mZHPE/NScrJWtywlnGQmcODTMSB7H89pN0R6kHKHyf157GywwQBokgfC6Xg/s8 yDyA== X-Gm-Message-State: AOAM531ahDjxHJrt5sTqO5Eo48VFbsmtLS9ZMwDcwUdJMzfO7cLiZiYH bYyZCHu3b346oHQnOj7A1+j8lpOmhWxX91tLxjg= X-Google-Smtp-Source: ABdhPJxq/9W+CSN20t5RWAl8IMT9zXenxHkI/YFbJaNsecxS8pjo5+/8AS87K2+c2ySOl42LuyNTYudpUhh8oAK3jhM= X-Received: by 2002:a02:2a88:: with SMTP id w130mr4420397jaw.60.1628166781105; Thu, 05 Aug 2021 05:33:01 -0700 (PDT) MIME-Version: 1.0 References: <20210804113410.3604616-1-jerinj@marvell.com> <20210805101046.4091894-1-jerinj@marvell.com> In-Reply-To: From: Jerin Jacob Date: Thu, 5 Aug 2021 18:02:35 +0530 Message-ID: To: "Dumitrescu, Cristian" Cc: "jerinj@marvell.com" , Thomas Monjalon , "Yigit, Ferruh" , Andrew Rybchenko , "dev@dpdk.org" , "arybchenko@solarflare.com" , "lizh@nvidia.com" , "ajit.khaparde@broadcom.com" , "Singh, Jasvinder" , "matan@nvidia.com" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [WARNING: UNSCANNABLE EXTRACTION FAILED][WARNING: UNSCANNABLE EXTRACTION FAILED] [PATCH v3] doc: mtr: add API walk through X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Aug 5, 2021 at 4:36 PM Dumitrescu, Cristian 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. > > > -----Original Message----- > > From: jerinj@marvell.com > > Sent: Thursday, August 5, 2021 11:11 AM > > To: Dumitrescu, Cristian ; Thomas Monjal= on > > ; Yigit, Ferruh ; Andrew > > Rybchenko > > Cc: dev@dpdk.org; arybchenko@solarflare.com; lizh@nvidia.com; > > ajit.khaparde@broadcom.com; Singh, Jasvinder > > ; matan@nvidia.com; Jerin Jacob > > > > Subject: [WARNING: UNSCANNABLE EXTRACTION FAILED][WARNING: > > UNSCANNABLE EXTRACTION FAILED][dpdk-dev] [PATCH v3] doc: mtr: add > > API walk through > > > > From: Jerin Jacob > > > > 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 > > --- > > 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 > > > > Just to avoid confusion with the rte_meter API (from the lib/meter librar= y), may change the name to rte_mtr_meter_chaining.svg ? Will do. > > > > 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. Ack > > > > > 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 inc= ludes > > the number of packets and bytes dropped or passed for each output co= lor. > > + > > +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 creat= es 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. Ack. > > > +#. 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. Ack > > > +#. A meter object consists of a profile and a policy. Use above create= d > > 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 s= tep 3). > > It is mainly the meter object configuration that consists of a profile an= d 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 funct= ion. 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. > > > +#. 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 ind= icates 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 associ= ated meter object ID set to this meter object. Ack > > > > +#. The API allows chaining the meter objects to create complex meterin= g > > topology > > + by specifying ``struct rte_mtr_meter_policy_params::actions`` actio= n 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 al= lows for a list of (any) rte_flow actions to be specified per color for eac= h 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) con= ditional of a specific color. Which is another (new) way to chain meters, r= ight? 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 t= he 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 M= eter Chaining - Approach B is also valid, with the (small) advantage that i= s conditional of a specific color. > > So, I think we should describe here both approaches. How about adding a n= ew distinct section for Meter Chaining, where we describe both approaches a= s valid? It would be great if you could have a separate diagram for each ap= proach :) 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. > > > > 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