[dpdk-dev,RFC,3/3] rte_flow: add new action for traffic metering and policing

  Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 lib/librte_ether/rte_flow.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
  

Hi Cristian,

On Tue, May 30, 2017 at 05:44:13PM +0100, Cristian Dumitrescu wrote:
> Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> ---
>  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 c47edbc..2942ca7 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -881,6 +881,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,
>  };
>  
>  /**
> @@ -974,6 +982,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.

Assuming this action is provided to the underlying PMD, can you describe
what happens next; what is a PMD supposed to do when creating the flow rule
and the impact on its data path?

It looks like mtr_id is arbitrarily set by the user calling
rte_mtr_create(), which means the PMD has to look up the associated MTR
context somehow.

How about making the rte_mtr_create() API return an opaque rte_mtr object
pointer provided back to all API functions as well as through this action
instead, and not leave it up to the user?
  

Hi Adrien,

Thanks for reviewing this proposal.

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, June 1, 2017 4:14 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net;
> jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Doherty,
> Declan <declan.doherty@intel.com>; Wiles, Keith <keith.wiles@intel.com>
> Subject: Re: [RFC 3/3] rte_flow: add new action for traffic metering and
> policing
> 
> Hi Cristian,
> 
> On Tue, May 30, 2017 at 05:44:13PM +0100, Cristian Dumitrescu wrote:
> > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > ---
> >  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 c47edbc..2942ca7 100644
> > --- a/lib/librte_ether/rte_flow.h
> > +++ b/lib/librte_ether/rte_flow.h
> > @@ -881,6 +881,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,
> >  };
> >
> >  /**
> > @@ -974,6 +982,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.
> 
> Assuming this action is provided to the underlying PMD, can you describe
> what happens next; what is a PMD supposed to do when creating the flow
> rule
> and the impact on its data path?
> 

Metering is just another flow action that needs to be supported by rte_flow API.

Typically, NICs supporting this action have an array of metering & policing contexts on their data path, which are abstracted as MTR objects in our API.
- rte_mtr_create() configures an MTR object, with no association to any of the known flows yet.
	- On NIC side, the driver configures one of the available metering & policing contexts.
- rte_flow_create() defines the flow (match rule) and its set of actions, with metering & policing as one of the actions.
	- On NIC side, the driver configures a flow/filter for traffic classification/distribution/bifurcation, with the metering & policing context enabled for this flow.

At run-time, any packet matching this flow will execute this action, which involves metering (packet is assigned a color) and policing (packet may be recolored or dropped, as configured), with stats being updated as well.

> It looks like mtr_id is arbitrarily set by the user calling
> rte_mtr_create(), which means the PMD has to look up the associated MTR
> context somehow.
> 
> How about making the rte_mtr_create() API return an opaque rte_mtr
> object
> pointer provided back to all API functions as well as through this action
> instead, and not leave it up to the user?
> 

Of course, it can be done this way as well, but IMHO probably not the best idea from the application perspective. We had a similar discussion when we defined the ethdev traffic management API [1].

Object handles can be integers, void pointers or pointers to opaque structures, and each of these approaches are allowed and used by DPDK APIs. Here is an example why I think using integers for MTR object handle makes the life of the application easier:
- Let's assume we have several actions for a flow (a1, a2, a3, ...).
- When handles are pointers to opaque structures, app typically needs to save all of them in a per flow data structure: struct a1 *p1, struct a2 *p2, struct a3 *p3.
	-This results in increased complexity and size for the app tables, which can be avoided.
- When handles are integers generated by the app as opposed of driver, the app can simply use a single index - let's cal it flow_id - and register it as the handle to each of these flow actions.
	- No more fake tables.
	- No more worries about the pointer being valid in one address space and not valid in another.

There is some handle lookup to be done by the driver, but this is a trivial task,  and checking the validity of the handle (input parameter) is the first thing done by any API function, regardless of which handle style is used.

[1] http://www.dpdk.org/ml/archives/dev/2017-February/057368.html


> --
> Adrien Mazarguil
> 6WIND

Regards,
Cristian
  

Hi Cristian,

Took me a while to reply and I didn't see any update in the meantime, is
this RFC still relevant?

More comments below.

On Tue, Jun 06, 2017 at 06:37:57PM +0000, Dumitrescu, Cristian wrote:
> Hi Adrien,
> 
> Thanks for reviewing this proposal.
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Thursday, June 1, 2017 4:14 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: dev@dpdk.org; thomas@monjalon.net;
> > jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Doherty,
> > Declan <declan.doherty@intel.com>; Wiles, Keith <keith.wiles@intel.com>
> > Subject: Re: [RFC 3/3] rte_flow: add new action for traffic metering and
> > policing
> > 
> > Hi Cristian,
> > 
> > On Tue, May 30, 2017 at 05:44:13PM +0100, Cristian Dumitrescu wrote:
> > > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > > ---
> > >  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 c47edbc..2942ca7 100644
> > > --- a/lib/librte_ether/rte_flow.h
> > > +++ b/lib/librte_ether/rte_flow.h
> > > @@ -881,6 +881,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,
> > >  };
> > >
> > >  /**
> > > @@ -974,6 +982,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.
> > 
> > Assuming this action is provided to the underlying PMD, can you describe
> > what happens next; what is a PMD supposed to do when creating the flow
> > rule
> > and the impact on its data path?
> > 
> 
> Metering is just another flow action that needs to be supported by rte_flow API.
> 
> Typically, NICs supporting this action have an array of metering & policing contexts on their data path, which are abstracted as MTR objects in our API.
> - rte_mtr_create() configures an MTR object, with no association to any of the known flows yet.
> 	- On NIC side, the driver configures one of the available metering & policing contexts.
> - rte_flow_create() defines the flow (match rule) and its set of actions, with metering & policing as one of the actions.
> 	- On NIC side, the driver configures a flow/filter for traffic classification/distribution/bifurcation, with the metering & policing context enabled for this flow.
> 
> At run-time, any packet matching this flow will execute this action, which involves metering (packet is assigned a color) and policing (packet may be recolored or dropped, as configured), with stats being updated as well.

Thanks, this description should be part of the documentation in the final
patch. The relationship between rte_mtr and rte_flow objects must be
described as well, for instance making clear that one cannot remove a mtr
object if a flow rule depends on it.

> > It looks like mtr_id is arbitrarily set by the user calling
> > rte_mtr_create(), which means the PMD has to look up the associated MTR
> > context somehow.
> > 
> > How about making the rte_mtr_create() API return an opaque rte_mtr
> > object
> > pointer provided back to all API functions as well as through this action
> > instead, and not leave it up to the user?
> > 
> 
> Of course, it can be done this way as well, but IMHO probably not the best idea from the application perspective. We had a similar discussion when we defined the ethdev traffic management API [1].
> 
> Object handles can be integers, void pointers or pointers to opaque structures, and each of these approaches are allowed and used by DPDK APIs. Here is an example why I think using integers for MTR object handle makes the life of the application easier:
> - Let's assume we have several actions for a flow (a1, a2, a3, ...).
> - When handles are pointers to opaque structures, app typically needs to save all of them in a per flow data structure: struct a1 *p1, struct a2 *p2, struct a3 *p3.
> 	-This results in increased complexity and size for the app tables, which can be avoided.
> - When handles are integers generated by the app as opposed of driver, the app can simply use a single index - let's cal it flow_id - and register it as the handle to each of these flow actions.
> 	- No more fake tables.
> 	- No more worries about the pointer being valid in one address space and not valid in another.
> 
> There is some handle lookup to be done by the driver, but this is a trivial task,  and checking the validity of the handle (input parameter) is the first thing done by any API function, regardless of which handle style is used.

All this sounds reasonable from the control plane standpoint. My comment was
more generally besides the above rte_flow action, are we sure mtr_id will
never be used to perform actions from the data plane?

Otherwise driver lookup is perhaps a trivial task but is nonetheless
expensive. This step can be avoided if mtr_id contains enough information to
locate the related object as fast as possible inside the PMD without the
need for an intermediate lookup table.

In which case, making mtr_id a pointer-sized opaque value (e.g. uintptr_t)
provided by the PMD when calling rte_mtr_create() gives more flexibility to
the PMD. For some, a basic index value could be enough while for others, an
intermediate structure could be necessary.

Admittedly this is exactly the same as a pointer type to an opaque object,
address space-related issues would have to be managed by the PMD either
way. Applications are not supposed to dereference such objects.

> [1] http://www.dpdk.org/ml/archives/dev/2017-February/057368.html
  

Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Monday, July 10, 2017 4:21 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net;
> jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Doherty,
> Declan <declan.doherty@intel.com>; Wiles, Keith <keith.wiles@intel.com>
> Subject: Re: [RFC 3/3] rte_flow: add new action for traffic metering and
> policing
> 
> Hi Cristian,
> 
> Took me a while to reply and I didn't see any update in the meantime, is
> this RFC still relevant?

Yes, absolutely!

> 
> More comments below.
> 

Thanks for continuing to look into this! I am actively looking for partners to evolve this API further and make it a good fit for DPDK devices. Now that Traffic Management API got merged, I can hopefully come back and spend more time on this.

> On Tue, Jun 06, 2017 at 06:37:57PM +0000, Dumitrescu, Cristian wrote:
> > Hi Adrien,
> >
> > Thanks for reviewing this proposal.
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > Sent: Thursday, June 1, 2017 4:14 PM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Cc: dev@dpdk.org; thomas@monjalon.net;
> > > jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Doherty,
> > > Declan <declan.doherty@intel.com>; Wiles, Keith
> <keith.wiles@intel.com>
> > > Subject: Re: [RFC 3/3] rte_flow: add new action for traffic metering and
> > > policing
> > >
> > > Hi Cristian,
> > >
> > > On Tue, May 30, 2017 at 05:44:13PM +0100, Cristian Dumitrescu wrote:
> > > > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > > > ---
> > > >  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 c47edbc..2942ca7 100644
> > > > --- a/lib/librte_ether/rte_flow.h
> > > > +++ b/lib/librte_ether/rte_flow.h
> > > > @@ -881,6 +881,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,
> > > >  };
> > > >
> > > >  /**
> > > > @@ -974,6 +982,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.
> > >
> > > Assuming this action is provided to the underlying PMD, can you describe
> > > what happens next; what is a PMD supposed to do when creating the
> flow
> > > rule
> > > and the impact on its data path?
> > >
> >
> > Metering is just another flow action that needs to be supported by
> rte_flow API.
> >
> > Typically, NICs supporting this action have an array of metering & policing
> contexts on their data path, which are abstracted as MTR objects in our API.
> > - rte_mtr_create() configures an MTR object, with no association to any of
> the known flows yet.
> > 	- On NIC side, the driver configures one of the available metering &
> policing contexts.
> > - rte_flow_create() defines the flow (match rule) and its set of actions,
> with metering & policing as one of the actions.
> > 	- On NIC side, the driver configures a flow/filter for traffic
> classification/distribution/bifurcation, with the metering & policing context
> enabled for this flow.
> >
> > At run-time, any packet matching this flow will execute this action, which
> involves metering (packet is assigned a color) and policing (packet may be
> recolored or dropped, as configured), with stats being updated as well.
> 
> Thanks, this description should be part of the documentation in the final
> patch. The relationship between rte_mtr and rte_flow objects must be
> described as well, for instance making clear that one cannot remove a mtr
> object if a flow rule depends on it.
> 

Yes, took the note to document this. Will expand this is Doxygen as well.

> > > It looks like mtr_id is arbitrarily set by the user calling
> > > rte_mtr_create(), which means the PMD has to look up the associated
> MTR
> > > context somehow.
> > >
> > > How about making the rte_mtr_create() API return an opaque rte_mtr
> > > object
> > > pointer provided back to all API functions as well as through this action
> > > instead, and not leave it up to the user?
> > >
> >
> > Of course, it can be done this way as well, but IMHO probably not the best
> idea from the application perspective. We had a similar discussion when we
> defined the ethdev traffic management API [1].
> >
> > Object handles can be integers, void pointers or pointers to opaque
> structures, and each of these approaches are allowed and used by DPDK
> APIs. Here is an example why I think using integers for MTR object handle
> makes the life of the application easier:
> > - Let's assume we have several actions for a flow (a1, a2, a3, ...).
> > - When handles are pointers to opaque structures, app typically needs to
> save all of them in a per flow data structure: struct a1 *p1, struct a2 *p2,
> struct a3 *p3.
> > 	-This results in increased complexity and size for the app tables,
> which can be avoided.
> > - When handles are integers generated by the app as opposed of driver,
> the app can simply use a single index - let's cal it flow_id - and register it as
> the handle to each of these flow actions.
> > 	- No more fake tables.
> > 	- No more worries about the pointer being valid in one address space
> and not valid in another.
> >
> > There is some handle lookup to be done by the driver, but this is a trivial
> task,  and checking the validity of the handle (input parameter) is the first
> thing done by any API function, regardless of which handle style is used.
> 
> All this sounds reasonable from the control plane standpoint. My comment
> was
> more generally besides the above rte_flow action, are we sure mtr_id will
> never be used to perform actions from the data plane?
> 

Yes, we are sure. This object ID is just used for configuration.

Even for the SW fall-back case (see the librte_meter) this ID is not used on data path. When flow is added with this action, the meter context is simply initialized in the flow table entry and used directly from here, no lookup takes place on data path.

> Otherwise driver lookup is perhaps a trivial task but is nonetheless
> expensive. This step can be avoided if mtr_id contains enough information to
> locate the related object as fast as possible inside the PMD without the
> need for an intermediate lookup table.
> 

Yes, agreed, any ID lookup/translation on data path is expensive and has to be avoided. As stated above, there is no ID lookup/translation on the data path to be performed here.

> In which case, making mtr_id a pointer-sized opaque value (e.g. uintptr_t)
> provided by the PMD when calling rte_mtr_create() gives more flexibility to
> the PMD. For some, a basic index value could be enough while for others, an
> intermediate structure could be necessary.
> 
> Admittedly this is exactly the same as a pointer type to an opaque object,
> address space-related issues would have to be managed by the PMD either
> way. Applications are not supposed to dereference such objects.
> 

Yes, these are the typical methods to avoid any ID lookup/translation on data path, but not needed here.

> > [1] http://www.dpdk.org/ml/archives/dev/2017-February/057368.html
> 
> --
> Adrien Mazarguil
> 6WIND

Regards,
Cristian