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

[Matan Azrad <matan@nvidia.com>]

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.
 

> > 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.

> 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