Kaike reported that in tests rdma hrtimers occasionaly stopped working. He
did great debugging, which provided enough context to decode the problem.
CPU 3 CPU 2
idle
start sched_timer expires = 712171000000
queue->next = sched_timer
start rdmavt timer. expires = 712172915662
lock(baseof(CPU3))
tick_nohz_stop_tick()
tick = 716767000000 timerqueue_add(tmr)
hrtimer_set_expires(sched_timer, tick);
sched_timer->expires = 716767000000 <---- FAIL
if (tmr->expires < queue->next->expires)
hrtimer_start(sched_timer) queue->next = tmr;
lock(baseof(CPU3))
unlock(baseof(CPU3))
timerqueue_remove()
timerqueue_add()
ts->sched_timer is queued and queue->next is pointing to it, but then
ts->sched_timer.expires is modified.
This not only corrupts the ordering of the timerqueue RB tree, it also
makes CPU2 see the new expiry time of timerqueue->next->expires when
checking whether timerqueue->next needs to be updated. So CPU2 sees that
the rdma timer is earlier than timerqueue->next and sets the rdma timer as
new next.
Depending on whether it had also seen the new time at RB tree enqueue, it
might have queued the rdma timer at the wrong place and then after removing
the sched_timer the RB tree is completely hosed.
The problem was introduced with a commit which tried to solve inconsistency
between the hrtimer in the tick_sched data and the underlying hardware
clockevent. It split out hrtimer_set_expires() to store the new tick time
in both the NOHZ and the NOHZ + HIGHRES case, but missed the fact that in
the NOHZ + HIGHRES case the hrtimer might still be queued.
Use hrtimer_start(timer, tick...) for the NOHZ + HIGHRES case which sets
timer->expires after canceling the timer and move the hrtimer_set_expires()
invocation into the NOHZ only code path which is not affected as it merily
uses the hrtimer as next event storage so code pathes can be shared with
the NOHZ + HIGHRES case.
Fixes: d4af6d933ccf ("nohz: Fix spurious warning when hrtimer and clockevent get out of sync")
Reported-by: "Wan Kaike" <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: "Marciniszyn Mike" <[email protected]>
Cc: Anna-Maria Gleixner <[email protected]>
Cc: [email protected]
Cc: "Dalessandro Dennis" <[email protected]>
Cc: "Fleck John" <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: "Weiny Ira" <[email protected]>
Cc: "[email protected]"
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/time/tick-sched.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -804,12 +804,12 @@ static void tick_nohz_stop_tick(struct t
return;
}
- hrtimer_set_expires(&ts->sched_timer, tick);
-
- if (ts->nohz_mode == NOHZ_MODE_HIGHRES)
- hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED);
- else
+ if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {
+ hrtimer_start(&ts->sched_timer, tick, HRTIMER_MODE_ABS_PINNED);
+ } else {
+ hrtimer_set_expires(&ts->sched_timer, tick);
tick_program_event(tick, 1);
+ }
}
static void tick_nohz_retain_tick(struct tick_sched *ts)
On Tue, Apr 24, 2018 at 09:22:18PM +0200, Thomas Gleixner wrote:
> Kaike reported that in tests rdma hrtimers occasionaly stopped working. He
> did great debugging, which provided enough context to decode the problem.
>
> CPU 3 CPU 2
>
> idle
> start sched_timer expires = 712171000000
> queue->next = sched_timer
> start rdmavt timer. expires = 712172915662
> lock(baseof(CPU3))
> tick_nohz_stop_tick()
> tick = 716767000000 timerqueue_add(tmr)
>
> hrtimer_set_expires(sched_timer, tick);
> sched_timer->expires = 716767000000 <---- FAIL
> if (tmr->expires < queue->next->expires)
> hrtimer_start(sched_timer) queue->next = tmr;
> lock(baseof(CPU3))
> unlock(baseof(CPU3))
> timerqueue_remove()
> timerqueue_add()
>
> ts->sched_timer is queued and queue->next is pointing to it, but then
> ts->sched_timer.expires is modified.
>
> This not only corrupts the ordering of the timerqueue RB tree, it also
> makes CPU2 see the new expiry time of timerqueue->next->expires when
> checking whether timerqueue->next needs to be updated. So CPU2 sees that
> the rdma timer is earlier than timerqueue->next and sets the rdma timer as
> new next.
>
> Depending on whether it had also seen the new time at RB tree enqueue, it
> might have queued the rdma timer at the wrong place and then after removing
> the sched_timer the RB tree is completely hosed.
>
> The problem was introduced with a commit which tried to solve inconsistency
> between the hrtimer in the tick_sched data and the underlying hardware
> clockevent. It split out hrtimer_set_expires() to store the new tick time
> in both the NOHZ and the NOHZ + HIGHRES case, but missed the fact that in
> the NOHZ + HIGHRES case the hrtimer might still be queued.
>
> Use hrtimer_start(timer, tick...) for the NOHZ + HIGHRES case which sets
> timer->expires after canceling the timer and move the hrtimer_set_expires()
> invocation into the NOHZ only code path which is not affected as it merily
> uses the hrtimer as next event storage so code pathes can be shared with
> the NOHZ + HIGHRES case.
>
> Fixes: d4af6d933ccf ("nohz: Fix spurious warning when hrtimer and clockevent get out of sync")
> Reported-by: "Wan Kaike" <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Thanks!
Commit-ID: 51fc4be41c7ea7a00f6a17ad15ac9ea684d07921
Gitweb: https://git.kernel.org/tip/51fc4be41c7ea7a00f6a17ad15ac9ea684d07921
Author: Thomas Gleixner <[email protected]>
AuthorDate: Tue, 24 Apr 2018 21:22:18 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 25 Apr 2018 16:11:58 +0200
tick/sched: Do not mess with an enqueued hrtimer
Kaike reported that in tests rdma hrtimers occasionaly stopped working. He
did great debugging, which provided enough context to decode the problem.
CPU 3 CPU 2
idle
start sched_timer expires = 712171000000
queue->next = sched_timer
start rdmavt timer. expires = 712172915662
lock(baseof(CPU3))
tick_nohz_stop_tick()
tick = 716767000000 timerqueue_add(tmr)
hrtimer_set_expires(sched_timer, tick);
sched_timer->expires = 716767000000 <---- FAIL
if (tmr->expires < queue->next->expires)
hrtimer_start(sched_timer) queue->next = tmr;
lock(baseof(CPU3))
unlock(baseof(CPU3))
timerqueue_remove()
timerqueue_add()
ts->sched_timer is queued and queue->next is pointing to it, but then
ts->sched_timer.expires is modified.
This not only corrupts the ordering of the timerqueue RB tree, it also
makes CPU2 see the new expiry time of timerqueue->next->expires when
checking whether timerqueue->next needs to be updated. So CPU2 sees that
the rdma timer is earlier than timerqueue->next and sets the rdma timer as
new next.
Depending on whether it had also seen the new time at RB tree enqueue, it
might have queued the rdma timer at the wrong place and then after removing
the sched_timer the RB tree is completely hosed.
The problem was introduced with a commit which tried to solve inconsistency
between the hrtimer in the tick_sched data and the underlying hardware
clockevent. It split out hrtimer_set_expires() to store the new tick time
in both the NOHZ and the NOHZ + HIGHRES case, but missed the fact that in
the NOHZ + HIGHRES case the hrtimer might still be queued.
Use hrtimer_start(timer, tick...) for the NOHZ + HIGHRES case which sets
timer->expires after canceling the timer and move the hrtimer_set_expires()
invocation into the NOHZ only code path which is not affected as it merily
uses the hrtimer as next event storage so code pathes can be shared with
the NOHZ + HIGHRES case.
Fixes: d4af6d933ccf ("nohz: Fix spurious warning when hrtimer and clockevent get out of sync")
Reported-by: "Wan Kaike" <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Cc: "Marciniszyn Mike" <[email protected]>
Cc: Anna-Maria Gleixner <[email protected]>
Cc: [email protected]
Cc: "Dalessandro Dennis" <[email protected]>
Cc: "Fleck John" <[email protected]>
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: "Weiny Ira" <[email protected]>
Cc: "[email protected]"
Link: https://lkml.kernel.org/r/[email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/time/tick-sched.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index e35a6fced00c..da9455a6b42b 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -795,12 +795,12 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
return;
}
- hrtimer_set_expires(&ts->sched_timer, tick);
-
- if (ts->nohz_mode == NOHZ_MODE_HIGHRES)
- hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED);
- else
+ if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {
+ hrtimer_start(&ts->sched_timer, tick, HRTIMER_MODE_ABS_PINNED);
+ } else {
+ hrtimer_set_expires(&ts->sched_timer, tick);
tick_program_event(tick, 1);
+ }
}
static void tick_nohz_retain_tick(struct tick_sched *ts)
> From: [email protected] [mailto:linux-rdma-
> [email protected]] On Behalf Of Thomas Gleixner
> Sent: Tuesday, April 24, 2018 3:22 PM
> To: Frederic Weisbecker <[email protected]>
> Cc: Wan, Kaike <[email protected]>; Marciniszyn, Mike
> <[email protected]>; Dalessandro, Dennis
> <[email protected]>; Weiny, Ira <[email protected]>; Fleck,
> John <[email protected]>; [email protected]; linux-
> [email protected]; Peter Zijlstra <[email protected]>; Anna-Maria
> Gleixner <[email protected]>; Frederic Weisbecker
> <[email protected]>; Ingo Molnar <[email protected]>
> Subject: [PATCH] tick/sched: Do not mess with an enqueued hrtimer
>
> Kaike reported that in tests rdma hrtimers occasionaly stopped working. He
> did great debugging, which provided enough context to decode the problem.
>
> CPU 3 CPU 2
>
> idle
> start sched_timer expires = 712171000000 queue->next = sched_timer
> start rdmavt timer. expires =
> 712172915662
> lock(baseof(CPU3))
> tick_nohz_stop_tick()
> tick = 716767000000 timerqueue_add(tmr)
>
> hrtimer_set_expires(sched_timer, tick);
> sched_timer->expires = 716767000000 <---- FAIL
> if (tmr->expires < queue->next-
> >expires)
> hrtimer_start(sched_timer) queue->next = tmr;
> lock(baseof(CPU3))
> unlock(baseof(CPU3))
> timerqueue_remove()
> timerqueue_add()
>
> ts->sched_timer is queued and queue->next is pointing to it, but then
> ts->sched_timer.expires is modified.
>
> This not only corrupts the ordering of the timerqueue RB tree, it also makes
> CPU2 see the new expiry time of timerqueue->next->expires when checking
> whether timerqueue->next needs to be updated. So CPU2 sees that the
> rdma timer is earlier than timerqueue->next and sets the rdma timer as new
> next.
>
> Depending on whether it had also seen the new time at RB tree enqueue, it
> might have queued the rdma timer at the wrong place and then after
> removing the sched_timer the RB tree is completely hosed.
>
> The problem was introduced with a commit which tried to solve
> inconsistency between the hrtimer in the tick_sched data and the underlying
> hardware clockevent. It split out hrtimer_set_expires() to store the new tick
> time in both the NOHZ and the NOHZ + HIGHRES case, but missed the fact
> that in the NOHZ + HIGHRES case the hrtimer might still be queued.
>
> Use hrtimer_start(timer, tick...) for the NOHZ + HIGHRES case which sets
> timer->expires after canceling the timer and move the
> timer->hrtimer_set_expires()
> invocation into the NOHZ only code path which is not affected as it merily
> uses the hrtimer as next event storage so code pathes can be shared with
> the NOHZ + HIGHRES case.
>
> Fixes: d4af6d933ccf ("nohz: Fix spurious warning when hrtimer and
> clockevent get out of sync")
> Reported-by: "Wan Kaike" <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: "Marciniszyn Mike" <[email protected]>
> Cc: Anna-Maria Gleixner <[email protected]>
> Cc: [email protected]
> Cc: "Dalessandro Dennis" <[email protected]>
> Cc: "Fleck John" <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Cc: "Weiny Ira" <[email protected]>
> Cc: "[email protected]"
> Cc: [email protected]
> Link:
> https://lkml.kernel.org/r/[email protected]
> utronix.de
>
> ---
> kernel/time/tick-sched.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -804,12 +804,12 @@ static void tick_nohz_stop_tick(struct t
> return;
> }
>
> - hrtimer_set_expires(&ts->sched_timer, tick);
> -
> - if (ts->nohz_mode == NOHZ_MODE_HIGHRES)
> - hrtimer_start_expires(&ts->sched_timer,
> HRTIMER_MODE_ABS_PINNED);
> - else
> + if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {
> + hrtimer_start(&ts->sched_timer, tick,
> HRTIMER_MODE_ABS_PINNED);
> + } else {
> + hrtimer_set_expires(&ts->sched_timer, tick);
> tick_program_event(tick, 1);
> + }
> }
>
> static void tick_nohz_retain_tick(struct tick_sched *ts)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the
> body of a message to [email protected] More majordomo info at
> http://vger.kernel.org/majordomo-info.html
Tested-by: Kaike Wan <[email protected]>
Commit-ID: 1f71addd34f4c442bec7d7c749acc1beb58126f2
Gitweb: https://git.kernel.org/tip/1f71addd34f4c442bec7d7c749acc1beb58126f2
Author: Thomas Gleixner <[email protected]>
AuthorDate: Tue, 24 Apr 2018 21:22:18 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Thu, 26 Apr 2018 14:53:32 +0200
tick/sched: Do not mess with an enqueued hrtimer
Kaike reported that in tests rdma hrtimers occasionaly stopped working. He
did great debugging, which provided enough context to decode the problem.
CPU 3 CPU 2
idle
start sched_timer expires = 712171000000
queue->next = sched_timer
start rdmavt timer. expires = 712172915662
lock(baseof(CPU3))
tick_nohz_stop_tick()
tick = 716767000000 timerqueue_add(tmr)
hrtimer_set_expires(sched_timer, tick);
sched_timer->expires = 716767000000 <---- FAIL
if (tmr->expires < queue->next->expires)
hrtimer_start(sched_timer) queue->next = tmr;
lock(baseof(CPU3))
unlock(baseof(CPU3))
timerqueue_remove()
timerqueue_add()
ts->sched_timer is queued and queue->next is pointing to it, but then
ts->sched_timer.expires is modified.
This not only corrupts the ordering of the timerqueue RB tree, it also
makes CPU2 see the new expiry time of timerqueue->next->expires when
checking whether timerqueue->next needs to be updated. So CPU2 sees that
the rdma timer is earlier than timerqueue->next and sets the rdma timer as
new next.
Depending on whether it had also seen the new time at RB tree enqueue, it
might have queued the rdma timer at the wrong place and then after removing
the sched_timer the RB tree is completely hosed.
The problem was introduced with a commit which tried to solve inconsistency
between the hrtimer in the tick_sched data and the underlying hardware
clockevent. It split out hrtimer_set_expires() to store the new tick time
in both the NOHZ and the NOHZ + HIGHRES case, but missed the fact that in
the NOHZ + HIGHRES case the hrtimer might still be queued.
Use hrtimer_start(timer, tick...) for the NOHZ + HIGHRES case which sets
timer->expires after canceling the timer and move the hrtimer_set_expires()
invocation into the NOHZ only code path which is not affected as it merily
uses the hrtimer as next event storage so code pathes can be shared with
the NOHZ + HIGHRES case.
Fixes: d4af6d933ccf ("nohz: Fix spurious warning when hrtimer and clockevent get out of sync")
Reported-by: "Wan Kaike" <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Cc: "Marciniszyn Mike" <[email protected]>
Cc: Anna-Maria Gleixner <[email protected]>
Cc: [email protected]
Cc: "Dalessandro Dennis" <[email protected]>
Cc: "Fleck John" <[email protected]>
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: "Weiny Ira" <[email protected]>
Cc: "[email protected]"
Link: https://lkml.kernel.org/r/[email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/time/tick-sched.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 646645e981f9..d31bec177fa5 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -804,12 +804,12 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
return;
}
- hrtimer_set_expires(&ts->sched_timer, tick);
-
- if (ts->nohz_mode == NOHZ_MODE_HIGHRES)
- hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED);
- else
+ if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {
+ hrtimer_start(&ts->sched_timer, tick, HRTIMER_MODE_ABS_PINNED);
+ } else {
+ hrtimer_set_expires(&ts->sched_timer, tick);
tick_program_event(tick, 1);
+ }
}
static void tick_nohz_retain_tick(struct tick_sched *ts)