Re: apmd(8) and hw.perfpolicy quirks
On 2020-09-27, Jeremie Courreges-Anglas wrote: > The diff below teaches apmd(8) -H to set hw.perfpolicy="manual" and > hw.setperf=100, instead of setting hw.perfpolicy="high". sure. if you would like to own this, by all means. :)
Re: apmd(8) and hw.perfpolicy quirks
On Thu, Sep 24 2020, Jeremie Courreges-Anglas wrote:
> On Wed, Sep 23 2020, "Ted Unangst" wrote:
>> On 2020-09-23, Jeremie Courreges-Anglas wrote:
>>
>>> ok?
>>
>> Seems fine.
>>
>>
>>> Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
>>> brings a question. I find it weird that there is a special "high"
>>> perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
>>> no "low" perfpolicy. Is "high" useful to anyone?
>>
>> If you're benchmarking or something, it's convenient to toggle between
>> high and auto. There's less use for low, since that's what auto defaults to.
>
> Well you can do auto->high easily with apm(8) -H or sysctl
> hw.perfpolicy=manual hw.setperf=100. I'm not sure a special "high"
> hw.perfpolicy choice brings much value and I would appreciate a simple
> "manual" vs "auto" situation.
>
> This has an impact on documentation. sysctl(2) doesn't mention that
> setting hw.perfpolicy=high also locks hw.setperf=100. The apm(8)
> manpage only talks about -H setting hw.setperf=100. The description for
> apmd(8) -H says
>
> Start apmd in *manual* performance adjustment mode,
> initialising hw.setperf to 100.
>
> which is not the whole story. If you use apm/apmd -H you can't later
> just run sysctl hw.setperf=50:
>
> sysctl: hw.setperf: Operation not permitted
The diff below teaches apmd(8) -H to set hw.perfpolicy="manual" and
hw.setperf=100, instead of setting hw.perfpolicy="high".
- matches the manpage
- apm(8) reporting becomes accurate
- more symmetry with -L ("low")
- avoids the "sysctl: hw.setperf: Operation not permitted" quirk
Teaching apm(8) how to print hw.perfpolicy="high" then becomes low
priority.
While here, simplify the sysctl(2) calls: in this function we don't care
about the old values.
ok?
Index: apmd.c
===
--- apmd.c.orig
+++ apmd.c
@@ -650,27 +650,27 @@ void
setperfpolicy(char *policy)
{
int hw_perfpol_mib[] = { CTL_HW, HW_PERFPOLICY };
- char oldpolicy[32];
- size_t oldsz = sizeof(oldpolicy);
- int setlo = 0;
+ int hw_perf_mib[] = { CTL_HW, HW_SETPERF };
+ int new_perf = -1;
if (strcmp(policy, "low") == 0) {
policy = "manual";
- setlo = 1;
+ new_perf = 0;
+ } else if (strcmp(policy, "high") == 0) {
+ policy = "manual";
+ new_perf = 100;
}
- if (sysctl(hw_perfpol_mib, 2, oldpolicy, ,
+ if (sysctl(hw_perfpol_mib, 2, NULL, NULL,
policy, strlen(policy) + 1) == -1)
logmsg(LOG_INFO, "cannot set hw.perfpolicy");
- if (setlo == 1) {
- int hw_perf_mib[] = {CTL_HW, HW_SETPERF};
- int perf;
- int new_perf = 0;
- size_t perf_sz = sizeof(perf);
- if (sysctl(hw_perf_mib, 2, , _sz, _perf, perf_sz)
== -1)
- logmsg(LOG_INFO, "cannot set hw.setperf");
- }
+ if (new_perf == -1)
+ return;
+
+ if (sysctl(hw_perf_mib, 2, NULL, NULL,
+ _perf, sizeof(new_perf)) == -1)
+ logmsg(LOG_INFO, "cannot set hw.setperf");
}
void
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: apmd(8) and hw.perfpolicy quirks
On Wed, Sep 23 2020, "Ted Unangst" wrote:
> On 2020-09-23, Jeremie Courreges-Anglas wrote:
>
>> ok?
>
> Seems fine.
>
>
>> Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
>> brings a question. I find it weird that there is a special "high"
>> perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
>> no "low" perfpolicy. Is "high" useful to anyone?
>
> If you're benchmarking or something, it's convenient to toggle between
> high and auto. There's less use for low, since that's what auto defaults to.
Well you can do auto->high easily with apm(8) -H or sysctl
hw.perfpolicy=manual hw.setperf=100. I'm not sure a special "high"
hw.perfpolicy choice brings much value and I would appreciate a simple
"manual" vs "auto" situation.
This has an impact on documentation. sysctl(2) doesn't mention that
setting hw.perfpolicy=high also locks hw.setperf=100. The apm(8)
manpage only talks about -H setting hw.setperf=100. The description for
apmd(8) -H says
Start apmd in *manual* performance adjustment mode,
initialising hw.setperf to 100.
which is not the whole story. If you use apm/apmd -H you can't later
just run sysctl hw.setperf=50:
sysctl: hw.setperf: Operation not permitted
Initially I had a diff to teach apm(8) about hw.perfpolicy="high" but
I'm not sure it's the right direction any more. Diff below fwiw, not
looking for oks.
Index: apm/apm.c
===
RCS file: /d/cvs/src/usr.sbin/apm/apm.c,v
retrieving revision 1.37
diff -u -p -r1.37 apm.c
--- apm/apm.c 23 Sep 2020 05:50:26 - 1.37
+++ apm/apm.c 24 Sep 2020 20:29:49 -
@@ -399,7 +399,8 @@ balony:
if (doperf)
printf("Performance adjustment mode: %s (%d MHz)\n",
- perf_mode(reply.perfmode), reply.cpuspeed);
+ perfmode_to_perfpolicy(reply.perfmode),
+ reply.cpuspeed);
break;
default:
break;
Index: apmd/apm-proto.h
===
RCS file: /d/cvs/src/usr.sbin/apmd/apm-proto.h,v
retrieving revision 1.10
diff -u -p -r1.10 apm-proto.h
--- apmd/apm-proto.h23 Sep 2020 05:50:26 - 1.10
+++ apmd/apm-proto.h24 Sep 2020 20:29:49 -
@@ -51,6 +51,7 @@ enum apm_perfmode {
PERF_NONE = -1,
PERF_MANUAL,
PERF_AUTO,
+ PERF_HIGH,
};
struct apm_command {
@@ -66,8 +67,9 @@ struct apm_reply {
struct apm_power_info batterystate;
};
-#define APMD_VNO 3
+#define APMD_VNO 4
extern const char *battstate(int state);
extern const char *ac_state(int state);
-extern const char *perf_mode(int mode);
+extern const char *perfmode_to_perfpolicy(int mode);
+extern enum apm_perfmode perfpolicy_to_perfmode(const char *);
Index: apmd/apmd.c
===
RCS file: /d/cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.98
diff -u -p -r1.98 apmd.c
--- apmd/apmd.c 24 Sep 2020 07:23:41 - 1.98
+++ apmd/apmd.c 24 Sep 2020 20:29:49 -
@@ -310,16 +310,11 @@ handle_client(int sock_fd, int ctl_fd)
break;
}
- reply.perfmode = PERF_NONE;
- if (sysctl(perfpol_mib, 2, perfpol, _sz, NULL, 0) == -1)
+ if (sysctl(perfpol_mib, 2, perfpol, _sz, NULL, 0) == -1) {
logmsg(LOG_INFO, "cannot read hw.perfpolicy");
- else {
- if (strcmp(perfpol, "manual") == 0 ||
- strcmp(perfpol, "high") == 0) {
- reply.perfmode = PERF_MANUAL;
- } else if (strcmp(perfpol, "auto") == 0)
- reply.perfmode = PERF_AUTO;
- }
+ reply.perfmode = PERF_NONE;
+ } else
+ reply.perfmode = perfpolicy_to_perfmode(perfpol);
if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1) {
logmsg(LOG_INFO, "cannot read hw.cpuspeed");
Index: apmd/apmsubr.c
===
RCS file: /d/cvs/src/usr.sbin/apmd/apmsubr.c,v
retrieving revision 1.9
diff -u -p -r1.9 apmsubr.c
--- apmd/apmsubr.c 23 Sep 2020 05:50:26 - 1.9
+++ apmd/apmsubr.c 24 Sep 2020 20:29:49 -
@@ -31,6 +31,8 @@
#include
#include
+#include
+
#include "apm-proto.h"
const char *
@@ -72,14 +74,29 @@ ac_state(int state)
}
const char *
-perf_mode(int mode)
+perfmode_to_perfpolicy(int mode)
{
switch (mode) {
- case PERF_MANUAL:
- return "manual";
case PERF_AUTO:
return "auto";
+ case PERF_HIGH:
+ return "high";
+ case PERF_MANUAL:
+ return "manual";
default:
return "invalid";
}
+}
+
+enum apm_perfmode
+perfpolicy_to_perfmode(const char *perfpolicy)
+{
+ if (strcmp(perfpolicy, "auto") == 0)
+
Re: apmd(8) and hw.perfpolicy quirks
On 2020-09-23, Jeremie Courreges-Anglas wrote: > ok? Seems fine. > Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which > brings a question. I find it weird that there is a special "high" > perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but > no "low" perfpolicy. Is "high" useful to anyone? If you're benchmarking or something, it's convenient to toggle between high and auto. There's less use for low, since that's what auto defaults to.
Re: apmd(8) and hw.perfpolicy quirks
On Wed, Sep 23 2020, Jeremie Courreges-Anglas wrote:
> Prompted by a report from Miod: setting hw.setperf works only if the
> kernel doesn't have a usable cpu_setperf implementation. The current
> apmd(8) code warns if setting hw.perfpolicy fails, but then handles
> back bogus values to apm(8) clients.
Some corrections:
> The easy fix is to just query the kernel about the actual hw.setperf
hw.perfpolicy
> value. This fixes other incorrect apmd(8) assumptions:
> - hw.setperf is initially set to "manual"
hw.perfpolicy
> - hw.setperf can't change behind apmd's back
hw.perfpolicy
> ok?
>
>
> Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
> brings a question. I find it weird that there is a special "high"
> perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
> no "low" perfpolicy. Is "high" useful to anyone?
>
> Index: apmd.c
> ===
> RCS file: /d/cvs/src/usr.sbin/apmd/apmd.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 apmd.c
> --- apmd.c23 Sep 2020 05:50:26 - 1.97
> +++ apmd.c23 Sep 2020 10:46:37 -
> @@ -59,8 +59,6 @@
>
> int debug = 0;
>
> -int doperf = PERF_NONE;
> -
> extern char *__progname;
>
> void usage(void);
> @@ -255,7 +253,10 @@ handle_client(int sock_fd, int ctl_fd)
> socklen_t fromlen;
> struct apm_command cmd;
> struct apm_reply reply;
> - int cpuspeed_mib[] = {CTL_HW, HW_CPUSPEED};
> + int perfpol_mib[] = { CTL_HW, HW_PERFPOLICY };
> + char perfpol[32];
> + size_t perfpol_sz = sizeof(perfpol);
> + int cpuspeed_mib[] = { CTL_HW, HW_CPUSPEED };
> int cpuspeed = 0;
> size_t cpuspeed_sz = sizeof(cpuspeed);
>
> @@ -290,19 +291,16 @@ handle_client(int sock_fd, int ctl_fd)
> reply.newstate = HIBERNATING;
> break;
> case SETPERF_LOW:
> - doperf = PERF_MANUAL;
> reply.newstate = NORMAL;
> logmsg(LOG_NOTICE, "setting hw.perfpolicy to low");
> setperfpolicy("low");
> break;
> case SETPERF_HIGH:
> - doperf = PERF_MANUAL;
> reply.newstate = NORMAL;
> logmsg(LOG_NOTICE, "setting hw.perfpolicy to high");
> setperfpolicy("high");
> break;
> case SETPERF_AUTO:
> - doperf = PERF_AUTO;
> reply.newstate = NORMAL;
> logmsg(LOG_NOTICE, "setting hw.perfpolicy to auto");
> setperfpolicy("auto");
> @@ -312,11 +310,22 @@ handle_client(int sock_fd, int ctl_fd)
> break;
> }
>
> - if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1)
> - logmsg(LOG_INFO, "cannot read hw.cpuspeed");
> + reply.perfmode = PERF_NONE;
> + if (sysctl(perfpol_mib, 2, perfpol, _sz, NULL, 0) == -1)
> + logmsg(LOG_INFO, "cannot read hw.perfpolicy");
> + else {
> + if (strcmp(perfpol, "manual") == 0 ||
> + strcmp(perfpol, "high") == 0) {
> + reply.perfmode = PERF_MANUAL;
> + } else if (strcmp(perfpol, "auto") == 0)
> + reply.perfmode = PERF_AUTO;
> + }
>
> + if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1) {
> + logmsg(LOG_INFO, "cannot read hw.cpuspeed");
> + cpuspeed = 0;
> + }
> reply.cpuspeed = cpuspeed;
> - reply.perfmode = doperf;
> reply.vno = APMD_VNO;
> if (send(cli_fd, , sizeof(reply), 0) != sizeof(reply))
> logmsg(LOG_INFO, "reply to client botched");
> @@ -386,6 +395,7 @@ main(int argc, char *argv[])
> const char *errstr;
> int kq, nchanges;
> struct kevent ev[2];
> + int doperf = PERF_NONE;
>
> while ((ch = getopt(argc, argv, "aACdHLsf:t:S:z:Z:")) != -1)
> switch(ch) {
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
apmd(8) and hw.perfpolicy quirks
Prompted by a report from Miod: setting hw.setperf works only if the
kernel doesn't have a usable cpu_setperf implementation. The current
apmd(8) code warns if setting hw.perfpolicy fails, but then handles
back bogus values to apm(8) clients.
The easy fix is to just query the kernel about the actual hw.setperf
value. This fixes other incorrect apmd(8) assumptions:
- hw.setperf is initially set to "manual"
- hw.setperf can't change behind apmd's back
ok?
Note: I inlined the apmd(8)->apm(8) perfpolicy conversion for now, which
brings a question. I find it weird that there is a special "high"
perfpolicy (effectively similar to perfpolicy=manual + setperf=100) but
no "low" perfpolicy. Is "high" useful to anyone?
Index: apmd.c
===
RCS file: /d/cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.97
diff -u -p -r1.97 apmd.c
--- apmd.c 23 Sep 2020 05:50:26 - 1.97
+++ apmd.c 23 Sep 2020 10:46:37 -
@@ -59,8 +59,6 @@
int debug = 0;
-int doperf = PERF_NONE;
-
extern char *__progname;
void usage(void);
@@ -255,7 +253,10 @@ handle_client(int sock_fd, int ctl_fd)
socklen_t fromlen;
struct apm_command cmd;
struct apm_reply reply;
- int cpuspeed_mib[] = {CTL_HW, HW_CPUSPEED};
+ int perfpol_mib[] = { CTL_HW, HW_PERFPOLICY };
+ char perfpol[32];
+ size_t perfpol_sz = sizeof(perfpol);
+ int cpuspeed_mib[] = { CTL_HW, HW_CPUSPEED };
int cpuspeed = 0;
size_t cpuspeed_sz = sizeof(cpuspeed);
@@ -290,19 +291,16 @@ handle_client(int sock_fd, int ctl_fd)
reply.newstate = HIBERNATING;
break;
case SETPERF_LOW:
- doperf = PERF_MANUAL;
reply.newstate = NORMAL;
logmsg(LOG_NOTICE, "setting hw.perfpolicy to low");
setperfpolicy("low");
break;
case SETPERF_HIGH:
- doperf = PERF_MANUAL;
reply.newstate = NORMAL;
logmsg(LOG_NOTICE, "setting hw.perfpolicy to high");
setperfpolicy("high");
break;
case SETPERF_AUTO:
- doperf = PERF_AUTO;
reply.newstate = NORMAL;
logmsg(LOG_NOTICE, "setting hw.perfpolicy to auto");
setperfpolicy("auto");
@@ -312,11 +310,22 @@ handle_client(int sock_fd, int ctl_fd)
break;
}
- if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1)
- logmsg(LOG_INFO, "cannot read hw.cpuspeed");
+ reply.perfmode = PERF_NONE;
+ if (sysctl(perfpol_mib, 2, perfpol, _sz, NULL, 0) == -1)
+ logmsg(LOG_INFO, "cannot read hw.perfpolicy");
+ else {
+ if (strcmp(perfpol, "manual") == 0 ||
+ strcmp(perfpol, "high") == 0) {
+ reply.perfmode = PERF_MANUAL;
+ } else if (strcmp(perfpol, "auto") == 0)
+ reply.perfmode = PERF_AUTO;
+ }
+ if (sysctl(cpuspeed_mib, 2, , _sz, NULL, 0) == -1) {
+ logmsg(LOG_INFO, "cannot read hw.cpuspeed");
+ cpuspeed = 0;
+ }
reply.cpuspeed = cpuspeed;
- reply.perfmode = doperf;
reply.vno = APMD_VNO;
if (send(cli_fd, , sizeof(reply), 0) != sizeof(reply))
logmsg(LOG_INFO, "reply to client botched");
@@ -386,6 +395,7 @@ main(int argc, char *argv[])
const char *errstr;
int kq, nchanges;
struct kevent ev[2];
+ int doperf = PERF_NONE;
while ((ch = getopt(argc, argv, "aACdHLsf:t:S:z:Z:")) != -1)
switch(ch) {
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
