improve cx_lowest logic

[Andriy Gapon]

I would like to propose the following change for review and testing:
http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff

The idea is to separate effective cx_lowest (the limit that the idling code
should not currently exceed) from user-set cx_lowest limit/target.
Specifically, this addresses dynamic C-state changes by ACPI platform.
Currently if a user sets cx_lowest to, say, C3, then available C-states change
so that C2 is the deepest available C-state and cx_lowest gets overridden to C2
and all memory of C3 setting is lost.
Also, there is a problem with asynchronous nature of userland notification about
power profile change and kernel notifications of _CST changes for each CPU in
SMP system.  That required work-arounds like one in r209213 which also
introduced quite a bit of confusion about how per-CPU cpu_cx_lowest, global
cpu_cx_lowest and dynamic C-state changes interact with each other.
To this day I can not unwind the complete logic behind the following lines:
if (sc->cpu_cx_lowest < cpu_cx_lowest)
   acpi_cpu_set_cx_lowest(sc, min(cpu_cx_lowest, sc->cpu_cx_count - 1));

So to avoid the problems described above I decided to split the cx_lowest limit
into the persistent user-set limit, cpu_cx_lowest_lim, and dynamic effective limit.
The logic to set cpu_cx_lowest based on cpu_cx_lowest_lim and cpu_cx_count is
moved to acpi_cpu_set_cx_lowest().

Note that per-CPU cpu_cx_lowest_lim does not "interact" with global
cpu_cx_lowest_lim.  Whichever is set later (via sysctl) just overrides the other.

With such a change it no longer makes sense to limit allowed cx_lowest sysctl
values to currently available C-state levels.  Indeed, if a user knows that his
system may provide C6 under some circumstance, but currently only C1 is
available, then there is no reason to disallow setting cx_lowest sysctl
(cpu_cx_lowest_lim) to C6.  So values up to MAX_CX_STATES are allowed.
After this change cpu_cx_count _global_ variable became unused (write-only) and
was removed.

Additionally, I introduced magic value of "Cmax" for cx_lowest sysctls to mean
that there should not be any limit on C-state depth.

The change is partially based on submission from Vitaly:
http://article.gmane.org/gmane.os.freebsd.devel.acpi/6892
--
Andriy Gapon

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Andriy Gapon]

on 08/07/2012 13:22 Andriy Gapon said the following:
> The change is partially based on submission from Vitaly:
> http://article.gmane.org/gmane.os.freebsd.devel.acpi/6892

Actually, Sean had a similar idea too with respect to cx_lowest valid values.

--
Andriy Gapon

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[John Baldwin]

On Sunday, July 08, 2012 6:22:49 am Andriy Gapon wrote:

>
> I would like to propose the following change for review and testing:
> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff
>
> The idea is to separate effective cx_lowest (the limit that the idling code
> should not currently exceed) from user-set cx_lowest limit/target.
> Specifically, this addresses dynamic C-state changes by ACPI platform.
> Currently if a user sets cx_lowest to, say, C3, then available C-states change
> so that C2 is the deepest available C-state and cx_lowest gets overridden to C2
> and all memory of C3 setting is lost.
> Also, there is a problem with asynchronous nature of userland notification about
> power profile change and kernel notifications of _CST changes for each CPU in
> SMP system.  That required work-arounds like one in r209213 which also
> introduced quite a bit of confusion about how per-CPU cpu_cx_lowest, global
> cpu_cx_lowest and dynamic C-state changes interact with each other.
> To this day I can not unwind the complete logic behind the following lines:
> if (sc->cpu_cx_lowest < cpu_cx_lowest)
>    acpi_cpu_set_cx_lowest(sc, min(cpu_cx_lowest, sc->cpu_cx_count - 1));
>
> So to avoid the problems described above I decided to split the cx_lowest limit
> into the persistent user-set limit, cpu_cx_lowest_lim, and dynamic effective limit.
> The logic to set cpu_cx_lowest based on cpu_cx_lowest_lim and cpu_cx_count is
> moved to acpi_cpu_set_cx_lowest().
>
> Note that per-CPU cpu_cx_lowest_lim does not "interact" with global
> cpu_cx_lowest_lim.  Whichever is set later (via sysctl) just overrides the other.
>
> With such a change it no longer makes sense to limit allowed cx_lowest sysctl
> values to currently available C-state levels.  Indeed, if a user knows that his
> system may provide C6 under some circumstance, but currently only C1 is
> available, then there is no reason to disallow setting cx_lowest sysctl
> (cpu_cx_lowest_lim) to C6.  So values up to MAX_CX_STATES are allowed.
> After this change cpu_cx_count _global_ variable became unused (write-only) and
> was removed.
>
> Additionally, I introduced magic value of "Cmax" for cx_lowest sysctls to mean
> that there should not be any limit on C-state depth.

