Scott Cheloha <[email protected]> wrote:
> On Thu, Jun 07, 2018 at 11:27:34AM +0000, [email protected] 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
> >
> > [email protected]:/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?
No, it doesn't. kevent(2) never times out (so rv is never zero) as
there is data available on ctl_fd every 10 seconds.
>
> 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 */
