Question about QOS

Question about QOS

[Nicolas DICHTEL]

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 ? or I miss
something ? Is this a bug ?


Thanks
Nicolas

Re: Question about QOS

[Thomas Graf]

* Nicolas DICHTEL <426E06F1.9000105@6wind.com> 2005-04-26 11:16
> 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.

Re: Question about QOS

[Nicolas DICHTEL]

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

Thomas Graf wrote:

>* Nicolas DICHTEL <426E06F1.9000105@6wind.com> 2005-04-26 11:16
>  
>
>>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 <nicolas.dichtel@6wind.com>


[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 456 bytes --]

diff -Nru linux-2.6-a/include/net/pkt_sched.h linux-2.6-b/include/net/pkt_sched.h
--- linux-2.6-a/include/net/pkt_sched.h	2005-04-26 15:45:07.074124664 +0200
+++ linux-2.6-b/include/net/pkt_sched.h	2005-04-26 15:47:26.215971888 +0200
@@ -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;
 }

Re: Question about QOS

[Thomas Graf]

* Nicolas DICHTEL <426E56DC.7000108@6wind.com> 2005-04-26 16:57
> 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 <nicolas.dichtel@6wind.com>
> 

> diff -Nru linux-2.6-a/include/net/pkt_sched.h linux-2.6-b/include/net/pkt_sched.h
> --- linux-2.6-a/include/net/pkt_sched.h	2005-04-26 15:45:07.074124664 +0200
> +++ linux-2.6-b/include/net/pkt_sched.h	2005-04-26 15:47:26.215971888 +0200
> @@ -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;
>  }

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?

Re: Question about QOS

[Nicolas DICHTEL]

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


>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 majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>  
>
You're right. Here is the new patch.

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

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>


[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 640 bytes --]

diff -Nru linux-2.6-a/include/net/pkt_sched.h linux-2.6-b/include/net/pkt_sched.h
--- linux-2.6-a/include/net/pkt_sched.h	2005-04-26 15:45:07.000000000 +0200
+++ linux-2.6-b/include/net/pkt_sched.h	2005-04-27 09:36:23.421634576 +0200
@@ -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; \
 })

Re: Question about QOS

[Thomas Graf]

* Nicolas DICHTEL <426F42F0.9020609@6wind.com> 2005-04-27 09:44
> 
> >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 <nicolas.dichtel@6wind.com>

Signed-off-by: Thomas Graf <tgraf@suug.ch>


> diff -Nru linux-2.6-a/include/net/pkt_sched.h linux-2.6-b/include/net/pkt_sched.h
> --- linux-2.6-a/include/net/pkt_sched.h	2005-04-26 15:45:07.000000000 +0200
> +++ linux-2.6-b/include/net/pkt_sched.h	2005-04-27 09:36:23.421634576 +0200
> @@ -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; \
>  })

Re: Question about QOS

[David S. Miller]

On Wed, 27 Apr 2005 13:42:16 +0200
Thomas Graf <tgraf@suug.ch> wrote:

> * Nicolas DICHTEL <426F42F0.9020609@6wind.com> 2005-04-27 09:44
> > 
> > >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 <nicolas.dichtel@6wind.com>
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Applied, thanks everyone.

Re: Question about QOS

[Patrick McHardy]

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

Thomas Graf wrote:
> * Nicolas DICHTEL <426F42F0.9020609@6wind.com> 2005-04-27 09:44
> 
>>>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.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 940 bytes --]

[PKT_SCHED]: Fix range in PSCHED_TDIFF_SAFE to 0..bound

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit d5f65671be760f33c1c7667f793ed75ad6e6a710
tree 4af3018396f125b0c2601882d924e63a0d74bef4
parent 980207c270e72e2cc6ae7b0de6ea6cd38a726e5b
author Patrick McHardy <kaber@trash.net> 1114776755 +0200
committer Patrick McHardy <kaber@trash.net> 1114776755 +0200

Index: include/net/pkt_sched.h
===================================================================
--- e38e7620d369bfa898a2e3f6aaf2f04c8826f38e/include/net/pkt_sched.h  (mode:100644 sha1:7352e455053cc857e70f0cb1e008eb7adabbe011)
+++ 4af3018396f125b0c2601882d924e63a0d74bef4/include/net/pkt_sched.h  (mode:100644 sha1:fcb05a387dbee560d6a0f20b6e8c3c4f1b2d93f1)
@@ -157,7 +157,8 @@
 	   case 1: \
 		   __delta += 1000000; \
 	   case 0: \
-		   __delta = abs(__delta); \
+ 		   if (__delta > bound || __delta < 0) \
+ 			__delta = bound; \
 	   } \
 	   __delta; \
 })

Re: Question about QOS

[Thomas Graf]

* Patrick McHardy <427225B2.6010705@trash.net> 2005-04-29 14:16
> 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.

Doh, of course. Absolutely correct.

Re: Question about QOS

[David S. Miller]

On Fri, 29 Apr 2005 14:16:50 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Thomas Graf wrote:
> > * Nicolas DICHTEL <426F42F0.9020609@6wind.com> 2005-04-27 09:44
> > 
> >>>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.

Applied, thanks Patrick.