[PATCH iproute2 1/2] tc_core: Use double in tc_core_time2tick()

[PATCH iproute2 1/2] tc_core: Use double in tc_core_time2tick()

[Jarek Poplawski]

Change 'time' parameters of tc_core_time2tick() and tc_core_time2big()
from unsigned to double. It is especially needed to use in
tc_calc_rtable() for kernels with increased psched ticks resolution,
but even without this, it looks reasonable to avoid rounding here.

Reported-by: Antonio Almeida <vexwek@gmail.com>
Tested-by: Antonio Almeida <vexwek@gmail.com>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 tc/tc_core.c |   10 +++++-----
 tc/tc_core.h |    4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tc/tc_core.c b/tc/tc_core.c
index 9a0ff39..6d74287 100644
--- a/tc/tc_core.c
+++ b/tc/tc_core.c
@@ -27,18 +27,18 @@
 static double tick_in_usec = 1;
 static double clock_factor = 1;
 
-int tc_core_time2big(unsigned time)
+int tc_core_time2big(double time)
 {
-	__u64 t = time;
+	__u64 t;
 
-	t *= tick_in_usec;
+	t = time * tick_in_usec + 0.5;
 	return (t >> 32) != 0;
 }
 
 
-unsigned tc_core_time2tick(unsigned time)
+unsigned tc_core_time2tick(double time)
 {
-	return time*tick_in_usec;
+	return time * tick_in_usec + 0.5;
 }
 
 unsigned tc_core_tick2time(unsigned tick)
diff --git a/tc/tc_core.h b/tc/tc_core.h
index 5a693ba..0ac65aa 100644
--- a/tc/tc_core.h
+++ b/tc/tc_core.h
@@ -13,8 +13,8 @@ enum link_layer {
 };
 
 
-int  tc_core_time2big(unsigned time);
-unsigned tc_core_time2tick(unsigned time);
+int  tc_core_time2big(double time);
+unsigned tc_core_time2tick(double time);
 unsigned tc_core_tick2time(unsigned tick);
 unsigned tc_core_time2ktime(unsigned time);
 unsigned tc_core_ktime2time(unsigned ktime);

Re: [PATCH iproute2 1/2] tc_core: Use double in tc_core_time2tick()

[David Miller]

This one should be "2/2" not "1/2" right? :-)

Re: [PATCH iproute2 1/2] tc_core: Use double in tc_core_time2tick()

[Jarek Poplawski]

On Tue, Jun 09, 2009 at 01:08:36AM -0700, David Miller wrote:
> 
> This one should be "2/2" not "1/2" right? :-)

Sure! Plus a different subject. I've just resent.

Thanks for noticing this,
Jarek P.

Re: [PATCH iproute2 1/2] tc_core: Use double in tc_core_time2tick()

[Patrick McHardy]

Jarek Poplawski wrote:
> Change 'time' parameters of tc_core_time2tick() and tc_core_time2big()
> from unsigned to double. It is especially needed to use in
> tc_calc_rtable() for kernels with increased psched ticks resolution,
> but even without this, it looks reasonable to avoid rounding here.

I would prefer to keep using integers, which are a lot easier to
analyze for rounding errors and overflows, besides having constant
granularity.

> -unsigned tc_core_time2tick(unsigned time)
> +unsigned tc_core_time2tick(double time)
>  {
> -	return time*tick_in_usec;
> +	return time * tick_in_usec + 0.5;

ceil()?

>  }

It seems inconsistent to have the time2tick() function take a
double, but return an unsigned from tick2time(). If we're going
to change this, please keep them symetrical (you could even use
floor() in tick2time() to make it more explicit).

Re: [PATCH iproute2 1/2] tc_core: Use double in tc_core_time2tick()

[Jarek Poplawski]

