From: Frederic Weisbecker <[email protected]>
[ ** BUGGY PATCH: I need to put more thinking into this ** ]
We want the nohz load balancer to be an idle CPU, thus
move that selection to strict dyntick idle logic.
Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Alessio Igor Bogani <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Avi Kivity <[email protected]>
Cc: Chris Metcalf <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Daniel Lezcano <[email protected]>
Cc: Geoff Levand <[email protected]>
Cc: Gilad Ben Yossef <[email protected]>
Cc: Hakan Akkan <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Max Krasnyansky <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Sven-Thorsten Dietrich <[email protected]>
Cc: Thomas Gleixner <[email protected]>
[ added movement of calc_load_exit_idle() ]
Signed-off-by: Steven Rostedt <[email protected]>
---
kernel/time/tick-sched.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index a402608..d6d16fe 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -372,9 +372,6 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
* the scheduler tick in nohz_restart_sched_tick.
*/
if (!ts->tick_stopped) {
- nohz_balance_enter_idle(cpu);
- calc_load_enter_idle();
-
ts->last_tick = hrtimer_get_expires(&ts->sched_timer);
ts->tick_stopped = 1;
}
@@ -466,8 +463,11 @@ static void __tick_nohz_idle_enter(struct tick_sched *ts)
ts->idle_expires = expires;
}
- if (!was_stopped && ts->tick_stopped)
+ if (!was_stopped && ts->tick_stopped) {
ts->idle_jiffies = ts->last_jiffies;
+ nohz_balance_enter_idle(cpu);
+ calc_load_enter_idle();
+ }
}
}
@@ -573,7 +573,6 @@ static void tick_nohz_restart_sched_tick(struct tick_sched *ts, ktime_t now)
tick_do_update_jiffies64(now);
update_cpu_load_nohz();
- calc_load_exit_idle();
touch_softlockup_watchdog();
/*
* Cancel the scheduled timer and restore the tick
@@ -628,6 +627,8 @@ void tick_nohz_idle_exit(void)
tick_nohz_stop_idle(cpu, now);
if (ts->tick_stopped) {
+ nohz_balance_enter_idle(cpu);
+ calc_load_exit_idle();
tick_nohz_restart_sched_tick(ts, now);
tick_nohz_account_idle_ticks(ts);
}
--
1.7.10.4
calc_load_exit_idle depends on updated jiffies, so you shouldn't move
this before tick_do_update_jiffies64.
And why should we do nohz_balance_enter_idle in tick_nohz_idle_exit?
It's nohz_balance_exit_idle here.
Regards,
Charles
On 10/30/2012 04:27 AM, Steven Rostedt wrote:
［snipped］
> @@ -573,7 +573,6 @@ static void tick_nohz_restart_sched_tick(struct
tick_sched *ts, ktime_t now)
> tick_do_update_jiffies64(now);
> update_cpu_load_nohz();
>
> - calc_load_exit_idle();
> touch_softlockup_watchdog();
> /*
> * Cancel the scheduled timer and restore the tick
> @@ -628,6 +627,8 @@ void tick_nohz_idle_exit(void)
> tick_nohz_stop_idle(cpu, now);
>
> if (ts->tick_stopped) {
> + nohz_balance_enter_idle(cpu);
> + calc_load_exit_idle();
> tick_nohz_restart_sched_tick(ts, now);
> tick_nohz_account_idle_ticks(ts);
> }
On Tue, 2012-10-30 at 16:32 +0800, Charles Wang wrote:
> calc_load_exit_idle depends on updated jiffies, so you shouldn't move
> this before tick_do_update_jiffies64.
OK, so it should be moved to the end of the if block. Note, that was a
change I made, as that function was added since Frederic did his code. I
looked into the code and it seemed that it should be moved for just idle
as well. But I agree with you that it should be after the jiffies
update.
> And why should we do nohz_balance_enter_idle in tick_nohz_idle_exit?
> It's nohz_balance_exit_idle here.
OK, that's my fault as well. As Frederic's original patch just moved
select_nohz_load_balance(0). But the commit c1cc017c59 "sched/nohz:
Clean up select_nohz_load_balancer()" replaced it with
nohz_balance_enter_idle(cpu), and removed it on exit, there was nothing
to replace for the current code.
Knowing this patch was considered "buggy" instead of just not moving it
(and forgetting about it), I did the change to remind myself to talk
about it :-)
My question is now, is there any reason to keep that call there? Or can
we just remove it as well.
-- Steve
>
> Regards,
> Charles
>
> On 10/30/2012 04:27 AM, Steven Rostedt wrote:
> ［snipped］
> > @@ -573,7 +573,6 @@ static void tick_nohz_restart_sched_tick(struct
> tick_sched *ts, ktime_t now)
> > tick_do_update_jiffies64(now);
> > update_cpu_load_nohz();
> >
> > - calc_load_exit_idle();
> > touch_softlockup_watchdog();
> > /*
> > * Cancel the scheduled timer and restore the tick
> > @@ -628,6 +627,8 @@ void tick_nohz_idle_exit(void)
> > tick_nohz_stop_idle(cpu, now);
> >
> > if (ts->tick_stopped) {
> > + nohz_balance_enter_idle(cpu);
> > + calc_load_exit_idle();
> > tick_nohz_restart_sched_tick(ts, now);
> > tick_nohz_account_idle_ticks(ts);
> > }