I like this change.

--
John Baldwin
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Giovanni Trematerra-2]

On Sun, Jul 8, 2012 at 12:22 PM, Andriy Gapon <[hidden email]> wrote:
>
> I would like to propose the following change for review and testing:
> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff

It seems good to me.

--
Gianni
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Ian Smith-12]

On Sun, 8 Jul 2012 13:22:49 +0300, Andriy Gapon wrote:

 > I would like to propose the following change for review and testing:
 > http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff
 >
 > The idea is to separate effective cx_lowest (the limit that the idling code
 > should not currently exceed) from user-set cx_lowest limit/target.
 > Specifically, this addresses dynamic C-state changes by ACPI platform.
 > Currently if a user sets cx_lowest to, say, C3, then available C-states change
 > so that C2 is the deepest available C-state and cx_lowest gets overridden to C2
 > and all memory of C3 setting is lost.

I wonder if that explains why setting C3 on aforesaid T23 has no effect
(in terms of dev.cpu.0.cx_usage indicating any time spent in C3) unless
the machine happened to be booted up on battery, in which case C3 is
shown as working whenever its enabled, by power_profile or manually?

Would I be wasting my time or yours trying this on an 8.2-R kernel?  Or
are there other major changes that probably preclude this?  The patch
applies cleanly, but the offsets look a bit daunting:

smithi on t23% patch < acpi_cpu_cx_lowest.diff
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/sys/dev/acpica/acpi_cpu.c b/sys/dev/acpica/acpi_cpu.c
|index e2a3dbf..e7ca14d 100644
|--- a/sys/dev/acpica/acpi_cpu.c
|+++ b/sys/dev/acpica/acpi_cpu.c
--------------------------
Patching file acpi_cpu.c using Plan A...
Hunk #1 succeeded at 86 (offset -3 lines).
Hunk #2 succeeded at 132 (offset -7 lines).
Hunk #3 succeeded at 170 (offset -3 lines).
Hunk #4 succeeded at 548 (offset -42 lines).
Hunk #5 succeeded at 792 (offset -29 lines).
Hunk #6 succeeded at 787 (offset -42 lines).
Hunk #7 succeeded at 812 (offset -29 lines).
Hunk #8 succeeded at 817 (offset -42 lines).
Hunk #9 succeeded at 1013 (offset -31 lines).
Hunk #10 succeeded at 1010 (offset -42 lines).
Hunk #11 succeeded at 1150 (offset -31 lines).
Hunk #12 succeeded at 1168 (offset -42 lines).
Hunk #13 succeeded at 1208 (offset -31 lines).
done

I hope to get another machine going soon on which I can play with
9-stable, maybe 10, but meanwhile this one is bread and butter ..

cheers, Ian
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Sean Bruno-5]

On Sun, 2012-07-08 at 03:22 -0700, Andriy Gapon wrote:
> I would like to propose the following change for review and testing:
> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff

Very nice.  After a review I went ahead and applied it for testing.  All
seems to be well on battery and A/C on my T520 so I'm very happy to see
this go into the tree.

Let me know if you want me to do the man page update for acpi_cpu(4)

Sean

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Vitaly Magerya]

Andriy Gapon <[hidden email]> wrote:
> I would like to propose the following change for review and testing:
> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff

I just tested this on a laptop that jumps between reporting only
C1, C1 and C2, and C1 through C3; in all cases your patch does
the right thing, so this removes the need for kludges I had to
do earlier; thank you, Andriy.

Will you also change how /etc/rc.d/power_profile selects
lowest_value? It currently uses the maximum from dev.cpu.0.cx_supported,
but with your patch it'll be the right thing to simply use Cmax.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Sean Bruno-5]

On Tue, 2012-07-10 at 07:27 -0700, Ian Smith wrote:
> I wonder if that explains why setting C3 on aforesaid T23 has no
> effect
> (in terms of dev.cpu.0.cx_usage indicating any time spent in C3)
> unless
> the machine happened to be booted up on battery, in which case C3 is
> shown as working whenever its enabled, by power_profile or manually?
>
>

silly question, did you set these in /etc/rc.conf ??

performance_cx_lowest="LOW"
economy_cx_lowest="LOW"

Sean

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Ian Smith-12]

On Tue, 10 Jul 2012 09:31:26 -0700, Sean Bruno wrote:
 > On Tue, 2012-07-10 at 07:27 -0700, Ian Smith wrote:
 > > I wonder if that explains why setting C3 on aforesaid T23 has no
 > > effect
 > > (in terms of dev.cpu.0.cx_usage indicating any time spent in C3)
 > > unless
 > > the machine happened to be booted up on battery, in which case C3 is
 > > shown as working whenever its enabled, by power_profile or manually?
 > >
 > >
 >
 > silly question, did you set these in /etc/rc.conf ??
 >
 > performance_cx_lowest="LOW"
 > economy_cx_lowest="LOW"

