Hi Adrien, > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > Sent: Tuesday, September 19, 2017 6:01 PM > To: Dumitrescu, Cristian <cristian.dumitre...@intel.com> > Cc: dev@dpdk.org; tho...@monjalon.net; > jerin.ja...@caviumnetworks.com; hemant.agra...@nxp.com > Subject: Re: [PATCH 3/3] rte_flow: add new action for traffic metering and > policing > > 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! >
No worries. > > > > > > +/** > > > > * 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. > Full doc and test-pmd have been implemented in V2 just sent earlier, so let me know if you are OK with V2. > -- > Adrien Mazarguil > 6WIND Thanks, Cristian
