On Fri, Sep 14, 2018 at 9:41 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Fri, Sep 14, 2018 at 08:59:03AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > There is a difference in behavior between suspend-to-idle and
> > suspend-to-RAM in the timekeeping handling that leads to functional
> > issues. Namely, every iteration of the loop in s2idle_loop()
> > increases the monotinic clock somewhat, even if timekeeping_suspend()
> > and timekeeping_resume() are invoked from s2idle_enter(), and if
> > many of them are carried out in a row, the monotonic clock can grow
> > significantly while the system is regarded as suspended, which
> > doesn't happen during suspend-to-RAM and so it is unexpected and
> > leads to confusion and misbehavior in user space (similar to what
> > ensued when we tried to combine the boottime and monotonic clocks).
> >
> > To avoid that, count all iterations of the loop in s2idle_loop()
> > as "sleep time" and adjust the clock for that on exit from
> > suspend-to-idle.
> >
> > [That also covers systems on which timekeeping is not suspended
> > by by s2idle_enter().]
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Do we want a 'warning' of sorts when the delta becomes significant (for
> whatever that is) ? That might be an indication that there are frequent
> wakeups which we might not be expecting. Of keep the number of spurious
> wakeups in a stat counter somewhere -- something to look at if the
> battery drains faster than expected.
If you echo 1 to /sys/power/pm_debug_messages, dmesg will tell you
that (with gory details). :-)
> Otherwise:
>
> Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
>
> One minor nit below:
>
> > ---
> > kernel/power/suspend.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > Index: linux-pm/kernel/power/suspend.c
> > ===================================================================
> > --- linux-pm.orig/kernel/power/suspend.c
> > +++ linux-pm/kernel/power/suspend.c
> > @@ -109,8 +109,12 @@ static void s2idle_enter(void)
> >
> > static void s2idle_loop(void)
> > {
> > + ktime_t start, delta;
> > +
> > pm_pr_dbg("suspend-to-idle\n");
> >
> > + start = ktime_get();
> > +
> > for (;;) {
> > int error;
> >
> > @@ -150,6 +154,20 @@ static void s2idle_loop(void)
> > pm_wakeup_clear(false);
> > }
> >
> > + /*
> > + * If the monotonic clock difference between the start of the loop and
> > + * this point is too large, user space may get confused about whether or
> > + * not the system has been suspended and tasks may get killed by
> > + * watchdogs etc., so count the loop as "sleep time" to compensate for
> > + * that.
> > + */
> > + delta = ktime_sub(ktime_get(), start);
> > + if (ktime_to_ns(delta) > 0) {
> > + struct timespec64 timespec64_delta = ktime_to_timespec64(delta);
> > +
> > + timekeeping_inject_sleeptime64(×pec64_delta);
> > + }
> > +
> > pm_pr_dbg("resume from suspend-to-idle\n");
> > }
>
> Like I mentioned yesterday; I myself prefer the form:
>
>
> u64 stamp = ktimer_get_ns();
>
> for (;;) {
> /* ... */
> }
>
> stamp = ktime_get_ns() - stamp;
> if (stamp)
> timekeeping_inject_sleeptime64(ns_to_timespec64(ns));
>
>
> Esp. since ktime_t _is_ s64 these days, there is no point in keep using
> all the weird ktime helpers that make the code harder to read.
Looks like a good idea, let me try that.
