Re: Patch [PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec = 1000000 seems wrong
From: Guillaume Chazarain [EMAIL PROTECTED]
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.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Patch [PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec = 1000000 seems wrong
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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Patch [PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec = 1000000 seems wrong
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
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 [EMAIL PROTECTED]
---
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;
Patch [PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec = 1000000 seems wrong
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
--- a/include/net/pkt_sched.h.before 2006-07-14 15:58:53.0 +0200
+++ b/include/net/pkt_sched.h 2006-07-14 15:59:10.0 +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 [EMAIL PROTECTED]
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 [EMAIL PROTECTED]
[PATCH] [PKT_SCHED]: PSCHED_TADD() and PSCHED_TADD2() can result,tv_usec = 1000000
I found two problems in PSCHED_TADD() and PSCHED_TADD2().
1) These function increment tv_sec if tv_usec 100.
But I think it should if tv_usec = 100.
2) tv_usec became 120 or more when I used CBQ and
experimented it. It is not correct to exceed 100
because tv_usec is micro seconds.
To fix 2), I think that it should do delta / 100,
add the quotient to tv_sec, and add the remainder to
tv_usec.
In both cases, because time when the transmission is restarted
reaches an illegal value, it is not possible to communicate at
the set rate.
To fix these problem I create following patch.
Are there any comments?
[Experiment]
* kernel: linux-2.6.15.5
* CBQ settings
--
tc qdisc add dev $IF root handle 1:0 cbq bandwidth 100Mbit \
avpkt 1000 mpu 64 ewma 5 cell 8
tc class add dev $IF parent 1:0 classid 1:10 cbq rate 32Kbit \
prio 1 ewma 5 cell 8 avpkt 138 mpu 64 bandwidth 100Mbit \
minburst 25 maxburst 50 bounded isolated
tc filter add dev $IF parent 1:0 protocol ip prio 16 u32 match \
ip dport 4952 0x flowid 1:10
---
* Traffic
dst port 4952: 138byte per 20msec.
[Result]
* In cbq_ovl_classic():
cl-undertime = { tv_sec = 1150368540, tv_usec = 1208301 }
~~
q-now= { tv_sec = 1150368539, tv_usec = 878917 }
delay = 1329384
cl-avgidle = -14781
cl-offtime = 1295394
[Patch]
diff -Nur linux-2.6.17-rc6.orig/include/net/pkt_sched.h
linux-2.6.17-rc6.mypatch/include/net/pkt_sched.h
--- linux-2.6.17-rc6.orig/include/net/pkt_sched.h 2006-06-06
09:57:02.0 +0900
+++ linux-2.6.17-rc6.mypatch/include/net/pkt_sched.h2006-06-16
11:29:08.0 +0900
@@ -169,17 +169,31 @@
#define PSCHED_TADD2(tv, delta, tv_res) \
({ \
- 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; } \
- (tv_res).tv_usec = __delta; \
+ int __delta = (delta); \
+ (tv_res) = (tv); \
+ if((delta) USEC_PER_SEC) { \
+(tv_res).tv_sec += (delta) / USEC_PER_SEC; \
+__delta -= (delta) % USEC_PER_SEC; \
+ } \
+ (tv_res).tv_usec += __delta; \
+ if((tv_res).tv_usec = USEC_PER_SEC) { \
+(tv_res).tv_sec++; \
+(tv_res).tv_usec -= USEC_PER_SEC; \
+ } \
})
#define PSCHED_TADD(tv, delta) \
({ \
- (tv).tv_usec += (delta); \
- if ((tv).tv_usec USEC_PER_SEC) { (tv).tv_sec++; \
-(tv).tv_usec -= USEC_PER_SEC; } \
+ int __delta = (delta); \
+ if((delta) USEC_PER_SEC) { \
+(tv).tv_sec += (delta) / USEC_PER_SEC; \
+__delta -= (delta) % USEC_PER_SEC; \
+ } \
+ (tv).tv_usec += __delta; \
+ if((tv).tv_usec = USEC_PER_SEC) { \
+(tv).tv_sec++; \
+(tv).tv_usec -= USEC_PER_SEC; \
+ } \
})
/* Set/check that time is in the past perfect;
--
Shuya Maeda
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
