On Mon, Aug 18, 2008 at 4:42 PM, Gilles Carry <gilles.carry@xxxxxxxx> wrote:
> This patch makes hrtimers initialized with hrtimer_init_sleeper
> to use another mode and then not be stuck in waitqueues when
> hrtimer_interrupt is very busy.
>
> The new mode is HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFIRQ.
> The above-mentionned timers have been moved from
> HRTIMER_CB_IRQSAFE_NO_SOFTIRQ to
> HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFIRQ.
>
> HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFIRQ timers use a slightly different
> state machine from HRTIMER_CB_IRQSAFE_NO_SOFTIRQ's as when removing the
> timer, __run_hrtimer sets the status to INACTIVE _then_
> wakes up the thread. This way, an awakened thread cannot enter
> hrtimer_cancel before the timer's status has changed.
>
> Signed-off-by: Gilles Carry <gilles.carry@xxxxxxxx>
>
> ---
> include/linux/hrtimer.h | 4 ++++
> kernel/hrtimer.c | 26 +++++++++++++++++++++-----
> 2 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
> index f3867fd..53f4d44 100644
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -49,12 +49,16 @@ enum hrtimer_restart {
> * does not restart the timer
> * HRTIMER_CB_IRQSAFE_NO_SOFTIRQ: Callback must run in hardirq context
> * Special mode for tick emultation
> + * HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ:
> + * Callback must run in hardirq context and
> + * does not restart the timer
> */
> enum hrtimer_cb_mode {
> HRTIMER_CB_SOFTIRQ,
> HRTIMER_CB_IRQSAFE,
> HRTIMER_CB_IRQSAFE_NO_RESTART,
> HRTIMER_CB_IRQSAFE_NO_SOFTIRQ,
> + HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ,
> };
>
> /*
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index fa63a24..778eb7e 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -668,6 +668,7 @@ static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
> /* Timer is expired, act upon the callback mode */
> switch(timer->cb_mode) {
> case HRTIMER_CB_IRQSAFE_NO_RESTART:
> + case HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ:
> debug_hrtimer_deactivate(timer);
> /*
> * We can call the callback from here. No restart
> @@ -1089,6 +1090,8 @@ int hrtimer_cancel(struct hrtimer *timer)
>
> if (ret >= 0)
> return ret;
> + WARN_ON (timer->cb_mode ==
Minor point here, we may as well squash checkpatch.pl complaints now,
to reduce any clean-up later to push to mainline. if you run
checkpatch.pl
WARNING: space prohibited between function name and open parenthesis '('
#69: FILE: kernel/hrtimer.c:1093:
+ WARN_ON (timer->cb_mode ==
> + HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ);
> hrtimer_wait_for_timer(timer);
> }
> }
> @@ -1298,11 +1301,18 @@ static void __run_hrtimer(struct hrtimer *timer)
> int restart;
>
> debug_hrtimer_deactivate(timer);
> - __remove_hrtimer(timer, base, HRTIMER_STATE_CALLBACK, 0);
> - timer_stats_account_hrtimer(timer);
>
> fn = timer->function;
> - if (timer->cb_mode == HRTIMER_CB_IRQSAFE_NO_SOFTIRQ) {
> +
> + switch (timer->cb_mode) {
> + case HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ:
> + __remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
Correct me if I'm wrong but there is a lot of code reworking and code
expansion here, when really the above line is the only difference
between your new HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ
and HRTIMER_CB_IRQSAFE_NO_SOFTIRQ.
It is not even strictly necessary to return, since the rest of the
function should be safe. I have attached an alternate smaller patch, -
maybe you'll find it simpler?
> + timer_stats_account_hrtimer(timer);
> + spin_unlock(&cpu_base->lock);
> + fn(timer);
> + spin_lock(&cpu_base->lock);
> + return;
> + case HRTIMER_CB_IRQSAFE_NO_SOFTIRQ:
> /*
> * Used for scheduler timers, avoid lock inversion with
> * rq->lock and tasklist_lock.
> @@ -1310,11 +1320,17 @@ static void __run_hrtimer(struct hrtimer *timer)
> * These timers are required to deal with enqueue expiry
> * themselves and are not allowed to migrate.
> */
> + __remove_hrtimer(timer, base, HRTIMER_STATE_CALLBACK, 0);
> + timer_stats_account_hrtimer(timer);
> spin_unlock(&cpu_base->lock);
> restart = fn(timer);
> spin_lock(&cpu_base->lock);
> - } else
> + break;
> + default:
> + __remove_hrtimer(timer, base, HRTIMER_STATE_CALLBACK, 0);
> + timer_stats_account_hrtimer(timer);
> restart = fn(timer);
> + }
>
> /*
> * Note: We clear the CALLBACK bit after enqueue_hrtimer to avoid
> @@ -1516,7 +1532,7 @@ void hrtimer_init_sleeper(struct hrtimer_sleeper *sl, struct task_struct *task)
> sl->timer.function = hrtimer_wakeup;
> sl->task = task;
> #ifdef CONFIG_HIGH_RES_TIMERS
> - sl->timer.cb_mode = HRTIMER_CB_IRQSAFE_NO_SOFTIRQ;
> + sl->timer.cb_mode = HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ;
> #endif
> }
>
> --
> 1.5.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
From: gilles.carry@xxxxxxxx Gilles Carry
To: linux-rt-users@xxxxxxxxxxxxxxx
Date: Mon, 18 Aug 2008 16:42:31 +0200
Subject: [PATCH 1/2] [RT] hrtimers stuck in waitqueue
This patch makes hrtimers initialized with hrtimer_init_sleeper
to use another mode and then not be stuck in waitqueues when
hrtimer_interrupt is very busy.
The new mode is HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFIRQ.
The above-mentionned timers have been moved from
HRTIMER_CB_IRQSAFE_NO_SOFTIRQ to
HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFIRQ.
HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFIRQ timers use a slightly different
state machine from HRTIMER_CB_IRQSAFE_NO_SOFTIRQ's as when removing the
timer, __run_hrtimer sets the status to INACTIVE _then_
wakes up the thread. This way, an awakened thread cannot enter
hrtimer_cancel before the timer's status has changed.
Signed-off-by: Gilles Carry <gilles.carry@xxxxxxxx>
-reworked the patch to reduce it's size and more closely match the original function.
- removed space after WARN_ON to remove checkpatch.pl warning.
Signed-off-by: John Kacur <jkacur at gmail dot com>
---
include/linux/hrtimer.h | 4 ++++
kernel/hrtimer.c | 26 +++++++++++++++++++++-----
2 files changed, 25 insertions(+), 5 deletions(-)
Index: linux-2.6.26.2-rt1-jk/include/linux/hrtimer.h
===================================================================
--- linux-2.6.26.2-rt1-jk.orig/include/linux/hrtimer.h
+++ linux-2.6.26.2-rt1-jk/include/linux/hrtimer.h
@@ -49,12 +49,16 @@ enum hrtimer_restart {
* does not restart the timer
* HRTIMER_CB_IRQSAFE_NO_SOFTIRQ: Callback must run in hardirq context
* Special mode for tick emultation
+ * HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ:
+ * Callback must run in hardirq context and
+ * does not restart the timer
*/
enum hrtimer_cb_mode {
HRTIMER_CB_SOFTIRQ,
HRTIMER_CB_IRQSAFE,
HRTIMER_CB_IRQSAFE_NO_RESTART,
HRTIMER_CB_IRQSAFE_NO_SOFTIRQ,
+ HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ,
};
/*
Index: linux-2.6.26.2-rt1-jk/kernel/hrtimer.c
===================================================================
--- linux-2.6.26.2-rt1-jk.orig/kernel/hrtimer.c
+++ linux-2.6.26.2-rt1-jk/kernel/hrtimer.c
@@ -668,6 +668,7 @@ static inline int hrtimer_enqueue_reprog
/* Timer is expired, act upon the callback mode */
switch(timer->cb_mode) {
case HRTIMER_CB_IRQSAFE_NO_RESTART:
+ case HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ:
debug_hrtimer_deactivate(timer);
/*
* We can call the callback from here. No restart
@@ -1089,6 +1090,8 @@ int hrtimer_cancel(struct hrtimer *timer
if (ret >= 0)
return ret;
+ WARN_ON(timer->cb_mode ==
+ HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ);
hrtimer_wait_for_timer(timer);
}
}
@@ -1298,11 +1301,15 @@ static void __run_hrtimer(struct hrtimer
int restart;
debug_hrtimer_deactivate(timer);
- __remove_hrtimer(timer, base, HRTIMER_STATE_CALLBACK, 0);
+ if (timer->cb_mode == HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ)
+ __remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
+ else
+ __remove_hrtimer(timer, base, HRTIMER_STATE_CALLBACK, 0);
timer_stats_account_hrtimer(timer);
fn = timer->function;
- if (timer->cb_mode == HRTIMER_CB_IRQSAFE_NO_SOFTIRQ) {
+
+ if (timer->cb_mode == HRTIMER_CB_IRQSAFE_NO_SOFTIRQ || timer->cb_mode == HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ) {
/*
* Used for scheduler timers, avoid lock inversion with
* rq->lock and tasklist_lock.
@@ -1516,7 +1523,7 @@ void hrtimer_init_sleeper(struct hrtimer
sl->timer.function = hrtimer_wakeup;
sl->task = task;
#ifdef CONFIG_HIGH_RES_TIMERS
- sl->timer.cb_mode = HRTIMER_CB_IRQSAFE_NO_SOFTIRQ;
+ sl->timer.cb_mode = HRTIMER_CB_IRQSAFE_NO_RESTART_NO_SOFTIRQ;
#endif
}
