Re: alarm timer/timerfd expiration does not abort suspend operation

Re: alarm timer/timerfd expiration does not abort suspend operation

[Thomas Gleixner]

On Fri, 3 Feb 2017, Gabriel Beddingfield wrote:

Cc'ing the PM folks

> Hi Thomas and John,
> 
> TL;DR: if an alarmtimer-backed timerfd expires just prior to
> alarmtimer_suspend() begin called, the system will continue to go into
> suspend (with possibly no future wakeups scheduled). The expected behavior
> is that the timer expiration would cause the suspend operation to abort. I
> see several ways to fix it and want to know your preference.
> 
> When using an alarmtimer-backed timerfd (i.e. CLOCK_BOOTTIME_ALARM or
> CLOCK_REALTIME_ALARM) we have observed the following race condition:
> 
> 1. Userspace commands the system to go into suspend (echo mem >
> /sys/power/state)
> 2. The alarmtimer for a timerfd expires, making the timer inactive until
> someone reads from the file descriptor.
> 3. alarmtimer_suspend() does not find any pending timers, and therefore
> does not schedule a wakeup.
> 4. device goes into suspend.
> 
> However, if steps 2 and 3 are swapped, alarmtimer_suspend() would have seen
> that an expiration was "soon" and cause an abort of the suspend. This can
> be reproduced on an idle system by having a process aggressively doing
> `echo mem > /sys/power/state' while another process sets a 4-sec repeating
> timerfd backed by CLOCK_BOOTTIME_ALARM. Eventually the system will go to
> sleep and not wake up.
> 
> I see a few ways to fix it:
> 
> 1. Create a wakeup_source for each timerfd, and if it's an alarm timer call
> __pm_stay_awake() in timerfd_triggered() and __pm_relax() in timerfd_read().
> 2. call pm_system_wakeup() in alarmtimer_fired()
> 3. call `if (isalarm(ctx)) pm_system_wakeup();' in timerfd_triggered()
> 4. call __pm_wakeup_event(ws, 2 * MSECS_PER_SEC) in alarmtimer_fired()
> 5. call `if (isalarm(ctc)) __pm_wakeup_event(ws, 2 * MSECS_PER_SEC);' in
> timerfd_triggered() (using a static struct wakeup_source).
> 
> I think #1 is right, followed by #2. They all have pros/cons:
> 
> * #1 Can eliminate race conditions (rather than an arbitrary 2-sec
> timeout)... but is effectively holding a hard-to-trace block on all PM
> operations (e.g. read/write of /sys/power/wakeup_count blocks until someone
> reads from the file descriptor).
> * #2 Matches the current behavior of the "happy case"... but bypasses the
> userspace policy system provided by wakeup.
> * #3 same pro/con as #2... but solution is specific to timerfd's.
> * #4 Matches the current behavior of the "happy case" if and only if
> userspace is using the 'wakeup' system, otherwise doesn't change any
> behavior. But, I wonder how many people think the current behavior is a bug.
> * #5 Same pro/con as #4... but solution is specific to timerfd's.
> 
> I've attached a patch that implements #1.
> 
> -gabe
> 

Re: alarm timer/timerfd expiration does not abort suspend operation

[John Stultz]

On Fri, Feb 10, 2017 at 1:43 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 3 Feb 2017, Gabriel Beddingfield wrote:
>> Hi Thomas and John,
>>
>> TL;DR: if an alarmtimer-backed timerfd expires just prior to
>> alarmtimer_suspend() begin called, the system will continue to go into
>> suspend (with possibly no future wakeups scheduled). The expected behavior
>> is that the timer expiration would cause the suspend operation to abort. I
>> see several ways to fix it and want to know your preference.
>>
>> When using an alarmtimer-backed timerfd (i.e. CLOCK_BOOTTIME_ALARM or
>> CLOCK_REALTIME_ALARM) we have observed the following race condition:
>>
>> 1. Userspace commands the system to go into suspend (echo mem >
>> /sys/power/state)
>> 2. The alarmtimer for a timerfd expires, making the timer inactive until
>> someone reads from the file descriptor.
>> 3. alarmtimer_suspend() does not find any pending timers, and therefore
>> does not schedule a wakeup.
>> 4. device goes into suspend.
>>
>> However, if steps 2 and 3 are swapped, alarmtimer_suspend() would have seen
>> that an expiration was "soon" and cause an abort of the suspend. This can
>> be reproduced on an idle system by having a process aggressively doing
>> `echo mem > /sys/power/state' while another process sets a 4-sec repeating
>> timerfd backed by CLOCK_BOOTTIME_ALARM. Eventually the system will go to
>> sleep and not wake up.
>>
>> I see a few ways to fix it:
>>
>> 1. Create a wakeup_source for each timerfd, and if it's an alarm timer call
>> __pm_stay_awake() in timerfd_triggered() and __pm_relax() in timerfd_read().
>> 2. call pm_system_wakeup() in alarmtimer_fired()
>> 3. call `if (isalarm(ctx)) pm_system_wakeup();' in timerfd_triggered()
>> 4. call __pm_wakeup_event(ws, 2 * MSECS_PER_SEC) in alarmtimer_fired()
>> 5. call `if (isalarm(ctc)) __pm_wakeup_event(ws, 2 * MSECS_PER_SEC);' in
>> timerfd_triggered() (using a static struct wakeup_source).
>>
>> I think #1 is right, followed by #2. They all have pros/cons:
>>
>> * #1 Can eliminate race conditions (rather than an arbitrary 2-sec
>> timeout)... but is effectively holding a hard-to-trace block on all PM
>> operations (e.g. read/write of /sys/power/wakeup_count blocks until someone
>> reads from the file descriptor).
>> * #2 Matches the current behavior of the "happy case"... but bypasses the
>> userspace policy system provided by wakeup.
>> * #3 same pro/con as #2... but solution is specific to timerfd's.
>> * #4 Matches the current behavior of the "happy case" if and only if
>> userspace is using the 'wakeup' system, otherwise doesn't change any
>> behavior. But, I wonder how many people think the current behavior is a bug.
>> * #5 Same pro/con as #4... but solution is specific to timerfd's.
>>
>> I've attached a patch that implements #1.

