Re: [PATCH 2/3] clockevents: Restart clockevent device before using it again

[Peter Zijlstra]

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);
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/