HI Cristian From: Dumitrescu, Cristian > Hi Matan, > > > -----Original Message----- > > From: Matan Azrad <ma...@nvidia.com> > > Sent: Tuesday, March 2, 2021 7:02 AM > > To: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; Li Zhang > > <l...@nvidia.com>; Dekel Peled <dek...@nvidia.com>; Ori Kam > > <or...@nvidia.com>; Slava Ovsiienko <viachesl...@nvidia.com> > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon <tho...@monjalon.net>; > > Raslan Darawsheh <rasl...@nvidia.com>; m...@smartsharesystems.com; > > ajit.khapa...@broadcom.com; Yigit, Ferruh <ferruh.yi...@intel.com>; > > Singh, Jasvinder <jasvinder.si...@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-boun...@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
