| Message ID | a8bce41503c36f6de6be94b0f8854ab646f6745d.1427475606.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 2 April 2015 at 19:04, Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Mar 27, 2015 at 10:44:28PM +0530, Viresh Kumar wrote: >> +++ b/kernel/time/hrtimer.c >> @@ -566,6 +566,7 @@ static int hrtimer_reprogram(struct hrtimer *timer, >> { >> + /* Switchback to ONESHOT state */ >> + if (unlikely(dev->state == CLOCK_EVT_STATE_ONESHOT_STOPPED)) >> + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT); >> + >> /* >> * Clockevents returns -ETIME, when the event was in the past. >> */ > > Should we not do this in tick_program_event() instead? Note that there > are a few more places that call that, the two in the hrtimer_interrupt() > should be safe because if we're handling the interrupt its cannot be > stopped anyhow. > > hrtimer_force_reprogram() seems to need the annotation regardless. Do you mean that we need the same modification here as well? In order to save myself against any bugs, I have added following to clockevents_program_event(): + /* We must be in ONESHOT state here */ + WARN_ONCE(dev->state != CLOCK_EVT_STATE_ONESHOT, "Current state: %d\n", + dev->state); And I never faced this WARN, or any such reports from Fengguang. So probably after all the hrtimers are gone, we will always call hrtimer_reprogram(). > Furthermore, by putting it in tick_program_event() you also don't need > to fixup tick_nohz_restart(). > > Or am I completely missing something? So yes, if we would have done that in tick_program_event(), it would have been a single place for doing this change.. But, when Thomas ranted [1] at me on this earlier, he said: " No, we are not doing a state change behind the scene and a magic restore. 2B) Implement the ONESHOT_STOPPED logic and make sure all of the core code is aware of it. " And so I did it explicitly, wherever it is required. -- viresh [1] https://lkml.org/lkml/2014/5/9/508 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index 04f6c3433f8e..e95896fd21fd 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -335,6 +335,10 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires, if (dev->state == CLOCK_EVT_STATE_SHUTDOWN) return 0; + /* We must be in ONESHOT state here */ + WARN_ONCE(dev->state != CLOCK_EVT_STATE_ONESHOT, "Current state: %d\n", + dev->state); + /* Shortcut for clockevent devices that can deal with ktime. */ if (dev->features & CLOCK_EVT_FEAT_KTIME) return dev->set_next_ktime(expires, dev); diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c index bee0c1f78091..045ba7e2be6c 100644 --- a/kernel/time/hrtimer.c +++ b/kernel/time/hrtimer.c @@ -566,6 +566,7 @@ static int hrtimer_reprogram(struct hrtimer *timer, { struct hrtimer_cpu_base *cpu_base = this_cpu_ptr(&hrtimer_bases); ktime_t expires = ktime_sub(hrtimer_get_expires(timer), base->offset); + struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev); int res; WARN_ON_ONCE(hrtimer_get_expires_tv64(timer) < 0); @@ -610,6 +611,10 @@ static int hrtimer_reprogram(struct hrtimer *timer, if (cpu_base->hang_detected) return 0; + /* Switchback to ONESHOT state */ + if (unlikely(dev->state == CLOCK_EVT_STATE_ONESHOT_STOPPED)) + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT); + /* * Clockevents returns -ETIME, when the event was in the past. */ diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index a4c4edac4528..47c04edd07df 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -694,8 +694,14 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, /* Check, if the timer was already in the past */ if (hrtimer_active(&ts->sched_timer)) goto out; - } else if (!tick_program_event(expires, 0)) + } else { + /* Switchback to ONESHOT state */ + if (unlikely(dev->state == CLOCK_EVT_STATE_ONESHOT_STOPPED)) + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT); + + if (!tick_program_event(expires, 0)) goto out; + } /* * We are past the event already. So we crossed a * jiffie boundary. Update jiffies and raise the @@ -873,6 +879,8 @@ ktime_t tick_nohz_get_sleep_length(void) static void tick_nohz_restart(struct tick_sched *ts, ktime_t now) { + struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev); + hrtimer_cancel(&ts->sched_timer); hrtimer_set_expires(&ts->sched_timer, ts->last_tick); @@ -887,6 +895,10 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now) if (hrtimer_active(&ts->sched_timer)) break; } else { + /* Switchback to ONESHOT state */ + if (likely(dev->state == CLOCK_EVT_STATE_ONESHOT_STOPPED)) + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT); + if (!tick_program_event( hrtimer_get_expires(&ts->sched_timer), 0)) break;
Next commit would update clockevents core to switch clockevent devices to ONESHOT_STOPPED state to avoid getting spurious interrupts on a tickless CPU. Before programming the clockevent device for next event, we must switch its state to ONESHOT. Switch state back to ONESHOT from ONESHOT_STOPPED at following places: 1.) NOHZ_MODE_LOWRES Mode Timers & hrtimers are dependent on tick for their working in this mode and the only place from where clockevent device is programmed is the tick-code. Two routines can restart ticks in LOWRES mode: tick_nohz_restart() and tick_nohz_stop_sched_tick(). 2.) NOHZ_MODE_HIGHRES Mode Tick & timers are dependent on hrtimers for their working in this mode and the only place from where clockevent device is programmed is the hrtimer-code. Only hrtimer_reprogram() is responsible for programming the clockevent device for next event, if the clockevent device is stopped earlier. And updating that alone is sufficient here. To make sure we haven't missed any corner case, add a WARN() for the case where we try to reprogram clockevent device while we aren't configured in ONESHOT_STOPPED state. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- kernel/time/clockevents.c | 4 ++++ kernel/time/hrtimer.c | 5 +++++ kernel/time/tick-sched.c | 14 +++++++++++++- 3 files changed, 22 insertions(+), 1 deletion(-)