[dpdk-dev,3/3] rte_flow: add new action for traffic metering and policing
Checks
Commit Message
Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
lib/librte_ether/rte_flow.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Comments
Hi Cristian,
This commit should probably come first in the series since the rest relies
on it, right?
Subject line does not conform past commits, it should start with "ethdev:"
and not mention "rte_flow" (use "flow API").
On Sat, Aug 26, 2017 at 01:06:13AM +0100, Cristian Dumitrescu wrote:
> Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
A short description of what this new item does and why it's added is
necessary. Context provided by the rest of the series will not always be
available.
More comments below.
> ---
> lib/librte_ether/rte_flow.h | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index bba6169..5569a87 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -915,6 +915,14 @@ enum rte_flow_action_type {
> * See struct rte_flow_action_vf.
> */
> RTE_FLOW_ACTION_TYPE_VF,
> +
> + /**
> + * Traffic metering and policing (MTR).
> + *
> + * See struct rte_flow_action_meter.
> + * See file rte_mtr.h for MTR object configuration.
> + */
> + RTE_FLOW_ACTION_TYPE_METER,
> };
>
> /**
> @@ -1008,6 +1016,20 @@ struct rte_flow_action_vf {
> };
>
> /**
> + * 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.
> +/**
> * 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.
Hi Adrien,
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, September 6, 2017 5:23 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net;
> jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com
> Subject: Re: [PATCH 3/3] rte_flow: add new action for traffic metering and
> policing
>
> Hi Cristian,
>
> This commit should probably come first in the series since the rest relies
> on it, right?
>
Yes, will do in V2.
> Subject line does not conform past commits, it should start with "ethdev:"
> and not mention "rte_flow" (use "flow API").
>
Yes, sir!
> On Sat, Aug 26, 2017 at 01:06:13AM +0100, Cristian Dumitrescu wrote:
> > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
>
> A short description of what this new item does and why it's added is
> necessary. Context provided by the rest of the series will not always be
> available.
>
Yes, will add here the relevant description, which is currently only in the cover letter.
> More comments below.
>
> > ---
> > lib/librte_ether/rte_flow.h | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> > index bba6169..5569a87 100644
> > --- a/lib/librte_ether/rte_flow.h
> > +++ b/lib/librte_ether/rte_flow.h
> > @@ -915,6 +915,14 @@ enum rte_flow_action_type {
> > * See struct rte_flow_action_vf.
> > */
> > RTE_FLOW_ACTION_TYPE_VF,
> > +
> > + /**
> > + * Traffic metering and policing (MTR).
> > + *
> > + * See struct rte_flow_action_meter.
> > + * See file rte_mtr.h for MTR object configuration.
> > + */
> > + RTE_FLOW_ACTION_TYPE_METER,
> > };
> >
> > /**
> > @@ -1008,6 +1016,20 @@ struct rte_flow_action_vf {
> > };
> >
> > /**
> > + * 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?
> > +/**
> > * 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?
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.
> --
> Adrien Mazarguil
> 6WIND
Regards,
Cristian
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.
Hi Adrien,
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, September 19, 2017 6:01 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net;
> jerin.jacob@caviumnetworks.com; hemant.agrawal@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
@@ -915,6 +915,14 @@ enum rte_flow_action_type {
* See struct rte_flow_action_vf.
*/
RTE_FLOW_ACTION_TYPE_VF,
+
+ /**
+ * Traffic metering and policing (MTR).
+ *
+ * See struct rte_flow_action_meter.
+ * See file rte_mtr.h for MTR object configuration.
+ */
+ RTE_FLOW_ACTION_TYPE_METER,
};
/**
@@ -1008,6 +1016,20 @@ struct rte_flow_action_vf {
};
/**
+ * 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(). */
+};
+
+/**
* Definition of a single action.
*
* A list of actions is terminated by a END action.