Hello Michael,
This code is maintained by the timekeeping maintainers, now in Cc and I
think John will be able to answer.
On 31/08/2019 20:32:06+0200, Michael wrote:
> Dear members of the linux-rtc list,
>
> currently I have a problem with the alarmtimer i'm using to cyclically wake
> up my i.MX6 ULL board from suspend to RAM.
>
> The problem is that in principle the timer wake ups work fine but seem to be
> not 100% stable. In about 1 percent the wake up alarm from suspend is
> missing.
>
> When I look at the code of alarmtimer in function alarmtimer_suspend
> (kernel/time/alarmtimer.c)
> I find the following:
>
> ....
>
> /* Find the soonest timer to expire*/
>
> ??? for (i = 0; i < ALARM_NUMTYPE; i++) {
> ??? ??? struct alarm_base *base = &alarm_bases[i];
> ??? ??? struct timerqueue_node *next;
> ??? ??? ktime_t delta;
>
> ??? ??? spin_lock_irqsave(&base->lock, flags);
> ??? ??? next = timerqueue_getnext(&base->timerqueue);
> ??? ??? spin_unlock_irqrestore(&base->lock, flags);
> ??? ??? if (!next)
> ??? ??? ??? continue;
> ??? ??? delta = ktime_sub(next->expires, base->gettime());
> ??? ??? if (!min || (delta < min)) {
> ??? ??? ??? expires = next->expires;
> ??? ??? ??? min = delta;
> ??? ??? ??? type = i;
> ??? ??? }
> ??? }
> ??? if (min == 0)
> ??? ??? return 0;
>
> ??? if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
> ??? ??? __pm_wakeup_event(ws, 2 * MSEC_PER_SEC);
> ??? ??? return -EBUSY;
> ??? }
>
> In my error case the alarm wake up always fails if the path "if(min==0)" is
> entered. If I understand this code correctly that means that
> when ever one of the timers in the list has a remaining tick time of zero,
> the function just returns 0 and continues the suspend process until
> it reaches suspend mode.
>
> If I implement a hack here "if(min == 0) {min = 1;}" and do not return, my
> system runs 100% ok, as the following -EBUSY path is hit.
>
> So my question to you is: Why is there a check if min < 2 seconds and do a
> return -EBUSY here, but handle (min==0) differently?
> Could there be some race condition here, where the function
> alarmtimer_suspend just returns 0 and shortly after this the alarmtimer
> expires
> right before the RTC driver was able to allow the wake up interrupt?
>
> If I look through the kernel versions I found the alarmtimer_suspend to be a
> very stable function, so I don't think there is anything wrong here.
>
> But do you have a hint for me where else I could have a look to encircle the
> error?
>
> Thank you very much!
>
> Br,
> Michael
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Michael,
On Mon, 2 Sep 2019, Alexandre Belloni wrote:
> On 31/08/2019 20:32:06+0200, Michael wrote:
> > currently I have a problem with the alarmtimer i'm using to cyclically wake
> > up my i.MX6 ULL board from suspend to RAM.
> >
> > The problem is that in principle the timer wake ups work fine but seem to be
> > not 100% stable. In about 1 percent the wake up alarm from suspend is
> > missing.
> > In my error case the alarm wake up always fails if the path "if(min==0)" is
> > entered. If I understand this code correctly that means that
> > when ever one of the timers in the list has a remaining tick time of zero,
> > the function just returns 0 and continues the suspend process until
> > it reaches suspend mode.
No. That code is simply broken because it tries to handle the case where a
alarmtimer nanosleep got woken up by the freezer. That's broken because it
makes the delta = 0 assumption which leads to the issue you discovered.
That whole cruft can be removed by switching alarmtimer nanosleep to use
freezable_schedule(). That keeps the timer queued and avoids all the issues.
Completely untested patch below.
Thanks,
tglx
8<----------------------
kernel/time/alarmtimer.c | 57 +++--------------------------------------------
1 file changed, 4 insertions(+), 53 deletions(-)
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -46,14 +46,6 @@ static struct alarm_base {
clockid_t base_clockid;
} alarm_bases[ALARM_NUMTYPE];
-#if defined(CONFIG_POSIX_TIMERS) || defined(CONFIG_RTC_CLASS)
-/* freezer information to handle clock_nanosleep triggered wakeups */
-static enum alarmtimer_type freezer_alarmtype;
-static ktime_t freezer_expires;
-static ktime_t freezer_delta;
-static DEFINE_SPINLOCK(freezer_delta_lock);
-#endif
-
#ifdef CONFIG_RTC_CLASS
static struct wakeup_source *ws;
@@ -241,19 +233,12 @@ EXPORT_SYMBOL_GPL(alarm_expires_remainin
*/
static int alarmtimer_suspend(struct device *dev)
{
- ktime_t min, now, expires;
+ ktime_t now, expires, min = KTIME_MAX;
int i, ret, type;
struct rtc_device *rtc;
unsigned long flags;
struct rtc_time tm;
- spin_lock_irqsave(&freezer_delta_lock, flags);
- min = freezer_delta;
- expires = freezer_expires;
- type = freezer_alarmtype;
- freezer_delta = 0;
- spin_unlock_irqrestore(&freezer_delta_lock, flags);
-
rtc = alarmtimer_get_rtcdev();
/* If we have no rtcdev, just return */
if (!rtc)
@@ -271,13 +256,13 @@ static int alarmtimer_suspend(struct dev
if (!next)
continue;
delta = ktime_sub(next->expires, base->gettime());
- if (!min || (delta < min)) {
+ if (delta < min) {
expires = next->expires;
min = delta;
type = i;
}
}
- if (min == 0)
+ if (min == KTIME_MAX)
return 0;
if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
@@ -479,38 +464,6 @@ u64 alarm_forward_now(struct alarm *alar
EXPORT_SYMBOL_GPL(alarm_forward_now);
#ifdef CONFIG_POSIX_TIMERS
-
-static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
-{
- struct alarm_base *base;
- unsigned long flags;
- ktime_t delta;
-
- switch(type) {
- case ALARM_REALTIME:
- base = &alarm_bases[ALARM_REALTIME];
- type = ALARM_REALTIME_FREEZER;
- break;
- case ALARM_BOOTTIME:
- base = &alarm_bases[ALARM_BOOTTIME];
- type = ALARM_BOOTTIME_FREEZER;
- break;
- default:
- WARN_ONCE(1, "Invalid alarm type: %d\n", type);
- return;
- }
-
- delta = ktime_sub(absexp, base->gettime());
-
- spin_lock_irqsave(&freezer_delta_lock, flags);
- if (!freezer_delta || (delta < freezer_delta)) {
- freezer_delta = delta;
- freezer_expires = absexp;
- freezer_alarmtype = type;
- }
- spin_unlock_irqrestore(&freezer_delta_lock, flags);
-}
-
/**
* clock2alarm - helper that converts from clockid to alarmtypes
* @clockid: clockid.
@@ -715,7 +668,7 @@ static int alarmtimer_do_nsleep(struct a
set_current_state(TASK_INTERRUPTIBLE);
alarm_start(alarm, absexp);
if (likely(alarm->data))
- schedule();
+ freezable_schedule();
alarm_cancel(alarm);
} while (alarm->data && !signal_pending(current));
@@ -727,8 +680,6 @@ static int alarmtimer_do_nsleep(struct a
if (!alarm->data)
return 0;
- if (freezing(current))
- alarmtimer_freezerset(absexp, type);
restart = ¤t->restart_block;
if (restart->nanosleep.type != TT_NONE) {
struct timespec64 rmt;
Thomas,
thank you very much for your patch. Unfortunately currently I can only
test it with a kernel 4.1.52 but i've tried to patch
your new logic into my older kernel version.
There seem to be rare cases where the "delta" value becomes negative.
Therefore I added
if(unlikely(delta < 0)) {
delta = 0;
}
before min-check.
Currently I still get returns here in the new code
+ if (min == KTIME_MAX)
return 0;
where the board afterwards is not woken up.So I think there is still
something missing.
I'm doing further tests and keep you informed.
Again Thanks!
Michael
On 02.09.2019 12:57, Thomas Gleixner wrote:
> Michael,
>
> On Mon, 2 Sep 2019, Alexandre Belloni wrote:
>> On 31/08/2019 20:32:06+0200, Michael wrote:
>>> currently I have a problem with the alarmtimer i'm using to cyclically wake
>>> up my i.MX6 ULL board from suspend to RAM.
>>>
>>> The problem is that in principle the timer wake ups work fine but seem to be
>>> not 100% stable. In about 1 percent the wake up alarm from suspend is
>>> missing.
>>> In my error case the alarm wake up always fails if the path "if(min==0)" is
>>> entered. If I understand this code correctly that means that
>>> when ever one of the timers in the list has a remaining tick time of zero,
>>> the function just returns 0 and continues the suspend process until
>>> it reaches suspend mode.
> No. That code is simply broken because it tries to handle the case where a
> alarmtimer nanosleep got woken up by the freezer. That's broken because it
> makes the delta = 0 assumption which leads to the issue you discovered.
>
> That whole cruft can be removed by switching alarmtimer nanosleep to use
> freezable_schedule(). That keeps the timer queued and avoids all the issues.
>
> Completely untested patch below.
>
> Thanks,
>
> tglx
>
> 8<----------------------
>
> kernel/time/alarmtimer.c | 57 +++--------------------------------------------
> 1 file changed, 4 insertions(+), 53 deletions(-)
>
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -46,14 +46,6 @@ static struct alarm_base {
> clockid_t base_clockid;
> } alarm_bases[ALARM_NUMTYPE];
>
> -#if defined(CONFIG_POSIX_TIMERS) || defined(CONFIG_RTC_CLASS)
> -/* freezer information to handle clock_nanosleep triggered wakeups */
> -static enum alarmtimer_type freezer_alarmtype;
> -static ktime_t freezer_expires;
> -static ktime_t freezer_delta;
> -static DEFINE_SPINLOCK(freezer_delta_lock);
> -#endif
> -
> #ifdef CONFIG_RTC_CLASS
> static struct wakeup_source *ws;
>
> @@ -241,19 +233,12 @@ EXPORT_SYMBOL_GPL(alarm_expires_remainin
> */
> static int alarmtimer_suspend(struct device *dev)
> {
> - ktime_t min, now, expires;
> + ktime_t now, expires, min = KTIME_MAX;
> int i, ret, type;
> struct rtc_device *rtc;
> unsigned long flags;
> struct rtc_time tm;
>
> - spin_lock_irqsave(&freezer_delta_lock, flags);
> - min = freezer_delta;
> - expires = freezer_expires;
> - type = freezer_alarmtype;
> - freezer_delta = 0;
> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
> -
> rtc = alarmtimer_get_rtcdev();
> /* If we have no rtcdev, just return */
> if (!rtc)
> @@ -271,13 +256,13 @@ static int alarmtimer_suspend(struct dev
> if (!next)
> continue;
> delta = ktime_sub(next->expires, base->gettime());
> - if (!min || (delta < min)) {
> + if (delta < min) {
> expires = next->expires;
> min = delta;
> type = i;
> }
> }
> - if (min == 0)
> + if (min == KTIME_MAX)
> return 0;
>
> if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
> @@ -479,38 +464,6 @@ u64 alarm_forward_now(struct alarm *alar
> EXPORT_SYMBOL_GPL(alarm_forward_now);
>
> #ifdef CONFIG_POSIX_TIMERS
> -
> -static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
> -{
> - struct alarm_base *base;
> - unsigned long flags;
> - ktime_t delta;
> -
> - switch(type) {
> - case ALARM_REALTIME:
> - base = &alarm_bases[ALARM_REALTIME];
> - type = ALARM_REALTIME_FREEZER;
> - break;
> - case ALARM_BOOTTIME:
> - base = &alarm_bases[ALARM_BOOTTIME];
> - type = ALARM_BOOTTIME_FREEZER;
> - break;
> - default:
> - WARN_ONCE(1, "Invalid alarm type: %d\n", type);
> - return;
> - }
> -
> - delta = ktime_sub(absexp, base->gettime());
> -
> - spin_lock_irqsave(&freezer_delta_lock, flags);
> - if (!freezer_delta || (delta < freezer_delta)) {
> - freezer_delta = delta;
> - freezer_expires = absexp;
> - freezer_alarmtype = type;
> - }
> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
> -}
> -
> /**
> * clock2alarm - helper that converts from clockid to alarmtypes
> * @clockid: clockid.
> @@ -715,7 +668,7 @@ static int alarmtimer_do_nsleep(struct a
> set_current_state(TASK_INTERRUPTIBLE);
> alarm_start(alarm, absexp);
> if (likely(alarm->data))
> - schedule();
> + freezable_schedule();
>
> alarm_cancel(alarm);
> } while (alarm->data && !signal_pending(current));
> @@ -727,8 +680,6 @@ static int alarmtimer_do_nsleep(struct a
> if (!alarm->data)
> return 0;
>
> - if (freezing(current))
> - alarmtimer_freezerset(absexp, type);
> restart = ¤t->restart_block;
> if (restart->nanosleep.type != TT_NONE) {
> struct timespec64 rmt;
On Tue, 3 Sep 2019, Michael wrote:
> Thomas,
>
> thank you very much for your patch. Unfortunately currently I can only test it
> with a kernel 4.1.52 but i've tried to patch
> your new logic into my older kernel version.
<formletter>
https://people.kernel.org/tglx/notes-about-netiquette
</formletter>
Hi Thomas
On Wed, Sep 04, 2019 at 12:49:21AM +0200, Thomas Gleixner wrote:
> On Tue, 3 Sep 2019, Michael wrote:
>
> > Thomas,
> >
> > thank you very much for your patch. Unfortunately currently I can only test it
> > with a kernel 4.1.52 but i've tried to patch
> > your new logic into my older kernel version.
>
Is this patch valid on mainline too? because apply it was let rtc
working 100% of the time
Michael
> <formletter>
>
> https://people.kernel.org/tglx/notes-about-netiquette
>
> </formletter>
>
Michael!
On Wed, Feb 08 2023 at 16:23, Michael Trimarchi wrote:
> On Wed, Sep 04, 2019 at 12:49:21AM +0200, Thomas Gleixner wrote:
>> On Tue, 3 Sep 2019, Michael wrote:
>> >
>> > thank you very much for your patch. Unfortunately currently I can only test it
>> > with a kernel 4.1.52 but i've tried to patch
>> > your new logic into my older kernel version.
>>
> Is this patch valid on mainline too? because apply it was let rtc
> working 100% of the time
I wrote that patch against the back then mainline code. No idea if it's
still applying, but the underlying issue is still the same AFAICT.
It needs some polishing and a proper changelog.
Thanks,
tglx
Hi Thomas
On Wed, Feb 8, 2023 at 7:06 PM Thomas Gleixner <[email protected]> wrote:
>
> Michael!
>
> On Wed, Feb 08 2023 at 16:23, Michael Trimarchi wrote:
> > On Wed, Sep 04, 2019 at 12:49:21AM +0200, Thomas Gleixner wrote:
> >> On Tue, 3 Sep 2019, Michael wrote:
> >> >
> >> > thank you very much for your patch. Unfortunately currently I can only test it
> >> > with a kernel 4.1.52 but i've tried to patch
> >> > your new logic into my older kernel version.
> >>
> > Is this patch valid on mainline too? because apply it was let rtc
> > working 100% of the time
>
> I wrote that patch against the back then mainline code. No idea if it's
> still applying, but the underlying issue is still the same AFAICT.
>
> It needs some polishing and a proper changelog.
>
Ok, I will try to update it on some mainline kernel in my environment
and test it back. I need
a little information if it's possible. Consider that I have no
experience in this area. I understand how
code was designed in general but the part around the freezer and all
those code you remove, what was the logic behind in the removed code?
Michael
> Thanks,
>
> tglx
>
>
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com
Michael!
On Thu, Feb 09 2023 at 12:19, Michael Nazzareno Trimarchi wrote:
> On Wed, Feb 8, 2023 at 7:06 PM Thomas Gleixner <[email protected]> wrote:
>> I wrote that patch against the back then mainline code. No idea if it's
>> still applying, but the underlying issue is still the same AFAICT.
>>
>> It needs some polishing and a proper changelog.
>>
> Ok, I will try to update it on some mainline kernel in my environment
> and test it back. I need
> a little information if it's possible. Consider that I have no
> experience in this area. I understand how
> code was designed in general but the part around the freezer and all
> those code you remove, what was the logic behind in the removed code?
What I can oracle out of that well commented gem is:
A userspace task invokes clock_nanosleep(CLOCK_*_ALARM, ...), which
arms an alarm timer. The expiry of an alarmtimer causes the system to
come out of suspend.
As the task invokes schedule() it can also be brought back from
schedule() via a signal. If that happens then the task cancels the
alarmtimer and returns to handle the signal. While doing that it can
be frozen, which means the alarm and therefore the wake from suspend
is lost.
To prevent that the code tries to save the earliest expiring alarm if
the task is marked freezing() and the suspend code takes that into
account.
John, did I summarize that correctly?
The change I made remove that magic and marks the task freezable when it
goes to schedule, which prevents the signal wakeup. That ensures that
the alarm stays armed during freeze/suspend.
That obviously needs some testing and scrunity by the folks which use
this mechanism. IIRC that's used by android, but I might be wrong.
Thanks,
tglx
On Thu, Feb 9, 2023 at 7:40 AM Thomas Gleixner <[email protected]> wrote:
> On Thu, Feb 09 2023 at 12:19, Michael Nazzareno Trimarchi wrote:
> > On Wed, Feb 8, 2023 at 7:06 PM Thomas Gleixner <[email protected]> wrote:
> >> I wrote that patch against the back then mainline code. No idea if it's
> >> still applying, but the underlying issue is still the same AFAICT.
> >>
> >> It needs some polishing and a proper changelog.
> >>
> > Ok, I will try to update it on some mainline kernel in my environment
> > and test it back. I need
> > a little information if it's possible. Consider that I have no
> > experience in this area. I understand how
> > code was designed in general but the part around the freezer and all
> > those code you remove, what was the logic behind in the removed code?
>
> What I can oracle out of that well commented gem is:
>
> A userspace task invokes clock_nanosleep(CLOCK_*_ALARM, ...), which
> arms an alarm timer. The expiry of an alarmtimer causes the system to
> come out of suspend.
>
> As the task invokes schedule() it can also be brought back from
> schedule() via a signal. If that happens then the task cancels the
> alarmtimer and returns to handle the signal. While doing that it can
> be frozen, which means the alarm and therefore the wake from suspend
> is lost.
>
> To prevent that the code tries to save the earliest expiring alarm if
> the task is marked freezing() and the suspend code takes that into
> account.
>
> John, did I summarize that correctly?
>
> The change I made remove that magic and marks the task freezable when it
> goes to schedule, which prevents the signal wakeup. That ensures that
> the alarm stays armed during freeze/suspend.
>
> That obviously needs some testing and scrunity by the folks which use
> this mechanism. IIRC that's used by android, but I might be wrong.
So, thanks for dredging this old thread up, I'm sorry I didn't see it
the first time it came around.
Not having a clear memory of why we do the (min == 0) early return, I
went digging in, and found it was in the original git commit, so I
went looking to the archives.
It's completely not present in the first version of the patch:
https://lore.kernel.org/lkml/[email protected]/
But it did appear in the second version:
https://lore.kernel.org/lkml/[email protected]/
And from there it's a clear case of wanting to avoid setting the RTC
if there were just no timers to expire.
But, indeed this is a bug, as initializing min to ktime_set(0, 0) as
the "invalid" case isn't a good plan, as it might be possible that
suspend is run right as an alarmtimer expires, and you get a real zero
delta value (as has been reported).
Instead it seemed I should have used KTIME_MAX as the "invalid" case
(as Thomas' patch uses).
However, before the patch was merged, it changed further to handle the
freezer waking a current sleeper:
https://lore.kernel.org/lkml/[email protected]/
Which was then used to initialize the min value (still erroneously
using 0 as an "invalid" value) in the case that the freezer woke a
task sleeping which would cause the alarm to be lost (as Thomas
summarized).
Thomas' patch fixes the erronious 0-as-invalid initialization issue
using KTIME_MAX but also simplifies the logic getting rid of the
freezer handling.
I don't have as much familiarity with the freezer handling change, so
while it looks sane, I can't say I would likely catch an issue doing a
visual review.
Michael: If you are still intending to send the patch out, please do,
otherwise I've already forward ported it so I can do some testing with
it. I'm happy to put together a commit message and send it out if
that's easier for you.
And, just for correct Reported-by: tags: Michael Trimarchi, are you
the same Michael ([email protected]) that originally reported this
issue?
thanks
-john
On Fri, Feb 10, 2023 at 5:04 PM John Stultz <[email protected]> wrote:
> Thomas' patch fixes the erronious 0-as-invalid initialization issue
> using KTIME_MAX but also simplifies the logic getting rid of the
> freezer handling.
>
> I don't have as much familiarity with the freezer handling change, so
> while it looks sane, I can't say I would likely catch an issue doing a
> visual review.
Actually, because of this, I'm going to split Thomas' change in two.
The first to just use KTIME_MAX as the invalid initialization value,
and the second to cleanup the freezer logic.
That way if the freezer change is problematic we can revert it and not
lose the fix here.
thanks
-john
Hi John
On Sat, Feb 11, 2023 at 2:18 AM John Stultz <[email protected]> wrote:
>
> On Fri, Feb 10, 2023 at 5:04 PM John Stultz <[email protected]> wrote:
> > Thomas' patch fixes the erronious 0-as-invalid initialization issue
> > using KTIME_MAX but also simplifies the logic getting rid of the
> > freezer handling.
> >
> > I don't have as much familiarity with the freezer handling change, so
> > while it looks sane, I can't say I would likely catch an issue doing a
> > visual review.
>
> Actually, because of this, I'm going to split Thomas' change in two.
>
> The first to just use KTIME_MAX as the invalid initialization value,
> and the second to cleanup the freezer logic.
>
> That way if the freezer change is problematic we can revert it and not
> lose the fix here.
>
> thanks
> -john
I'm not that Michael but it's ok I think to add both as a reporter. I can try to
test on my side too and then send a tested-by
Michael
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com