New: CPU frequency always on highest frequency after wakeup from suspend to disk

Bug 8671 In review [email protected] 4 participants 27 messages Jun 25, 2007 – Aug 9, 2007 4 messages not in the archive CPU frequency scaling framework

lore

Share & tools

lore

[email protected]

author

2007-06-25 11:57 UTC #0 lore raw

http://bugzilla.kernel.org/show_bug.cgi?id=8671

Summary: CPU frequency always on highest frequency after wakeup
from suspend to disk
Product: Power Management
Version: 2.5
KernelVersion: 2.6.22-rc5-git8
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: low
Priority: P1
Component: cpufreq
AssignedTo: [email protected]
ReportedBy: [email protected]

Most recent kernel where this bug did not occur: unkown
Distribution: Gentoo Linux
Hardware Environment: IBM Thinkpad T43
Software Environment:
Problem Description: cpufreq_conservative doesn't scale CPU frequency anymore after wakeup from suspend to disk, instead CPU runs always with the highest available frequency. Scaling driver is acpi-cpufreq.

Steps to reproduce: Choose cpufreq_conservative as scaling driver and execute "echo disk > /sys/power/state".

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-06-25 13:34 UTC #1 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #1 from [email protected] 2007-06-25 06:37 ------- aaaah, maybe that's because CPU1 is offlined and when resumed .init is called which sets the CPUFREQ_DEFAULT_GOVERNOR.
In fact after resume CPU1 has the performance governor here, could you check it's the case for you as well?

Any suggestion on how to solve that?
Maybe cpufreq_add_dev should set the CPU0 policy as default when other CPUs are up-ed?

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Mattia Dongili

2007-06-25 14:11 UTC #3 lore raw

