Re: apmd(8): poll timer miscalculation
sunil+b...@nimmagadda.net wrote:
> 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.
>
> Revised diff without whitespace changes and...
>
> 1. While querying for changed objects using kevent(2), request changes
> for all objects registered instead of just 1 and handle them.
>
Ping! Could someone familiar with apmd(8) please take a look and
comment; thanks.
Prior discussion on bugs@ about the problem...
https://marc.info/?t=15078412582=1=2
> Index: apmd.c
> ===
> RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
> retrieving revision 1.81
> diff -u -p -w -r1.81 apmd.c
> --- apmd.c15 Oct 2017 15:14:49 - 1.81
> +++ apmd.c8 Jun 2018 10:16:53 -
> @@ -56,6 +56,9 @@
> #define AUTO_SUSPEND 1
> #define AUTO_HIBERNATE 2
>
> +#define TIMO (10*60) /* 10 minutes */
> +#define TIMER_ID 1
> +
> const char apmdev[] = _PATH_APM_CTLDEV;
> const char sockfile[] = _PATH_APM_SOCKET;
>
> @@ -341,8 +344,6 @@ hibernate(int ctl_fd)
> ioctl(ctl_fd, APM_IOC_HIBERNATE, 0);
> }
>
> -#define TIMO (10*60) /* 10 minutes */
> -
> int
> main(int argc, char *argv[])
> {
> @@ -353,13 +354,12 @@ main(int argc, char *argv[])
> int statonly = 0;
> int powerstatus = 0, powerbak = 0, powerchange = 0;
> int noacsleep = 0;
> - struct timespec ts = {TIMO, 0}, sts = {0, 0};
> struct apm_power_info pinfo;
> - time_t apmtimeout = 0;
> + time_t apmtimeout = TIMO, counter = 0;
> const char *sockname = sockfile;
> const char *errstr;
> int kq, nchanges;
> - struct kevent ev[2];
> + struct kevent ev[3];
> int ncpu_mib[2] = { CTL_HW, HW_NCPU };
> int ncpu;
> size_t ncpu_sz = sizeof(ncpu);
> @@ -379,8 +379,8 @@ main(int argc, char *argv[])
> sockname = optarg;
> break;
> case 't':
> - ts.tv_sec = strtoul(optarg, NULL, 0);
> - if (ts.tv_sec == 0)
> + apmtimeout = strtoul(optarg, NULL, 0);
> + if (apmtimeout == 0)
> usage();
> break;
> case 's': /* status only */
> @@ -466,57 +466,30 @@ main(int argc, char *argv[])
>
> EV_SET([0], sock_fd, EVFILT_READ, EV_ADD | EV_ENABLE | EV_CLEAR,
> 0, 0, NULL);
> + EV_SET([1], TIMER_ID, EVFILT_TIMER, EV_ADD, 0, 1, NULL);
> if (ctl_fd == -1)
> - nchanges = 1;
> + nchanges = 2;
> else {
> - EV_SET([1], ctl_fd, EVFILT_READ, EV_ADD | EV_ENABLE |
> + EV_SET([2], ctl_fd, EVFILT_READ, EV_ADD | EV_ENABLE |
> EV_CLEAR, 0, 0, NULL);
> - nchanges = 2;
> + nchanges = 3;
> }
> - if (kevent(kq, ev, nchanges, NULL, 0, ) < 0)
> + if (kevent(kq, ev, nchanges, NULL, 0, NULL) < 0)
> error("kevent", NULL);
>
> if (sysctl(ncpu_mib, 2, , _sz, NULL, 0) < 0)
> error("cannot read hw.ncpu", NULL);
>
> for (;;) {
> - int rv;
> + int i, rv;
>
> - sts = ts;
> -
> - apmtimeout += 1;
> - if ((rv = kevent(kq, NULL, 0, ev, 1, )) < 0)
> + if ((rv = kevent(kq, NULL, 0, ev, nchanges, NULL)) < 0)
> break;
>
> - if (apmtimeout >= ts.tv_sec) {
> - apmtimeout = 0;
> -
> - /* wakeup for timeout: take status */
> - powerbak = power_status(ctl_fd, 0, );
> - if (powerstatus != powerbak) {
> -
Re: apmd(8): poll timer miscalculation
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.
Revised diff without whitespace changes and...
1. While querying for changed objects using kevent(2), request changes
for all objects registered instead of just 1 and handle them.
Index: apmd.c
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.81
diff -u -p -w -r1.81 apmd.c
--- apmd.c 15 Oct 2017 15:14:49 - 1.81
+++ apmd.c 8 Jun 2018 10:16:53 -
@@ -56,6 +56,9 @@
#define AUTO_SUSPEND 1
#define AUTO_HIBERNATE 2
+#define TIMO (10*60) /* 10 minutes */
+#define TIMER_ID 1
+
const char apmdev[] = _PATH_APM_CTLDEV;
const char sockfile[] = _PATH_APM_SOCKET;
@@ -341,8 +344,6 @@ hibernate(int ctl_fd)
ioctl(ctl_fd, APM_IOC_HIBERNATE, 0);
}
-#define TIMO (10*60) /* 10 minutes */
-
int
main(int argc, char *argv[])
{
@@ -353,13 +354,12 @@ main(int argc, char *argv[])
int statonly = 0;
int powerstatus = 0, powerbak = 0, powerchange = 0;
int noacsleep = 0;
- struct timespec ts = {TIMO, 0}, sts = {0, 0};
struct apm_power_info pinfo;
- time_t apmtimeout = 0;
+ time_t apmtimeout = TIMO, counter = 0;
const char *sockname = sockfile;
const char *errstr;
int kq, nchanges;
- struct kevent ev[2];
+ struct kevent ev[3];
int ncpu_mib[2] = { CTL_HW, HW_NCPU };
int ncpu;
size_t ncpu_sz = sizeof(ncpu);
@@ -379,8 +379,8 @@ main(int argc, char *argv[])
sockname = optarg;
break;
case 't':
- ts.tv_sec = strtoul(optarg, NULL, 0);
- if (ts.tv_sec == 0)
+ apmtimeout = strtoul(optarg, NULL, 0);
+ if (apmtimeout == 0)
usage();
break;
case 's': /* status only */
@@ -466,57 +466,30 @@ main(int argc, char *argv[])
EV_SET([0], sock_fd, EVFILT_READ, EV_ADD | EV_ENABLE | EV_CLEAR,
0, 0, NULL);
+ EV_SET([1], TIMER_ID, EVFILT_TIMER, EV_ADD, 0, 1, NULL);
if (ctl_fd == -1)
- nchanges = 1;
+ nchanges = 2;
else {
- EV_SET([1], ctl_fd, EVFILT_READ, EV_ADD | EV_ENABLE |
+ EV_SET([2], ctl_fd, EVFILT_READ, EV_ADD | EV_ENABLE |
EV_CLEAR, 0, 0, NULL);
- nchanges = 2;
+ nchanges = 3;
}
- if (kevent(kq, ev, nchanges, NULL, 0, ) < 0)
+ if (kevent(kq, ev, nchanges, NULL, 0, NULL) < 0)
error("kevent", NULL);
if (sysctl(ncpu_mib, 2, , _sz, NULL, 0) < 0)
error("cannot read hw.ncpu", NULL);
for (;;) {
- int rv;
+ int i, rv;
- sts = ts;
-
- apmtimeout += 1;
- if ((rv = kevent(kq, NULL, 0, ev, 1, )) < 0)
+ if ((rv = kevent(kq, NULL, 0, ev, nchanges, NULL)) < 0)
break;
- if (apmtimeout >= ts.tv_sec) {
- apmtimeout = 0;
-
- /* wakeup for timeout: take status */
- powerbak = power_status(ctl_fd, 0, );
- if (powerstatus != powerbak) {
- powerstatus = powerbak;
- powerchange = 1;
- }
-
- if (!powerstatus && autoaction &&
- autolimit > (int)pinfo.battery_life) {
- syslog(LOG_NOTICE,
-
Re: apmd(8): poll timer miscalculation
Scott Cheloha wrote:
> On Thu, Jun 07, 2018 at 11:27:34AM +, 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?
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 - 1.81
> +++ usr.sbin/apmd/apmd.c 7 Jun 2018 20:26:11 -
> @@ -488,7 +488,7 @@ main(int argc, char *argv[])
> if ((rv = kevent(kq, NULL, 0, ev, 1, )) < 0)
> break;
>
> - if (apmtimeout >= ts.tv_sec) {
> + if (rv == 0) {
> apmtimeout = 0;
>
> /* wakeup for timeout: take status */
Re: apmd(8): poll timer miscalculation
On Thu, Jun 07, 2018 at 11:27:34AM +, 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.c15 Oct 2017 15:14:49 - 1.81
+++ usr.sbin/apmd/apmd.c7 Jun 2018 20:26:11 -
@@ -488,7 +488,7 @@ main(int argc, char *argv[])
if ((rv = kevent(kq, NULL, 0, ev, 1, )) < 0)
break;
- if (apmtimeout >= ts.tv_sec) {
+ if (rv == 0) {
apmtimeout = 0;
/* wakeup for timeout: take status */
apmd(8): poll timer miscalculation
>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.
Index: apmd.c
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.81
diff -u -p -w -r1.81 apmd.c
--- apmd.c 15 Oct 2017 15:14:49 - 1.81
+++ apmd.c 7 Jun 2018 10:56:06 -
@@ -56,6 +56,9 @@
#define AUTO_SUSPEND 1
#define AUTO_HIBERNATE 2
+#define TIMO (10*60) /* 10 minutes */
+#define TIMER_ID 1
+
const char apmdev[] = _PATH_APM_CTLDEV;
const char sockfile[] = _PATH_APM_SOCKET;
@@ -341,8 +344,6 @@ hibernate(int ctl_fd)
ioctl(ctl_fd, APM_IOC_HIBERNATE, 0);
}
-#define TIMO (10*60) /* 10 minutes */
-
int
main(int argc, char *argv[])
{
@@ -353,13 +354,12 @@ main(int argc, char *argv[])
int statonly = 0;
int powerstatus = 0, powerbak = 0, powerchange = 0;
int noacsleep = 0;
- struct timespec ts = {TIMO, 0}, sts = {0, 0};
struct apm_power_info pinfo;
- time_t apmtimeout = 0;
+ time_t apmtimeout = TIMO, counter = 0;
const char *sockname = sockfile;
const char *errstr;
int kq, nchanges;
- struct kevent ev[2];
+ struct kevent ev[3];
int ncpu_mib[2] = { CTL_HW, HW_NCPU };
int ncpu;
size_t ncpu_sz = sizeof(ncpu);
@@ -379,8 +379,8 @@ main(int argc, char *argv[])
sockname = optarg;
break;
case 't':
- ts.tv_sec = strtoul(optarg, NULL, 0);
- if (ts.tv_sec == 0)
+ apmtimeout = strtoul(optarg, NULL, 0);
+ if (apmtimeout == 0)
usage();
break;
case 's': /* status only */
@@ -466,14 +466,15 @@ main(int argc, char *argv[])
EV_SET([0], sock_fd, EVFILT_READ, EV_ADD | EV_ENABLE | EV_CLEAR,
0, 0, NULL);
+ EV_SET([1], TIMER_ID, EVFILT_TIMER, EV_ADD, 0, 1, NULL);
if (ctl_fd == -1)
- nchanges = 1;
+ nchanges = 2;
else {
- EV_SET([1], ctl_fd, EVFILT_READ, EV_ADD | EV_ENABLE |
+ EV_SET([2], ctl_fd, EVFILT_READ, EV_ADD | EV_ENABLE |
EV_CLEAR, 0, 0, NULL);
- nchanges = 2;
+ nchanges = 3;
}
- if (kevent(kq, ev, nchanges, NULL, 0, ) < 0)
+ if (kevent(kq, ev, nchanges, NULL, 0, NULL) < 0)
error("kevent", NULL);
if (sysctl(ncpu_mib, 2, , _sz, NULL, 0) < 0)
@@ -482,41 +483,14 @@ main(int argc, char *argv[])
for (;;) {
int rv;
- sts = ts;
-
- apmtimeout += 1;
- if ((rv = kevent(kq, NULL, 0, ev, 1, )) < 0)
+ if ((rv = kevent(kq, NULL, 0, ev, 1, NULL)) < 0)
break;
- if (apmtimeout >= ts.tv_sec) {
- apmtimeout = 0;
-
- /* wakeup for timeout: take status */
- powerbak = power_status(ctl_fd, 0, );
- if (powerstatus != powerbak) {
- powerstatus = powerbak;
- powerchange = 1;
- }
-
- if (!powerstatus && autoaction &&
- autolimit > (int)pinfo.battery_life) {
- syslog(LOG_NOTICE,
- "estimated battery life %d%%, "
- "autoaction limit set to %d%% .",
- pinfo.battery_life,
- autolimit
- );
-
- if (autoaction == AUTO_SUSPEND)
