| Message ID | 85371219-A098-4873-B3B9-0E881E812F2A@8x8.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, 5 Feb 2025 16:16:21 -0500 Jonathan Lennox <jonathan.lennox@8x8.com> wrote: > The logic in tc that converts between sizes and times for a given rate (the > functions tc_calc_xmittime and tc_calc_xmitsize) suffers from double rounding, > with intermediate values getting cast to unsigned int. > > As a result, for example, on my test system (where tick_in_usec=15.625, > clock_factor=1, and hz=1000000000) for a bitrate of 1Gbps, all tc htb burst > values between 0 and 999 get encoded as 0; all values between 1000 and 1999 > get encoded as 15 (equivalent to 960 bytes); all values between 2000 and 2999 > as 31 (1984 bytes); etc. > > The attached patch changes this so these calculations are done entirely in > floating-point, and only rounded to integer values when the value is returned. > It also changes tc_calc_xmittime to round its calculated value up, rather than > down, to ensure that the calculated time is actually sufficient for the requested > size. > > This is a userspace-only fix to tc; no kernel changes are necessary. > > (Please let me know if anything is wrong with this patch, this is my first > time submitting to any Linux kernel mailing lists.) > > --- The concept makes sense, but is missing a valid Signed-Off-by: and therefore needs to be resent. Also, you don't need to rename the functions, why not always use floating point
> On Feb 17, 2025, at 1:14 AM, Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Wed, 5 Feb 2025 16:16:21 -0500 > Jonathan Lennox <jonathan.lennox@8x8.com> wrote: > >> The logic in tc that converts between sizes and times for a given rate (the >> functions tc_calc_xmittime and tc_calc_xmitsize) suffers from double rounding, >> with intermediate values getting cast to unsigned int. > > The concept makes sense, but is missing a valid Signed-Off-by: and therefore > needs to be resent. Thanks, will fix in v2. > Also, you don't need to rename the functions, why not always use floating point There was another function using tc_calc_xmittime (tbv_print_opt) so I was worried about breaking it, but on inspection I realize it’s also doing a calculation in double and it’s just printing data, so it should be fine (and hopefully an improvement). I’ve fixed this in v2 as well.
diff --git a/tc/tc_core.c b/tc/tc_core.c index 37547e9b..ba8d5bf0 100644 --- a/tc/tc_core.c +++ b/tc/tc_core.c @@ -23,11 +23,16 @@ static double tick_in_usec = 1; static double clock_factor = 1; -static unsigned int tc_core_time2tick(unsigned int time) +static double tc_core_time2tick_d(double time) { return time * tick_in_usec; } +static double tc_core_tick2time_d(double tick) +{ + return tick / tick_in_usec; +} + unsigned int tc_core_tick2time(unsigned int tick) { return tick / tick_in_usec; @@ -45,12 +50,12 @@ unsigned int tc_core_ktime2time(unsigned int ktime) unsigned int tc_calc_xmittime(__u64 rate, unsigned int size) { - return tc_core_time2tick(TIME_UNITS_PER_SEC*((double)size/(double)rate)); + return ceil(tc_core_time2tick_d(TIME_UNITS_PER_SEC*((double)size/(double)rate))); } unsigned int tc_calc_xmitsize(__u64 rate, unsigned int ticks) { - return ((double)rate*tc_core_tick2time(ticks))/TIME_UNITS_PER_SEC; + return ((double)rate*tc_core_tick2time_d(ticks))/TIME_UNITS_PER_SEC; } /*