From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751620AbbBMBkS (ORCPT ); Thu, 12 Feb 2015 20:40:18 -0500 Received: from v094114.home.net.pl ([79.96.170.134]:53624 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1750944AbbBMBkO (ORCPT ); Thu, 12 Feb 2015 20:40:14 -0500 From: "Rafael J. Wysocki" To: John Stultz Cc: Thomas Gleixner , Peter Zijlstra , Alan Cox , "Li, Aubrey" , LKML , Linux PM list , ACPI Devel Maling List , Kristen Carlson Accardi , Len Brown Subject: Re: [PATCH 3/6] timekeeping: Make it safe to use the fast timekeeper while suspended Date: Fri, 13 Feb 2015 03:03:20 +0100 Message-ID: <2403461.IZYtJE89Ir@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/3.19.0+; KDE/4.11.5; x86_64; ; ) In-Reply-To: References: <8292243.ibkmfVtXac@vostro.rjw.lan> <1521825.MkZ1sG1M8S@vostro.rjw.lan> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday, February 13, 2015 08:53:38 AM John Stultz wrote: > On Wed, Feb 11, 2015 at 12:03 PM, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki > > > > Theoretically, ktime_get_mono_fast_ns() may be executed after > > timekeeping has been suspended (or before it is resumed) which > > in turn may lead to undefined behavior, for example, when the > > clocksource read from timekeeping_get_ns() called by it is > > not accessible at that time. > > And the callers of the ktime_get_mono_fast_ns() have to get back a > value? Yes, they do. > Or can we return an error on timekeeping_suspended like we do > w/ __getnstimeofday64()? No, we can't. > Also, what exactly is the case when the clocksource being read isn't > accessible? I see this is conditionalized on > CLOCK_SOURCE_SUSPEND_NONSTOP, so is the concern on resume we read the > clocksource and its been reset causing a crazy time value? The clocksource's ->suspend method may have been called (during suspend) and depending on what that did we may even crash things theoretically. During resume, before the clocksource's ->resume callback, it may just be undefined behavior (random data etc). For system suspend as we have today the window is quite narrow, but after patch [4/6] from this series suspend-to-idle may suspend timekeeping and just sit there in idle for extended time (hours even) which broadens the potential exposure quite a bit. Of course, it does that with interrupts disabled, but ktime_get_mono_fast_ns() is for NMI, so theoretically, if an NMI happens while we're in suspend-to-idle with timekeeping suspended and the clocksource is not CLOCK_SOURCE_SUSPEND_NONSTOP and the NMI calls ktime_get_mono_fast_ns(), strange and undesirable things may happen. > > Prevent that from happening by setting up a dummy readout base for > > the fast timekeeper during timekeeping_suspend() such that it will > > always return the same number of cycles. > > > > After the last timekeeping_update() in timekeeping_suspend() the > > clocksource is read and the result is stored as cycles_at_suspend. > > The readout base from the current timekeeper is copied onto the > > dummy and the ->read pointer of the dummy is set to a routine > > unconditionally returning cycles_at_suspend. Next, the dummy is > > passed to update_fast_timekeeper(). > > > > Then, ktime_get_mono_fast_ns() will work until the subsequent > > timekeeping_resume() and the proper readout base for the fast > > timekeeper will be restored by the timekeeping_update() called > > right after clearing timekeeping_suspended. > > > > Signed-off-by: Rafael J. Wysocki > > --- > > kernel/time/timekeeping.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > Index: linux-pm/kernel/time/timekeeping.c > > =================================================================== > > --- linux-pm.orig/kernel/time/timekeeping.c > > +++ linux-pm/kernel/time/timekeeping.c > > @@ -1249,9 +1249,23 @@ static void timekeeping_resume(void) > > hrtimers_resume(); > > } > > > > +/* > > + * Dummy readout base and suspend-time cycles value for the fast timekeeper to > > + * work in a consistent way after timekeeping has been suspended if the core > > + * timekeeper clocksource is not suspend-nonstop. > > + */ > > +static struct tk_read_base tkr_dummy; > > +static cycle_t cycles_at_suspend; > > + > > +static cycle_t dummy_clock_read(struct clocksource *cs) > > +{ > > + return cycles_at_suspend; > > +} > > + > > static int timekeeping_suspend(void) > > { > > struct timekeeper *tk = &tk_core.timekeeper; > > + struct clocksource *clock = tk->tkr.clock; > > unsigned long flags; > > struct timespec64 delta, delta_delta; > > static struct timespec64 old_delta; > > @@ -1294,6 +1308,14 @@ static int timekeeping_suspend(void) > > } > > > > timekeeping_update(tk, TK_MIRROR); > > + > > + if (!(clock->flags & CLOCK_SOURCE_SUSPEND_NONSTOP)) { > > + memcpy(&tkr_dummy, &tk->tkr, sizeof(tkr_dummy)); > > + cycles_at_suspend = tk->tkr.read(clock); > > + tkr_dummy.read = dummy_clock_read; > > + update_fast_timekeeper(&tkr_dummy); > > + } > > Its a little ugly... though I'm not sure I have a better idea right off. > > thanks > -john > -- > 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/ -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.