[PATCH] sched: fix never executed code due to expression always false

[PATCH] sched: fix never executed code due to expression always false

[Jesper Juhl]

There are two expressions in kernel/sched.c that are always false since 
they test for <0 but the result of the expression is unsigned so they will 
never be less than zero. This patch implement the logic that I believe is 
intended without the signedness issue and without the nasty casts.
<disclaimer>patch is compile tested only</disclaimer>


Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
---

 sched.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

--- linux-2.6.12-rc2-mm3-orig/kernel/sched.c	2005-04-11 21:20:56.000000000 +0200
+++ linux-2.6.12-rc2-mm3/kernel/sched.c	2005-04-15 01:37:48.000000000 +0200
@@ -2697,9 +2697,10 @@ need_resched_nonpreemptible:
 	schedstat_inc(rq, sched_cnt);
 	now = sched_clock();
 	if (likely((long long)now - prev->timestamp < NS_MAX_SLEEP_AVG)) {
-		run_time = now - prev->timestamp;
-		if (unlikely((long long)now - prev->timestamp < 0))
+		if (unlikely(prev->timestamp > now))
 			run_time = 0;
+		else
+			run_time = now - prev->timestamp;
 	} else
 		run_time = NS_MAX_SLEEP_AVG;
 
@@ -2776,7 +2777,7 @@ go_idle:
 
 	if (!rt_task(next) && next->activated > 0) {
 		unsigned long long delta = now - next->timestamp;
-		if (unlikely((long long)now - next->timestamp < 0))
+		if (unlikely(next->timestamp > now))
 			delta = 0;
 
 		if (next->activated == 1)

Re: [PATCH] sched: fix never executed code due to expression always false

[Nick Piggin]

Jesper Juhl wrote:
> There are two expressions in kernel/sched.c that are always false since 
> they test for <0 but the result of the expression is unsigned so they will 
> never be less than zero. This patch implement the logic that I believe is 
> intended without the signedness issue and without the nasty casts.
> <disclaimer>patch is compile tested only</disclaimer>
> 

This is not *quite* the intended behaviour. It is OK for prev->timestamp
to be '0 - a bit' and now to be '0 + a bit' in the case of wrapping.

Although considering they're 64-bit values, I'm not sure how much we care.

-- 
SUSE Labs, Novell Inc.

Re: [PATCH] sched: fix never executed code due to expression always false

[Jesper Juhl]

On Fri, 15 Apr 2005, Nick Piggin wrote:

> Jesper Juhl wrote:
> > There are two expressions in kernel/sched.c that are always false since they
> > test for <0 but the result of the expression is unsigned so they will never
> > be less than zero. This patch implement the logic that I believe is intended
> > without the signedness issue and without the nasty casts.
> > <disclaimer>patch is compile tested only</disclaimer>
> > 
> 
> This is not *quite* the intended behaviour. It is OK for prev->timestamp
> to be '0 - a bit' and now to be '0 + a bit' in the case of wrapping.
> 
> Although considering they're 64-bit values, I'm not sure how much we care.
> 
How do you propose to fix this then?  As the code is now the expressionsa 
are always false - should we just remove the them?  Or do you have a 
sensible definition of "a bit" ?  or ome other suggestion alltogether?


-- 
Jesper

Re: [PATCH] sched: fix never executed code due to expression always false

[Nick Piggin]

Jesper Juhl wrote:
> On Fri, 15 Apr 2005, Nick Piggin wrote:
> 
> 
>>Jesper Juhl wrote:
>>
>>>There are two expressions in kernel/sched.c that are always false since they
>>>test for <0 but the result of the expression is unsigned so they will never
>>>be less than zero. This patch implement the logic that I believe is intended
>>>without the signedness issue and without the nasty casts.
>>><disclaimer>patch is compile tested only</disclaimer>
>>>
>>This is not *quite* the intended behaviour. It is OK for prev->timestamp
>>to be '0 - a bit' and now to be '0 + a bit' in the case of wrapping.
>>
>>Although considering they're 64-bit values, I'm not sure how much we care.
>>
> 
> How do you propose to fix this then?  As the code is now the expressionsa 
> are always false - should we just remove the them?  Or do you have a 
> sensible definition of "a bit" ?  or ome other suggestion alltogether?
> 

Make it a signed comparison?

-- 
SUSE Labs, Novell Inc.

Re: [PATCH] sched: fix never executed code due to expression always false

[Jesper Juhl]

On Fri, 15 Apr 2005, Nick Piggin wrote:

> Jesper Juhl wrote:
> > On Fri, 15 Apr 2005, Nick Piggin wrote:
> > 
> > 
> > > Jesper Juhl wrote:
> > > 
> > > > There are two expressions in kernel/sched.c that are always false since
> > > > they
> > > > test for <0 but the result of the expression is unsigned so they will
> > > > never
> > > > be less than zero. This patch implement the logic that I believe is
> > > > intended
> > > > without the signedness issue and without the nasty casts.
> > > > <disclaimer>patch is compile tested only</disclaimer>
> > > > 
> > > This is not *quite* the intended behaviour. It is OK for prev->timestamp
> > > to be '0 - a bit' and now to be '0 + a bit' in the case of wrapping.
> > > 
> > > Although considering they're 64-bit values, I'm not sure how much we care.
> > > 
> > 
> > How do you propose to fix this then?  As the code is now the expressionsa
> > are always false - should we just remove the them?  Or do you have a
> > sensible definition of "a bit" ?  or ome other suggestion alltogether?
> > 
> 
> Make it a signed comparison?
> 
As per this patch perhaps? : 

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

--- linux-2.6.12-rc2-mm3-orig/kernel/sched.c	2005-04-11 21:20:56.000000000 +0200
+++ linux-2.6.12-rc2-mm3/kernel/sched.c	2005-04-15 02:21:34.000000000 +0200
@@ -2697,9 +2697,10 @@ need_resched_nonpreemptible:
 	schedstat_inc(rq, sched_cnt);
 	now = sched_clock();
 	if (likely((long long)now - prev->timestamp < NS_MAX_SLEEP_AVG)) {
-		run_time = now - prev->timestamp;
-		if (unlikely((long long)now - prev->timestamp < 0))
+		if (unlikely(((long long)now - (long long)prev->timestamp) < 0))
 			run_time = 0;
+		else
+			run_time = now - prev->timestamp;
 	} else
 		run_time = NS_MAX_SLEEP_AVG;
 
@@ -2776,7 +2777,7 @@ go_idle:
 
 	if (!rt_task(next) && next->activated > 0) {
 		unsigned long long delta = now - next->timestamp;
-		if (unlikely((long long)now - next->timestamp < 0))
+		if (unlikely(((long long)now - (long long)next->timestamp) < 0))
 			delta = 0;
 
 		if (next->activated == 1)

Re: [PATCH] sched: fix never executed code due to expression always false

[Nick Piggin]

Jesper Juhl wrote:

> 
> As per this patch perhaps? : 
> 

Thanks. I'll make sure it gets to the right place if nobody picks it up.

> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> 
> --- linux-2.6.12-rc2-mm3-orig/kernel/sched.c	2005-04-11 21:20:56.000000000 +0200
> +++ linux-2.6.12-rc2-mm3/kernel/sched.c	2005-04-15 02:21:34.000000000 +0200
> @@ -2697,9 +2697,10 @@ need_resched_nonpreemptible:
>  	schedstat_inc(rq, sched_cnt);
>  	now = sched_clock();
>  	if (likely((long long)now - prev->timestamp < NS_MAX_SLEEP_AVG)) {
> -		run_time = now - prev->timestamp;
> -		if (unlikely((long long)now - prev->timestamp < 0))
> +		if (unlikely(((long long)now - (long long)prev->timestamp) < 0))
>  			run_time = 0;
> +		else
> +			run_time = now - prev->timestamp;
>  	} else
>  		run_time = NS_MAX_SLEEP_AVG;
>  
> @@ -2776,7 +2777,7 @@ go_idle:
>  
>  	if (!rt_task(next) && next->activated > 0) {
>  		unsigned long long delta = now - next->timestamp;
> -		if (unlikely((long long)now - next->timestamp < 0))
> +		if (unlikely(((long long)now - (long long)next->timestamp) < 0))
>  			delta = 0;
>  
>  		if (next->activated == 1)
> 
> 
> 


-- 
SUSE Labs, Novell Inc.

Re: [PATCH] sched: fix never executed code due to expression always false

[Matt Mackall]

On Fri, Apr 15, 2005 at 10:23:11AM +1000, Nick Piggin wrote:
> Jesper Juhl wrote:
> 
> >
> >As per this patch perhaps? : 
> >
> 
> Thanks. I'll make sure it gets to the right place if nobody picks it up.

Perhaps this ought to be wrapped up in sched_clock_before() or some
such.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] sched: fix never executed code due to expression always false

[Herbert Xu]

Jesper Juhl <juhl-lkml@dif.dk> wrote:
>
> -               if (unlikely((long long)now - prev->timestamp < 0))
> +               if (unlikely(((long long)now - (long long)prev->timestamp) < 0))

You can write this as

(long long)(now - prev->timestamp)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] sched: fix never executed code due to expression always false

[Nick Piggin]

On Fri, 2005-04-15 at 12:59 +1000, Herbert Xu wrote:
> Jesper Juhl <juhl-lkml@dif.dk> wrote:
> >
> > -               if (unlikely((long long)now - prev->timestamp < 0))
> > +               if (unlikely(((long long)now - (long long)prev->timestamp) < 0))
> 
> You can write this as
> 
> (long long)(now - prev->timestamp)
> 

True. Combined that with Matt's suggestion, and we probably have
the cleanest solution. Thanks.

Re: [PATCH] sched: fix never executed code due to expression always false

[Ingo Molnar]

* Jesper Juhl <juhl-lkml@dif.dk> wrote:

> As per this patch perhaps? : 
> 
> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

this is still not enough (there was one more comparison to cover). Also, 
it's a bit cleaner to just cast the left side to signed than cast every 
member separately.

	Ingo

--

fix signed comparisons of long long.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- linux/kernel/sched.c.orig
+++ linux/kernel/sched.c
@@ -2695,9 +2695,9 @@ need_resched_nonpreemptible:
 
 	schedstat_inc(rq, sched_cnt);
 	now = sched_clock();
-	if (likely((long long)now - prev->timestamp < NS_MAX_SLEEP_AVG)) {
+	if (likely((long long)(now - prev->timestamp) < NS_MAX_SLEEP_AVG)) {
 		run_time = now - prev->timestamp;
-		if (unlikely((long long)now - prev->timestamp < 0))
+		if (unlikely((long long)(now - prev->timestamp) < 0))
 			run_time = 0;
 	} else
 		run_time = NS_MAX_SLEEP_AVG;
@@ -2775,7 +2775,7 @@ go_idle:
 
 	if (!rt_task(next) && next->activated > 0) {
 		unsigned long long delta = now - next->timestamp;
-		if (unlikely((long long)now - next->timestamp < 0))
+		if (unlikely((long long)(now - next->timestamp) < 0))
 			delta = 0;
 
 		if (next->activated == 1)

Re: [PATCH] sched: fix never executed code due to expression always false

[Jesper Juhl]

On Fri, 15 Apr 2005, Ingo Molnar wrote:

> 
> * Jesper Juhl <juhl-lkml@dif.dk> wrote:
> 
> > As per this patch perhaps? : 
> > 
> > Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> 
> this is still not enough (there was one more comparison to cover). Also, 
> it's a bit cleaner to just cast the left side to signed than cast every 
> member separately.
> 
> 	Ingo
> 
> --
> 
> fix signed comparisons of long long.
> 
> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> --- linux/kernel/sched.c.orig
> +++ linux/kernel/sched.c
> @@ -2695,9 +2695,9 @@ need_resched_nonpreemptible:
>  
>  	schedstat_inc(rq, sched_cnt);
>  	now = sched_clock();
> -	if (likely((long long)now - prev->timestamp < NS_MAX_SLEEP_AVG)) {
> +	if (likely((long long)(now - prev->timestamp) < NS_MAX_SLEEP_AVG)) {
>  		run_time = now - prev->timestamp;
> -		if (unlikely((long long)now - prev->timestamp < 0))
> +		if (unlikely((long long)(now - prev->timestamp) < 0))
>  			run_time = 0;
>  	} else
>  		run_time = NS_MAX_SLEEP_AVG;
> @@ -2775,7 +2775,7 @@ go_idle:
>  
>  	if (!rt_task(next) && next->activated > 0) {
>  		unsigned long long delta = now - next->timestamp;
> -		if (unlikely((long long)now - next->timestamp < 0))
> +		if (unlikely((long long)(now - next->timestamp) < 0))
>  			delta = 0;
>  
>  		if (next->activated == 1)
> 

Right, that's a better version. Thanks.

-- 
Jesper