On Tue, Jun 09, 2009 at 04:03:16PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> Change 'time' parameters of tc_core_time2tick() and tc_core_time2big()
>> from unsigned to double. It is especially needed to use in
>> tc_calc_rtable() for kernels with increased psched ticks resolution,
>> but even without this, it looks reasonable to avoid rounding here.
>
> I would prefer to keep using integers, which are a lot easier to
> analyze for rounding errors and overflows, besides having constant
> granularity.
>
>> -unsigned tc_core_time2tick(unsigned time)
>> +unsigned tc_core_time2tick(double time)
>>  {
>> -	return time*tick_in_usec;
>> +	return time * tick_in_usec + 0.5;
>
> ceil()?

OK.

>
>>  }
>
> It seems inconsistent to have the time2tick() function take a
> double, but return an unsigned from tick2time(). If we're going
> to change this, please keep them symetrical (you could even use
> floor() in tick2time() to make it more explicit).

Let's see how it's used (mainly) in tc_calc_xmittime() e.g. for
Antonio's mostly 800byte and 555Mbit rate:

tc_core_time2tick(TIME_UNITS_PER_SEC * (double)size/rate)
tc_core_time2tick(1000000 * (double) 800/69375000)
tc_core_time2tick(11.53153)
tc_core_time2tick(11)

and let's say it's multiplied by 10. 5 ticks really make a difference.
Does it make sense to round it here before full calculations?

tc_core_tick2time() is used differently; it usually gets integers, to
show them as time. But even if not so, we have division here. So, I'm
not sure it's really needed, but if you confirm I'll do it in a
separate patch.

Thanks,
Jarek P.
--------------------> take 2
Change 'time' parameters of tc_core_time2tick() and tc_core_time2big()
from unsigned to double. It is especially needed to use in
tc_calc_rtable() for kernels with increased psched ticks resolution,
but even without this, it looks reasonable to avoid rounding here.
Additionally rounding with ceil() is used (it's safer to stay below
configured rate than going beyond limits).

With feedback from: Patrick McHardy <kaber@trash.net>

Reported-by: Antonio Almeida <vexwek@gmail.com>
Tested-by: Antonio Almeida <vexwek@gmail.com>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 tc/tc_core.c |   10 +++++-----
 tc/tc_core.h |    4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tc/tc_core.c b/tc/tc_core.c
index 9a0ff39..c9c76b4 100644
--- a/tc/tc_core.c
+++ b/tc/tc_core.c
@@ -27,18 +27,18 @@
 static double tick_in_usec = 1;
 static double clock_factor = 1;
 
-int tc_core_time2big(unsigned time)
+int tc_core_time2big(double time)
 {
-	__u64 t = time;
+	__u64 t;
 
-	t *= tick_in_usec;
+	t = ceil(time * tick_in_usec);
 	return (t >> 32) != 0;
 }
 
 
-unsigned tc_core_time2tick(unsigned time)
+unsigned tc_core_time2tick(double time)
 {
-	return time*tick_in_usec;
+	return ceil(time * tick_in_usec);
 }
 
 unsigned tc_core_tick2time(unsigned tick)
diff --git a/tc/tc_core.h b/tc/tc_core.h
index 5a693ba..0ac65aa 100644
--- a/tc/tc_core.h
+++ b/tc/tc_core.h
@@ -13,8 +13,8 @@ enum link_layer {
 };
 
 
-int  tc_core_time2big(unsigned time);
-unsigned tc_core_time2tick(unsigned time);
+int  tc_core_time2big(double time);
+unsigned tc_core_time2tick(double time);
 unsigned tc_core_tick2time(unsigned tick);
 unsigned tc_core_time2ktime(unsigned time);
 unsigned tc_core_ktime2time(unsigned ktime);

Re: [PATCH iproute2 1/2] tc_core: Use double in tc_core_time2tick()

[Jarek Poplawski]

On Tue, Jun 09, 2009 at 11:54:25PM +0200, Jarek Poplawski wrote:
> On Tue, Jun 09, 2009 at 04:03:16PM +0200, Patrick McHardy wrote:
...
> > It seems inconsistent to have the time2tick() function take a
> > double, but return an unsigned from tick2time(). If we're going
> > to change this, please keep them symetrical (you could even use
> > floor() in tick2time() to make it more explicit).
...
> tc_core_tick2time() is used differently; it usually gets integers, to
> show them as time. But even if not so, we have division here. So, I'm
> not sure it's really needed, but if you confirm I'll do it in a
> separate patch.

