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
