HI Cristian From: Dumitrescu, Cristian > Hi Matan, > > > -----Original Message----- > > From: Matan Azrad <[email protected]> > > Sent: Tuesday, March 2, 2021 7:02 AM > > To: Dumitrescu, Cristian <[email protected]>; Li Zhang > > <[email protected]>; Dekel Peled <[email protected]>; Ori Kam > > <[email protected]>; Slava Ovsiienko <[email protected]> > > Cc: [email protected]; NBU-Contact-Thomas Monjalon <[email protected]>; > > Raslan Darawsheh <[email protected]>; [email protected]; > > [email protected]; Yigit, Ferruh <[email protected]>; > > Singh, Jasvinder <[email protected]> > > 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 <[email protected]> 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
