Thomas,
My jitter test has been bothering me that I was always get a large
jitter for the smallest increment. For example:
reported res = 0.000001000
starting calibrate
finished calibrate: 367.9413MHz 367941272
Requested timex Max Min error
--------------- --- --- -----
0.000001000 0.000206748 0.000014750 0.000205748 (205.748 usecs)
0.000010000 0.000021071 0.000018930 0.000011071 (11.071 usecs)
0.000100000 0.000113026 0.000108795 0.000013026 (13.026 usecs)
0.001000000 0.001022049 0.001009267 0.000022049 (22.049 usecs)
The test of 1us would seem to always have a jitter of a few hundreds of
usecs. So looking at the code, I found the reason. And this is my
solution:
When sys_nanosleep got down to enqueue_hrtimer, if the time has passed
since it took to get there, here's the current code of what is done:
static void enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
{
[...]
if (!base->first || timer->expires.tv64 <
rb_entry(base->first, struct hrtimer, node)->expires.tv64) {
#ifdef CONFIG_HIGH_RES_TIMERS
/*
* High resolution timers, when active try
* to reprogram. If the timer is in the
* past we just move it to the expired list
* and schedule the softirq.
*/
if (hrtimer_hres_active && hrtimer_reprogram(timer, base)) {
list_add_tail(&timer->list, &base->expired);
timer->state = HRTIMER_PENDING_CALLBACK;
raise_softirq(HRTIMER_SOFTIRQ);
return;
}
#endif
base->first = &timer->node;
}
[...]
The timer would be put on the expired queue, the hrtimer softirqd would
be woken up, and the task put to sleep. So instead of returning
immediately, the nanosleep, would have to be scheduled out, the softirq
woken up, and then the task woken back up. This wastes sever hundreds of
usecs as was shown in the jitter test.
My patch does the following:
- Changes enqueue_hrtimer from void to int and returns 1 and does
nothing else in the case of the timer has passed.
- Change hrtimer_start to return 1 if the timer has passed and not when
the timer was active. I searched the kernel and I could not find one
instance where this hrtimer_start had its return code checked.
- Changed schedule_hrtimer to not go to sleep if the time has passed.
So with the patch I get the following jitter:
reported res = 0.000001000
starting calibrate
finished calibrate: 367.9363MHz 367936300
Requested timex Max Min error
--------------- --- --- -----
0.000001000 0.000018185 0.000003998 0.000017185 (17.185 usecs)
0.000010000 0.000021381 0.000018278 0.000011381 (11.381 usecs)
0.000100000 0.000109614 0.000108443 0.000009614 (9.614 usecs)
The jitter test can be found here:
http://www.kihontech.com/tests/hrtimer/jitter_inc.c
10 consecutive runs of the jitter test without the patch.
http://www.kihontech.com/tests/hrtimer/jitter_nopatch.txt
10 consecutive runs of the jitter test with the patch.
http://www.kihontech.com/tests/hrtimer/jitter_patch.txt
I'll be adding this patch to my 2.6.15-rt1-sr2 patch to be released soon.
-- Steve
Index: linux-2.6.15-rt1/kernel/hrtimer.c
===================================================================
--- linux-2.6.15-rt1.orig/kernel/hrtimer.c 2006-01-03 07:41:48.000000000 -0500
+++ linux-2.6.15-rt1/kernel/hrtimer.c 2006-01-05 22:17:45.000000000 -0500
@@ -226,6 +226,10 @@
* for which the hrt time source was armed.
*
* Called with interrupts disabled and base lock held
+ *
+ * Returns:
+ * 0 on success
+ * 1 if time has already past.
*/
static int hrtimer_reprogram(struct hrtimer *timer, struct hrtimer_base *base)
{
@@ -239,6 +243,8 @@
res = clockevents_set_next_event(expires);
if (!res)
*expires_next = expires;
+ else
+ res = 1;
return res;
}
@@ -381,11 +387,24 @@
smp_processor_id());
}
+/*
+ * kick_off_hrtimer - queue the timer to the expire list and
+ * raise the hrtimer softirq.
+ */
+static inline void
+kick_off_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
+{
+ list_add_tail(&timer->list, &base->expired);
+ timer->state = HRTIMER_PENDING_CALLBACK;
+ raise_softirq(HRTIMER_SOFTIRQ);
+}
+
#else /* CONFIG_HIGH_RES_TIMERS */
# define hrtimer_hres_active 0
# define hres_enqueue_expired(t,b,n) 0
# define hrtimer_check_clocks() do { } while (0)
+# define kick_off_hrtimer do { } while (0)
#endif /* !CONFIG_HIGH_RES_TIMERS */
@@ -501,8 +520,12 @@
*
* The timer is inserted in expiry order. Insertion into the
* red black tree is O(log(n)). Must hold the base lock.
+ *
+ * Returns:
+ * 0 on success
+ * 1 if time has already past.
*/
-static void enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
+static int enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
{
struct rb_node **link = &base->active.rb_node;
struct rb_node *parent = NULL;
@@ -534,12 +557,8 @@
* past we just move it to the expired list
* and schedule the softirq.
*/
- if (hrtimer_hres_active && hrtimer_reprogram(timer, base)) {
- list_add_tail(&timer->list, &base->expired);
- timer->state = HRTIMER_PENDING_CALLBACK;
- raise_softirq(HRTIMER_SOFTIRQ);
- return;
- }
+ if (hrtimer_hres_active && hrtimer_reprogram(timer, base))
+ return 1;
#endif
base->first = &timer->node;
}
@@ -551,6 +570,7 @@
rb_insert_color(&timer->node, &base->active);
timer->state = HRTIMER_PENDING;
+ return 0;
}
/*
@@ -598,7 +618,7 @@
*
* Returns:
* 0 on success
- * 1 when the timer was active
+ * 1 if the time has already past
*/
int
hrtimer_start(struct hrtimer *timer, ktime_t tim, const enum hrtimer_mode mode)
@@ -610,7 +630,7 @@
base = lock_hrtimer_base(timer, &flags);
/* Remove an active timer from the queue: */
- ret = remove_hrtimer(timer, base);
+ remove_hrtimer(timer, base);
/* Switch the timer base, if necessary: */
new_base = switch_hrtimer_base(timer, base);
@@ -619,7 +639,7 @@
tim = ktime_add(tim, new_base->get_time());
timer->expires = tim;
- enqueue_hrtimer(timer, new_base);
+ ret = enqueue_hrtimer(timer, new_base);
unlock_hrtimer_base(timer, &flags);
@@ -864,9 +884,10 @@
spin_lock_irq(&base->lock);
- if (restart == HRTIMER_RESTART)
- enqueue_hrtimer(timer, base);
- else
+ if (restart == HRTIMER_RESTART) {
+ if (enqueue_hrtimer(timer, base))
+ kick_off_hrtimer(timer, base);
+ } else
timer->state = HRTIMER_EXPIRED;
set_curr_timer(base, NULL);
}
@@ -922,9 +943,10 @@
spin_lock_irq(&base->lock);
- if (restart == HRTIMER_RESTART)
- enqueue_hrtimer(timer, base);
- else
+ if (restart == HRTIMER_RESTART) {
+ if (enqueue_hrtimer(timer, base))
+ kick_off_hrtimer(timer, base);
+ } else
timer->state = HRTIMER_EXPIRED;
set_curr_timer(base, NULL);
}
@@ -983,9 +1005,13 @@
/* fn stays NULL, meaning single-shot wakeup: */
timer->data = current;
- hrtimer_start(timer, timer->expires, mode);
+ if (hrtimer_start(timer, timer->expires, mode)) {
+ /* time already past */
+ timer->state = HRTIMER_EXPIRED;
+ set_current_state(TASK_RUNNING);
+ } else
+ schedule();
- schedule();
hrtimer_cancel(timer);
/* Return the remaining time: */
@@ -1128,7 +1154,8 @@
timer = rb_entry(node, struct hrtimer, node);
__remove_hrtimer(timer, old_base);
timer->base = new_base;
- enqueue_hrtimer(timer, new_base);
+ if (enqueue_hrtimer(timer, new_base))
+ kick_off_hrtimer(timer, base);
}
}
Steven Rostedt wrote:
> Thomas,
~
> usecs as was shown in the jitter test.
>
> My patch does the following:
>
> - Changes enqueue_hrtimer from void to int and returns 1 and does
> nothing else in the case of the timer has passed.
>
> - Change hrtimer_start to return 1 if the timer has passed and not when
> the timer was active. I searched the kernel and I could not find one
> instance where this hrtimer_start had its return code checked.
>
> - Changed schedule_hrtimer to not go to sleep if the time has passed.
At this time the posix timer code depends on the call back. Either you will
need to make this an option or change that code to do the right thing.
George
>
> So with the patch I get the following jitter:
>
> reported res = 0.000001000
> starting calibrate
> finished calibrate: 367.9363MHz 367936300
>
> Requested timex Max Min error
> --------------- --- --- -----
> 0.000001000 0.000018185 0.000003998 0.000017185 (17.185 usecs)
> 0.000010000 0.000021381 0.000018278 0.000011381 (11.381 usecs)
> 0.000100000 0.000109614 0.000108443 0.000009614 (9.614 usecs)
>
>
> The jitter test can be found here:
>
> http://www.kihontech.com/tests/hrtimer/jitter_inc.c
>
> 10 consecutive runs of the jitter test without the patch.
> http://www.kihontech.com/tests/hrtimer/jitter_nopatch.txt
>
> 10 consecutive runs of the jitter test with the patch.
> http://www.kihontech.com/tests/hrtimer/jitter_patch.txt
>
>
> I'll be adding this patch to my 2.6.15-rt1-sr2 patch to be released soon.
>
> -- Steve
>
> Index: linux-2.6.15-rt1/kernel/hrtimer.c
> ===================================================================
> --- linux-2.6.15-rt1.orig/kernel/hrtimer.c 2006-01-03 07:41:48.000000000 -0500
> +++ linux-2.6.15-rt1/kernel/hrtimer.c 2006-01-05 22:17:45.000000000 -0500
> @@ -226,6 +226,10 @@
> * for which the hrt time source was armed.
> *
> * Called with interrupts disabled and base lock held
> + *
> + * Returns:
> + * 0 on success
> + * 1 if time has already past.
> */
> static int hrtimer_reprogram(struct hrtimer *timer, struct hrtimer_base *base)
> {
> @@ -239,6 +243,8 @@
> res = clockevents_set_next_event(expires);
> if (!res)
> *expires_next = expires;
> + else
> + res = 1;
> return res;
> }
>
> @@ -381,11 +387,24 @@
> smp_processor_id());
> }
>
> +/*
> + * kick_off_hrtimer - queue the timer to the expire list and
> + * raise the hrtimer softirq.
> + */
> +static inline void
> +kick_off_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
> +{
> + list_add_tail(&timer->list, &base->expired);
> + timer->state = HRTIMER_PENDING_CALLBACK;
> + raise_softirq(HRTIMER_SOFTIRQ);
> +}
> +
> #else /* CONFIG_HIGH_RES_TIMERS */
>
> # define hrtimer_hres_active 0
> # define hres_enqueue_expired(t,b,n) 0
> # define hrtimer_check_clocks() do { } while (0)
> +# define kick_off_hrtimer do { } while (0)
>
> #endif /* !CONFIG_HIGH_RES_TIMERS */
>
> @@ -501,8 +520,12 @@
> *
> * The timer is inserted in expiry order. Insertion into the
> * red black tree is O(log(n)). Must hold the base lock.
> + *
> + * Returns:
> + * 0 on success
> + * 1 if time has already past.
> */
> -static void enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
> +static int enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
> {
> struct rb_node **link = &base->active.rb_node;
> struct rb_node *parent = NULL;
> @@ -534,12 +557,8 @@
> * past we just move it to the expired list
> * and schedule the softirq.
> */
> - if (hrtimer_hres_active && hrtimer_reprogram(timer, base)) {
> - list_add_tail(&timer->list, &base->expired);
> - timer->state = HRTIMER_PENDING_CALLBACK;
> - raise_softirq(HRTIMER_SOFTIRQ);
> - return;
> - }
> + if (hrtimer_hres_active && hrtimer_reprogram(timer, base))
> + return 1;
> #endif
> base->first = &timer->node;
> }
> @@ -551,6 +570,7 @@
> rb_insert_color(&timer->node, &base->active);
>
> timer->state = HRTIMER_PENDING;
> + return 0;
> }
>
> /*
> @@ -598,7 +618,7 @@
> *
> * Returns:
> * 0 on success
> - * 1 when the timer was active
> + * 1 if the time has already past
> */
> int
> hrtimer_start(struct hrtimer *timer, ktime_t tim, const enum hrtimer_mode mode)
> @@ -610,7 +630,7 @@
> base = lock_hrtimer_base(timer, &flags);
>
> /* Remove an active timer from the queue: */
> - ret = remove_hrtimer(timer, base);
> + remove_hrtimer(timer, base);
>
> /* Switch the timer base, if necessary: */
> new_base = switch_hrtimer_base(timer, base);
> @@ -619,7 +639,7 @@
> tim = ktime_add(tim, new_base->get_time());
> timer->expires = tim;
>
> - enqueue_hrtimer(timer, new_base);
> + ret = enqueue_hrtimer(timer, new_base);
>
> unlock_hrtimer_base(timer, &flags);
>
> @@ -864,9 +884,10 @@
>
> spin_lock_irq(&base->lock);
>
> - if (restart == HRTIMER_RESTART)
> - enqueue_hrtimer(timer, base);
> - else
> + if (restart == HRTIMER_RESTART) {
> + if (enqueue_hrtimer(timer, base))
> + kick_off_hrtimer(timer, base);
> + } else
> timer->state = HRTIMER_EXPIRED;
> set_curr_timer(base, NULL);
> }
> @@ -922,9 +943,10 @@
>
> spin_lock_irq(&base->lock);
>
> - if (restart == HRTIMER_RESTART)
> - enqueue_hrtimer(timer, base);
> - else
> + if (restart == HRTIMER_RESTART) {
> + if (enqueue_hrtimer(timer, base))
> + kick_off_hrtimer(timer, base);
> + } else
> timer->state = HRTIMER_EXPIRED;
> set_curr_timer(base, NULL);
> }
> @@ -983,9 +1005,13 @@
> /* fn stays NULL, meaning single-shot wakeup: */
> timer->data = current;
>
> - hrtimer_start(timer, timer->expires, mode);
> + if (hrtimer_start(timer, timer->expires, mode)) {
> + /* time already past */
> + timer->state = HRTIMER_EXPIRED;
> + set_current_state(TASK_RUNNING);
> + } else
> + schedule();
>
> - schedule();
> hrtimer_cancel(timer);
>
> /* Return the remaining time: */
> @@ -1128,7 +1154,8 @@
> timer = rb_entry(node, struct hrtimer, node);
> __remove_hrtimer(timer, old_base);
> timer->base = new_base;
> - enqueue_hrtimer(timer, new_base);
> + if (enqueue_hrtimer(timer, new_base))
> + kick_off_hrtimer(timer, base);
> }
> }
>
>
>
> -
> 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]
HRT (High-res-timers): http://sourceforge.net/projects/high-res-timers/
On Fri, 2006-01-06 at 14:27 -0800, George Anzinger wrote:
> Steven Rostedt wrote:
> > Thomas,
> ~
>
> > usecs as was shown in the jitter test.
> >
> > My patch does the following:
> >
> > - Changes enqueue_hrtimer from void to int and returns 1 and does
> > nothing else in the case of the timer has passed.
> >
> > - Change hrtimer_start to return 1 if the timer has passed and not when
> > the timer was active. I searched the kernel and I could not find one
> > instance where this hrtimer_start had its return code checked.
> >
> > - Changed schedule_hrtimer to not go to sleep if the time has passed.
>
> At this time the posix timer code depends on the call back. Either you will
> need to make this an option or change that code to do the right thing.
George,
Thanks for showing me this. How about the following patch. It leaves
hrtimer_restart queuing the timer by adding an internal function
__hrtimer_restart that adds the option "queue". Since the only time you
don't want to queue it (that I know of) is internally to nanosleep. But
then again maybe others will want too. But anyway, this keeps the
current API to hrtimers unchanged.
-- Steve
Index: linux-2.6.15-rt2/kernel/hrtimer.c
===================================================================
--- linux-2.6.15-rt2.orig/kernel/hrtimer.c 2006-01-06 17:49:47.000000000 -0500
+++ linux-2.6.15-rt2/kernel/hrtimer.c 2006-01-06 17:53:25.000000000 -0500
@@ -226,6 +226,10 @@
* for which the hrt time source was armed.
*
* Called with interrupts disabled and base lock held
+ *
+ * Returns:
+ * 0 on success
+ * 1 if time has already past.
*/
static int hrtimer_reprogram(struct hrtimer *timer, struct hrtimer_base *base)
{
@@ -239,6 +243,8 @@
res = clockevents_set_next_event(expires);
if (!res)
*expires_next = expires;
+ else
+ res = 1;
return res;
}
@@ -381,11 +387,24 @@
smp_processor_id());
}
+/*
+ * kick_off_hrtimer - queue the timer to the expire list and
+ * raise the hrtimer softirq.
+ */
+static void
+kick_off_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
+{
+ list_add_tail(&timer->list, &base->expired);
+ timer->state = HRTIMER_PENDING_CALLBACK;
+ raise_softirq(HRTIMER_SOFTIRQ);
+}
+
#else /* CONFIG_HIGH_RES_TIMERS */
# define hrtimer_hres_active 0
# define hres_enqueue_expired(t,b,n) 0
# define hrtimer_check_clocks() do { } while (0)
+# define kick_off_hrtimer do { } while (0)
#endif /* !CONFIG_HIGH_RES_TIMERS */
@@ -501,9 +520,14 @@
*
* The timer is inserted in expiry order. Insertion into the
* red black tree is O(log(n)). Must hold the base lock.
+ *
+ * Returns:
+ * 0 on success
+ * 1 if time has already past.
*/
-static void enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
+static int enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
{
+
struct rb_node **link = &base->active.rb_node;
struct rb_node *parent = NULL;
struct hrtimer *entry;
@@ -534,12 +558,8 @@
* past we just move it to the expired list
* and schedule the softirq.
*/
- if (hrtimer_hres_active && hrtimer_reprogram(timer, base)) {
- list_add_tail(&timer->list, &base->expired);
- timer->state = HRTIMER_PENDING_CALLBACK;
- raise_softirq(HRTIMER_SOFTIRQ);
- return;
- }
+ if (hrtimer_hres_active && hrtimer_reprogram(timer, base))
+ return 1;
#endif
base->first = &timer->node;
}
@@ -551,6 +571,7 @@
rb_insert_color(&timer->node, &base->active);
timer->state = HRTIMER_PENDING;
+ return 0;
}
/*
@@ -598,10 +619,11 @@
*
* Returns:
* 0 on success
- * 1 when the timer was active
+ * 1 if the time has already past
*/
-int
-hrtimer_start(struct hrtimer *timer, ktime_t tim, const enum hrtimer_mode mode)
+static int
+__hrtimer_start(struct hrtimer *timer, ktime_t tim, const enum hrtimer_mode mode,
+ int queue)
{
struct hrtimer_base *base, *new_base;
unsigned long flags;
@@ -610,7 +632,7 @@
base = lock_hrtimer_base(timer, &flags);
/* Remove an active timer from the queue: */
- ret = remove_hrtimer(timer, base);
+ remove_hrtimer(timer, base);
/* Switch the timer base, if necessary: */
new_base = switch_hrtimer_base(timer, base);
@@ -619,13 +641,21 @@
tim = ktime_add(tim, new_base->get_time());
timer->expires = tim;
- enqueue_hrtimer(timer, new_base);
+ if ((ret = enqueue_hrtimer(timer, new_base)) && queue)
+ kick_off_hrtimer(timer, new_base);
unlock_hrtimer_base(timer, &flags);
return ret;
}
+int
+hrtimer_start(struct hrtimer *timer, ktime_t tim, const enum hrtimer_mode mode)
+{
+ return __hrtimer_start(timer, tim, mode, 1);
+}
+
+
/**
* hrtimer_try_to_cancel - try to deactivate a timer
*
@@ -864,9 +894,10 @@
spin_lock_irq(&base->lock);
- if (restart == HRTIMER_RESTART)
- enqueue_hrtimer(timer, base);
- else
+ if (restart == HRTIMER_RESTART) {
+ if (enqueue_hrtimer(timer, base))
+ kick_off_hrtimer(timer, base);
+ } else
timer->state = HRTIMER_EXPIRED;
set_curr_timer(base, NULL);
}
@@ -922,9 +953,10 @@
spin_lock_irq(&base->lock);
- if (restart == HRTIMER_RESTART)
- enqueue_hrtimer(timer, base);
- else
+ if (restart == HRTIMER_RESTART) {
+ if (enqueue_hrtimer(timer, base))
+ kick_off_hrtimer(timer, base);
+ } else
timer->state = HRTIMER_EXPIRED;
set_curr_timer(base, NULL);
}
@@ -983,9 +1015,13 @@
/* fn stays NULL, meaning single-shot wakeup: */
timer->data = current;
- hrtimer_start(timer, timer->expires, mode);
+ if (__hrtimer_start(timer, timer->expires, mode, 0)) {
+ /* time already past */
+ timer->state = HRTIMER_EXPIRED;
+ set_current_state(TASK_RUNNING);
+ } else
+ schedule();
- schedule();
hrtimer_cancel(timer);
/* Return the remaining time: */
@@ -1128,7 +1164,8 @@
timer = rb_entry(node, struct hrtimer, node);
__remove_hrtimer(timer, old_base);
timer->base = new_base;
- enqueue_hrtimer(timer, new_base);
+ if (enqueue_hrtimer(timer, new_base))
+ kick_off_hrtimer(timer, base);
}
}
Steven Rostedt wrote:
> On Fri, 2006-01-06 at 14:27 -0800, George Anzinger wrote:
>
>>Steven Rostedt wrote:
>>
>>>Thomas,
>>
>>~
>>
>>
>>>usecs as was shown in the jitter test.
>>>
>>>My patch does the following:
>>>
>>>- Changes enqueue_hrtimer from void to int and returns 1 and does
>>> nothing else in the case of the timer has passed.
>>>
>>>- Change hrtimer_start to return 1 if the timer has passed and not when
>>> the timer was active. I searched the kernel and I could not find one
>>> instance where this hrtimer_start had its return code checked.
>>>
>>>- Changed schedule_hrtimer to not go to sleep if the time has passed.
>>
>>At this time the posix timer code depends on the call back. Either you will
>>need to make this an option or change that code to do the right thing.
>
>
> George,
>
> Thanks for showing me this. How about the following patch. It leaves
> hrtimer_restart queuing the timer by adding an internal function
> __hrtimer_restart that adds the option "queue". Since the only time you
> don't want to queue it (that I know of) is internally to nanosleep. But
> then again maybe others will want too. But anyway, this keeps the
> current API to hrtimers unchanged.
Uh... I have been wondering about the "mode" thing, thinking "flags" might be
better. And allowing, say, a "return if elapsed" flag as well as the ABS
flag. Then all you would need to do is to add the "return if elapsed" flag
to the nanosleep calls. I have other reasons for wanting to expand the
"mode" to more that two states... but, even with out that, I think the result
would be a) less code, and b) easier to follow and understand. I just have
trouble pushing a word on the stack to make a call and then use only one bit
of it when it could be combined...
Never the less, the following code looks like is does the right thing.
George
>
> -- Steve
>
> Index: linux-2.6.15-rt2/kernel/hrtimer.c
> ===================================================================
> --- linux-2.6.15-rt2.orig/kernel/hrtimer.c 2006-01-06 17:49:47.000000000 -0500
> +++ linux-2.6.15-rt2/kernel/hrtimer.c 2006-01-06 17:53:25.000000000 -0500
> @@ -226,6 +226,10 @@
> * for which the hrt time source was armed.
> *
> * Called with interrupts disabled and base lock held
> + *
> + * Returns:
> + * 0 on success
> + * 1 if time has already past.
> */
> static int hrtimer_reprogram(struct hrtimer *timer, struct hrtimer_base *base)
> {
> @@ -239,6 +243,8 @@
> res = clockevents_set_next_event(expires);
> if (!res)
> *expires_next = expires;
> + else
> + res = 1;
> return res;
> }
>
> @@ -381,11 +387,24 @@
> smp_processor_id());
> }
>
> +/*
> + * kick_off_hrtimer - queue the timer to the expire list and
> + * raise the hrtimer softirq.
> + */
> +static void
> +kick_off_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
> +{
> + list_add_tail(&timer->list, &base->expired);
> + timer->state = HRTIMER_PENDING_CALLBACK;
> + raise_softirq(HRTIMER_SOFTIRQ);
> +}
> +
> #else /* CONFIG_HIGH_RES_TIMERS */
>
> # define hrtimer_hres_active 0
> # define hres_enqueue_expired(t,b,n) 0
> # define hrtimer_check_clocks() do { } while (0)
> +# define kick_off_hrtimer do { } while (0)
>
> #endif /* !CONFIG_HIGH_RES_TIMERS */
>
> @@ -501,9 +520,14 @@
> *
> * The timer is inserted in expiry order. Insertion into the
> * red black tree is O(log(n)). Must hold the base lock.
> + *
> + * Returns:
> + * 0 on success
> + * 1 if time has already past.
> */
> -static void enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
> +static int enqueue_hrtimer(struct hrtimer *timer, struct hrtimer_base *base)
> {
> +
> struct rb_node **link = &base->active.rb_node;
> struct rb_node *parent = NULL;
> struct hrtimer *entry;
> @@ -534,12 +558,8 @@
> * past we just move it to the expired list
> * and schedule the softirq.
> */
> - if (hrtimer_hres_active && hrtimer_reprogram(timer, base)) {
> - list_add_tail(&timer->list, &base->expired);
> - timer->state = HRTIMER_PENDING_CALLBACK;
> - raise_softirq(HRTIMER_SOFTIRQ);
> - return;
> - }
> + if (hrtimer_hres_active && hrtimer_reprogram(timer, base))
> + return 1;
> #endif
> base->first = &timer->node;
> }
> @@ -551,6 +571,7 @@
> rb_insert_color(&timer->node, &base->active);
>
> timer->state = HRTIMER_PENDING;
> + return 0;
> }
>
> /*
> @@ -598,10 +619,11 @@
> *
> * Returns:
> * 0 on success
> - * 1 when the timer was active
> + * 1 if the time has already past
> */
> -int
> -hrtimer_start(struct hrtimer *timer, ktime_t tim, const enum hrtimer_mode mode)
> +static int
> +__hrtimer_start(struct hrtimer *timer, ktime_t tim, const enum hrtimer_mode mode,
> + int queue)
> {
> struct hrtimer_base *base, *new_base;
> unsigned long flags;
> @@ -610,7 +632,7 @@
> base = lock_hrtimer_base(timer, &flags);
>
> /* Remove an active timer from the queue: */
> - ret = remove_hrtimer(timer, base);
> + remove_hrtimer(timer, base);
>
> /* Switch the timer base, if necessary: */
> new_base = switch_hrtimer_base(timer, base);
> @@ -619,13 +641,21 @@
> tim = ktime_add(tim, new_base->get_time());
> timer->expires = tim;
>
> - enqueue_hrtimer(timer, new_base);
> + if ((ret = enqueue_hrtimer(timer, new_base)) && queue)
> + kick_off_hrtimer(timer, new_base);
>
> unlock_hrtimer_base(timer, &flags);
>
> return ret;
> }
>
> +int
> +hrtimer_start(struct hrtimer *timer, ktime_t tim, const enum hrtimer_mode mode)
> +{
> + return __hrtimer_start(timer, tim, mode, 1);
> +}
> +
> +
> /**
> * hrtimer_try_to_cancel - try to deactivate a timer
> *
> @@ -864,9 +894,10 @@
>
> spin_lock_irq(&base->lock);
>
> - if (restart == HRTIMER_RESTART)
> - enqueue_hrtimer(timer, base);
> - else
> + if (restart == HRTIMER_RESTART) {
> + if (enqueue_hrtimer(timer, base))
> + kick_off_hrtimer(timer, base);
> + } else
> timer->state = HRTIMER_EXPIRED;
> set_curr_timer(base, NULL);
> }
> @@ -922,9 +953,10 @@
>
> spin_lock_irq(&base->lock);
>
> - if (restart == HRTIMER_RESTART)
> - enqueue_hrtimer(timer, base);
> - else
> + if (restart == HRTIMER_RESTART) {
> + if (enqueue_hrtimer(timer, base))
> + kick_off_hrtimer(timer, base);
> + } else
> timer->state = HRTIMER_EXPIRED;
> set_curr_timer(base, NULL);
> }
> @@ -983,9 +1015,13 @@
> /* fn stays NULL, meaning single-shot wakeup: */
> timer->data = current;
>
> - hrtimer_start(timer, timer->expires, mode);
> + if (__hrtimer_start(timer, timer->expires, mode, 0)) {
> + /* time already past */
> + timer->state = HRTIMER_EXPIRED;
> + set_current_state(TASK_RUNNING);
> + } else
> + schedule();
>
> - schedule();
> hrtimer_cancel(timer);
>
> /* Return the remaining time: */
> @@ -1128,7 +1164,8 @@
> timer = rb_entry(node, struct hrtimer, node);
> __remove_hrtimer(timer, old_base);
> timer->base = new_base;
> - enqueue_hrtimer(timer, new_base);
> + if (enqueue_hrtimer(timer, new_base))
> + kick_off_hrtimer(timer, base);
> }
> }
>
>
>
> -
> 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]
HRT (High-res-timers): http://sourceforge.net/projects/high-res-timers/
On Mon, 9 Jan 2006, George Anzinger wrote:
> Steven Rostedt wrote:
>
> Uh... I have been wondering about the "mode" thing, thinking "flags" might be
> better. And allowing, say, a "return if elapsed" flag as well as the ABS
> flag. Then all you would need to do is to add the "return if elapsed" flag
> to the nanosleep calls. I have other reasons for wanting to expand the
> "mode" to more that two states... but, even with out that, I think the result
> would be a) less code, and b) easier to follow and understand. I just have
> trouble pushing a word on the stack to make a call and then use only one bit
> of it when it could be combined...
And I perfectly agree with you :)
The problem is that the hrtimes is not my code, and I don't like doing
too many changes in code that I don't understand the consequences of. As
you showed me earlier, that the previous change broke the posix_timers.
So I really only did the bare minimum to fix what I considered a bug, and
let Thomas, John, Ingo or yourself do a proper fix. Someone that
understands the timers better than I do.
Currently, it seems those people are too busy, and I just wanted a quick
fix. I personally didn't like the patch, but my nose is stuck more into
the scheduling, memory and Ingo's rt_mutex now to spend time understanding
all the timer code. ;)
>
> Never the less, the following code looks like is does the right thing.
>
This was all I asked for.
Thanks,
-- Steve
Steven Rostedt wrote:
> On Mon, 9 Jan 2006, George Anzinger wrote:
>
>>Steven Rostedt wrote:
>>
>>Uh... I have been wondering about the "mode" thing, thinking "flags" might be
>>better. And allowing, say, a "return if elapsed" flag as well as the ABS
>>flag. Then all you would need to do is to add the "return if elapsed" flag
>>to the nanosleep calls. I have other reasons for wanting to expand the
>>"mode" to more that two states... but, even with out that, I think the result
>>would be a) less code, and b) easier to follow and understand. I just have
>>trouble pushing a word on the stack to make a call and then use only one bit
>>of it when it could be combined...
>
>
> And I perfectly agree with you :)
>
> The problem is that the hrtimes is not my code, and I don't like doing
> too many changes in code that I don't understand the consequences of. As
> you showed me earlier, that the previous change broke the posix_timers.
> So I really only did the bare minimum to fix what I considered a bug, and
> let Thomas, John, Ingo or yourself do a proper fix. Someone that
> understands the timers better than I do.
>
> Currently, it seems those people are too busy, and I just wanted a quick
> fix. I personally didn't like the patch, but my nose is stuck more into
> the scheduling, memory and Ingo's rt_mutex now to spend time understanding
> all the timer code. ;)
>
I am working on a general clean up of this area. I will roll you idea into
it and see what they say.
>
--
George Anzinger [email protected]
HRT (High-res-timers): http://sourceforge.net/projects/high-res-timers/