Fair enough question.  For years, since 7.0-R, I've been running
 performance_cx_lowest=C2
 economy_cx_lowest=C3
but as reported yesterday, either way it wasn't actually using C3, and
I'd checked it several times over maybe 20 minutes.

However, after (inadvertantly) leaving hw.acpi.cpu.cx_lowest manually
set to C3 while on AC power overnight, this morning I discover:

dev.cpu.0.freq: 733
dev.cpu.0.freq_levels: 1133/19100 733/12500
dev.cpu.0.cx_supported: C1/0 C2/84 C3/120
dev.cpu.0.cx_lowest: C3
dev.cpu.0.cx_usage: 0.09% 11.29% 88.61% last 635us

So, hmm, C3 must have kicked in sometime later?  Now I can merrily
change cpu.cx_lowest between C1, C2 or C3 and have that reflected in
usage; now C3 on AC: dev.cpu.0.cx_usage: 0.03% 0.75% 99.21% last 683us

confused :), Ian
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Andriy Gapon]

Many thanks to all who reviewed and tested!
I've just committed this change.


--
Andriy Gapon


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Andriy Gapon]

on 10/07/2012 18:49 Sean Bruno said the following:
> On Sun, 2012-07-08 at 03:22 -0700, Andriy Gapon wrote:
>> I would like to propose the following change for review and testing:
>> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff
>
> Very nice.  After a review I went ahead and applied it for testing.  All
> seems to be well on battery and A/C on my T520 so I'm very happy to see
> this go into the tree.
>
> Let me know if you want me to do the man page update for acpi_cpu(4)

Sean,

I would appreciate this a lot as I am currently a little bit busier than usual.

On a related note I also have this change for acpi.4:
--- a/share/man/man4/acpi.4
+++ b/share/man/man4/acpi.4
@@ -80,8 +80,12 @@ A scheduling algorithm will select states between
 and this setting
 as system load dictates.
 To enable ACPI CPU idling control,
-.Va machdep.cpu_idle_hlt
-must be set to 1.
+.Va machdep.idle
+should be set to
+.Li acpi
+if it is listed in
+.Va machdep.idle_available
+.
 .It Va hw.acpi.cpu.cx_supported
 List of supported CPU idle states and their transition latency
 in microseconds.

How does this look?
--
Andriy Gapon


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Andriy Gapon]

on 10/07/2012 19:30 Vitaly Magerya said the following:

> Andriy Gapon <[hidden email]> wrote:
>> I would like to propose the following change for review and testing:
>> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff
>
> I just tested this on a laptop that jumps between reporting only
> C1, C1 and C2, and C1 through C3; in all cases your patch does
> the right thing, so this removes the need for kludges I had to
> do earlier; thank you, Andriy.
>
> Will you also change how /etc/rc.d/power_profile selects
> lowest_value? It currently uses the maximum from dev.cpu.0.cx_supported,
> but with your patch it'll be the right thing to simply use Cmax.

I've put this on my to do list.  Hope to commit this trivial change soon-ish.

--
Andriy Gapon


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Vitaly Magerya]

On 08/07/2012, Andriy Gapon <[hidden email]> wrote:
> I would like to propose the following change for review and testing:
> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff

On 13/07/2012, Andriy Gapon <[hidden email]> wrote:
> Many thanks to all who reviewed and tested!
> I've just committed this change.

Andriy, will this change be available in 9.1?

I'm not seeing it in 9.1-RC1, and using my previous workarounds now
that a proper solution is available is a pain.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Andriy Gapon]

on 25/08/2012 13:52 Vitaly Magerya said the following:

> On 08/07/2012, Andriy Gapon <[hidden email]> wrote:
>> I would like to propose the following change for review and testing:
>> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff
>
> On 13/07/2012, Andriy Gapon <[hidden email]> wrote:
>> Many thanks to all who reviewed and tested!
>> I've just committed this change.
>
> Andriy, will this change be available in 9.1?
>
> I'm not seeing it in 9.1-RC1, and using my previous workarounds now
> that a proper solution is available is a pain.

Vitaly,

sorry for taking so long to reply.
I've managed to "sneak" the "Cmax" commit into releng/9.1 branch, so it should
appear in 9.1-RC2.
Thank you for prodding me about that.

--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Vitaly Magerya]

