LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Ingo Molnar <mingo@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Linaro Kernel Mailman List <linaro-kernel@lists.linaro.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Kevin Hilman <khilman@linaro.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Preeti U Murthy <preeti@linux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@gmail.com>
Subject: Re: [PATCH 2/3] clockevents: Restart clockevent device before using it again
Date: Thu, 2 Apr 2015 17:06:49 +0200	[thread overview]
Message-ID: <20150402150649.GD23123@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <CAKohpok=HEL4H22-OggBqokgdJvMcK2kBeccpfFk9+aacRX2Mw@mail.gmail.com>

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);
 }
 

  reply	other threads:[~2015-04-02 15:07 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-27 17:14 [PATCH 0/3] clockevents: Introduce CLOCK_EVT_STATE_ONESHOT_STOPPED Viresh Kumar
2015-03-27 17:14 ` [PATCH 1/3] clockevents: Introduce CLOCK_EVT_STATE_ONESHOT_STOPPED state Viresh Kumar
2015-03-30  5:49   ` Preeti U Murthy
2015-04-06 21:26   ` Kevin Hilman
2015-03-27 17:14 ` [PATCH 2/3] clockevents: Restart clockevent device before using it again Viresh Kumar
2015-03-30  5:52   ` Preeti U Murthy
2015-04-02 13:34   ` Peter Zijlstra
2015-04-02 13:50     ` Viresh Kumar
2015-04-02 15:06       ` Peter Zijlstra [this message]
2015-04-02 16:04         ` Ingo Molnar
2015-03-27 17:14 ` [PATCH 3/3] clockevents: Switch state to ONESHOT_STOPPED for unused clockevent devices Viresh Kumar
2015-03-30  5:50   ` Preeti U Murthy
2015-04-02 13:37   ` Peter Zijlstra
2015-04-02 13:51     ` Viresh Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150402150649.GD23123@twins.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=fweisbec@gmail.com \
    --cc=khilman@linaro.org \
    --cc=linaro-kernel@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=preeti@linux.vnet.ibm.com \
    --cc=tglx@linutronix.de \
    --cc=viresh.kumar@linaro.org \
    --subject='Re: [PATCH 2/3] clockevents: Restart clockevent device before using it again' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).