| Message ID | B6A0B441-A9C9-40B5-8944-B596CB57CF0E@8x8.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2,v2] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 2/18/25 1:10 PM, Jonathan Lennox wrote: > lacking a commit message. What is the problem with the current code and how do this patch fix it. Add an example that led you down this path as well. > Signed-off-by: Jonathan Lennox <jonathan.lennox@8x8.com> > --- > tc/tc_core.c | 6 +++--- > tc/tc_core.h | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) >
> On Feb 23, 2025, at 10:06 PM, David Ahern <dsahern@kernel.org> wrote: > > On 2/18/25 1:10 PM, Jonathan Lennox wrote: >> > > lacking a commit message. What is the problem with the current code and > how do this patch fix it. Add an example that led you down this path as > well. > >> Signed-off-by: Jonathan Lennox <jonathan.lennox@8x8.com> >> --- >> tc/tc_core.c | 6 +++--- >> tc/tc_core.h | 2 +- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> > Sorry; this was the v2 patch and I explained it in the v1, but I don’t think the threading worked. The problem is that tc_calc_xmittime and tc_calc_xmitsize round from double to int three times — once when they call tc_core_time2tick / tc_core_tick2time (whose argument is int), once when those functions return (their return value is int), and then finally when the tc_calc_* functions return. This leads to extremely granular and inaccurate conversions. 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 bytes get encoded as 0 ticks; all values between 1000 and 1999 bytes get encoded as 15 ticks (equivalent to 960 bytes); all values between 2000 and 2999 bytes as 31 ticks (1984 bytes); etc. The patch changes the code so these calculations are done internally 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. Can you let me know the desired style for commit messages — how much of this explanation should be in it? I can submit a v3 with the desired explanation in the commit message. Thanks! Jonathan Lennox
On 2/24/25 9:36 AM, Jonathan Lennox wrote: #### > The problem is that tc_calc_xmittime and tc_calc_xmitsize round from > double to int three times — once when they call tc_core_time2tick / tc_core_tick2time > (whose argument is int), once when those functions return (their return value is int), > and then finally when the tc_calc_* functions return. This leads to extremely > granular and inaccurate conversions. > > 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 bytes get encoded as 0 ticks; all values between > 1000 and 1999 bytes get encoded as 15 ticks (equivalent to 960 bytes); all > values between 2000 and 2999 bytes as 31 ticks (1984 bytes); etc. > > The patch changes the code so these calculations are done internally 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. #### put that blurb as the commit message and re-send. Thanks, > > > Can you let me know the desired style for commit messages — how much of this > explanation should be in it? I can submit a v3 with the desired explanation in > the commit message. > > Thanks! > > Jonathan Lennox >
diff --git a/tc/tc_core.c b/tc/tc_core.c index 37547e9b..32fd094f 100644 --- a/tc/tc_core.c +++ b/tc/tc_core.c @@ -23,12 +23,12 @@ 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(double time) { return time * tick_in_usec; } -unsigned int tc_core_tick2time(unsigned int tick) +double tc_core_tick2time(double tick) { return tick / tick_in_usec; } @@ -45,7 +45,7 @@ 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(TIME_UNITS_PER_SEC*((double)size/(double)rate))); } unsigned int tc_calc_xmitsize(__u64 rate, unsigned int ticks) diff --git a/tc/tc_core.h b/tc/tc_core.h index 7a986ac2..c0fb7481 100644 --- a/tc/tc_core.h +++ b/tc/tc_core.h @@ -12,7 +12,7 @@ enum link_layer { }; -unsigned tc_core_tick2time(unsigned tick); +double tc_core_tick2time(double tick); unsigned tc_core_time2ktime(unsigned time); unsigned tc_core_ktime2time(unsigned ktime); unsigned tc_calc_xmittime(__u64 rate, unsigned size);
Signed-off-by: Jonathan Lennox <jonathan.lennox@8x8.com> --- tc/tc_core.c | 6 +++--- tc/tc_core.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)