[ovs-dev] [PATCH RESEND ovs v3 4/4] dpif: Don't set "burst_size" to "rate" if not specified.

[Ilya Maximets]

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.

Best regards, Ilya Maximets.