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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150402150649.GD23123@twins.programming.kicks-ass.net>
Date:	Thu, 2 Apr 2015 17:06:49 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Kevin Hilman <khilman@...aro.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: [PATCH 2/3] clockevents: Restart clockevent device before using
 it again

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/