Patch "[PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec >= 1000000" seems wrong

Patch "[PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec >= 1000000" seems wrong

[Guillaume Chazarain]

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

Hello,

This patch:
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=4ee303dfeac6451b402e3d8512723d3a0f861857

looks wrong.

In PSCHED_TADD2, if delta is less than tv.tv_usec (so, less than 
USEC_PER_SEC too) then tv_res will be before tv. The
affectation (tv_res).tv_usec = __delta; is wrong. The same applies to 
PSCHED_TADD.

I think the correct fix is simply to restore the original code and 
change the 'if' in a 'while'.

So here are two patches for the two possible solutions :

o Reverting the commit and applying from_before_patch_to_correct.diff

o Applying from_tip_to_correct.diff


Thanks.


-- 
Guillaume


[-- Attachment #2: from_before_patch_to_correct.diff --]
[-- Type: text/x-patch, Size: 751 bytes --]

--- a/include/net/pkt_sched.h.before	2006-07-14 15:58:53.000000000 +0200
+++ b/include/net/pkt_sched.h	2006-07-14 15:59:10.000000000 +0200
@@ -171,14 +171,14 @@
 ({ \
 	   int __delta = (tv).tv_usec + (delta); \
 	   (tv_res).tv_sec = (tv).tv_sec; \
-	   if (__delta > USEC_PER_SEC) { (tv_res).tv_sec++; __delta -= USEC_PER_SEC; } \
+	   while (__delta > USEC_PER_SEC) { (tv_res).tv_sec++; __delta -= USEC_PER_SEC; } \
 	   (tv_res).tv_usec = __delta; \
 })
 
 #define PSCHED_TADD(tv, delta) \
 ({ \
 	   (tv).tv_usec += (delta); \
-	   if ((tv).tv_usec > USEC_PER_SEC) { (tv).tv_sec++; \
+	   while ((tv).tv_usec > USEC_PER_SEC) { (tv).tv_sec++; \
 		 (tv).tv_usec -= USEC_PER_SEC; } \
 })

--  
Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>

[-- Attachment #3: from_tip_to_correct.diff --]
[-- Type: text/x-patch, Size: 1096 bytes --]

diff -r 672dc37bf355 include/net/pkt_sched.h
--- a/include/net/pkt_sched.h	Fri Jul 14 06:57:04 2006 +0700
+++ b/include/net/pkt_sched.h	Fri Jul 14 15:58:04 2006 +0200
@@ -169,23 +169,17 @@ psched_tod_diff(int delta_sec, int bound
 
 #define PSCHED_TADD2(tv, delta, tv_res) \
 ({ \
-	   int __delta = (delta); \
-	   (tv_res) = (tv); \
-	   while(__delta >= USEC_PER_SEC){ \
-		 (tv_res).tv_sec++; \
-		 __delta -= USEC_PER_SEC; \
-	   } \
+	   int __delta = (tv).tv_usec + (delta); \
+	   (tv_res).tv_sec = (tv).tv_sec; \
+	   while (__delta > USEC_PER_SEC) { (tv_res).tv_sec++; __delta -= USEC_PER_SEC; } \
 	   (tv_res).tv_usec = __delta; \
 })
 
 #define PSCHED_TADD(tv, delta) \
 ({ \
-	   int __delta = (delta); \
-	   while(__delta >= USEC_PER_SEC){ \
-		 (tv).tv_sec++; \
-		 __delta -= USEC_PER_SEC; \
-	   } \
-	   (tv).tv_usec = __delta; \
+	   (tv).tv_usec += (delta); \
+	   while ((tv).tv_usec > USEC_PER_SEC) { (tv).tv_sec++; \
+		 (tv).tv_usec -= USEC_PER_SEC; } \
 })
 
 /* Set/check that time is in the "past perfect";

-- 
Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>

Re: Patch "[PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec >= 1000000" seems wrong

[Shuya MAEDA]

Hello.

> In PSCHED_TADD2, if delta is less than tv.tv_usec (so, less than 
> USEC_PER_SEC too) then tv_res will be before tv. The
> affectation (tv_res).tv_usec = __delta; is wrong. The same applies to 
> PSCHED_TADD.

You are right. It is my mistake.

> I think the correct fix is simply to restore the original code and 
> change the 'if' in a 'while'.

In Guillaume's patch,
"while (__delta > USEC_PER_SEC){ ... }", but I think it should be
"while (__delta >= USEC_PER_SEC){ ... }". Is it right?

Thank you very much.
-- 
Shuya MAEDA

Re: Patch "[PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec >= 1000000" seems wrong

[Guillaume Chazarain]

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

Shuya MAEDA wrote :
> "while (__delta > USEC_PER_SEC){ ... }", but I think it should be
> "while (__delta >= USEC_PER_SEC){ ... }". Is it right?

I agree, good catch :-)

Thanks.

-- 
Guillaume


[-- Attachment #2: fix_psched_tadd.diff --]
[-- Type: text/x-patch, Size: 1503 bytes --]

In PSCHED_TADD and PSCHED_TADD2, if delta is less than tv.tv_usec (so, less
than USEC_PER_SEC too) then tv_res will be smaller than tv. The
affectation "(tv_res).tv_usec = __delta;" is wrong.
The fix is to revert to the original code before
4ee303dfeac6451b402e3d8512723d3a0f861857 and change the 'if' in 'while'.

[Shuya MAEDA: "while (__delta >= USEC_PER_SEC){ ... }" instead of
"while (__delta > USEC_PER_SEC){ ... }"]

Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
---

 pkt_sched.h |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -169,23 +169,17 @@ psched_tod_diff(int delta_sec, int bound
 
 #define PSCHED_TADD2(tv, delta, tv_res) \
 ({ \
-	   int __delta = (delta); \
-	   (tv_res) = (tv); \
-	   while(__delta >= USEC_PER_SEC){ \
-		 (tv_res).tv_sec++; \
-		 __delta -= USEC_PER_SEC; \
-	   } \
+	   int __delta = (tv).tv_usec + (delta); \
+	   (tv_res).tv_sec = (tv).tv_sec; \
+	   while (__delta >= USEC_PER_SEC) { (tv_res).tv_sec++; __delta -= USEC_PER_SEC; } \
 	   (tv_res).tv_usec = __delta; \
 })
 
 #define PSCHED_TADD(tv, delta) \
 ({ \
-	   int __delta = (delta); \
-	   while(__delta >= USEC_PER_SEC){ \
-		 (tv).tv_sec++; \
-		 __delta -= USEC_PER_SEC; \
-	   } \
-	   (tv).tv_usec = __delta; \
+	   (tv).tv_usec += (delta); \
+	   while ((tv).tv_usec >= USEC_PER_SEC) { (tv).tv_sec++; \
+		 (tv).tv_usec -= USEC_PER_SEC; } \
 })
 
 /* Set/check that time is in the "past perfect";

Re: Patch "[PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec >= 1000000" seems wrong

[David Miller]

From: Guillaume Chazarain <guichaz@yahoo.fr>
Date: Wed, 19 Jul 2006 14:47:34 +0200

> Shuya MAEDA wrote :
> > "while (__delta > USEC_PER_SEC){ ... }", but I think it should be
> > "while (__delta >= USEC_PER_SEC){ ... }". Is it right?
> 
> I agree, good catch :-)

Applied, thanks a lot.