On Fri, Aug 6, 2021 at 3:47 AM Dumitrescu, Cristian <cristian.dumitre...@intel.com> wrote: > > Hi Jerin, > > > > On Thu, Aug 5, 2021 at 4:36 PM Dumitrescu, Cristian > > <cristian.dumitre...@intel.com> wrote: > > > > > > HI Jerin, > > > > > > Thanks for your patch! > > > > > > Initially, it looked like an easy job to review it, but there is actually > > > an > > elephant in the room on how to chain the meters, see below. > > > > Yes. That's why I send this patch to finalize how to chain the meter. > > It was hard for me to follow that's why added a digram. > > > > OK, got it :) > > <snip> > > > > > +#. A meter object consists of a profile and a policy. Use above created > > > > objects to create > > > > + meter object using ``rte_mtr_create()``. Application uses > > > > + ``struct rte_mtr_params::meter_profile_id`` and ``struct > > > > rte_mtr_params::meter_policy_id`` > > > > + to specify the profile (created in step 2) and policy (created in > > > > step 3). > > > > > > It is mainly the meter object configuration that consists of a profile > > > and a > > policy, but not exactly the meter object itself. > > > > > > How about: > > > The application creates a meter object using the rte_mtr_create() > > > API > > function. One of the previously created meter profile and meter policy are > > provided as arguments at this step. > > > > Sure. I just though to add exact struct field so that it easy for end > > users to know. > > > > How about your version + struct name. > > > > The application creates a meter object using the rte_mtr_create() API > > function. > > One of the previously created meter profile (`struct > > rte_mtr_params::meter_profile_id``) and meter policy > > ``struct rte_mtr_params::meter_policy_id`` are provided as arguments > > at this step. > > > > Great, thanks! > > <snip> > > > > > +#. The API allows chaining the meter objects to create complex > > metering > > > > topology > > > > + by specifying ``struct rte_mtr_meter_policy_params::actions`` action > > as > > > > + ``RTE_FLOW_ACTION_TYPE_METER`` to the parent meter object > > encoded > > > > as > > > > + ``struct rte_flow_action_meter::mtr_id``. > > > > > > Now this could be the elephant in the room: > > > > > > With the latest API changes that went in recently, the rte_mtr API now > > allows for a list of (any) rte_flow actions to be specified per color for > > each > > meter object, which opens up the door for a meter action to call one (or > > more) subsequent meter actions (on the same or different meter objects) > > conditional of a specific color. Which is another (new) way to chain meters, > > right? Let's refer to this as the Meter Chaining - Approach B. > > > > Yes, my diagram is approach B. > > > > > > > > Before these API changes, the only way to chain meters was by specifying > > multiple meter actions (action on the same or different meter object) for > > the > > same rte_flow. Let's refer to this as the Meter Chaining - Approach A. > > > > Yes. Me too was assuming that way. But no one really implemented the > > chaining. With the MLX patch that got changed. > > > > > > > > The Meter Chaining - Approach A is valid. After a bit of thinking, I > > > don't see > > any reason to invalidate the approach you describe, so I agree that Meter > > Chaining - Approach B is also valid, with the (small) advantage that is > > conditional of a specific color. > > > > > > So, I think we should describe here both approaches. How about adding a > > new distinct section for Meter Chaining, where we describe both approaches > > as valid? It would be great if you could have a separate diagram for each > > approach :) > > > > This is the exact reason to send the patch. Approach B is a superset. > > Should we allow two approaches ? It will complicate the driver as it > > needs to track two ways of changing. > > Can we keep only the new approach, Just to make everyone's life easy? > > We started implementing this driver and realized two paths are painful > > for the driver. > > > > I agree the latest API changes push more complexity into the driver, but both > approaches logically make sense and are already part of the API, so I think > it is better to implement them both, if possible, and also describe them both > in the doc. Supporting both will avoid confusing the users. > > Also this problem that you mention is true for any action, not just for the > meter action: now there are two different paths to enable any action type, > one is enablement as a flow action, and the other one is enablement as a > meter action; allowing just one path for the meter action leads to allowing > just one path for all the other actions as well, which defeats the purpose > of the API update, right?
OK. I will describe another model too. > > <snip> > > Regards, > Cristian
