Re: [dpdk-dev] [RFC v4 1/4] ethdev: add meter PPS profile

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Matan,

> -----Original Message-----
> From: Matan Azrad <matan@nvidia.com>
> Sent: Tuesday, March 2, 2021 12:37 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Li Zhang
> <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Raslan Darawsheh <rasland@nvidia.com>;
> mb@smartsharesystems.com; ajit.khaparde@broadcom.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Singh, Jasvinder <jasvinder.singh@intel.com>
> Subject: RE: [dpdk-dev] [RFC v4 1/4] ethdev: add meter PPS profile
> 
> HI Cristian
> 
> From: Dumitrescu, Cristian
> > Hi Matan,
> >
> > > -----Original Message-----
> > > From: Matan Azrad <matan@nvidia.com>
> > > Sent: Tuesday, March 2, 2021 7:02 AM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Li Zhang
> > > <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Ori Kam
> > > <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>
> > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>;
> > > Raslan Darawsheh <rasland@nvidia.com>; mb@smartsharesystems.com;
> > > ajit.khaparde@broadcom.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > Singh, Jasvinder <jasvinder.singh@intel.com>
> > > Subject: RE: [dpdk-dev] [RFC v4 1/4] ethdev: add meter PPS profile
> > >
> > >
> > >
> > > Hi Cristian
> > >
> > > Thank you for review, please see inline.
> > >
> > > From: Dumitrescu, Cristian
> > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Li Zhang
> > > <snip>
> > > > We had this same problem earlier for the rte_tm.h API, where people
> > > asked to
> > > > add support for WRED and shaper rates specified in packets to the
> > > > existing
> > > byte
> > > > rate support. I am more than happy to support adding the same here,
> > > > but please let's adopt the same solution here rather than invent a
> > > > different approach.
> > > >
> > > > Please refer to struct rte_tm_wred_params and struct
> > > rte_tm_shaper_params
> > > > from rte_tm.h: the packets vs. bytes mode is explicitly specified
> > > > through
> > > the use
> > > > of a flag called packet_mode that is added to the WRED and shaper
> profile.
> > > > When packet_mode is 0, the profile rates and bucket sizes are
> > > > specified in bytes per second and bytes, respectively; when
> > > > packet_mode is not 0, the profile rates and bucket sizes are
> > > > specified in packets and packets per
> > > second,
> > > > respectively. The same profile parameters are used, no need to
> > > > invent additional algorithms (such as srTCM - packet mode) or
> > > > profile data
> > > structures.
> > > > Can we do the same here, please?
> > >
> > > This flag approach is very intuitive suggestion and it has advantages.
> > >
> > > The main problem with the flag approach is that it breaks ABI and API.
> > > The profile structure size is changed due to a new field - ABI breakage.
> > > The user must initialize the flag with zero to get old behavior - API
> breakage.
> > >
> >
> > The rte_mtr API is experimental, all the API functions are correctly marked
> > with __rte_experimental in rte_mtr.h file, so we can safely change the API
> and
> > the ABI breakage is not applicable here. Therefore, this problem does not
> exist,
> > correct?
> 
> Yes, but still meter is not new API and I know that a lot of user uses it for a
> long time.
> Forcing them to change while we have good solution that don't force it, looks
> me problematic.
> 

Not really, only 3 drivers are currently implementing this API.

Even to these drivers, the required changes are none or extremely small: as Ajit was also noting, as the default value of 0 continues to represent the existing byte mode, all you have to do is make sure the new flag is set to zero in the profile params structure, which is already done implicitly in most places as this structure is initialized to all-zeros.

A simple search exercise for struct rte_mtr_meter_profile is all that is needed. You also agreed the flag approach is very intuitive, hence better and nicer, with no additional work needed for you, so why not do it?

> 
> > > I don't see issues with Li suggestion, Do you think Li suggestion has
> > > critical issues?
> >
> > It is probably better to keep the rte_mtr and the rte_tm APIs aligned, it
> > simplifies the code maintenance and improves the user experience, which
> > always pays off in the long run. Both APIs configure token buckets in either
> > packet mode or byte mode, and it is desirable to have them work in the
> same
> > way. Also, I think we should avoid duplicating configuration data structures
> for
> > to support essentially the same algorithms (such as srTCM or trTCM) if we
> can.
> >
> 
> Yes, but I don't think this motivation is critical.

I really disagree. As API maintainer, making every effort to keep the APIs clear and consistent is a critical task for me. We don't want to proliferate the API data structures and parameters if there is a good way to avoid it. Especially in cases like this, when the drivers are just beginning to pick up this (still experimental) API,  we have the rare chance to make things right and therefore we should do it. Please also keep in mind that, as more feature are added to the API, small corner cuts like this one that might not look like a big deal now, eventually come back as unnecessary complexity in the drivers themselves.

So, please, let's try to keep the quality of the APIs high.

> 
> > The flag proposal is actually reducing the amount of work that you guys
> need to
> > do to implement your proposal. There is no negative impact to your
> proposal
> > and no big change, right?
> 
> Yes you right, but the implementation effect is not our concern.
> 
> 
> > > > This is a quick summary of the required API changes to add support
> > > > for the packet mode, they are minimal:
> > > > a) Introduce the packet_mode flag in the profile parameters data
> > > structure.
> > > > b) Change the description (comment) of the rate and bucket size
> > > parameters in
> > > > the meter profile parameters data structures to reflect that their
> > > > values represents either bytes or packets, depending on the value of
> > > > the new flag packet_mode from the same structure.
> > > > c) Add the relevant capabilities: just search for "packet" in the
> > > > rte_tm.h capabilities data structures and apply the same to the
> > > > rte_mtr.h
> > > capabilities,
> > > > when applicable.
> > >
> > > > Regards,
> > > > Cristian
> >
> > Regards,
> > Cristian

Regards,
Cristian