Re: Alternate cpu policy on battery
On Sat, Sep 25, 2021 at 08:26:06PM +0200, Solene Rapenne wrote:
> +void
> +setperf_powersaving(void *v)
> +{
> + static uint64_t *idleticks, *totalticks;
[...]
> +
> + if (!idleticks)
> + if (!(idleticks = mallocarray(ncpusfound, sizeof(*idleticks),
> + M_DEVBUF, M_NOWAIT | M_ZERO)))
> + return;
> + if (!totalticks)
> + if (!(totalticks = mallocarray(ncpusfound, sizeof(*totalticks),
> + M_DEVBUF, M_NOWAIT | M_ZERO))) {
> + free(idleticks, M_DEVBUF,
> + sizeof(*idleticks) * ncpusfound);
> + return;
> + }
This is unrelated to your patch.
I noticed something that looks wrong in code your patch was based on.
Should setperf_auto() not reset idleticks to NULL after freeing idleticks
in the error path of the totalticks allocation?
Otherwise, if we come back into this function a second time, the static(!)
variable idleticks will still point at freed memory and won't be reallocated
since !idleticks is now false.
A very unlikely edge case. Still worth fixing?
diff ba3fb53d3e94158190d84a1771da21f620e46e26 /usr/src
blob - 956ea24ea07fa9a7301f2e7156698e4e090e9cfb
file + sys/kern/sched_bsd.c
--- sys/kern/sched_bsd.c
+++ sys/kern/sched_bsd.c
@@ -565,10 +565,11 @@ setperf_auto(void *v)
if (!totalticks)
if (!(totalticks = mallocarray(ncpusfound, sizeof(*totalticks),
M_DEVBUF, M_NOWAIT | M_ZERO))) {
free(idleticks, M_DEVBUF,
sizeof(*idleticks) * ncpusfound);
+ idleticks = NULL;
return;
}
alltotal = allidle = 0;
j = 0;
Re: Alternate cpu policy on battery
Hello,
I've been using this diff for a week now.
I didn't encounter any issue.
As my laptop has a quite old battery, I noticed improvements : I can
work 30min longer on battery now.
The systemp is a bit slower though (ie libreoffice longer to start), but
that was expected and I must say I didn't feel any discomfort either.
>From a non-power-tech user point of view, this patch is a great
improvement.
Regards.
prx
* Solene Rapenne le [25-09-2021 20:26:06 +0200]:
> Last time I proposed a CPU frequency scheduling change to make it
> more performance oriented, however this would hurt badly the battery
> life for nomad users.
>
> As we don't like tweaks and complicated settings, I create a new
> CPU frequency policy that is oriented for nomad users, it reduces
> performance a bit but from my (short) experiments it would give a
> way better battery life while keeping the system responsive which
> is not allowing apm -L
>
> The current auto mode is kept as it is now and I added a new
> powersaving mode that will be triggered automatically by apmd when
> the system is on battery AND automatic mode is activated.
>
> So, on A/C you have the usual automatic policy and on battery it
> would switch on the new powersaving policy.
>
> I hope this could be useful for nomad people and later we could
> tune the automatic mode (to be used on A/C) to be giving more
> performance, without affecting people on battery.
>
> The code change is relatively simple, I'm pretty sure this can be
> improved but I wanted to share it as this for now so people can
> comment and/or try it out.
>
> I created a new policy which is basically a copy/paste of the current
> one with a different name and differents thresholds, declared it
> in apmd. There is another change in apmd to check on event if we
> are using the policy auto and if on battery or not, this takes care
> of changing the policy automatically. I'm not sure it's at the right
> place. As for the code in the kernel, I'm quite sure it could be
> merged into one and use different thresholds depending on the policy
> name currently in use.
>
> Kernel + usr.sbin/apmd + usr.sbin/apm recompilation required
>
> Index: sys/kern//sched_bsd.c
> ===
> RCS file: /home/reposync/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 sched_bsd.c
> --- sys/kern//sched_bsd.c 9 Sep 2021 18:41:39 - 1.69
> +++ sys/kern//sched_bsd.c 25 Sep 2021 18:09:40 -
> @@ -531,6 +531,7 @@ void (*cpu_setperf)(int);
> #define PERFPOL_MANUAL 0
> #define PERFPOL_AUTO 1
> #define PERFPOL_HIGH 2
> +#define PERFPOL_POWERSAVING 4
> int perflevel = 100;
> int perfpolicy = PERFPOL_MANUAL;
>
> @@ -541,7 +542,9 @@ int perfpolicy = PERFPOL_MANUAL;
> #include
>
> void setperf_auto(void *);
> +void setperf_powersaving(void *);
> struct timeout setperf_to = TIMEOUT_INITIALIZER(setperf_auto, NULL);
> +struct timeout setperf_to_powersaving =
> TIMEOUT_INITIALIZER(setperf_powersaving, NULL);
>
> void
> setperf_auto(void *v)
> @@ -606,6 +609,73 @@ setperf_auto(void *v)
> timeout_add_msec(_to, 100);
> }
>
> +void
> +setperf_powersaving(void *v)
> +{
> + static uint64_t *idleticks, *totalticks;
> + static int downbeats;
> +
> + int i, j;
> + int speedup;
> + CPU_INFO_ITERATOR cii;
> + struct cpu_info *ci;
> + uint64_t idle, total, allidle, alltotal;
> +
> + if (perfpolicy != PERFPOL_POWERSAVING)
> + return;
> +
> + if (!idleticks)
> + if (!(idleticks = mallocarray(ncpusfound, sizeof(*idleticks),
> + M_DEVBUF, M_NOWAIT | M_ZERO)))
> + return;
> + if (!totalticks)
> + if (!(totalticks = mallocarray(ncpusfound, sizeof(*totalticks),
> + M_DEVBUF, M_NOWAIT | M_ZERO))) {
> + free(idleticks, M_DEVBUF,
> + sizeof(*idleticks) * ncpusfound);
> + return;
> + }
> +
> + alltotal = allidle = 0;
> + j = 0;
> + speedup = 0;
> + CPU_INFO_FOREACH(cii, ci) {
> + if (!cpu_is_online(ci))
> + continue;
> + total = 0;
> + for (i = 0; i < CPUSTATES; i++) {
> + total += ci->ci_schedstate.spc_cp_time[i];
> + }
> + total -= totalticks[j];
> + idle = ci->ci_schedstate.spc_cp_time[CP_IDLE] - idleticks[j];
> + // speedup if at least one core idle time < 33%
> + if (idle < total / 3)
> + speedup = 1;
> + alltotal += total;
> + allidle += idle;
> + idleticks[j] += idle;
> + totalticks[j] += total;
> + j++;
> + }
> + if (allidle < alltotal / 3)
> + speedup = 1;
> + if (speedup)
> + /* twice as long here because we check every 200ms */
> + downbeats
Alternate cpu policy on battery
Last time I proposed a CPU frequency scheduling change to make it
more performance oriented, however this would hurt badly the battery
life for nomad users.
As we don't like tweaks and complicated settings, I create a new
CPU frequency policy that is oriented for nomad users, it reduces
performance a bit but from my (short) experiments it would give a
way better battery life while keeping the system responsive which
is not allowing apm -L
The current auto mode is kept as it is now and I added a new
powersaving mode that will be triggered automatically by apmd when
the system is on battery AND automatic mode is activated.
So, on A/C you have the usual automatic policy and on battery it
would switch on the new powersaving policy.
I hope this could be useful for nomad people and later we could
tune the automatic mode (to be used on A/C) to be giving more
performance, without affecting people on battery.
The code change is relatively simple, I'm pretty sure this can be
improved but I wanted to share it as this for now so people can
comment and/or try it out.
I created a new policy which is basically a copy/paste of the current
one with a different name and differents thresholds, declared it
in apmd. There is another change in apmd to check on event if we
are using the policy auto and if on battery or not, this takes care
of changing the policy automatically. I'm not sure it's at the right
place. As for the code in the kernel, I'm quite sure it could be
merged into one and use different thresholds depending on the policy
name currently in use.
Kernel + usr.sbin/apmd + usr.sbin/apm recompilation required
Index: sys/kern//sched_bsd.c
===
RCS file: /home/reposync/src/sys/kern/sched_bsd.c,v
retrieving revision 1.69
diff -u -p -r1.69 sched_bsd.c
--- sys/kern//sched_bsd.c 9 Sep 2021 18:41:39 - 1.69
+++ sys/kern//sched_bsd.c 25 Sep 2021 18:09:40 -
@@ -531,6 +531,7 @@ void (*cpu_setperf)(int);
#define PERFPOL_MANUAL 0
#define PERFPOL_AUTO 1
#define PERFPOL_HIGH 2
+#define PERFPOL_POWERSAVING 4
int perflevel = 100;
int perfpolicy = PERFPOL_MANUAL;
@@ -541,7 +542,9 @@ int perfpolicy = PERFPOL_MANUAL;
#include
void setperf_auto(void *);
+void setperf_powersaving(void *);
struct timeout setperf_to = TIMEOUT_INITIALIZER(setperf_auto, NULL);
+struct timeout setperf_to_powersaving =
TIMEOUT_INITIALIZER(setperf_powersaving, NULL);
void
setperf_auto(void *v)
@@ -606,6 +609,73 @@ setperf_auto(void *v)
timeout_add_msec(_to, 100);
}
+void
+setperf_powersaving(void *v)
+{
+ static uint64_t *idleticks, *totalticks;
+ static int downbeats;
+
+ int i, j;
+ int speedup;
+ CPU_INFO_ITERATOR cii;
+ struct cpu_info *ci;
+ uint64_t idle, total, allidle, alltotal;
+
+ if (perfpolicy != PERFPOL_POWERSAVING)
+ return;
+
+ if (!idleticks)
+ if (!(idleticks = mallocarray(ncpusfound, sizeof(*idleticks),
+ M_DEVBUF, M_NOWAIT | M_ZERO)))
+ return;
+ if (!totalticks)
+ if (!(totalticks = mallocarray(ncpusfound, sizeof(*totalticks),
+ M_DEVBUF, M_NOWAIT | M_ZERO))) {
+ free(idleticks, M_DEVBUF,
+ sizeof(*idleticks) * ncpusfound);
+ return;
+ }
+
+ alltotal = allidle = 0;
+ j = 0;
+ speedup = 0;
+ CPU_INFO_FOREACH(cii, ci) {
+ if (!cpu_is_online(ci))
+ continue;
+ total = 0;
+ for (i = 0; i < CPUSTATES; i++) {
+ total += ci->ci_schedstate.spc_cp_time[i];
+ }
+ total -= totalticks[j];
+ idle = ci->ci_schedstate.spc_cp_time[CP_IDLE] - idleticks[j];
+ // speedup if at least one core idle time < 33%
+ if (idle < total / 3)
+ speedup = 1;
+ alltotal += total;
+ allidle += idle;
+ idleticks[j] += idle;
+ totalticks[j] += total;
+ j++;
+ }
+ if (allidle < alltotal / 3)
+ speedup = 1;
+ if (speedup)
+ /* twice as long here because we check every 200ms */
+ downbeats = 1;
+
+ if (speedup && perflevel != 100) {
+ perflevel = 100;
+ cpu_setperf(perflevel);
+ } else if (!speedup && perflevel != 0 && --downbeats <= 0) {
+ perflevel = 0;
+ cpu_setperf(perflevel);
+ }
+
+/* every 200ms to have a better resolution of the load */
+ timeout_add_msec(_to_powersaving, 200);
+}
+
+
int
sysctl_hwsetperf(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
{
@@ -644,6 +714,9 @@ sysctl_hwperfpolicy(void *oldp, size_t *
case PERFPOL_AUTO:
strlcpy(policy, "auto",