Andriy Gapon wrote:
> sorry for taking so long to reply.
> I've managed to "sneak" the "Cmax" commit into releng/9.1 branch, so it should
> appear in 9.1-RC2.

Cool, thank you.

> Thank you for prodding me about that.

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Kevin Oberman-3]

On Tue, Sep 4, 2012 at 11:28 PM, Andriy Gapon <[hidden email]> wrote:

> on 25/08/2012 13:52 Vitaly Magerya said the following:
>> On 08/07/2012, Andriy Gapon <[hidden email]> wrote:
>>> I would like to propose the following change for review and testing:
>>> http://people.freebsd.org/~avg/acpi_cpu_cx_lowest.diff
>>
>> On 13/07/2012, Andriy Gapon <[hidden email]> wrote:
>>> Many thanks to all who reviewed and tested!
>>> I've just committed this change.
>>
>> Andriy, will this change be available in 9.1?
>>
>> I'm not seeing it in 9.1-RC1, and using my previous workarounds now
>> that a proper solution is available is a pain.
>
> Vitaly,
>
> sorry for taking so long to reply.
> I've managed to "sneak" the "Cmax" commit into releng/9.1 branch, so it should
> appear in 9.1-RC2.
> Thank you for prodding me about that.

Thanks so much! This should finally make Cx states work on my
ThinkPad! I really appreciate it. Guess it's time to do my weekly
upgrade of this system.
--
R. Kevin Oberman, Network Engineer
E-mail: [hidden email]
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Andriy Gapon]

on 05/09/2012 18:17 Kevin Oberman said the following:
> Thanks so much! This should finally make Cx states work on my
> ThinkPad! I really appreciate it. Guess it's time to do my weekly
> upgrade of this system.

I haven't sneaked in that other commit :-(

--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Kevin Oberman-3]

On Wed, Sep 5, 2012 at 9:12 AM, Andriy Gapon <[hidden email]> wrote:
> on 05/09/2012 18:17 Kevin Oberman said the following:
>> Thanks so much! This should finally make Cx states work on my
>> ThinkPad! I really appreciate it. Guess it's time to do my weekly
>> upgrade of this system.
>
> I haven't sneaked in that other commit :-(

Oops! :-(

Oh, well. At least it should make it to /base/stable/9 soon. Right???
(I only run release/ or releng/ or for an occasional test.)
--
R. Kevin Oberman, Network Engineer
E-mail: [hidden email]
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Andriy Gapon]

on 05/09/2012 19:23 Kevin Oberman said the following:

> On Wed, Sep 5, 2012 at 9:12 AM, Andriy Gapon <[hidden email]> wrote:
>> on 05/09/2012 18:17 Kevin Oberman said the following:
>>> Thanks so much! This should finally make Cx states work on my
>>> ThinkPad! I really appreciate it. Guess it's time to do my weekly
>>> upgrade of this system.
>>
>> I haven't sneaked in that other commit :-(
>
> Oops! :-(
>
> Oh, well. At least it should make it to /base/stable/9 soon. Right???
> (I only run release/ or releng/ or for an occasional test.)
>

It's already in stable/9 :)

--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"

[Kevin Oberman-3]

On Wed, Sep 5, 2012 at 9:32 AM, Andriy Gapon <[hidden email]> wrote:

> on 05/09/2012 19:23 Kevin Oberman said the following:
>> On Wed, Sep 5, 2012 at 9:12 AM, Andriy Gapon <[hidden email]> wrote:
>>> on 05/09/2012 18:17 Kevin Oberman said the following:
>>>> Thanks so much! This should finally make Cx states work on my
>>>> ThinkPad! I really appreciate it. Guess it's time to do my weekly
>>>> upgrade of this system.
>>>
>>> I haven't sneaked in that other commit :-(
>>
>> Oops! :-(
>>
>> Oh, well. At least it should make it to /base/stable/9 soon. Right???
>> (I only run release/ or releng/ or for an occasional test.)
>>
>
> It's already in stable/9 :)

Ahh! I now see C3/109, but I see some strange behavior. When on AC
power, only C1/1 and C2/104 are available, but cx_lowest is C3, even
though C3 is not available. If I switch to battery, C1/1, C2/80 and
C3/109 are available (???), but cx_lowest is set to C2. I find the Cx
value sets a bit odd, but the setting of cx_lowest appears to be a
bug, at least to me. I can manually set cx_lowest to C3 and I actually
use C3.

My suspicion is that there is either a race or a logic issue where
x_lowest is reset to the lowest value before the available Cx values
are set, so cx_lowest is always set the the lowest Cx state from the
previous power configuration. (This is a guess, but it fits what I am
seeing very well.)
--
R. Kevin Oberman, Network Engineer
E-mail: [hidden email]
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[hidden email]"