Re: [Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to a message not in the archive

Quoted 15 lines · Thomas Renninger

Quoted 9 lines · On Mon, 2007-06-25 at 06:34 -0700, [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=8671
------- Comment #1 from [email protected] 2007-06-25 06:37 ------- aaaah, maybe that's because CPU1 is offlined and when resumed .init is called which sets the CPUFREQ_DEFAULT_GOVERNOR.
In fact after resume CPU1 has the performance governor here, could you check it's the case for you as well?
Quoted 4 lines
Any suggestion on how to solve that?
Maybe cpufreq_add_dev should set the CPU0 policy as default when other CPUs are up-ed?

Quoted 5 lines

Can you reproduce this by simply offlining the cpu and checking governors?:
echo 0 >/sys/devices/cpu/cpu1/online
echo 1 >/sys/devices/cpu/cpu1/online
cat /sys/devices/cpu/cpu1/cpufreq/scaling_governor

yep, definitely reproducible.

Quoted 2 lines

Do you still have an older kernel (2.6.21 or older) with working cpufreq installed? Can you double check on this, pls.

herm... no. I'm on 2.6.22-rc4-mm2, I have 2.6.22-rc5 handy, all the other .21 are -mm patched.

Anyway it seems rather odd that the cpu drivers are setting the governor in place of the cpufreq subsystem itself. To me it looks more natural to think it's cpufreq_add_dev who has set the governor.

Signature · 2 lines

mattia
:wq!

[email protected]

author

2007-06-25 14:33 UTC #10 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #2 from [email protected] 2007-06-25 07:36 ------- eerm, in fact that's a one processor system and /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor says that the conservative governer is active for CPU0.

/proc/cpuinfo:
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 13
model name : Intel(R) Pentium(R) M processor 2.00GHz
stepping : 8
cpu MHz : 800.000
cache size : 2048 KB
fdiv_bug : no
hlt_bug : no
f00f_bug : no
coma_bug : no
fpu : yes
fpu_exception : yes
cpuid level : 2
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat clflush dts acpi mmx fxsr sse sse2 ss tm pbe nx est tm2
bogomips : 1597.58
clflush size : 64

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-06-25 15:12 UTC #11 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

[email protected] changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected]

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-06-25 16:18 UTC #12 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #3 from [email protected] 2007-06-25 09:22 ------- ops :)
does it happen with ondemand as well?
would you mind setting cpufreq debug=7
echo 7 > /sys/module/cpufreq/parameters/debug
perform a suspend/resume cycle and attach the log?

thanks

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-06-25 16:48 UTC #13 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #4 from [email protected] 2007-06-25 09:52 ------- this is the debug log for cpufreq_conservative:
cpufreq-core: suspending cpu 0
swsusp: critical section:
swsusp: Need to copy 14851 pages
Intel machine check architecture supported.
Intel machine check reporting enabled on CPU#0.
cpufreq-core: resuming cpu 0
cpufreq-core: Warning: CPU frequencyis 2000000, cpufreq assumed 800000 kHz.
cpufreq-core: scaling loops_per_jiffy to 7987872for frequency 2000000 kHz
cpufreq-core: handle_update for cpu 0 called
cpufreq-core: updating policy for CPU 0
cpufreq-core: setting new policy for CPU 0: 800000 - 2000000 kHz
cpufreq-core: new min and max freqs are 800000 - 2000000 kHz
cpufreq-core: governor: change or update limits
cpufreq-core: __cpufreq_governor for CPU 0, event 3

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-06-25 16:55 UTC #14 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #5 from [email protected] 2007-06-25 09:59 ------- ondemand seems to work well.

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Mattia Dongili

2007-06-26 06:22 UTC #7 lore raw

PATCH 1/1 keep policy->user_policy in sync and use CPU0 policy as a template for other CPUs

In reply to a message not in the archive

Quoted 53 lines · Thomas Renninger

Quoted 42 lines · Mattia Dongili
Quoted 33 lines · Thomas Renninger
Quoted 22 lines · Mattia Dongili
Quoted 15 lines · Thomas Renninger
Quoted 9 lines · On Mon, 2007-06-25 at 06:34 -0700, [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=8671
------- Comment #1 from [email protected] 2007-06-25 06:37 ------- aaaah, maybe that's because CPU1 is offlined and when resumed .init is called which sets the CPUFREQ_DEFAULT_GOVERNOR.
In fact after resume CPU1 has the performance governor here, could you check it's the case for you as well?
Quoted 4 lines
Any suggestion on how to solve that?
Maybe cpufreq_add_dev should set the CPU0 policy as default when other CPUs are up-ed?
Quoted 5 lines
Can you reproduce this by simply offlining the cpu and checking governors?:
echo 0 >/sys/devices/cpu/cpu1/online
echo 1 >/sys/devices/cpu/cpu1/online
cat /sys/devices/cpu/cpu1/cpufreq/scaling_governor
Quoted 7 lines
yep, definitely reproducible.
Quoted 2 lines
Do you still have an older kernel (2.6.21 or older) with working cpufreq installed? Can you double check on this, pls.
herm... no. I'm on 2.6.22-rc4-mm2, I have 2.6.22-rc5 handy, all the other .21 are -mm patched.
Ok, if you already have one compiled, then also checking on 2.6.22-rc5 would help.
Quoted 7 lines
If this one is also affected, it's pretty much the patch from me who breaks this...
If not, I'll still have a look at mm..., but then it's at least not that sever...
Ahh in the bug it's stated that 2.6.22-rc5-git8 is affected? I'll have a look...
Quoted 6 lines
Yes, but it looks slightly different as this is on UP. Anyway changes in -mm look pretty trivial so I'm pretty sure also -rc5 is affected anyway. Will test later just to confirm.
BTW: which patch are you talking about? I can't see much activity in drivers/cpufreq recently.
The two patches are attached. The third, small one could be the fix. I still don't really get why, because policy->governor gets initialised in ->init ..., anyway, now there is no functional change anymore with my patch, I should have done it like this from the beginning...

...

Quoted 13 lines

Index: linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
===================================================================
drivers/cpufreq/cpufreq.c +0 -1
@@ -770,7 +770,6 @@ static int cpufreq_add_dev (struct sys_d
 	}
 	policy->user_policy.min = policy->cpuinfo.min_freq;
 	policy->user_policy.max = policy->cpuinfo.max_freq;
-	policy->user_policy.governor = policy->governor;
 
 #ifdef CONFIG_SMP
 	for_each_cpu_mask(j, policy->cpus) {

drivers/cpufreq/cpufreq.c static int cpufreq_add_dev (struct sys_d · line 869

Quoted 6 lines

drivers/cpufreq/cpufreq.c +1 -0

@@ -865,6 +864,7 @@ static int cpufreq_add_dev (struct sys_d
 	/* set default policy */
 	ret = __cpufreq_set_policy(policy, &new_policy);
 	policy->user_policy.policy = policy->policy;
+	policy->user_policy.governor = policy->governor;
 
 	unlock_policy_rwsem_write(cpu);

It's not a matter of ordering, policy->governor is already wrong here because the cpu driver set it to CPUFREQ_DEFAULT_GOVERNOR. Also, policy->user_policy is not kept in sync now that cpufreq_set_policy disappeared and cpufreq_update_policy simply sets the wrong policy when it runs on resume.

The following patch fixes this for me but I don't know if locking is needed in the first hunk.

[CPUFREQ]
set the same policy as CPU0 on other CPUs when up-ed and keep policy->user_policy in sync.

Signed-off-by Mattia Dongili <[email protected]>

---

drivers/cpufreq/cpufreq.c +16 -4

index 9375095..21933e2 100644
@@ -768,11 +768,19 @@ static int cpufreq_add_dev (struct sys_device * sys_dev)
 		unlock_policy_rwsem_write(cpu);
 		goto err_out;
 	}
-	policy->user_policy.min = policy->cpuinfo.min_freq;
-	policy->user_policy.max = policy->cpuinfo.max_freq;
-	policy->user_policy.governor = policy->governor;
 
 #ifdef CONFIG_SMP
+	if (cpu != 0) {
+		managed_policy = cpufreq_cpu_get(0);
+		if (likely(managed_policy)) {
+			policy->governor = managed_policy->user_policy.governor;
+			policy->min = managed_policy->user_policy.min;
+			policy->max = managed_policy->user_policy.max;
+			cpufreq_cpu_put(managed_policy);
+			dprintk("Governor for CPU%u is %s\n", cpu,
+					policy->governor->name);
+		}
+	}
 	for_each_cpu_mask(j, policy->cpus) {
 		if (cpu == j)
 			continue;

@@ -869,7 +877,6 @@ static int cpufreq_add_dev (struct sys_device * sys_dev)
 
 	/* set default policy */
 	ret = __cpufreq_set_policy(policy, &new_policy);
-	policy->user_policy.policy = policy->policy;
 
 	unlock_policy_rwsem_write(cpu);

@@ -1625,6 +1632,11 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
 		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
 	}
 
+	data->user_policy.min = data->min;
+	data->user_policy.max = data->max;
+	data->user_policy.policy = data->policy;
+	data->user_policy.governor = data->governor;
+
 error_out:
 	cpufreq_debug_enable_ratelimit();
 	return ret;

Signature · 2 lines

mattia
:wq!

[email protected]

author

2007-06-26 06:24 UTC #15 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #6 from [email protected] 2007-06-25 23:28 ------- Created an attachment (id=11881)
--> (http://bugzilla.kernel.org/attachment.cgi?id=11881&action=view)
Keep policy->user_policy in sync and use CPU0 policy as a template for other CPUs policy when up-ed

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-06-26 06:24 UTC #16 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

[email protected] changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected]

------- Comment #7 from [email protected] 2007-06-25 23:28 ------- Markus, does the attached patch fixes it for you?

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Thomas Renninger

2007-06-26 10:37 UTC #8 lore raw

Re: [PATCH 2.6.22-rc6?] keep policy->user_policy in sync and use CPU0 policy as a template for other CPUs

In reply to Mattia Dongili

Quoted 95 lines · Mattia Dongili

Quoted 53 lines · Thomas Renninger
Quoted 42 lines · Mattia Dongili
Quoted 33 lines · Thomas Renninger
Quoted 22 lines · Mattia Dongili
Quoted 15 lines · Thomas Renninger
Quoted 9 lines · On Mon, 2007-06-25 at 06:34 -0700, [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=8671
------- Comment #1 from [email protected] 2007-06-25 06:37 ------- aaaah, maybe that's because CPU1 is offlined and when resumed .init is called which sets the CPUFREQ_DEFAULT_GOVERNOR.
In fact after resume CPU1 has the performance governor here, could you check it's the case for you as well?
Quoted 4 lines
Any suggestion on how to solve that?
Maybe cpufreq_add_dev should set the CPU0 policy as default when other CPUs are up-ed?
Quoted 5 lines
Can you reproduce this by simply offlining the cpu and checking governors?:
echo 0 >/sys/devices/cpu/cpu1/online
echo 1 >/sys/devices/cpu/cpu1/online
cat /sys/devices/cpu/cpu1/cpufreq/scaling_governor
Quoted 7 lines
yep, definitely reproducible.
Quoted 2 lines
Do you still have an older kernel (2.6.21 or older) with working cpufreq installed? Can you double check on this, pls.
herm... no. I'm on 2.6.22-rc4-mm2, I have 2.6.22-rc5 handy, all the other .21 are -mm patched.
Ok, if you already have one compiled, then also checking on 2.6.22-rc5 would help.
Quoted 7 lines
If this one is also affected, it's pretty much the patch from me who breaks this...
If not, I'll still have a look at mm..., but then it's at least not that sever...
Ahh in the bug it's stated that 2.6.22-rc5-git8 is affected? I'll have a look...
Quoted 6 lines
Yes, but it looks slightly different as this is on UP. Anyway changes in -mm look pretty trivial so I'm pretty sure also -rc5 is affected anyway. Will test later just to confirm.
BTW: which patch are you talking about? I can't see much activity in drivers/cpufreq recently.
The two patches are attached. The third, small one could be the fix. I still don't really get why, because policy->governor gets initialised in ->init ..., anyway, now there is no functional change anymore with my patch, I should have done it like this from the beginning...
...
Quoted 13 lines
Index: linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
===================================================================
drivers/cpufreq/cpufreq.c +0 -1
@@ -770,7 +770,6 @@ static int cpufreq_add_dev (struct sys_d
 	}
 	policy->user_policy.min = policy->cpuinfo.min_freq;
 	policy->user_policy.max = policy->cpuinfo.max_freq;
-	policy->user_policy.governor = policy->governor;
 
 #ifdef CONFIG_SMP
 	for_each_cpu_mask(j, policy->cpus) {
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev (struct sys_d · line 869
Quoted 6 lines
drivers/cpufreq/cpufreq.c +1 -0
@@ -865,6 +864,7 @@ static int cpufreq_add_dev (struct sys_d
 	/* set default policy */
 	ret = __cpufreq_set_policy(policy, &new_policy);
 	policy->user_policy.policy = policy->policy;
+	policy->user_policy.governor = policy->governor;
 
 	unlock_policy_rwsem_write(cpu);
It's not a matter of ordering, policy->governor is already wrong here because the cpu driver set it to CPUFREQ_DEFAULT_GOVERNOR. Also, policy->user_policy is not kept in sync now that cpufreq_set_policy disappeared and cpufreq_update_policy simply sets the wrong policy when it runs on resume.
The following patch fixes this for me but I don't know if locking is needed in the first hunk.
[CPUFREQ]
set the same policy as CPU0 on other CPUs when up-ed and keep policy->user_policy in sync.
Signed-off-by Mattia Dongili <[email protected]>
---
drivers/cpufreq/cpufreq.c +0 -0
index 9375095..21933e2 100644

drivers/cpufreq/cpufreq.c static int cpufreq_add_dev (struct sys_device * sys_dev) · line 773

Quoted 6 lines

drivers/cpufreq/cpufreq.c +0 -3

@@ -768,11 +768,19 @@ static int cpufreq_add_dev (struct sys_device * sys_dev)
 		unlock_policy_rwsem_write(cpu);
 		goto err_out;
 	}
-	policy->user_policy.min = policy->cpuinfo.min_freq;
-	policy->user_policy.max = policy->cpuinfo.max_freq;
-	policy->user_policy.governor = policy->governor;

Yes, those should go down. But IMO not into __cpufreq_set_policy, but after __cpufreq_set_policy below (marked with XXXX).

Quoted 1 line

****************************************

drivers/cpufreq/cpufreq.c static int cpufreq_add_dev (struct sys_device * sys_dev) · line 783

Quoted 12 lines

#ifdef CONFIG_SMP

drivers/cpufreq/cpufreq.c +11 -0

+	if (cpu != 0) {
+		managed_policy = cpufreq_cpu_get(0);
+		if (likely(managed_policy)) {
+			policy->governor = managed_policy->user_policy.governor;
+			policy->min = managed_policy->user_policy.min;
+			policy->max = managed_policy->user_policy.max;
+			cpufreq_cpu_put(managed_policy);
+			dprintk("Governor for CPU%u is %s\n", cpu,
+					policy->governor->name);
+		}
+	}

****************************************
But why has it worked before my patch then, this was never needed? Or has it just not yet been discovered?
Ok, I reproduced this even without my patch, this part (inside the stars) fixes this specific problem, right?
And now I get what this is about.
This is a general problem, that the policy of the removed CPU is not stored.
We could:
a) store the governor of each removed CPU. Not sure how this can be done (nicely) ...
b) restore to the default governor (currently done)
c) restore to the governor CPU0 uses (that's what you propose) But you might have run different governors on the CPUs, so this patch still does not ensure that the same governor is run as before removing the CPU.

I am really unsure what is better..., I have a patch in queue that ondemand can be used as default governor, then it's not that bad. If governors got switched by userspace, userspace still need to check and rewrite the governor after adding a CPU or resuming. AFAIK we do that, Holger should know whether we do that in the powersaved or hal. Not sure whether hal knows which governor runs on which CPU, maybe it should learn about?

No userspace needed of course would be best. Only solution I have in my mind is a pointer array for each possible CPU, pointing to (storing) a governor.
Any better ideas here?
What shall we go for?

Quoted 11 lines

drivers/cpufreq/cpufreq.c +0 -1

 	for_each_cpu_mask(j, policy->cpus) {
 		if (cpu == j)
 			continue;

@@ -869,7 +877,6 @@ static int cpufreq_add_dev (struct sys_device * sys_dev)
 
 	/* set default policy */
 	ret = __cpufreq_set_policy(policy, &new_policy);
-	policy->user_policy.policy = policy->policy;
 	unlock_policy_rwsem_write(cpu);

drivers/cpufreq/cpufreq.c static int __cpufreq_set_policy(struct cpufreq_policy *data, · line 1636

Quoted 5 lines

drivers/cpufreq/cpufreq.c +2 -0

@@ -1625,6 +1632,11 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
 		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
 	}
 
+	data->user_policy.min = data->min;
+	data->user_policy.max = data->max;

This will break _PPC again. If you e.g. boot on battery with freq limited, data->max will get reduced in __cpufreq_set_policy, then data->user_policy.max will get a limited freq and max freq will not be increased by update_policy:
policy.max = data->user_policy.max;
should restore the old max value there again.

drivers/cpufreq/cpufreq.c static int __cpufreq_set_policy(struct cpufreq_policy *data, · line 1642

Quoted 6 lines

drivers/cpufreq/cpufreq.c +3 -0

+	data->user_policy.policy = data->policy;
+	data->user_policy.governor = data->governor;
+
 error_out:
 	cpufreq_debug_enable_ratelimit();
 	return ret;

Thomas

[email protected]

author

2007-06-26 11:35 UTC #17 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #8 from [email protected] 2007-06-26 04:39 ------- No, the patch doesn't change anything (tested on 2.6.22-rc6).

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Mattia Dongili

2007-06-26 12:09 UTC #9 lore raw

Re: [PATCH 2.6.22-rc6?] keep policy->user_policy in sync and use CPU0 policy as a template for other CPUs

In reply to Thomas Renninger

Quoted 89 lines · Thomas Renninger

Quoted 88 lines · Mattia Dongili
Quoted 53 lines · Thomas Renninger
Quoted 42 lines · Mattia Dongili
Quoted 33 lines · Thomas Renninger
Quoted 22 lines · Mattia Dongili
Quoted 15 lines · Thomas Renninger
Quoted 9 lines · On Mon, 2007-06-25 at 06:34 -0700, [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=8671
------- Comment #1 from [email protected] 2007-06-25 06:37 ------- aaaah, maybe that's because CPU1 is offlined and when resumed .init is called which sets the CPUFREQ_DEFAULT_GOVERNOR.
In fact after resume CPU1 has the performance governor here, could you check it's the case for you as well?
Quoted 4 lines
Any suggestion on how to solve that?
Maybe cpufreq_add_dev should set the CPU0 policy as default when other CPUs are up-ed?
Quoted 5 lines
Can you reproduce this by simply offlining the cpu and checking governors?:
echo 0 >/sys/devices/cpu/cpu1/online
echo 1 >/sys/devices/cpu/cpu1/online
cat /sys/devices/cpu/cpu1/cpufreq/scaling_governor
Quoted 7 lines
yep, definitely reproducible.
Quoted 2 lines
Do you still have an older kernel (2.6.21 or older) with working cpufreq installed? Can you double check on this, pls.
herm... no. I'm on 2.6.22-rc4-mm2, I have 2.6.22-rc5 handy, all the other .21 are -mm patched.
Ok, if you already have one compiled, then also checking on 2.6.22-rc5 would help.
Quoted 7 lines
If this one is also affected, it's pretty much the patch from me who breaks this...
If not, I'll still have a look at mm..., but then it's at least not that sever...
Ahh in the bug it's stated that 2.6.22-rc5-git8 is affected? I'll have a look...
Quoted 6 lines
Yes, but it looks slightly different as this is on UP. Anyway changes in -mm look pretty trivial so I'm pretty sure also -rc5 is affected anyway. Will test later just to confirm.
BTW: which patch are you talking about? I can't see much activity in drivers/cpufreq recently.
The two patches are attached. The third, small one could be the fix. I still don't really get why, because policy->governor gets initialised in ->init ..., anyway, now there is no functional change anymore with my patch, I should have done it like this from the beginning...
...
Quoted 13 lines
Index: linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
===================================================================
drivers/cpufreq/cpufreq.c +0 -1
@@ -770,7 +770,6 @@ static int cpufreq_add_dev (struct sys_d
 	}
 	policy->user_policy.min = policy->cpuinfo.min_freq;
 	policy->user_policy.max = policy->cpuinfo.max_freq;
-	policy->user_policy.governor = policy->governor;
 
 #ifdef CONFIG_SMP
 	for_each_cpu_mask(j, policy->cpus) {
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev (struct sys_d · line 869
Quoted 6 lines
drivers/cpufreq/cpufreq.c +1 -0
@@ -865,6 +864,7 @@ static int cpufreq_add_dev (struct sys_d
 	/* set default policy */
 	ret = __cpufreq_set_policy(policy, &new_policy);
 	policy->user_policy.policy = policy->policy;
+	policy->user_policy.governor = policy->governor;
 
 	unlock_policy_rwsem_write(cpu);
It's not a matter of ordering, policy->governor is already wrong here because the cpu driver set it to CPUFREQ_DEFAULT_GOVERNOR. Also, policy->user_policy is not kept in sync now that cpufreq_set_policy disappeared and cpufreq_update_policy simply sets the wrong policy when it runs on resume.
The following patch fixes this for me but I don't know if locking is needed in the first hunk.
[CPUFREQ]
set the same policy as CPU0 on other CPUs when up-ed and keep policy->user_policy in sync.

Quoted 15 lines

drivers/cpufreq/cpufreq.c static int cpufreq_add_dev (struct sys_device * sys_dev) · line 773
Quoted 13 lines
Signed-off-by Mattia Dongili <[email protected]>
---
drivers/cpufreq/cpufreq.c +0 -3
index 9375095..21933e2 100644
@@ -768,11 +768,19 @@ static int cpufreq_add_dev (struct sys_device * sys_dev)
 		unlock_policy_rwsem_write(cpu);
 		goto err_out;
 	}
-	policy->user_policy.min = policy->cpuinfo.min_freq;
-	policy->user_policy.max = policy->cpuinfo.max_freq;
-	policy->user_policy.governor = policy->governor;
Yes, those should go down. But IMO not into __cpufreq_set_policy, but after __cpufreq_set_policy below (marked with XXXX).

Ok, I failed to see that the sysfs store_* functions already kept user_policy in sync.

Quoted 10 lines

****************************************

Quoted 9 lines

drivers/cpufreq/cpufreq.c +8 -0

 #ifdef CONFIG_SMP
+	if (cpu != 0) {
+		managed_policy = cpufreq_cpu_get(0);
+		if (likely(managed_policy)) {
+			policy->governor = managed_policy->user_policy.governor;
+			policy->min = managed_policy->user_policy.min;
+			policy->max = managed_policy->user_policy.max;
+			cpufreq_cpu_put(managed_policy);
+			dprintk("Governor for CPU%u is %s\n", cpu,

Quoted 8 lines

drivers/cpufreq/cpufreq.c static int cpufreq_add_dev (struct sys_device * sys_dev) · line 783
Quoted 3 lines
drivers/cpufreq/cpufreq.c +3 -0
+					policy->governor->name);
+		}
+	}
****************************************
But why has it worked before my patch then, this was never needed? Or has it just not yet been discovered?
Ok, I reproduced this even without my patch, this part (inside the stars) fixes this specific problem, right?

yes, I cheated, two bugs in a single patch :)
I only realized later that the patch should be split but I was waiting for some feedback.

Quoted 20 lines

And now I get what this is about.
This is a general problem, that the policy of the removed CPU is not stored.
We could:
a) store the governor of each removed CPU. Not sure how this can be done (nicely) ...
b) restore to the default governor (currently done)
c) restore to the governor CPU0 uses (that's what you propose) But you might have run different governors on the CPUs, so this patch still does not ensure that the same governor is run as before removing the CPU.
I am really unsure what is better..., I have a patch in queue that ondemand can be used as default governor, then it's not that bad. If governors got switched by userspace, userspace still need to check and rewrite the governor after adding a CPU or resuming. AFAIK we do that, Holger should know whether we do that in the powersaved or hal. Not sure whether hal knows which governor runs on which CPU, maybe it should learn about?

Quoted 4 lines

No userspace needed of course would be best. Only solution I have in my mind is a pointer array for each possible CPU, pointing to (storing) a governor.
Any better ideas here?

well, we now have the CPU_{ONLINE,OFFLINE}_FROZEN that could be exploited for that purpose allowing for a different behaviour on a simple cpu_up v/s cpu_up because of resume.
But I foresee a rehash of the old different-governors-on-different-cpus[1] argument I saw some time ago.

I'd still like an
- use settings from cpu0 by default
- use previous settings on resume

Maybe changing slightly cpufreq_{add,remove}_dev it may be possible to achive something like that.

[1]: and the related hw/sw coordination

Signature · 2 lines

mattia
:wq!

[email protected]

author

2007-06-26 12:58 UTC #18 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #9 from [email protected] 2007-06-26 06:02 ------- Created an attachment (id=11884)
--> (http://bugzilla.kernel.org/attachment.cgi?id=11884&action=view)
Restore previously used governor on a hot-replugged CPU

Can you try this one, pls.

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-06-26 13:33 UTC #19 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #10 from [email protected] 2007-06-26 06:37 ------- Yepp, the patch from Thomas works.

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Thomas Renninger

2007-06-26 13:57 UTC #20 lore raw

Re: [Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

Quoted 4 lines · On Tue, 2007-06-26 at 06:33 -0700, [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=8671
------- Comment #10 from [email protected] 2007-06-26 06:37 ------- Yepp, the patch from Thomas works.

Dave, is this acceptable?
If yes, I expect you are going to pick it for the next kernel iteration and people still need to workaround this one in userspace or need to pick up the patch separately?

Thanks,

Thomas

[email protected]

author

2007-06-26 14:12 UTC #24 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

[email protected] changed:

What |Removed |Added
----------------------------------------------------------------------------

  Attachment #11881|0                           |1

is obsolete| |

------- Comment #11 from [email protected] 2007-06-26 07:16 ------- (From update of attachment 11881)
looks like Tomas had a better (and working) fix

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Dave Jones

2007-06-26 20:31 UTC #21 lore raw

Re: [Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to Thomas Renninger

Quoted 11 lines · Thomas Renninger

Quoted 4 lines · On Tue, 2007-06-26 at 06:33 -0700, [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=8671
------- Comment #10 from [email protected] 2007-06-26 06:37 ------- Yepp, the patch from Thomas works.
Dave, is this acceptable?
If yes, I expect you are going to pick it for the next kernel iteration and people still need to workaround this one in userspace or need to pick up the patch separately?

Yeah, looks like the easiest way to solve that. Can you mail me a copy off-list, and I'll get to it as soon as I get back from OLS ?

Thanks,

Dave

Signature · 1 line

http://www.codemonkey.org.uk

Thomas Renninger

2007-06-27 09:28 UTC #22 lore raw

Re: [Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

Quoted 8 lines · Dave Jones

Quoted 7 lines · Thomas Renninger
Quoted 4 lines · On Tue, 2007-06-26 at 06:33 -0700, [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=8671
------- Comment #10 from [email protected] 2007-06-26 06:37 ------- Yepp, the patch from Thomas works.

Quoted 9 lines

Quoted 4 lines
Dave, is this acceptable?
If yes, I expect you are going to pick it for the next kernel iteration and people still need to workaround this one in userspace or need to pick up the patch separately?
Yeah, looks like the easiest way to solve that. Can you mail me a copy off-list, and I'll get to it as soon as I get back from OLS ?

Ok.
But people should be aware that as soon as userspace integrates a policy to add/remove CPUs for e.g. power saving reasons dynamically, userspace still must rewrite governors for specific events.
For these solutions:

a) store the governor of each removed CPU.
b) restore to the default governor (currently done)
c) restore to the governor CPU0 uses (that's what you propose) But you might have run different governors on the CPUs, so this patch still does not ensure that the same governor is run as before removing the CPU.

This action needs to be taken in user space:

a) If a CPU got removed and user space switches governors, it must make sure the governor gets written as soon as the CPU gets added again.
Not sure about resume. If all CPUs (also the removed ones) are brought up again, the governor must be written to them.

b) If run on an alternative governor governor must be rewritten in resume and re-add case.

c) Best solution for userspace if only one governor can run at the same time.
Synchronization needed at resume and hot-adding if several governors can run at the same time.

I am really not sure what is best here....
Reducing the abilities so that only one governor can be run at the same time, would make this all much easier, then we should go for c) and everything is fine.
Some people liked the idea, some not..., again is this really necessary?

Thomas

Mattia Dongili

2007-06-28 04:16 UTC #23 lore raw

Re: [Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

Quoted 47 lines · Thomas Renninger

Quoted 8 lines · Dave Jones
Quoted 7 lines · Thomas Renninger
Quoted 4 lines · On Tue, 2007-06-26 at 06:33 -0700, [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=8671
------- Comment #10 from [email protected] 2007-06-26 06:37 ------- Yepp, the patch from Thomas works.
Quoted 9 lines
Quoted 4 lines
Dave, is this acceptable?
If yes, I expect you are going to pick it for the next kernel iteration and people still need to workaround this one in userspace or need to pick up the patch separately?
Yeah, looks like the easiest way to solve that. Can you mail me a copy off-list, and I'll get to it as soon as I get back from OLS ?
Ok.
But people should be aware that as soon as userspace integrates a policy to add/remove CPUs for e.g. power saving reasons dynamically, userspace still must rewrite governors for specific events.
For these solutions:
a) store the governor of each removed CPU.
b) restore to the default governor (currently done)
c) restore to the governor CPU0 uses (that's what you propose) But you might have run different governors on the CPUs, so this patch still does not ensure that the same governor is run as before removing the CPU.
This action needs to be taken in user space:
a) If a CPU got removed and user space switches governors, it must make sure the governor gets written as soon as the CPU gets added again.
Not sure about resume. If all CPUs (also the removed ones) are brought up again, the governor must be written to them.
b) If run on an alternative governor governor must be rewritten in resume and re-add case.
c) Best solution for userspace if only one governor can run at the same time.
Synchronization needed at resume and hot-adding if several governors can run at the same time.

Quoted 5 lines

I am really not sure what is best here....
Reducing the abilities so that only one governor can be run at the same time, would make this all much easier, then we should go for c) and everything is fine.
Some people liked the idea, some not..., again is this really necessary?

I have the impression that what your patch is doing is already enough for most cases and if userspace wants to play deadly games with cpu {on,off}lining and governors then this is an entire userspace problem, e.g.: userspace is implementing its own powersaving policy and can't expect the kernel to follow on that.

What I expect from the kernel side is to be consistent (not much in the case of {off,on}lining but in the resume case at least) wrt applying governors on newly seen cpus.
My suggestion would actually be a mix yours and mine solutions:
- apply the policy of CPU0 to newly added cpus (as in "never seen before")
- apply the previous policy if the pointer in the array exists

PS: Thomas would you mind pushing your patch to akpm in case the next -mm comes before the end of OLS? :)

Signature · 2 lines

mattia
:wq!

[email protected]

author

2007-06-30 23:53 UTC #25 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

[email protected] changed:

What |Removed |Added
----------------------------------------------------------------------------

                 CC|                            |[email protected]
             Status|NEW                         |RESOLVED
         Resolution|                            |DUPLICATE

------- Comment #12 from [email protected] 2007-06-30 16:57 -------

*** This bug has been marked as a duplicate of bug 7216 ***

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-07-01 00:00 UTC #26 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

[email protected] changed:

What |Removed |Added
----------------------------------------------------------------------------

             Status|RESOLVED                    |REOPENED
         Resolution|DUPLICATE                   |

------- Comment #13 from [email protected] 2007-06-30 17:04 ------- Sorry, wrong field.

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-07-01 00:00 UTC #27 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

[email protected] changed:

What |Removed |Added
----------------------------------------------------------------------------

OtherBugsDependingO|                            |7216
              nThis|                            |

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-08-08 16:57 UTC #28 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #14 from [email protected] 2007-08-08 10:03 ------- (In reply to comment #9)

Quoted 1 line

Created an attachment (id=11884)

--> (http://bugzilla.kernel.org/attachment.cgi?id=11884&action=view) [details]

Quoted 1 line

Restore previously used governor on a hot-replugged CPU

Can you please tell me what the status of this patch is?

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-08-09 04:35 UTC #29 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

------- Comment #15 from [email protected] 2007-08-08 21:42 ------- It's in Linus' tree for 2.6.23

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[email protected]

author

2007-08-09 14:07 UTC #30 lore raw

[Bug 8671] CPU frequency always on highest frequency after wakeup from suspend to disk

In reply to [email protected]

http://bugzilla.kernel.org/show_bug.cgi?id=8671

[email protected] changed:

What |Removed |Added
----------------------------------------------------------------------------

             Status|REOPENED                    |CLOSED
         Resolution|                            |CODE_FIX

Signature · 3 lines

Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.