OOPS! I argued for double as argument, but you're right tick2time()
should return double for consistency. I'll send a separate patch.

Sorry,
Jarek P.

[PATCH iproute2 3/2] tc_core: Return double from tc_core_tick2time()

[Jarek Poplawski]

Hmm... I'm not sure this floor() is needed here, but maybe I'll get it
tomorrow... 

Thanks,
Jarek P.
-------------------------->
Patrick McHardy wrote:
> It seems inconsistent to have the time2tick() function take a
> double, but return an unsigned from tick2time(). If we're going
> to change this, please keep them symetrical (you could even use
> floor() in tick2time() to make it more explicit).

This patch removes this inconsistency.

Suggested-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

diff -Nurp a/tc/tc_core.c b/tc/tc_core.c
--- a/tc/tc_core.c	2009-06-09 22:59:42.000000000 +0200
+++ b/tc/tc_core.c	2009-06-10 00:26:10.000000000 +0200
@@ -41,9 +41,9 @@ unsigned tc_core_time2tick(double time)
 	return ceil(time * tick_in_usec);
 }
 
-unsigned tc_core_tick2time(unsigned tick)
+double tc_core_tick2time(unsigned tick)
 {
-	return tick/tick_in_usec;
+	return floor(tick / tick_in_usec);
 }
 
 unsigned tc_core_time2ktime(unsigned time)
diff -Nurp a/tc/tc_core.h b/tc/tc_core.h
--- a/tc/tc_core.h	2009-06-09 22:57:17.000000000 +0200
+++ b/tc/tc_core.h	2009-06-10 00:26:58.000000000 +0200
@@ -15,7 +15,7 @@ enum link_layer {
 
 int  tc_core_time2big(double time);
 unsigned tc_core_time2tick(double time);
-unsigned tc_core_tick2time(unsigned tick);
+double tc_core_tick2time(unsigned tick);
 unsigned tc_core_time2ktime(unsigned time);
 unsigned tc_core_ktime2time(unsigned ktime);
 unsigned tc_calc_xmittime(unsigned rate, unsigned size);

Re: [PATCH iproute2 3/2] tc_core: Return double from tc_core_tick2time()

[Patrick McHardy]

Jarek Poplawski wrote:
> Hmm... I'm not sure this floor() is needed here, but maybe I'll get it
> tomorrow... 

Its not. It was mainly a suggestion to make the symetry more explicit.
I hope gcc understands :)

[PATCH iproute2 3/2 v2] tc_core: Return double from tc_core_tick2time()

[Jarek Poplawski]

On Wed, Jun 10, 2009 at 01:04:36AM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> Hmm... I'm not sure this floor() is needed here, but maybe I'll get it
>> tomorrow... 
>
> Its not. It was mainly a suggestion to make the symetry more explicit.
> I hope gcc understands :)

If so, then maybe let's reconsider previous example:

> Let's see how it's used (mainly) in tc_calc_xmittime() e.g. for
> Antonio's mostly 800byte and 555Mbit rate:
> 
> tc_core_time2tick(TIME_UNITS_PER_SEC * (double)size/rate)
> tc_core_time2tick(1000000 * (double) 800/69375000)
> tc_core_time2tick(11.53153)

Let's say it returns 116 (after x10 with ceil()). Then, going backwards:

tc_calc_xmitsize()
{
	return ((double) 69375000 * tc_core_tick2time(116)) / TIME_UNITS_PER_SEC;
}

would give us: 763 after /10 with floor(), and 804 without floor().
The latter looks nicer to me but I leave the choice to you or Stephen.

