On Thu, Apr 02, 2015 at 07:20:50PM +0530, Viresh Kumar wrote:
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. "
lkml.org didn't work for me, alternative link:
http://marc.info/?l=linux-kernel&m=139966616803683&w=2
So I've read that (several times) and I thing Thomas meant something else.
So I think he disliked what you did to the clockevent layer, not so much you touching tick_program_event(). But the last_state thing (which was broken), and you imposing the SHUTDOWN policy for everybody.
But with the optional ONESHOT_STOPPED state both those are gone, and we'd end up with the much simpler patch below.
Further note that tick-oneshot is the natural place to do this, that is the glue layer between the (oneshot) clockevents stuff and the timer stuff. Pulling clockevents into hrtimers feels wrong.
Ingo, do you agree with that after reading Thomas' email?
--- --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -335,6 +335,9 @@ int clockevents_program_event(struct clo if (dev->state == CLOCK_EVT_STATE_SHUTDOWN) return 0;
+ 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); --- a/kernel/time/hrtimer.c +++ b/kernel/time/hrtimer.c @@ -543,8 +543,7 @@ hrtimer_force_reprogram(struct hrtimer_c if (cpu_base->hang_detected) return;
- if (cpu_base->expires_next.tv64 != KTIME_MAX) - tick_program_event(cpu_base->expires_next, 1); + tick_program_event(cpu_base->expires_next, 1); }
/* @@ -1308,8 +1307,7 @@ void hrtimer_interrupt(struct clock_even raw_spin_unlock(&cpu_base->lock);
/* Reprogramming necessary ? */ - if (expires_next.tv64 == KTIME_MAX || - !tick_program_event(expires_next, 0)) { + if (!tick_program_event(expires_next, 0)) { cpu_base->hang_detected = 0; return; } --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -28,6 +28,13 @@ int tick_program_event(ktime_t expires, { struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
+ if (expires.tv64 == KTIME_MAX) { + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT_STOPPED); + return 0; + } else if (unlikely(dev->state == CLOCK_EVT_STATE_ONESHOT_STOPPED)) { + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT); + } + return clockevents_program_event(dev, expires, force); }