Hi Cristian

Good discussion, thank you for that!

From: Dumitrescu, Cristian

Hi Matan,

quoted

-----Original Message-----
From: Matan Azrad <redacted>
Sent: Tuesday, March 2, 2021 12:37 PM
To: Dumitrescu, Cristian <redacted>; Li Zhang
[off-list ref]; Dekel Peled [off-list ref]; Ori Kam
[off-list ref]; Slava Ovsiienko [off-list ref]
Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon <redacted>;
Raslan Darawsheh [off-list ref]; mb@smartsharesystems.com;
ajit.khaparde@broadcom.com; Yigit, Ferruh [off-list ref];
Singh, Jasvinder [off-list ref]
Subject: RE: [dpdk-dev] [RFC v4 1/4] ethdev: add meter PPS profile

HI Cristian

From: Dumitrescu, Cristian

quoted

Hi Matan,

quoted

-----Original Message-----
From: Matan Azrad <redacted>
Sent: Tuesday, March 2, 2021 7:02 AM
To: Dumitrescu, Cristian <redacted>; Li Zhang
[off-list ref]; Dekel Peled [off-list ref]; Ori Kam
[off-list ref]; Slava Ovsiienko [off-list ref]
Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon

[off-list ref];

quoted

quoted

Raslan Darawsheh [off-list ref]; mb@smartsharesystems.com;
ajit.khaparde@broadcom.com; Yigit, Ferruh
[off-list ref]; Singh, Jasvinder
[off-list ref]
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

quoted

quoted

From: dev <redacted> On Behalf Of Li Zhang

<snip>

quoted

We had this same problem earlier for the rte_tm.h API, where
people

asked to

quoted

add support for WRED and shaper rates specified in packets to
the existing

byte

quoted

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

quoted

from rte_tm.h: the packets vs. bytes mode is explicitly
specified through

the use

quoted

of a flag called packet_mode that is added to the WRED and
shaper

profile.

quoted

quoted

quoted

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,

quoted

respectively. The same profile parameters are used, no need to
invent additional algorithms (such as srTCM - packet mode) or
profile data

structures.

quoted

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.

quoted

quoted

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

quoted

the ABI breakage is not applicable here. Therefore, this problem
does not

exist,

quoted

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.

The user is not the PMD, the PMDs are the providers.
I'm talking about all our customers, all the current DPDK based applications like OVS and others (I familiar with at least 4 ConnectX customer applications) which use the meter API and I'm sure there are more around the world.
  

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.

Are you sure all the world initialize the struct to 0? and also in this case, without new compilation, not all the struct will be zeroes(the old size is smaller). 

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?

Do you understand that any current application that use the meter API must recompile the code of the application? Part of them also probably need to set the flag to 0....
Do you understand also the potential issues for the applications which are not aware to the change? Debug time, etc....

quoted

quoted

quoted

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

quoted

way. Also, I think we should avoid duplicating configuration data
structures

for

quoted

to support essentially the same algorithms (such as srTCM or trTCM)
if we

can.

quoted

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.

New pps profile is also clear and simple.

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.

I don't see a complexity in the current suggestion.

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

Also by this way is high.


Look, the flag approach is also good and makes the job.
The two approaches are clear, simple and in high quality.
I don't care which one from the 2 to take but I want to be sure we are all understand the pros and cons.

If you understand my concern on flag approach and still insist to take the flag approach we will align.

And if we so, and we are going to break the API\ABI, we are going to introduce new meter policy API soon and there, breaking API can help, lets see in other discussion later.

One more point:
Currently, the meter_id is managed by the user, I think it is better to let the PMDs to manage the meter_id.

Searching the PMD meter handler inside the PMD is very expensive for the API call rate when the meter_id is managed by the user.

Same for profile_id.

Also all the rte_flow API including the shared action API taking the PMD management approach.

What do you think?

quoted

quoted

The flag proposal is actually reducing the amount of work that you
guys

need to

quoted

do to implement your proposal. There is no negative impact to your

proposal

quoted

and no big change, right?

Yes you right, but the implementation effect is not our concern.

quoted

quoted

quoted

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.

quoted

b) Change the description (comment) of the rate and bucket size

parameters in

quoted

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,

quoted

when applicable.

quoted

Regards,
Cristian

Regards,
Cristian

Regards,
Cristian