Set htb burst and cburst size to the minimum useful size. #194
Conversation
Work around a bug in tc where burst values are triply-rounded.
| # Note that "tc class show" will still show burst that are too small, | ||
| # due to the inverse bug, but if you use "tc -r class show" you will see | ||
| # the actual size of the cbuffer in bytes. | ||
| mtu = 1600 |
There was a problem hiding this comment.
MTU may vary depending on the environment.
Would a fixed value of 1600 be appropriate?
There was a problem hiding this comment.
This is the value that tc sets as its default when it does this calculation. It's overridable from the command line, but tcconfig doesn't override it. Maybe I should also add it as a parameter for tcconfig?
tcconfig/shaper/htb.py
Outdated
| # Emulation of tc's buggy time2tick implementation, which | ||
| # rounds to int twice | ||
| def time2tick_bug(time): | ||
| return int(int(time) * tick_in_usec) | ||
|
|
||
| # An accurate implementation of tick2time (unlike tc's), not rounding. | ||
| def tick2time_true(tick): | ||
| return tick / tick_in_usec | ||
|
|
||
| def calc_xmittime_bug(rate, size): | ||
| return int(time2tick_bug(TIME_UNITS_PER_SEC * (float(size) / rate))) | ||
|
|
||
| def calc_xmitsize_true(rate, ticks): | ||
| return (float(rate)*tick2time_true(ticks))/TIME_UNITS_PER_SEC |
There was a problem hiding this comment.
Could you add some simple unit tests to these functions?
There was a problem hiding this comment.
The difficulty here is that tick_in_usec is a system-dependent value, so it's hard to write reproducible tests for these functions.
I could override it in the tests, but that's scribbling on the inside of pyroute2's internal state, which seems fraught at best?
There was a problem hiding this comment.
Alternately, I could add an extra parameter to the function to allow me to pass in an external value of tick_in_usec for testing, but then the tests don't use the same code path as the production code...
| # the actual size of the cbuffer in bytes. | ||
| mtu = 1600 | ||
| rate = bandwidth.byte_per_sec | ||
| desired_burst = int(rate / get_hz() + mtu) |
There was a problem hiding this comment.
Is the reason for adding mtu in this calculation to provide a minimum guaranteed value?
If so, the following seems appropriate:
| desired_burst = int(rate / get_hz() + mtu) | |
| desired_burst = max(int(rate / get_hz()), mtu) |
There was a problem hiding this comment.
This is the default burst size calculation that tc tries to set, see its source. (Unfortunately the value it sets is then mangled by tc's buggy calc_xmittime implementation.)
The comment there is
/* compute minimal allowed burst from rate; mtu is added here to make
sure that buffer is larger than mtu and to have some safeguard space */
so I followed what they implemented.
tcconfig/shaper/htb.py
Outdated
| if calc_xmitsize_true(rate, calc_xmittime_bug(rate, burst)) >= desired_burst: | ||
| break |
There was a problem hiding this comment.
Can you be sure that this calculation will not go into an infinite loop for any value?
Wouldn't it be safer to set a maximum number of loops?
There was a problem hiding this comment.
I've switched it to use bisect_left (following the inspiration of https://fanchenbao.medium.com/full-powered-binary-search-with-bisect-in-python-3-10-fb4a76110746) which is O(log(n)) and guaranteed to halt.
O(log(n)) rather than O(n) worst-case, and guaranteed to halt.
JonathanLennox
force-pushed
the
fix-cburst-size
branch
from
77ba49d to
fa0a80c
Compare
|
Note that there are lint errors in files I didn't touch. |
This avoids problems where bursts of data can be sent at line speed rather than the limited speed.
Work around a bug in tc where burst values are triply-rounded.