Changes from previous version:
1. Add comments
2. Change names easy to understand. For example, now time_now_after(a) means time now after a will return true.
---
Signed-off-by: Dave Young <[email protected]>
---
include/linux/jiffies.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff -upr linux/include/linux/jiffies.h linux.new/include/linux/jiffies.h
--- linux/include/linux/jiffies.h 2008-03-07 19:56:02.000000000 +0800
+++ linux.new/include/linux/jiffies.h 2008-03-07 20:10:25.000000000 +0800
@@ -135,6 +135,22 @@ static inline u64 get_jiffies_64(void)
#define time_before_eq64(a,b) time_after_eq64(b,a)
/*
+ * These four macros compare jiffies and 'a' for convenience.
+ */
+
+/* time_now_after(a) return true if now (jiffies) is after a */
+#define time_now_after(a) time_after(jiffies, a)
+
+/* time_now_before(a) return true if now (jiffies) is before a */
+#define time_now_before(a) time_before(jiffies, a)
+
+/* time_now_after_eq(a) return true if now (jiffies) is after or equal to a */
+#define time_now_after_eq(a) time_after_eq(jiffies, a)
+
+/* time_now_before_eq(a) return true if now (jiffies) is before or equal to a */
+#define time_now_before_eq(a) time_before_eq(jiffies, a)
+
+/*
* Have the 32 bit jiffies value wrap 5 minutes after boot
* so jiffies wrap bugs show up earlier.
*/
Hi Dave, Andrew and all
Dave Young <[email protected]> writes:
> +/* time_now_after(a) return true if now (jiffies) is after a */
> +#define time_now_after(a) time_after(jiffies, a)
> +
> +/* time_now_before(a) return true if now (jiffies) is before a */
> +#define time_now_before(a) time_before(jiffies, a)
> +
> +/* time_now_after_eq(a) return true if now (jiffies) is after or equal to a */
> +#define time_now_after_eq(a) time_after_eq(jiffies, a)
> +
> +/* time_now_before_eq(a) return true if now (jiffies) is before or equal to a */
> +#define time_now_before_eq(a) time_before_eq(jiffies, a)
How about even more obvious names like time_is_past(), time_is_future(),
...?
Sorry, I missed v1. Should have proposed that earlier.
Hannes
On Sun, Mar 9, 2008 at 12:12 AM, Johannes Weiner <[email protected]> wrote:
> Hi Dave, Andrew and all
>
>
> Dave Young <[email protected]> writes:
>
> > +/* time_now_after(a) return true if now (jiffies) is after a */
> > +#define time_now_after(a) time_after(jiffies, a)
> > +
> > +/* time_now_before(a) return true if now (jiffies) is before a */
> > +#define time_now_before(a) time_before(jiffies, a)
> > +
> > +/* time_now_after_eq(a) return true if now (jiffies) is after or equal to a */
> > +#define time_now_after_eq(a) time_after_eq(jiffies, a)
> > +
> > +/* time_now_before_eq(a) return true if now (jiffies) is before or equal to a */
> > +#define time_now_before_eq(a) time_before_eq(jiffies, a)
>
> How about even more obvious names like time_is_past(), time_is_future(),
> ...?
Thanks for comment.
Then how do we name the _eq version? IMHO, the time_now_* is enough.
>
> Sorry, I missed v1. Should have proposed that earlier.
>
> Hannes
>
Regards
dave
> > > +/* time_now_before_eq(a) return true if now (jiffies) is before or equal to a */
> > > +#define time_now_before_eq(a) time_before_eq(jiffies, a)
> >
> > How about even more obvious names like time_is_past(), time_is_future(),
> > ...?
>
> Thanks for comment.
>
> Then how do we name the _eq version? IMHO, the time_now_* is enough.
Why do you even need them. I don't see the point of *any* of these extra
macros as they simply obfuscate code and hide what is actually going on.
The initial macros were added because of the type safety and correct
comparison rules being complex. They have a purpose.
Even if you want these you can use !time_future() if you don't want the
_eq variants. Generally speaking drivers should be using timers not
polled loops, and most of our loops comparing with jiffies want removing.
Alan
On 3/9/08, Alan Cox <[email protected]> wrote:
> > > > +/* time_now_before_eq(a) return true if now (jiffies) is before or equal to a */
> > > > +#define time_now_before_eq(a) time_before_eq(jiffies, a)
> > >
> > > How about even more obvious names like time_is_past(), time_is_future(),
> > > ...?
> >
> > Thanks for comment.
> >
> > Then how do we name the _eq version? IMHO, the time_now_* is enough.
>
>
> Why do you even need them. I don't see the point of *any* of these extra
> macros as they simply obfuscate code and hide what is actually going on.
> The initial macros were added because of the type safety and correct
> comparison rules being complex. They have a purpose.
Yes, This has a purpose as well. You will find most of the usage of these
macros are just compare some value with jiffies after doing some grep.
For these cases this adding will easy use and understand.
>
> Even if you want these you can use !time_future() if you don't want the
> _eq variants. Generally speaking drivers should be using timers not
> polled loops, and most of our loops comparing with jiffies want removing.
>
IMO time_after is a confusing name actually, If I don't read the comment I
will think it as time_future
To some extent I agree with you that the timer will be better.
>
> Alan
>
Thanks
> Yes, This has a purpose as well. You will find most of the usage of these
> macros are just compare some value with jiffies after doing some grep.
That is no reason to add more macros that hide how they do it, or make
jiffies itself invisible to the grep search or variable usage finding
tools (and for tickless that very thing is important)
> For these cases this adding will easy use and understand.
It hides information.
Alan
On Sun, 9 Mar 2008 09:58:02 +0000 Alan Cox <[email protected]> wrote:
> > > > +/* time_now_before_eq(a) return true if now (jiffies) is before or equal to a */
> > > > +#define time_now_before_eq(a) time_before_eq(jiffies, a)
> > >
> > > How about even more obvious names like time_is_past(), time_is_future(),
> > > ...?
> >
> > Thanks for comment.
> >
> > Then how do we name the _eq version? IMHO, the time_now_* is enough.
>
> Why do you even need them. I don't see the point of *any* of these extra
> macros as they simply obfuscate code and hide what is actually going on.
Two reasons:
a) the existing macros are (I believe) a right royal pita. It's very
hard to remember which order the args are supposed to be in.
So each time I see a time_foo() when reviewing a patch I have to go
off and re-read the implementation then have a big think to check that
they got it right (a sure sign of a poor interface, no)?
And I'm not the only one - people get this wrong fairly regularly.
b) around 90% of the usages of time_after() are to compare against jiffies!
The macros which Dave is developing aren't just less-error-prone,
easier-to-review transformations - they offer higher-level functionality.
Because time_after() is just a basic comparison operation, whereas
time_now_before() is an *application* of that operation.
Trust me on this - they will lead to easier-to-review code and less bugs.
> The initial macros were added because of the type safety and correct
> comparison rules being complex. They have a purpose.
They are hard to use and hard to review.
> Even if you want these you can use !time_future() if you don't want the
> _eq variants. Generally speaking drivers should be using timers not
> polled loops, and most of our loops comparing with jiffies want removing.
On Sun, 9 Mar 2008 11:08:08 +0000 Alan Cox <[email protected]> wrote:
> > Yes, This has a purpose as well. You will find most of the usage of these
> > macros are just compare some value with jiffies after doing some grep.
>
> That is no reason to add more macros that hide how they do it, or make
> jiffies itself invisible to the grep search or variable usage finding
> tools (and for tickless that very thing is important)
We could call them time_is_after_jiffies(), time_is_before_jiffies(), etc.
Those names are actually better than time_after_now(), etc.
> a) the existing macros are (I believe) a right royal pita. It's very
> hard to remember which order the args are supposed to be in.
Its operator(first, second) - the C++ infix operators aren't available in
C so there isn't much choice.
Really we should be trying to stamp out all these uses of jiffies tests
so that we can make tickless work ever better.
Alan
> We could call them time_is_after_jiffies(), time_is_before_jiffies(), etc.
>
> Those names are actually better than time_after_now(), etc.
Agreed - and if you think its worth doing I bow to your expertise on
buggy patch submissions..
On Mon, Mar 10, 2008 at 3:01 AM, Andrew Morton
<[email protected]> wrote:
> On Sun, 9 Mar 2008 11:08:08 +0000 Alan Cox <[email protected]> wrote:
>
> > > Yes, This has a purpose as well. You will find most of the usage of these
> > > macros are just compare some value with jiffies after doing some grep.
> >
> > That is no reason to add more macros that hide how they do it, or make
> > jiffies itself invisible to the grep search or variable usage finding
> > tools (and for tickless that very thing is important)
>
> We could call them time_is_after_jiffies(), time_is_before_jiffies(), etc.
>
> Those names are actually better than time_after_now(), etc.
>
>
Andrew, Hannes, Alan,
thanks for your comment, I have send the new time_is_* patch as v3.
Hannes, It's more like your is_past/is_future but looks better when
along with _eq
Hi Dave,
"Dave Young" <[email protected]> writes:
> Hannes, It's more like your is_past/is_future but looks better when
> along with _eq
I agree. And it's better to have `jiffies' in the names, anyway.
Hannes