Question about QOS

I set CONFIG_NET_SCH_CLK_GETTIMEOFDAY in my kernel. The macro PSCHED_TDIFF_SAFE calculates
the difference between two timestamps and uses the function psched_tod_diff() to do this.
If the clock is readjusted (due to ntp for example), this function can return a negative number
(if bound > 1000000) and then the flow is blocked by the kernel. Am I right ?

* Nicolas DICHTEL <[email protected]> 2005-04-26 11:16

Quoted 6 lines

I set CONFIG_NET_SCH_CLK_GETTIMEOFDAY in my kernel. The macro PSCHED_TDIFF_SAFE calculates the difference between two timestamps and uses the function psched_tod_diff() to do this. If the clock is readjusted (due to ntp for example), this function can return a negative number (if bound > 1000000) and then the flow is blocked by the kernel. Am I right ?

do_gettimeofday takes care of ntp adjustments so we _should_ be safe, however, it might be wise to enforce a range of 0..bound instead of INT_MIN..bound because qdiscs like red are relying on this. Assuming we have a delta of -4 seconds and return -4e6 red will horribly crash when acccessing the array with idle_time>>cell_log.

You can have the same kind of problem with a ingress filter. I propose the following patch to fix the range to 0..bound

[SCHED] Fix range in psched_tod_diff() to 0..bound

Signed-off-by Nicolas Dichtel <[email protected]>

diff -Nru linux-2.6-a/include/net/pkt_sched.h linux-2.6-b/include/net/pkt_sched.h

Yes I agree, it doesn't really matter what value we return and `bound' is most likely to be correct. I think we should also fix the unlikely but still possible case when tv1.tv_usec is slightly smaller than tv2.tv_usec. I know it is very unlikely but do_gettimeofday really is not that reliable and we have users which rely on a positive delta. Can you extend your patch to return abs(delta) for case 0 in PSCHED_TDIFF_SAFE?
-
To unsubscribe from this list: send the line "unsubscribe linux-net" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines

Yes I agree, it doesn't really matter what value we return and `bound' is most likely to be correct. I think we should also fix the unlikely but still possible case when tv1.tv_usec is slightly smaller than tv2.tv_usec. I know it is very unlikely but do_gettimeofday really is not that reliable and we have users which rely on a positive delta. Can you extend your patch to return abs(delta) for case 0 in PSCHED_TDIFF_SAFE?

You're right. Here is the new patch.

[SCHED] Fix range in PSCHED_TDIFF_SAFE to 0..bound

Signed-off-by Nicolas Dichtel <[email protected]>

diff -Nru linux-2.6-a/include/net/pkt_sched.h linux-2.6-b/include/net/pkt_sched.h

include/net/pkt_sched.h +3 -2

@@ -140,7 +140,7 @@
 	if (bound <= 1000000 || delta_sec > (0x7FFFFFFF/1000000)-1)
 		return bound;
 	delta = delta_sec * 1000000;
-	if (delta > bound)
+	if (delta > bound || delta < 0)
 		delta = bound;
 	return delta;
 }

@@ -156,7 +156,8 @@
 		   __delta += 1000000; \
 	   case 1: \
 		   __delta += 1000000; \
-	   case 0: ; \
+	   case 0: \
+		   __delta = abs(__delta); \
 	   } \
 	   __delta; \
 })

* Nicolas DICHTEL <[email protected]> 2005-04-27 09:44

Quoted 9 lines

Quoted 9 lines

Yes I agree, it doesn't really matter what value we return and `bound' is most likely to be correct. I think we should also fix the unlikely but still possible case when tv1.tv_usec is slightly smaller than tv2.tv_usec. I know it is very unlikely but do_gettimeofday really is not that reliable and we have users which rely on a positive delta. Can you extend your patch to return abs(delta) for case 0 in PSCHED_TDIFF_SAFE?

Quoted 5 lines

You're right. Here is the new patch.

[SCHED] Fix range in PSCHED_TDIFF_SAFE to 0..bound

Signed-off-by Nicolas Dichtel <[email protected]>

Signed-off-by Thomas Graf <[email protected]>

* Nicolas DICHTEL <[email protected]> 2005-04-27 09:44

Quoted 7 lines

Quoted 7 lines

Yes I agree, it doesn't really matter what value we return and `bound' is most likely to be correct. I think we should also fix the unlikely but still possible case when tv1.tv_usec is slightly smaller than tv2.tv_usec. I know it is very unlikely but do_gettimeofday really is not that reliable and we have users which rely on a positive delta. Can you extend your patch to return abs(delta) for case 0 in PSCHED_TDIFF_SAFE?

Why abs(delta)? It could be above bound, in fact all cases besides delta_sec > 2 doesn't take care to stay inside [0..bound] at all.

Quoted 9 lines · Thomas Graf

* Nicolas DICHTEL <[email protected]> 2005-04-27 09:44

Quoted 7 lines

Quoted 7 lines

Yes I agree, it doesn't really matter what value we return and `bound' is most likely to be correct. I think we should also fix the unlikely but still possible case when tv1.tv_usec is slightly smaller than tv2.tv_usec. I know it is very unlikely but do_gettimeofday really is not that reliable and we have users which rely on a positive delta. Can you extend your patch to return abs(delta) for case 0 in PSCHED_TDIFF_SAFE?

Why abs(delta)? It could be above bound, in fact all cases besides delta_sec > 2 doesn't take care to stay inside [0..bound] at all.