Thanks,
Jarek P.
--------------------------> take 2
Patrick McHardy wrote:
> It seems inconsistent to have the time2tick() function take a
> double, but return an unsigned from tick2time(). If we're going
> to change this, please keep them symetrical (you could even use
> floor() in tick2time() to make it more explicit).

This patch removes this inconsistency.

Suggested-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

diff -Nurp a/tc/tc_core.c b/tc/tc_core.c
--- a/tc/tc_core.c	2009-06-10 07:18:30.000000000 +0000
+++ b/tc/tc_core.c	2009-06-10 07:20:38.000000000 +0000
@@ -41,9 +41,9 @@ unsigned tc_core_time2tick(double time)
 	return ceil(time * tick_in_usec);
 }
 
-unsigned tc_core_tick2time(unsigned tick)
+double tc_core_tick2time(unsigned tick)
 {
-	return tick/tick_in_usec;
+	return tick / tick_in_usec;
 }
 
 unsigned tc_core_time2ktime(unsigned time)
diff -Nurp a/tc/tc_core.h b/tc/tc_core.h
--- a/tc/tc_core.h	2009-06-10 07:17:34.000000000 +0000
+++ b/tc/tc_core.h	2009-06-10 07:19:41.000000000 +0000
@@ -15,7 +15,7 @@ enum link_layer {
 
 int  tc_core_time2big(double time);
 unsigned tc_core_time2tick(double time);
-unsigned tc_core_tick2time(unsigned tick);
+double tc_core_tick2time(unsigned tick);
 unsigned tc_core_time2ktime(unsigned time);
 unsigned tc_core_ktime2time(unsigned ktime);
 unsigned tc_calc_xmittime(unsigned rate, unsigned size);

Re: [PATCH iproute2 3/2 v2] tc_core: Return double from tc_core_tick2time()

[Patrick McHardy]

Jarek Poplawski wrote:
> On Wed, Jun 10, 2009 at 01:04:36AM +0200, Patrick McHardy wrote:
>> Jarek Poplawski wrote:
>>> Hmm... I'm not sure this floor() is needed here, but maybe I'll get it
>>> tomorrow... 
>> Its not. It was mainly a suggestion to make the symetry more explicit.
>> I hope gcc understands :)
> 
> If so, then maybe let's reconsider previous example:
> 
>> Let's see how it's used (mainly) in tc_calc_xmittime() e.g. for
>> Antonio's mostly 800byte and 555Mbit rate:
>>
>> tc_core_time2tick(TIME_UNITS_PER_SEC * (double)size/rate)
>> tc_core_time2tick(1000000 * (double) 800/69375000)
>> tc_core_time2tick(11.53153)
> 
> Let's say it returns 116 (after x10 with ceil()). Then, going backwards:
> 
> tc_calc_xmitsize()
> {
> 	return ((double) 69375000 * tc_core_tick2time(116)) / TIME_UNITS_PER_SEC;
> }
> 
> would give us: 763 after /10 with floor(), and 804 without floor().
> The latter looks nicer to me but I leave the choice to you or Stephen.

Agreed, I made a thinko somewhere.

[PATCH iproute2 1/2] tc_core: Use double in tc_core_time2tick()

[Jarek Poplawski]

CBQ minidle parameter is provided in microseconds, so needs time2tick
conversion.

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 tc/q_cbq.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tc/q_cbq.c b/tc/q_cbq.c
index c99dc3b..7f2bd0c 100644
--- a/tc/q_cbq.c
+++ b/tc/q_cbq.c
@@ -406,7 +406,7 @@ static int cbq_parse_class_opt(struct qdisc_util *qu, int argc, char **argv, str
 		lss.change |= TCF_CBQ_LSS_EWMA|TCF_CBQ_LSS_AVPKT;
 	}
 	if (lss.change&TCF_CBQ_LSS_MINIDLE) {
-		lss.minidle <<= lss.ewma_log;
+		lss.minidle = tc_core_time2tick(lss.minidle << lss.ewma_log);
 		lss.change |= TCF_CBQ_LSS_EWMA;
 	}