On 4/7/21 1:45 PM, Tonghao Zhang wrote: > On Fri, Apr 2, 2021 at 4:35 AM Ilya Maximets <[email protected]> wrote: >> >> On 3/31/21 12:29 AM, Jean Tourrilhes wrote: >>> On Tue, Mar 30, 2021 at 02:27:11PM -0700, Ben Pfaff wrote: >>>> On Tue, Mar 30, 2021 at 11:16:48PM +0200, Ilya Maximets wrote: >>>>> >>>>> OpenFlow spec is a bit loose in definition of what should >>>>> be behavior if burst is not set: >>>>> """ >>>>> If the flag OFPMF_BURST is not set the burst_size values from meter >>>>> bands are ignored, and if the meter implementation uses a burst value, >>>>> this burst value must be set to an implementation defined optimal value. >>>>> """ >>>>> >>>>> In our case, historically, "implementation defined optimal value" was >>>>> value equal to rate. I have no idea why, but it's hard to argue with >>>>> it since the spec gives a great freedom to choose. >>>>> >>>>> Actually, the "burst" itself as a term makes very little sense to me. >>>>> It's defined by the spec as: >>>>> """ >>>>> It defines the granularity of the meter band, for all packet or byte >>>>> bursts whose length is greater than burst value, the meter rate will >>>>> always be strictly enforced. >>>>> """ >>>>> >>>>> But what is the burst? How the implementation should define which >>>>> packets are in the burst and which are from the next one? >>>>> >>>>> Current implementation just assumes that bursts are measured per second. >>>>> But the rate is measured per second too. So, burst and rate is >>>>> essentially the same thing and implementations just sums them together >>>>> to get the bucket size. So, I do not understand why "burst" and >>>>> "burst_size" exist at all. Why not just set the rate a bit higher? >>>>> >>>>> Ben, can you shed some light on this? What was the original idea >>>>> behind the meter burst? Or maybe I'm missing something? >>> >>> I don't understand how you can confuse a rate and a size. The >>> OpenFlow spec clearly says it's in kilobits or packets (not per >>> seconds). >>> A basic token bucket has only two parameters, the commited >>> rate and the burst size (i.e. maximum number of tokens in the >>> bucket). The spec reflect that in a generic way to avoid mandating an >>> implementation. >> >> Thanks, Jean. >> >> My problem, actually, was that I started from the implementation in the >> kernel datapath and it looked super weird. Especially, this part: >> https://elixir.bootlin.com/linux/latest/source/net/openvswitch/meter.c#L644 >> Than I tried to find the truth inside the spec, but it doesn't define >> the implementation on purpose. So the only option I had is to guess >> how this suppose to work. Was it 11pm or something else, but my guessing >> engine didn't came up with anything that might make sense. :) >> >>> Burst rate is only defined for more fancy rate limiters, such >>> as two colors rate limiters. In this case, you also have two burst >>> size, one for each token bucket. The OpenFlow spec does not support >>> those extra parameters (as of version 1.5.1). >>> For Linux 'police' filter : rate == rate ; burst_size == burst >>> For Linux 'htb' qdisc : rate == rate ; burst_size == burst ; >>> ceil and cburst are not supported. >> >> This totally makes sense. OTOH, Implementation inside both datapaths >> doesn't. >> >> Thanks for pushing me in right direction. >> >> For the implementations: I think, they needs to be reworked. >> At least, we need to get rid of 'rate' in a calculation of a maximum >> bucket size. It should not depend on rate, only on a burst size. >> i.e. instead of: >> max_bucket_size = (band->burst_size + band->rate) * 1000; >> there should be: >> max_bucket_size = band->burst_size * 1000; >> >> This way implementations will have at least a bit of sense. >> >> Summing burst size with rate is like summing apples with oranges. >> And that is what misled me. >> >> About having a value for a burst size being numerically equal to the >> configured rate: this looks like some kind of estimated value, but >> it's really hard to argue with it, because research is needed to >> define the "good value for most cases". >> >> Anyway, we need to fix calculation of a maximum bucket size first. > Hi Ilya > Would you plan to send a patch to fix it ? I have no idea to fix it.
OK. I'll prepare and send a patch later this week. For both userspace and kernel. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
