On Thu, Jun 07, 2018 at 11:27:34AM +0000, sunil+b...@nimmagadda.net wrote:
> >Synopsis: apmd(8) poll timer off by 10x
> >Category: system
> >Environment:
> System : OpenBSD 6.3
> Details : OpenBSD 6.3-current (GENERIC.MP) #54: Wed May 30 23:03:50
> MDT 2018
>
> dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>
> Architecture: OpenBSD.amd64
> Machine : amd64
> >Description:
> With apmd_flags="-Az10", expected apmd(8) to suspend when
> battery is at 10%, however, it didn't check in time and
> laptop ran of out power.
> >How-To-Repeat:
> Disconnect A/C adapter and run with -z percent greater
> than current estimated battery life reported by apm(8);
> poll every minute, for example...
> # rcctl stop apmd
> # apmd -A -z90 -t60
> should suspend in a minute, however, it suspends after 10
> minutes.
> >Fix:
> The following diff...
>
> 1. Provides a dedicated timer that fires every 10 seconds
> instead of relying on EVFILT_READ freqency.
>
> 2. Increments a counter and checks against timeout value,
> if it exceeds, invokes auto-action.
>
> 3. Wraps a few long lines that exceed 80 cols upon code
> shuffle.
I don't think we need to introduce an additional timer, or do so much
code shufflin' here, to fix your issue. The problem seems to be that
apmtimeout is incremented once per iteration but must meet or exceed
ts.tv_sec to trigger a status check, so the period for battery status
checks is at least n^2 seconds.
I'm pretty sure this was unintentional, though it makes it unlikely
that apmd will catch a low battery percentage and suspend the machine
before the battery is totally exhausted. Especially since, by default,
n = 600.
Here's a minimal diff that checks if we timed out on return from kevent.
There's additional cleanup that this change implies, but I've left it out
for the moment.
Of note is that an event for either of the descriptors resets the
timeout, regardless of how long it's been since we checked the battery
status: this is effectively the current behavior. If people want, we
can add logic to decrement the maximum timeout accordingly on each
iteration and reset it when kevent truly times out. This sounds closer
to what the manpage advertises for the -t option.
Caveat: I'm unfamiliar with apmd(8) and I don't have time just this
second scour the change log to figure out why the behavior is what it
is now. Someone more familiar with the code will need to corroborate
what I've said and the attached diff.
That said, feel free to try this diff in the meantime. Does this work
for you?
Anyone more familiar with apmd(8) wanna chime in here?
--
Scott Cheloha
Index: usr.sbin/apmd/apmd.c
===================================================================
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.81
diff -u -p -r1.81 apmd.c
--- usr.sbin/apmd/apmd.c 15 Oct 2017 15:14:49 -0000 1.81
+++ usr.sbin/apmd/apmd.c 7 Jun 2018 20:26:11 -0000
@@ -488,7 +488,7 @@ main(int argc, char *argv[])
if ((rv = kevent(kq, NULL, 0, ev, 1, &sts)) < 0)
break;
- if (apmtimeout >= ts.tv_sec) {
+ if (rv == 0) {
apmtimeout = 0;
/* wakeup for timeout: take status */
