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.mazarg...@6wind.com] > > Sent: Thursday, June 1, 2017 4:14 PM > > To: Dumitrescu, Cristian <cristian.dumitre...@intel.com> > > Cc: dev@dpdk.org; tho...@monjalon.net; > > jerin.ja...@caviumnetworks.com; hemant.agra...@nxp.com; Doherty, > > Declan <declan.dohe...@intel.com>; Wiles, Keith <keith.wi...@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.dumitre...@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 -- Adrien Mazarguil 6WIND
