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 664E0A0C46; Fri, 6 Aug 2021 10:49:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F418E4069E; Fri, 6 Aug 2021 10:49:49 +0200 (CEST) Received: from mail-il1-f175.google.com (mail-il1-f175.google.com [209.85.166.175]) by mails.dpdk.org (Postfix) with ESMTP id D4AA74014D for ; Fri, 6 Aug 2021 10:49:48 +0200 (CEST) Received: by mail-il1-f175.google.com with SMTP id r1so8065230iln.6 for ; Fri, 06 Aug 2021 01:49:48 -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=OoNZbZof5aZyWFoF3ozvrg9zHAK5gnOkBfZg0MYQ9n4=; b=Oj0R1resB5XEItKFcXPQMqlMqIqKtzfzHkcKQZqf09ylXzwO3Oudd/AtVUDylaungt ZGcZo0fP6gpRU97JkQ3CcfZNQ7yXAKv7rbRH5nFpx9s3og2CJX0qva5KGdErTxbqSeXs Kal9LSmi+aQPFXrc5bB2o/OwJzI0vAPykmVJUyl2BXWRX3nPNNiwVhNIBsJ+Wskg5ZZo +UtQRBYREjtcIoI1ILg5YBdUxy/uR0UjBuDk7arKm6GZaPGrug4AZocOVrwMTFAkGxKa RTSSBK/mpm4xbs4lODiaxtC/cUlNZDtqT5XRs6OFePznw/loi5Qz6nQ9wzfIQpyx/UWx 17YA== 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=OoNZbZof5aZyWFoF3ozvrg9zHAK5gnOkBfZg0MYQ9n4=; b=kBpWDSLv0d78uRf1Jp+m5+5bgJGp8nNNxvUNAMxc0sDCvYb/H37h3swlWkl9GuMd6Q u6IQxYMk7b/tN/6C6zm4po+Q6fTS32ab8TayXLwa09dj6fVUQQVBBWNukwAQZukF7wuI TwFTVltN6KeEOe4riMyxtTdxpjKty4bfib5/0vbIpb7Cva5sTIT/OgpOzja5mschjUza Embuhg2bkgKbLzXI/AvZt6uJxZ6ImJELhZs9jwTN23pXsgHBSZAbKW+nVf5i6yDNs2Fs RZqI9rziMn6X97d3bmPTP9txKHrCsw1cXsa///H85nyM35IYdXcRBrYxh8VSvdET2Q2c CBAg== X-Gm-Message-State: AOAM533UFpLVqz3d2duT4V9d0Qyb/WifDsLJE55mTIHu2vXh79fw4whY YJxT1sNxEkPbV29EBwTDslKkSSLGU9yO0DHIpXg= X-Google-Smtp-Source: ABdhPJxMB947LrESJ9zuoRJrBPdn0Uir48l0pQ87jynMyHkyIaYKveCdPnpUQoed7c8vdkfnNqjWESg6eJ/WmFVL+e0= X-Received: by 2002:a05:6e02:12e7:: with SMTP id l7mr872566iln.60.1628239788186; Fri, 06 Aug 2021 01:49:48 -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: Fri, 6 Aug 2021 14:19:22 +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 Fri, Aug 6, 2021 at 3:47 AM Dumitrescu, Cristian wrote: > > Hi Jerin, > > > > 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 actu= ally 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 :) > > > > > > > +#. A meter object consists of a profile and a policy. Use above cr= eated > > > > 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 profil= e and a > > policy, but not exactly the meter object itself. > > > > > > How about: > > > The application creates a meter object using the rte_mtr_crea= te() 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! > > > > > > > +#. The API allows chaining the meter objects to create complex > > metering > > > > topology > > > > + by specifying ``struct rte_mtr_meter_policy_params::actions`` a= ction > > 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 no= w > > allows for a list of (any) rte_flow actions to be specified per color f= or each > > meter object, which opens up the door for a meter action to call one (o= r > > more) subsequent meter actions (on the same or different meter objects) > > conditional of a specific color. Which is another (new) way to chain me= ters, > > 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 specify= ing > > multiple meter actions (action on the same or different meter object) f= or 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 Met= er > > 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 approac= hes > > as valid? It would be great if you could have a separate diagram for ea= ch > > 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 t= hem 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 t= he meter action: now there are two different paths to enable any action typ= e, 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 allowin= g just one path for all the other actions as well, which defeats the purpo= se of the API update, right? OK. I will describe another model too. > > > > Regards, > Cristian