Messages in this thread Patch in this message | | | From | Michael Nazzareno Trimarchi <> | Date | Mon, 20 Feb 2023 22:32:13 +0100 | Subject | Re: [RFC][PATCH 2/2] time: alarmtimer: Use TASK_FREEZABLE to cleanup freezer handling |
| |
Hi
On Mon, Feb 20, 2023 at 10:18 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > Michael! > > On Mon, Feb 20 2023 at 19:11, Michael Nazzareno Trimarchi wrote: > > On Mon, Feb 20, 2023 at 6:48 PM Thomas Gleixner <tglx@linutronix.de> wrote: > >> [ 27.349352] alarmtimer_enqueue() > >> > >> U: Before SUSPEND > >> > >> [ 31.353228] PM: suspend entry (s2idle) > >> [ 31.388857] Filesystems sync: 0.033 seconds > >> [ 31.418427] Freezing user space processes > >> [ 31.422406] Freezing user space processes completed (elapsed 0.002 seconds) > >> [ 31.425435] OOM killer disabled. > >> [ 31.426833] Freezing remaining freezable tasks > >> [ 31.429838] Freezing remaining freezable tasks completed (elapsed 0.001 seconds) > >> [ 31.432922] printk: Suspending console(s) (use no_console_suspend to debug) > >> [ 31.435912] alarmtimer alarmtimer.0.auto: PM: dpm_run_callback(): platform_pm_suspend+0x0/0x50 returns -16 > >> [ 31.435954] alarmtimer alarmtimer.0.auto: PM: failed to suspend: error -16 > >> > >> That means the RTC interrupt was raised before the system was able to suspend. > > > > if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) { > > pm_wakeup_event(dev, 2 * MSEC_PER_SEC); > > return -EBUSY; > > } > > > > I think that above happens to you. So it means that you are too close > > to this limit, can be? > > Maybe. > > > Yes but the alarm for me was set to be fired just before freezing. Is > > this a valid scenario? > > Sure why not? > > > Let's say that I set an alarm to be fired just before the userspace > > freeze happens. If I'm close to it then then process will not process > > the signal to enquene again the alarm in the list and then during > > alarm suspend the list will be empty for the above. > > Halfways, but slowly your explanations start to make sense. Here is the > dmesg output you provided: > > > [ 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 seconds) > > 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 == > 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. > > That's completely unrelated to Johns patches. That hole has been there > forever. >
That was clear, but I was trying to test the race here.
> I assume that this horrible freezer hackery was supposed to plug that > hole, but that gem is not solving anything as far as I understand what > it is doing. I'm still failing to understand what it is supposed to > solve, but that's a different story. >
Yes, cleaning up was good.
> Aside of that I can clearly reproduce the issue, now that I understand > what you are trying to tell, on plain 6.2 without and with John's > cleanup. > > Something like the below plugs the hole reliably. >
Some comments below
> Thanks, > > tglx > --- > --- a/kernel/time/alarmtimer.c > +++ b/kernel/time/alarmtimer.c > @@ -26,6 +26,7 @@ > #include <linux/freezer.h> > #include <linux/compat.h> > #include <linux/module.h> > +#include <linux/suspend.h> > #include <linux/time_namespace.h> > > #include "posix-timers.h" > @@ -176,6 +177,7 @@ static void alarmtimer_dequeue(struct al > alarm->state &= ~ALARMTIMER_STATE_ENQUEUED; > } > > +static atomic_t alarmtimer_wakeup; > > /** > * alarmtimer_fired - Handles alarm hrtimer being fired. > @@ -194,6 +196,8 @@ static enum hrtimer_restart alarmtimer_f > int ret = HRTIMER_NORESTART; > int restart = ALARMTIMER_NORESTART; > > + atomic_inc(&alarmtimer_wakeup); > +
ptr->it_active = 0; if (ptr->it_interval) { atomic_inc(&alarmtimer_wakeup); si_private = ++ptr->it_requeue_pending; }
Should I not go to the alarm_handle_timer? and only if it's a periodic one?
Michael
> spin_lock_irqsave(&base->lock, flags); > alarmtimer_dequeue(base, alarm); > spin_unlock_irqrestore(&base->lock, flags); > @@ -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 = 0; i < ALARM_NUMTYPE; i++) { > struct alarm_base *base = &alarm_bases[i]; > @@ -296,6 +310,31 @@ static int alarmtimer_resume(struct devi > return 0; > } > > +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; > +} > + > +static struct notifier_block alarmtimer_pm_notifier = { > + .notifier_call = alarmtimer_pm_notifier_fn, > +}; > + > +static inline int alarmtimer_register_pm_notifier(void) > +{ > + return register_pm_notifier(&alarmtimer_pm_notifier); > +} > + > +static inline void alarmtimer_unregister_pm_notifier(void) > +{ > + unregister_pm_notifier(&alarmtimer_pm_notifier); > +} > #else > static int alarmtimer_suspend(struct device *dev) > { > @@ -306,6 +345,15 @@ static int alarmtimer_resume(struct devi > { > return 0; > } > + > +static inline int alarmtimer_register_pm_notifier(void) > +{ > + return 0; > +} > + > +static inline void alarmtimer_unregister_pm_notifier(void) > +{ > +} > #endif > > static void > @@ -904,11 +952,17 @@ static int __init alarmtimer_init(void) > if (error) > return error; > > - error = platform_driver_register(&alarmtimer_driver); > + error = alarmtimer_register_pm_notifier(); > if (error) > goto out_if; > > + error = platform_driver_register(&alarmtimer_driver); > + if (error) > + goto out_pm; > + > return 0; > +out_pm: > + alarmtimer_unregister_pm_notifier(); > out_if: > alarmtimer_rtc_interface_remove(); > return error; > > > > > > > >
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com www.amarulasolutions.com
| |