diff mbox series

[iproute2,v3] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize.

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

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Jonathan Lennox Feb. 26, 2025, 6:53 p.m. UTC
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(-)

Comments

patchwork-bot+netdevbpf@kernel.org Feb. 28, 2025, 3:50 p.m. UTC | #1
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!
Pedro Tammela March 3, 2025, 6:39 p.m. UTC | #2
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
Jonathan Lennox March 3, 2025, 7:43 p.m. UTC | #3
> 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?
Pedro Tammela March 3, 2025, 8:35 p.m. UTC | #4
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
Jonathan Lennox March 3, 2025, 10:13 p.m. UTC | #5
> 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?
Pedro Tammela March 4, 2025, 5:51 p.m. UTC | #6
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 mbox series

Patch

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);