From mboxrd@z Thu Jan  1 00:00:00 1970
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Subject: Re: [PATCH 3/3] rte_flow: add new action for traffic
 metering and policing
Date: Tue, 19 Sep 2017 19:00:51 +0200
Message-ID: <20170919170051.GP2481@6wind.com>
References: <1496162653-137817-3-git-send-email-cristian.dumitrescu@intel.com>
 <1503705973-80742-1-git-send-email-cristian.dumitrescu@intel.com>
 <1503705973-80742-4-git-send-email-cristian.dumitrescu@intel.com>
 <20170906162302.GD4301@6wind.com>
 <3EB4FA525960D640B5BDFFD6A3D891267BABC13D@IRSMSX108.ger.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: "dev@dpdk.org" <dev@dpdk.org>, "thomas@monjalon.net" <thomas@monjalon.net>,
 "jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
 "hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>
To: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wr0-f171.google.com (mail-wr0-f171.google.com
 [209.85.128.171]) by dpdk.org (Postfix) with ESMTP id ADFA4199AE
 for <dev@dpdk.org>; Tue, 19 Sep 2017 19:01:02 +0200 (CEST)
Received: by mail-wr0-f171.google.com with SMTP id o42so197243wrb.3
 for <dev@dpdk.org>; Tue, 19 Sep 2017 10:01:02 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891267BABC13D@IRSMSX108.ger.corp.intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi Cristian,

On Tue, Sep 19, 2017 at 04:36:50PM +0000, Dumitrescu, Cristian wrote:
<snip>
> > >  /**
> > > + * RTE_FLOW_ACTION_TYPE_METER
> > > + *
> > > + * Traffic metering and policing (MTR).
> > > + *
> > > + * Packets matched by items of this type can be either dropped or passed
> > to the
> > > + * next item with their color set by the MTR object.
> > > + *
> > > + * Non-terminating by default.
> > > + */
> > > +struct rte_flow_action_meter {
> > > +	uint32_t mtr_id; /**< MTR object ID created with rte_mtr_create().
> > */
> > > +};
> > > +
> > 
> > Default mask definition is missing.
> > 
> 
> I do not understand this comment. This is a flow action, not a flow item (that might be a packet field with an associated mask); this mtr_id is similar to queue ID/index/VF ID from other flow actions, none having any mask attached. Adrien, can you please clarify?

Yes, I actually misread it as a pattern item definition for some
reason. Nothing to see here, move along!

> 
> > > +/**
> > >   * Definition of a single action.
> > >   *
> > >   * A list of actions is terminated by a END action.
> > > --
> > > 2.7.4
> > >
> > 
> > Even if MTR is a separate API, please add to this commit:
> > 
> > - Documentation update: guides/prog_guide/rte_flow.rst
> > - Testpmd update: app/test-pmd/cmdline_flow.c
> > - Testpmd documentation update:
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > 
> > You can find examples in previous commits related to rte_flow.
> > 
> 
> All of these items are a must and will get done, but do they have to be done in the same patch set?

That'd be much better as far as I'm involved in this review (rte_flow
changes). You should put them in the same patch as the above.

> My plan was to introduce test-pmd updates through separate patch sets after the API is accepted. I know you had these items done in the same patch set for rte_flow, but there are other APIs such as eventdev and ethdev traffic management which introduced sample app one release later.

In that case, could you split these changes in two parts?

This patch could bring the basic MTR action support in testpmd, by this I
mean the ability to type a flow command with such an action and not get
rejected with a "bad arguments" error since it is actually part of the API,
even if nothing is connected to that action at this point. The rule should
however get rejected by its lack of support in the underlying PMD.

Same idea for rte_flow and testpmd documentation update, this patch only
needs the minimum amount describing what this action is without a link to
the actual MTR documentation, which is not present at this point.

Subsequent commits shall update these as they complete the MTR API.

-- 
Adrien Mazarguil
6WIND