Message ID | 20250226185321.3243593-1-jonathan.lennox@8x8.com (mailing list archive) |
---|---|
State | Accepted |
Commit | d947f365602b30657d1b797e7464000d0ab88d5a |
Delegated to: | David Ahern |
Headers | show |
Series | [iproute2,v3] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Not a local patch |
Hello: This patch was applied to iproute2/iproute2-next.git (main) by David Ahern <dsahern@kernel.org>: On Wed, 26 Feb 2025 18:53:21 +0000 you wrote: > Currently, 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. > > [...] Here is the summary with links: - [iproute2,v3] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=d947f365602b You are awesome, thank you!
On 28/02/2025 12:50, patchwork-bot+netdevbpf@kernel.org wrote: > Hello: > > This patch was applied to iproute2/iproute2-next.git (main) > by David Ahern <dsahern@kernel.org>: > > On Wed, 26 Feb 2025 18:53:21 +0000 you wrote: >> Currently, 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. >> >> [...] > > Here is the summary with links: > - [iproute2,v3] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. > https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=d947f365602b > > You are awesome, thank you! Hi, This patch broke tdc: https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/17084/1-tdc-sh/stdout#L2323
> On Mar 3, 2025, at 1:39 PM, Pedro Tammela <pctammela@mojatatu.com> wrote: > > On 28/02/2025 12:50, patchwork-bot+netdevbpf@kernel.org wrote: >> Hello: >> This patch was applied to iproute2/iproute2-next.git (main) >> by David Ahern <dsahern@kernel.org>: >> On Wed, 26 Feb 2025 18:53:21 +0000 you wrote: >>> Currently, 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. >>> >>> [...] >> Here is the summary with links: >> - [iproute2,v3] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. >> https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=d947f365602b >> You are awesome, thank you! > > Hi, > > This patch broke tdc: > https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/17084/1-tdc-sh/stdout#L2323 > I’m afraid I’m not familiar with this test suite — can you point me at where it lives, what it’s doing, and what the expected output is?
On 03/03/2025 16:43, Jonathan Lennox wrote: > > >> On Mar 3, 2025, at 1:39 PM, Pedro Tammela <pctammela@mojatatu.com> wrote: >> >> On 28/02/2025 12:50, patchwork-bot+netdevbpf@kernel.org wrote: >>> Hello: >>> This patch was applied to iproute2/iproute2-next.git (main) >>> by David Ahern <dsahern@kernel.org>: >>> On Wed, 26 Feb 2025 18:53:21 +0000 you wrote: >>>> Currently, 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. >>>> >>>> [...] >>> Here is the summary with links: >>> - [iproute2,v3] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. >>> https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=d947f365602b >>> You are awesome, thank you! >> >> Hi, >> >> This patch broke tdc: >> https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/17084/1-tdc-sh/stdout#L2323 >> > > I’m afraid I’m not familiar with this test suite — can you point me at where it lives, what it’s doing, > and what the expected output is? tdc lives here: https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/tc-testing The broken tests are here: https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/tc-testing/tc-tests/actions/police.json Unrelated but useful is to use tools like vng to test your changes to tdc very quickly: https://github.com/arighi/virtme-ng
> On Mar 3, 2025, at 3:35 PM, Pedro Tammela <pctammela@mojatatu.com> wrote: > > On 03/03/2025 16:43, Jonathan Lennox wrote: >>> On Mar 3, 2025, at 1:39 PM, Pedro Tammela <pctammela@mojatatu.com> wrote: >>> >>> On 28/02/2025 12:50, patchwork-bot+netdevbpf@kernel.org wrote: >>>> Hello: >>>> This patch was applied to iproute2/iproute2-next.git (main) >>>> by David Ahern <dsahern@kernel.org>: >>>> On Wed, 26 Feb 2025 18:53:21 +0000 you wrote: >>>>> Currently, 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. >>>>> >>>>> [...] >>>> Here is the summary with links: >>>> - [iproute2,v3] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. >>>> https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=d947f365602b >>>> You are awesome, thank you! >>> >>> Hi, >>> >>> This patch broke tdc: >>> https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/17084/1-tdc-sh/stdout#L2323 >>> >> I’m afraid I’m not familiar with this test suite — can you point me at where it lives, what it’s doing, >> and what the expected output is? > > tdc lives here: > https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/tc-testing > > The broken tests are here: > https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/tc-testing/tc-tests/actions/police.json > > Unrelated but useful is to use tools like vng to test your changes to tdc very quickly: > https://github.com/arighi/virtme-ng What’s happening is that when the tests are verifying the test result, with "$TC actions get action police index 1” and the like after the command "$TC actions add action police rate 7mbit burst 1m pipe index 1” and the like, the burst size is now being printed as “1Mb” rather than as “1024Kb”. (And the same for the subsequent tests.) This is because, due to the rounding errors the patch fixed, the precise value being printed for the burst was previously 1048574 (2b less than 1Mb) which sprint_size prints as “1024Kb”. The value being printed is now 1048576 (exactly 1Mb). I would argue that the new output is more correct, given the input “1m”, and the test cases should be updated. What is the proper procedure for submitting a patch for the tests? Does it also go to this mailing list?
On 03/03/2025 19:13, Jonathan Lennox wrote: >> On Mar 3, 2025, at 3:35 PM, Pedro Tammela <pctammela@mojatatu.com> wrote: >> >> On 03/03/2025 16:43, Jonathan Lennox wrote: >>>> On Mar 3, 2025, at 1:39 PM, Pedro Tammela <pctammela@mojatatu.com> wrote: >>>> >>>> On 28/02/2025 12:50, patchwork-bot+netdevbpf@kernel.org wrote: >>>>> Hello: >>>>> This patch was applied to iproute2/iproute2-next.git (main) >>>>> by David Ahern <dsahern@kernel.org>: >>>>> On Wed, 26 Feb 2025 18:53:21 +0000 you wrote: >>>>>> Currently, 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. >>>>>> >>>>>> [...] >>>>> Here is the summary with links: >>>>> - [iproute2,v3] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize. >>>>> https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=d947f365602b >>>>> You are awesome, thank you! >>>> >>>> Hi, >>>> >>>> This patch broke tdc: >>>> https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/17084/1-tdc-sh/stdout#L2323 >>>> >>> I’m afraid I’m not familiar with this test suite — can you point me at where it lives, what it’s doing, >>> and what the expected output is? >> >> tdc lives here: >> https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/tc-testing >> >> The broken tests are here: >> https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/tc-testing/tc-tests/actions/police.json >> >> Unrelated but useful is to use tools like vng to test your changes to tdc very quickly: >> https://github.com/arighi/virtme-ng > > What’s happening is that when the tests are verifying the test result, with > "$TC actions get action police index 1” > > and the like after the command > "$TC actions add action police rate 7mbit burst 1m pipe index 1” > > and the like, the burst size is now being printed as “1Mb” rather than as > “1024Kb”. (And the same for the subsequent tests.) > > This is because, due to the rounding errors the patch fixed, the precise > value being printed for the burst was previously 1048574 (2b less than > 1Mb) which sprint_size prints as “1024Kb”. The value being printed is now > 1048576 (exactly 1Mb). > > I would argue that the new output is more correct, given the input “1m”, and > the test cases should be updated. Makes sense > > What is the proper procedure for submitting a patch for the tests? Does it > also go to this mailing list? Yes: https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt net-next is the tree you should submit to.
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);
Currently, 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. 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(-)