Sorry for not getting back to you earlier (been traveling this week).

I'm surprised this issue hasn't bitten any of the android folks, as I
believe they have been making use of this for some time now. CC'ing a
few just to make sure we're all on the same page.

The approach you took in your patch looks basically ok to me, though I
think the __pm_wakeup_event() method in #4 sounds safer, just to avoid
the problematic issue if no one is waiting on the fd.

Though I worry I'm not quite understanding the con for that case
properly, so maybe you can clarify what concerns you there?

thanks
-john

Re: alarm timer/timerfd expiration does not abort suspend operation

[Gabriel Beddingfield]

Hi John,

Re-sending because VGER rejected my hipster HTML mail... sorry!

On Fri, Feb 10, 2017 at 10:49 AM, John Stultz <john.stultz@linaro.org> wrote:
>> I see a few ways to fix it:
>>
>> 1. Create a wakeup_source for each timerfd, and if it's an alarm timer
>> call
>> __pm_stay_awake() in timerfd_triggered() and __pm_relax() in
>> timerfd_read().
>> 2. call pm_system_wakeup() in alarmtimer_fired()
>> 3. call `if (isalarm(ctx)) pm_system_wakeup();' in timerfd_triggered()
>> 4. call __pm_wakeup_event(ws, 2 * MSECS_PER_SEC) in alarmtimer_fired()
>> 5. call `if (isalarm(ctc)) __pm_wakeup_event(ws, 2 * MSECS_PER_SEC);'
>> in
>> timerfd_triggered() (using a static struct wakeup_source).
[snip]
>> * #4 Matches the current behavior of the "happy case" if and only if
>> userspace is using the 'wakeup' system, otherwise doesn't change any
>> behavior. But, I wonder how many people think the current behavior is a
>> bug.
[snip]
> The approach you took in your patch looks basically ok to me, though I
>
> think the __pm_wakeup_event() method in #4 sounds safer, just to avoid
> the problematic issue if no one is waiting on the fd.
>
> Though I worry I'm not quite understanding the con for that case
> properly, so maybe you can clarify what concerns you there?

The concern is born of my personal experience: I was ignorant of the
"wakeup_count" protocol, and so I wasn't using it. Because of this
__pm_wakeup_event() would not block a suspend because I never wrote to
wakeup_count. On the other hand, method #2 will work unconditionally.

-gabe

Re: alarm timer/timerfd expiration does not abort suspend operation

[Rafael J. Wysocki]

On Fri, Feb 10, 2017 at 8:23 PM, Gabriel Beddingfield <gabe@nestlabs.com> wrote:
> Hi John,
>
> Re-sending because VGER rejected my hipster HTML mail... sorry!
>
> On Fri, Feb 10, 2017 at 10:49 AM, John Stultz <john.stultz@linaro.org> wrote:
>>> I see a few ways to fix it:
>>>
>>> 1. Create a wakeup_source for each timerfd, and if it's an alarm timer
>>> call
>>> __pm_stay_awake() in timerfd_triggered() and __pm_relax() in
>>> timerfd_read().
>>> 2. call pm_system_wakeup() in alarmtimer_fired()
>>> 3. call `if (isalarm(ctx)) pm_system_wakeup();' in timerfd_triggered()
>>> 4. call __pm_wakeup_event(ws, 2 * MSECS_PER_SEC) in alarmtimer_fired()
>>> 5. call `if (isalarm(ctc)) __pm_wakeup_event(ws, 2 * MSECS_PER_SEC);'
>>> in
>>> timerfd_triggered() (using a static struct wakeup_source).
> [snip]
>>> * #4 Matches the current behavior of the "happy case" if and only if
>>> userspace is using the 'wakeup' system, otherwise doesn't change any
>>> behavior. But, I wonder how many people think the current behavior is a
>>> bug.
> [snip]
>> The approach you took in your patch looks basically ok to me, though I
>>
>> think the __pm_wakeup_event() method in #4 sounds safer, just to avoid
>> the problematic issue if no one is waiting on the fd.
>>
>> Though I worry I'm not quite understanding the con for that case
>> properly, so maybe you can clarify what concerns you there?
>
> The concern is born of my personal experience: I was ignorant of the
> "wakeup_count" protocol, and so I wasn't using it. Because of this
> __pm_wakeup_event() would not block a suspend because I never wrote to
> wakeup_count. On the other hand, method #2 will work unconditionally.

Right.

So if you want the timerfd behavior to not depend on whether or not
wakeup_count is used by user space, #2 is the way to go.

That said, creating a wakeup source for each timerfd would be nicer
from the diagnostics perspective, so maybe you can combine #1 and #2
such that if user space doesn't use wakeup_count, alarmtimer_fired()
will still abort suspends in a hard way?

Thanks,
Rafael