MIME-Version: 1.0
References: <20230211064527.3481754-1-jstultz@google.com> <20230211064527.3481754-2-jstultz@google.com>
 <CAOf5uwnW1u=nfFnj3C8kCVmhgwRaVh6sHZR1RGnXdbrCNpkGVg@mail.gmail.com>
 <87o7porea9.ffs@tglx> <CAOf5uwmhtQ8GXhMiE-Y3-wgL5=xfjOv0Tpq1vqPB8p=LyZHBmw@mail.gmail.com>
 <CAOf5uwmpayJwpAFzUS6qsCgdpyek1f-2t2t9YNr76vnRjSC8=w@mail.gmail.com>
 <87a618qlcp.ffs@tglx> <CAOf5uw=a2RYYFj+4_WOX+KaF_FCXSsUgfM+T2m2XjVuqKMdooA@mail.gmail.com>
 <87sff0ox1a.ffs@tglx>
In-Reply-To: <87sff0ox1a.ffs@tglx>
From:   John Stultz <jstultz@google.com>
Date:   Mon, 27 Feb 2023 16:03:36 -0800
Message-ID: <CANDhNCrfoiz1WdTs+5B8y+TVv8cn4_J-770=bsPqC9Xe=j14hA@mail.gmail.com>
Subject: Re: [RFC][PATCH 2/2] time: alarmtimer: Use TASK_FREEZABLE to cleanup
 freezer handling
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Michael Nazzareno Trimarchi <michael@amarulasolutions.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Stephen Boyd <sboyd@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Michael <michael@mipisi.de>, kernel-team@android.com,
        Peter Zijlstra <peterz@infradead.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Feb 20, 2023 at 1:18=E2=80=AFPM Thomas Gleixner <tglx@linutronix.de=
> wrote:
> On Mon, Feb 20 2023 at 19:11, Michael Nazzareno Trimarchi wrote:
> > [   89.674127] PM: suspend entry (deep)
> > [   89.714916] Filesystems sync: 0.037 seconds
> > [   89.733594] Freezing user space processes
> > [   89.740680] Freezing user space processes completed (elapsed 0.002 s=
econds)
>
> User space tasks are frozen now.
>
> > [   89.748593] OOM killer disabled.
> > [   89.752257] Freezing remaining freezable tasks
> > [   89.756807] alarmtimer_fired: called
> > [   89.756831] alarmtimer_dequeue: called <---- HERE
>
> Here fires the underlying hrtimer before devices are suspended, so the
> sig_sendqueue() cannot wake up the task because task->state =3D=3D
> TASK_FROZEN, which means the signal wont be handled and the timer wont
> be rearmed until the task is thawed.
>
> And as you correctly observed the alarmtimer_suspend() path won't see a
> pending timer anymore because it is dequeued.
>
> So precisely the time between freeze(alarmtask) and alarmtimer_suspend()
> is a gaping hole which guarantees lost wakeups.
>

Hey Michael, Thomas,
 Sorry for being a little slow to respond the last few weeks. :(

So I've managed to reproduce the issue following Michael's
instructions and your analysis, Thomas, looks right!

Though I'm a bit confused on the suggested solution:


> @@ -194,6 +196,8 @@ static enum hrtimer_restart alarmtimer_f
>         int ret =3D HRTIMER_NORESTART;
>         int restart =3D ALARMTIMER_NORESTART;
>
> +       atomic_inc(&alarmtimer_wakeup);
> +
>         spin_lock_irqsave(&base->lock, flags);
>         alarmtimer_dequeue(base, alarm);
>         spin_unlock_irqrestore(&base->lock, flags);

Ok, so here we're incrementing the wakeup counter for each
alarmtimer_fired call.

> @@ -244,6 +248,16 @@ static int alarmtimer_suspend(struct dev
>         if (!rtc)
>                 return 0;
>
> +       /*
> +        * Handle wakeups which happened between the start of suspend and
> +        * now as those wakeups might have tried to wake up a frozen task
> +        * which means they are not longer in the alarm timer list.
> +        */
> +       if (atomic_read(&alarmtimer_wakeup)) {
> +               pm_wakeup_event(dev, 0);
> +               return -EBUSY;
> +       }
> +
>         /* Find the soonest timer to expire*/
>         for (i =3D 0; i < ALARM_NUMTYPE; i++) {
>                 struct alarm_base *base =3D &alarm_bases[i];

And here we're bailing on alarmtimer_suspend if an alarmtimer_fired
happened recently.  Still looks okish.


> +static int alarmtimer_pm_notifier_fn(struct notifier_block *bl, unsigned=
 long state,
> +                                    void *unused)
> +{
> +       switch (state) {
> +       case PM_HIBERNATION_PREPARE:
> +       case PM_POST_HIBERNATION:
> +               atomic_set(&alarmtimer_wakeup, 0);
> +               break;
> +       }
> +       return NOTIFY_DONE;

But here, we're setting the alarmtimer_wakeup count to zero if we get
PM_HIBERNATION_PREPARE or  PM_POST_HIBERNATION notifications?
And Michael noted we need to add  PM_SUSPEND_PREPARE and
PM_POST_SUSPEND there for this to seemingly work.

This zeroing out the counter here feels a little sloppy, as it seems
nothing prevents the notifier from racing with the other added logic.

If the issue is that when we're expiring timers in alarmtimer_fire(),
a suspend event may come in midway after the alarmtimer_dequeue(),
while the timer is running but before the alarmtimer_enqueue(),
causing recurring timers to not be re-armed, it seems we probably
should do the accounting fully in alarmtimer_fire(), doing an
atomic_dec(&alarmtimer_wakeup) at the end of that function.  That way
we avoid suspending while running an alarmtimer, so the recurring
timers will always be back on the timer list when we do suspend.

This sort of has a risk that if there's timers that run for too long,
you'd prevent suspend for that period, but we already avoid suspending
if there's a alarmtimer within two seconds in the future, so having
alarmtimers recur quickly is inherently going to prevent you from
suspending.

I'll send out a reworked patch here in a moment after I validate it as work=
ing.

thanks
-john