Replace the nanosleep private sleeper functionality by the generic
hrtimer sleeper.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
kernel/hrtimer.c | 34 +++++++---------------------------
1 file changed, 7 insertions(+), 27 deletions(-)
Index: linux-2.6.16/kernel/hrtimer.c
===================================================================
--- linux-2.6.16.orig/kernel/hrtimer.c
+++ linux-2.6.16/kernel/hrtimer.c
@@ -655,7 +655,6 @@ void hrtimer_run_queues(void)
/*
* Sleep related functions:
*/
-
static int hrtimer_wakeup(struct hrtimer *timer)
{
struct hrtimer_sleeper *t =
@@ -675,28 +674,9 @@ void hrtimer_init_sleeper(struct hrtimer
sl->task = task;
}
-struct sleep_hrtimer {
- struct hrtimer timer;
- struct task_struct *task;
- int expired;
-};
-
-static int nanosleep_wakeup(struct hrtimer *timer)
-{
- struct sleep_hrtimer *t =
- container_of(timer, struct sleep_hrtimer, timer);
-
- t->expired = 1;
- wake_up_process(t->task);
-
- return HRTIMER_NORESTART;
-}
-
-static int __sched do_nanosleep(struct sleep_hrtimer *t, enum hrtimer_mode mode)
+static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mode)
{
- t->timer.function = nanosleep_wakeup;
- t->task = current;
- t->expired = 0;
+ hrtimer_init_sleeper(t, current);
do {
set_current_state(TASK_INTERRUPTIBLE);
@@ -704,18 +684,18 @@ static int __sched do_nanosleep(struct s
schedule();
- if (unlikely(!t->expired)) {
+ if (unlikely(t->task)) {
hrtimer_cancel(&t->timer);
mode = HRTIMER_ABS;
}
- } while (!t->expired && !signal_pending(current));
+ } while (t->task && !signal_pending(current));
- return t->expired;
+ return t->task == NULL;
}
static long __sched nanosleep_restart(struct restart_block *restart)
{
- struct sleep_hrtimer t;
+ struct hrtimer_sleeper t;
struct timespec __user *rmtp;
struct timespec tu;
ktime_t time;
@@ -748,7 +728,7 @@ long hrtimer_nanosleep(struct timespec *
const enum hrtimer_mode mode, const clockid_t clockid)
{
struct restart_block *restart;
- struct sleep_hrtimer t;
+ struct hrtimer_sleeper t;
struct timespec tu;
ktime_t rem;
--
Thomas Gleixner <[email protected]> wrote:
>
>
> Replace the nanosleep private sleeper functionality by the generic
> hrtimer sleeper.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
>
>
> kernel/hrtimer.c | 34 +++++++---------------------------
> 1 file changed, 7 insertions(+), 27 deletions(-)
>
> Index: linux-2.6.16/kernel/hrtimer.c
> ===================================================================
> --- linux-2.6.16.orig/kernel/hrtimer.c
> +++ linux-2.6.16/kernel/hrtimer.c
> @@ -655,7 +655,6 @@ void hrtimer_run_queues(void)
> /*
> * Sleep related functions:
> */
> -
> static int hrtimer_wakeup(struct hrtimer *timer)
> {
> struct hrtimer_sleeper *t =
> @@ -675,28 +674,9 @@ void hrtimer_init_sleeper(struct hrtimer
> sl->task = task;
> }
>
> -struct sleep_hrtimer {
> - struct hrtimer timer;
> - struct task_struct *task;
> - int expired;
> -};
> -
> -static int nanosleep_wakeup(struct hrtimer *timer)
> -{
> - struct sleep_hrtimer *t =
> - container_of(timer, struct sleep_hrtimer, timer);
> -
> - t->expired = 1;
> - wake_up_process(t->task);
> -
> - return HRTIMER_NORESTART;
> -}
> -
> -static int __sched do_nanosleep(struct sleep_hrtimer *t, enum hrtimer_mode mode)
> +static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mode)
> {
> - t->timer.function = nanosleep_wakeup;
> - t->task = current;
> - t->expired = 0;
> + hrtimer_init_sleeper(t, current);
>
> do {
> set_current_state(TASK_INTERRUPTIBLE);
> @@ -704,18 +684,18 @@ static int __sched do_nanosleep(struct s
>
> schedule();
>
> - if (unlikely(!t->expired)) {
> + if (unlikely(t->task)) {
> hrtimer_cancel(&t->timer);
> mode = HRTIMER_ABS;
> }
> - } while (!t->expired && !signal_pending(current));
> + } while (t->task && !signal_pending(current));
>
> - return t->expired;
> + return t->task == NULL;
> }
This all looks vaguely racy. hrtimer_wakeup() will set t->task to NULL
without barriers, locks or anything. And the waiter here can break out of
schedule() due to signal delivery while a wakeup is in progress.
So the value of t->task here is fairly meaningless. Ot just depends on how
far the waker has got through hrtimer_wakeup().
Maybe that doesn't matter, because hrtimer_cancel() will spin until
hrtimer_wakeup() has completed anyway, but could you please recheck and
confirm that this is all solid?
On Sat, 2006-03-25 at 18:32 -0800, Andrew Morton wrote:
> This all looks vaguely racy. hrtimer_wakeup() will set t->task to NULL
> without barriers, locks or anything. And the waiter here can break out of
> schedule() due to signal delivery while a wakeup is in progress.
We set task = NULL before wake_up_process() which acts as a barrier.
> So the value of t->task here is fairly meaningless. Ot just depends on how
> far the waker has got through hrtimer_wakeup().
>
> Maybe that doesn't matter, because hrtimer_cancel() will spin until
> hrtimer_wakeup() has completed anyway, but could you please recheck and
> confirm that this is all solid?
Right, either it waits for the running timer or in case the wakeup
happens between
if (unlikely(t->task)) {
and
hrtimer_cancel(&t->timer);
then hrtimer_cancel will see that the timer is inactive and we drop out
of the loop because the while(t->task) condition is not longer true.
tglx
I also think this is racy.
CPU_0 CPU_1
hrtimer_wakeup:
task = t->task;
t->task = NULL;
<--- INTERRUPT --->
task is woken by signal,
do_nanosleep() sees t->task == NULL,
returns without hrtimer_cancel(),
and __exits__.
<--- RESUME --->
wake_up_process(task);
Instead of exit(), 'task' can go to TASK_STOPPED or TASK_UNINTERRUPTIBLE
after return from do_nanosleep(), it will be awakened by hrtimer_wakeup()
unexpectedly.
Oleg.
Hi,
On Tue, 28 Mar 2006, Oleg Nesterov wrote:
> I also think this is racy.
>
> CPU_0 CPU_1
>
> hrtimer_wakeup:
>
> task = t->task;
> t->task = NULL;
>
> <--- INTERRUPT --->
>
> task is woken by signal,
> do_nanosleep() sees t->task == NULL,
> returns without hrtimer_cancel(),
> and __exits__.
>
> <--- RESUME --->
>
> wake_up_process(task);
>
> Instead of exit(), 'task' can go to TASK_STOPPED or TASK_UNINTERRUPTIBLE
> after return from do_nanosleep(), it will be awakened by hrtimer_wakeup()
> unexpectedly.
Indeed and my original patch did call hrtimer_cancel() unconditionally to
synchronize with a possibly running timer.
Thomas, could you please document it a bit better, when you modify my
patches?
bye, Roman
On Tue, 2006-03-28 at 03:55 +0400, Oleg Nesterov wrote:
> Instead of exit(), 'task' can go to TASK_STOPPED or TASK_UNINTERRUPTIBLE
> after return from do_nanosleep(), it will be awakened by hrtimer_wakeup()
> unexpectedly.
Yep, you are right.
Index: linux-2.6.16/kernel/hrtimer.c
===================================================================
--- linux-2.6.16.orig/kernel/hrtimer.c
+++ linux-2.6.16/kernel/hrtimer.c
@@ -684,10 +684,9 @@ static int __sched do_nanosleep(struct h
schedule();
- if (unlikely(t->task)) {
- hrtimer_cancel(&t->timer);
- mode = HRTIMER_ABS;
- }
+ hrtimer_cancel(&t->timer);
+ mode = HRTIMER_ABS;
+
} while (t->task && !signal_pending(current));
return t->task == NULL;