2005-05-18 22:46:19

by Coywolf Qi Hunt

[permalink] [raw]
Subject: [patch] time_after_eq fix

Hello,

The two macros time_after and time_after_eq were added to do wrapping
correctly, but only time_after does it the right way, time_after_eq has
been wrong since the very beginning(v2.1.127, 07-Nov-1998). Now this
patch fixes it.

And I don't agree with the the original code comment. I don't think this
is gcc's fault. If it is "a good compiler" or "a really good compiler",
trying to be smarter than human, it wouldn't still be a C compiler.
So I'd like it be removed.

Signed-off-by: Coywolf Qi Hunt <[email protected]>
---

jiffies.h | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)

diff -pruN 2.6.12-rc4-mm2/include/linux/jiffies.h 2.6.12-rc4-mm2-cy/include/linux/jiffies.h
--- 2.6.12-rc4-mm2/include/linux/jiffies.h 2005-03-03 17:12:13.000000000 +0800
+++ 2.6.12-rc4-mm2-cy/include/linux/jiffies.h 2005-05-19 05:32:52.000000000 +0800
@@ -102,9 +102,7 @@ static inline u64 get_jiffies_64(void)
*
* time_after(a,b) returns true if the time a is after time b.
*
- * Do this with "<0" and ">=0" to only test the sign of the result. A
- * good compiler would generate better code (and a really good compiler
- * wouldn't care). Gcc is currently neither.
+ * Do this with "<0" and "<=0" to only test the sign of the result.
*/
#define time_after(a,b) \
(typecheck(unsigned long, a) && \
@@ -115,7 +113,7 @@ static inline u64 get_jiffies_64(void)
#define time_after_eq(a,b) \
(typecheck(unsigned long, a) && \
typecheck(unsigned long, b) && \
- ((long)(a) - (long)(b) >= 0))
+ ((long)(b) - (long)(a) <= 0))
#define time_before_eq(a,b) time_after_eq(b,a)

/*


2005-05-18 23:15:13

by Chris Friesen

[permalink] [raw]
Subject: Re: [patch] time_after_eq fix

Coywolf Qi Hunt wrote:
> Hello,
>
> The two macros time_after and time_after_eq were added to do wrapping
> correctly, but only time_after does it the right way, time_after_eq has
> been wrong since the very beginning(v2.1.127, 07-Nov-1998).

> - ((long)(a) - (long)(b) >= 0))
> + ((long)(b) - (long)(a) <= 0))

Why does it matter which way you do it? In what circumstances does your
code give a different answer?


Chris

2005-05-18 23:34:40

by Linus Torvalds

[permalink] [raw]
Subject: Re: [patch] time_after_eq fix



On Thu, 19 May 2005, Coywolf Qi Hunt wrote:
>
> And I don't agree with the the original code comment. I don't think this
> is gcc's fault. If it is "a good compiler" or "a really good compiler",
> trying to be smarter than human, it wouldn't still be a C compiler.
> So I'd like it be removed.

The original comment is correct, and your changed comment is nonsensical,
since "<= 0" doesn't actually test the sign of the result like your
comment says.

Also, your patch itself seems not very sensible. Why do you think your
patch matters?

> - ((long)(a) - (long)(b) >= 0))
> + ((long)(b) - (long)(a) <= 0))

Now, tell me why that one line would make any difference, except for the
(undefined) case where we don't know and the time is as far behind as it
is ahead?

Notice: you switch the order of the subtraction, and you switch the sign
of the test. The original code allowed old gcc versions to generate better
code. Your new code doesn't.

What am I missing?

Linus

2005-05-19 01:05:12

by George Anzinger

[permalink] [raw]
Subject: Re: [patch] time_after_eq fix

Coywolf Qi Hunt wrote:
> Hello,
>
> The two macros time_after and time_after_eq were added to do wrapping
> correctly, but only time_after does it the right way, time_after_eq has
> been wrong since the very beginning(v2.1.127, 07-Nov-1998). Now this
> patch fixes it.

I may be especially dense today, but could you give an example where your change
actually gives a result different from what exists?

george
--
>
> And I don't agree with the the original code comment. I don't think this
> is gcc's fault. If it is "a good compiler" or "a really good compiler",
> trying to be smarter than human, it wouldn't still be a C compiler.
> So I'd like it be removed.
>
> Signed-off-by: Coywolf Qi Hunt <[email protected]>
> ---
>
> jiffies.h | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff -pruN 2.6.12-rc4-mm2/include/linux/jiffies.h 2.6.12-rc4-mm2-cy/include/linux/jiffies.h
> --- 2.6.12-rc4-mm2/include/linux/jiffies.h 2005-03-03 17:12:13.000000000 +0800
> +++ 2.6.12-rc4-mm2-cy/include/linux/jiffies.h 2005-05-19 05:32:52.000000000 +0800
> @@ -102,9 +102,7 @@ static inline u64 get_jiffies_64(void)
> *
> * time_after(a,b) returns true if the time a is after time b.
> *
> - * Do this with "<0" and ">=0" to only test the sign of the result. A
> - * good compiler would generate better code (and a really good compiler
> - * wouldn't care). Gcc is currently neither.
> + * Do this with "<0" and "<=0" to only test the sign of the result.
> */
> #define time_after(a,b) \
> (typecheck(unsigned long, a) && \
> @@ -115,7 +113,7 @@ static inline u64 get_jiffies_64(void)
> #define time_after_eq(a,b) \
> (typecheck(unsigned long, a) && \
> typecheck(unsigned long, b) && \
> - ((long)(a) - (long)(b) >= 0))
> + ((long)(b) - (long)(a) <= 0))
> #define time_before_eq(a,b) time_after_eq(b,a)
>
> /*
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/