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

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

[Jonathan Lennox]

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

---
tc/tc_core.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

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

/*

Re: [PATCH iproute2] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize.

[Stephen Hemminger]

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

Re: [PATCH iproute2] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize.

[Jonathan Lennox]

> 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.

[PATCH iproute2 v2] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize.

[Jonathan Lennox]

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

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

Re: [PATCH iproute2 v2] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize.

[David Ahern]

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

Re: [PATCH iproute2 v2] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize.

[Jonathan Lennox]

> 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

Re: [PATCH iproute2 v2] tc: Fix rounding in tc_calc_xmittime and tc_calc_xmitsize.

[David Ahern]

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
> 

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

[Jonathan Lennox]

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

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

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

[David Ahern]

On 2/24/25 11:42 AM, Jonathan Lennox wrote:
> 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);


git am (and patch for that matter) is not liking your patch. Please make
sure the patch is against iproute2-next and top of tree.

You should also try sending the patch to yourself, saving to a file and
applying using `git am`.

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

[Jonathan Lennox]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-tc-Fix-rounding-in-tc_calc_xmittime-and-tc_calc_xmit.patch --]
[-- Type: text/x-patch; name="0001-tc-Fix-rounding-in-tc_calc_xmittime-and-tc_calc_xmit.patch", Size: 1215 bytes --]

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

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

[Jonathan Lennox]

> On Feb 26, 2025, at 11:06 AM, David Ahern <dsahern@kernel.org> wrote:
> 
> git am (and patch for that matter) is not liking your patch. Please make
> sure the patch is against iproute2-next and top of tree.
> 
> You should also try sending the patch to yourself, saving to a file and
> applying using `git am`.

Sorry, my mailer was mangling the patch (adding quoted-printable) and I couldn’t get
git send-email to work with my corporate e-mail. I’ve resent it from my personal gmail account.

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Pedro Tammela]

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

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

[Jonathan Lennox]

> 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?

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

[Pedro Tammela]

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

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

[Jonathan Lennox]

> 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?

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

[Pedro Tammela]

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.

[PATCH net-next] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Jonathan Lennox]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]


Before tc's recent change to fix rounding errors, several tests which
specified a burst size of "1m" would translate back to being 1048574
bytes (2b less than 1Mb).  sprint_size prints this as "1024Kb".

With the tc fix, the burst size is instead correctly reported as
1048576 bytes (precisely 1Mb), which sprint_size prints as "1Mb".

This updates the expected output in the tests' matchPattern values
accordingly.

Signed-off-by: Jonathan Lennox <jonathan.lennox@8x8.com>
---
 .../selftests/tc-testing/tc-tests/actions/police.json  | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-tc-tests-Update-tc-police-action-tests-for-tc-buffer.patch --]
[-- Type: text/x-patch; name="0001-tc-tests-Update-tc-police-action-tests-for-tc-buffer.patch", Size: 2864 bytes --]

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
index dd8109768f8f..ae31dbeb45d8 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
@@ -689,7 +689,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m continue index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action police index 1",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action continue",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1Mb mtu 2Kb action continue",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -716,7 +716,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m drop index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action drop",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1Mb mtu 2Kb action drop",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -743,7 +743,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m ok index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action pass",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1Mb mtu 2Kb action pass",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -770,7 +770,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m reclassify index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action police index 1",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1Mb mtu 2Kb action reclassify",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -797,7 +797,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m pipe index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action pipe",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1Mb mtu 2Kb action pipe",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"

Re: [PATCH net-next] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Paolo Abeni]

Adding the relevant maintainers and Pedro.

On 3/4/25 8:38 PM, Jonathan Lennox wrote:
> Before tc's recent change to fix rounding errors, several tests which
> specified a burst size of "1m" would translate back to being 1048574
> bytes (2b less than 1Mb).  sprint_size prints this as "1024Kb".
> 
> With the tc fix, the burst size is instead correctly reported as
> 1048576 bytes (precisely 1Mb), which sprint_size prints as "1Mb".
> 
> This updates the expected output in the tests' matchPattern values
> accordingly.
> 
> Signed-off-by: Jonathan Lennox <jonathan.lennox@8x8.com>

This is MIME multipart message, please send plaintext message instead
(PW surprisingly digest it, but not my tools).

AFAICS this fix will break the tests when running all version of
iproute2 except the upcoming one. I think this is not good enough; you
should detect the tc tool version and update expected output accordingly.

If that is not possible, I think it would be better to simply revert the
TC commit.

Thanks,

Paolo

Re: [PATCH net-next] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Jakub Kicinski]

On Tue, 11 Mar 2025 10:16:14 +0100 Paolo Abeni wrote:
> AFAICS this fix will break the tests when running all version of
> iproute2 except the upcoming one. I think this is not good enough; you
> should detect the tc tool version and update expected output accordingly.
> 
> If that is not possible, I think it would be better to simply revert the
> TC commit.

Alternatively since it's a regex match, maybe we could accept both?

-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1Mb|1024Kb) mtu 2Kb action reclassify",

? Not sure which option is most "correct" from TDC's perspective..

Re: [PATCH net-next] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Jamal Hadi Salim]

On Tue, Mar 11, 2025 at 5:49 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 11 Mar 2025 10:16:14 +0100 Paolo Abeni wrote:
> > AFAICS this fix will break the tests when running all version of
> > iproute2 except the upcoming one. I think this is not good enough; you
> > should detect the tc tool version and update expected output accordingly.
> >
> > If that is not possible, I think it would be better to simply revert the
> > TC commit.
>
> Alternatively since it's a regex match, maybe we could accept both?
>
> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1Mb|1024Kb) mtu 2Kb action reclassify",
>
> ? Not sure which option is most "correct" from TDC's perspective..

It should work. Paolo's suggestion is also reasonable.

cheers,
jamal

[PATCH net-next v2] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Jonathan Lennox]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]


Before tc's recent change to fix rounding errors, several tests which
specified a burst size of "1m" would translate back to being 1048574
bytes (2b less than 1Mb).  sprint_size prints this as "1024Kb".

With the tc fix, the burst size is instead correctly reported as
1048576 bytes (precisely 1Mb), which sprint_size prints as "1Mb".

This updates the expected output in the tests' matchPattern values
to accept either the old or the new output.

Signed-off-by: Jonathan Lennox <jonathan.lennox@8x8.com>
---
 .../selftests/tc-testing/tc-tests/actions/police.json  | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-tc-tests-Update-tc-police-action-tests-for-tc-buffer.patch --]
[-- Type: text/x-patch; name="0001-tc-tests-Update-tc-police-action-tests-for-tc-buffer.patch", Size: 2909 bytes --]

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
index dd8109768f8f..5596f4df0e9f 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
@@ -689,7 +689,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m continue index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action police index 1",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action continue",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action continue",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -716,7 +716,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m drop index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action drop",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action drop",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -743,7 +743,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m ok index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action pass",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action pass",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -770,7 +770,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m reclassify index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action police index 1",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action reclassify",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -797,7 +797,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m pipe index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action pipe",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action pipe",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"

Re: [PATCH net-next] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Jonathan Lennox]

I've submitted an updated version of the patch with the regex accepting both the old and new versions.

> On Mar 11, 2025, at 7:15 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> 
> On Tue, Mar 11, 2025 at 5:49 AM Jakub Kicinski <kuba@kernel.org> wrote:
>> 
>> On Tue, 11 Mar 2025 10:16:14 +0100 Paolo Abeni wrote:
>>> AFAICS this fix will break the tests when running all version of
>>> iproute2 except the upcoming one. I think this is not good enough; you
>>> should detect the tc tool version and update expected output accordingly.
>>> 
>>> If that is not possible, I think it would be better to simply revert the
>>> TC commit.
>> 
>> Alternatively since it's a regex match, maybe we could accept both?
>> 
>> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
>> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1Mb|1024Kb) mtu 2Kb action reclassify",
>> 
>> ? Not sure which option is most "correct" from TDC's perspective..
> 
> It should work. Paolo's suggestion is also reasonable.
> 
> cheers,
> jamal

[PATCH net-next v3] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Jonathan Lennox]

Before tc's recent change to fix rounding errors, several tests which
specified a burst size of "1m" would translate back to being 1048574
bytes (2b less than 1Mb).  sprint_size prints this as "1024Kb".

With the tc fix, the burst size is instead correctly reported as
1048576 bytes (precisely 1Mb), which sprint_size prints as "1Mb".

This updates the expected output in the tests' matchPattern values
to accept either the old or the new output.

Signed-off-by: Jonathan Lennox <jonathan.lennox@8x8.com>
---
 .../selftests/tc-testing/tc-tests/actions/police.json  | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
index dd8109768f8f..5596f4df0e9f 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
@@ -689,7 +689,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m continue index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action police index 1",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action continue",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action continue",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -716,7 +716,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m drop index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action drop",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action drop",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -743,7 +743,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m ok index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action pass",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action pass",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -770,7 +770,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m reclassify index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action police index 1",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action reclassify",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
@@ -797,7 +797,7 @@
         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m pipe index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions ls action police",
-        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action pipe",
+        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action pipe",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action police"
-- 
2.34.1

Re: [PATCH net-next v3] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Jonathan Lennox]

And with this I’ve resent it not as a MIME attachment, sorry about that.

> On Mar 12, 2025, at 1:48 PM, Jonathan Lennox <jonathan.lennox42@gmail.com> wrote:
> 
> Before tc's recent change to fix rounding errors, several tests which
> specified a burst size of "1m" would translate back to being 1048574
> bytes (2b less than 1Mb).  sprint_size prints this as "1024Kb".
> 
> With the tc fix, the burst size is instead correctly reported as
> 1048576 bytes (precisely 1Mb), which sprint_size prints as "1Mb".
> 
> This updates the expected output in the tests' matchPattern values
> to accept either the old or the new output.
> 
> Signed-off-by: Jonathan Lennox <jonathan.lennox@8x8.com>
> ---
> .../selftests/tc-testing/tc-tests/actions/police.json  | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
> index dd8109768f8f..5596f4df0e9f 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
> @@ -689,7 +689,7 @@
>         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m continue index 1",
>         "expExitCode": "0",
>         "verifyCmd": "$TC actions get action police index 1",
> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action continue",
> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action continue",
>         "matchCount": "1",
>         "teardown": [
>             "$TC actions flush action police"
> @@ -716,7 +716,7 @@
>         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m drop index 1",
>         "expExitCode": "0",
>         "verifyCmd": "$TC actions ls action police",
> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action drop",
> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action drop",
>         "matchCount": "1",
>         "teardown": [
>             "$TC actions flush action police"
> @@ -743,7 +743,7 @@
>         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m ok index 1",
>         "expExitCode": "0",
>         "verifyCmd": "$TC actions ls action police",
> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action pass",
> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action pass",
>         "matchCount": "1",
>         "teardown": [
>             "$TC actions flush action police"
> @@ -770,7 +770,7 @@
>         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m reclassify index 1",
>         "expExitCode": "0",
>         "verifyCmd": "$TC actions get action police index 1",
> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action reclassify",
>         "matchCount": "1",
>         "teardown": [
>             "$TC actions flush action police"
> @@ -797,7 +797,7 @@
>         "cmdUnderTest": "$TC actions add action police rate 7mbit burst 1m pipe index 1",
>         "expExitCode": "0",
>         "verifyCmd": "$TC actions ls action police",
> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action pipe",
> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1024Kb|1Mb) mtu 2Kb action pipe",
>         "matchCount": "1",
>         "teardown": [
>             "$TC actions flush action police"
> -- 
> 2.34.1
> 

Re: [PATCH net-next v3] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Wed, 12 Mar 2025 17:48:04 +0000 you wrote:
> Before tc's recent change to fix rounding errors, several tests which
> specified a burst size of "1m" would translate back to being 1048574
> bytes (2b less than 1Mb).  sprint_size prints this as "1024Kb".
> 
> With the tc fix, the burst size is instead correctly reported as
> 1048576 bytes (precisely 1Mb), which sprint_size prints as "1Mb".
> 
> [...]

Here is the summary with links:
  - [net-next,v3] tc-tests: Update tc police action tests for tc buffer size rounding fixes.
    https://git.kernel.org/netdev/net-next/c/756f88ff9c6a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Jakub Kicinski]

On Tue, 11 Mar 2025 07:15:26 -0400 Jamal Hadi Salim wrote:
> > On Tue, 11 Mar 2025 10:16:14 +0100 Paolo Abeni wrote:  
> > > AFAICS this fix will break the tests when running all version of
> > > iproute2 except the upcoming one. I think this is not good enough; you
> > > should detect the tc tool version and update expected output accordingly.
> > >
> > > If that is not possible, I think it would be better to simply revert the
> > > TC commit.  
> >
> > Alternatively since it's a regex match, maybe we could accept both?
> >
> > -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
> > +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1Mb|1024Kb) mtu 2Kb action reclassify",
> >
> > ? Not sure which option is most "correct" from TDC's perspective..  
> 
> It should work. Paolo's suggestion is also reasonable.

Sorry for the ping but where are we with this? TDC has been "red" for
the last 3 weeks, would be really neat to get a clear run before we
ship the net-next tree to Linus :(

Re: [PATCH net-next] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Pedro Tammela]

On 26/03/2025 08:39, Jakub Kicinski wrote:
> On Tue, 11 Mar 2025 07:15:26 -0400 Jamal Hadi Salim wrote:
>>> On Tue, 11 Mar 2025 10:16:14 +0100 Paolo Abeni wrote:
>>>> AFAICS this fix will break the tests when running all version of
>>>> iproute2 except the upcoming one. I think this is not good enough; you
>>>> should detect the tc tool version and update expected output accordingly.
>>>>
>>>> If that is not possible, I think it would be better to simply revert the
>>>> TC commit.
>>>
>>> Alternatively since it's a regex match, maybe we could accept both?
>>>
>>> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst 1024Kb mtu 2Kb action reclassify",
>>> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 7Mbit burst (1Mb|1024Kb) mtu 2Kb action reclassify",
>>>
>>> ? Not sure which option is most "correct" from TDC's perspective..
>>
>> It should work. Paolo's suggestion is also reasonable.
> 
> Sorry for the ping but where are we with this? TDC has been "red" for
> the last 3 weeks, would be really neat to get a clear run before we
> ship the net-next tree to Linus :(

Jonathan's issue is solved.
A new one popped in iproute-2:
https://web.git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=afbfd2f2b0a633d068990775f8e1b73b8ee83733

Changed the nat's "default" ip address from 0.0.0.0/32 to 0.0.0.0/0,
which makes tdc fail :)

https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/50205/1-tdc-sh/stdout#L2213

Re: [PATCH net-next] tc-tests: Update tc police action tests for tc buffer size rounding fixes.

[Pedro Tammela]

On 26/03/2025 16:04, Pedro Tammela wrote:
> On 26/03/2025 08:39, Jakub Kicinski wrote:
>> On Tue, 11 Mar 2025 07:15:26 -0400 Jamal Hadi Salim wrote:
>>>> On Tue, 11 Mar 2025 10:16:14 +0100 Paolo Abeni wrote:
>>>>> AFAICS this fix will break the tests when running all version of
>>>>> iproute2 except the upcoming one. I think this is not good enough; you
>>>>> should detect the tc tool version and update expected output 
>>>>> accordingly.
>>>>>
>>>>> If that is not possible, I think it would be better to simply 
>>>>> revert the
>>>>> TC commit.
>>>>
>>>> Alternatively since it's a regex match, maybe we could accept both?
>>>>
>>>> -        "matchPattern": "action order [0-9]*:  police 0x1 rate 
>>>> 7Mbit burst 1024Kb mtu 2Kb action reclassify",
>>>> +        "matchPattern": "action order [0-9]*:  police 0x1 rate 
>>>> 7Mbit burst (1Mb|1024Kb) mtu 2Kb action reclassify",
>>>>
>>>> ? Not sure which option is most "correct" from TDC's perspective..
>>>
>>> It should work. Paolo's suggestion is also reasonable.
>>
>> Sorry for the ping but where are we with this? TDC has been "red" for
>> the last 3 weeks, would be really neat to get a clear run before we
>> ship the net-next tree to Linus :(
> 
> Jonathan's issue is solved.
> A new one popped in iproute-2:
> https://web.git.kernel.org/pub/scm/network/iproute2/iproute2.git/ 
> commit/?id=afbfd2f2b0a633d068990775f8e1b73b8ee83733

I pasted the wrong commit, it should be this one:
https://web.git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=667817b4c34944175deaf6ca9aa3afdf5b668fc5

> 
> Changed the nat's "default" ip address from 0.0.0.0/32 to 0.0.0.0/0,
> which makes tdc fail :)
> 
> https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/50205/1- 
> tdc-sh/stdout#L2213
>