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

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

[bugme-daemon]

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: cpufreq@www.linux.org.uk
        ReportedBy: linux@markus-schaub.de


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".


-- 
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.

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

[bugme-daemon]

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





------- Comment #1 from malattia@linux.it  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?


-- 
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.

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

[Mattia Dongili]

On Mon, Jun 25, 2007 at 04:01:34PM +0200, Thomas Renninger wrote:
> On Mon, 2007-06-25 at 06:34 -0700, bugme-daemon@bugzilla.kernel.org
> wrote:
> > http://bugzilla.kernel.org/show_bug.cgi?id=8671
> > 
> 
> > ------- Comment #1 from malattia@linux.it  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?
> > 
> 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.

> 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.

-- 
mattia
:wq!

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

[bugme-daemon]

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





------- Comment #2 from linux@markus-schaub.de  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


-- 
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.

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

[bugme-daemon]

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


trenn@suse.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |trenn@suse.de




-- 
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.

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

[bugme-daemon]

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





------- Comment #3 from malattia@linux.it  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


-- 
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.

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

[bugme-daemon]

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





------- Comment #4 from linux@markus-schaub.de  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


-- 
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.

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

[bugme-daemon]

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





------- Comment #5 from linux@markus-schaub.de  2007-06-25 09:59 -------
ondemand seems to work well.


-- 
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.

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

[Mattia Dongili]

On Mon, Jun 25, 2007 at 09:00:22PM +0200, Thomas Renninger wrote:
> On Tue, 2007-06-26 at 00:34 +0900, Mattia Dongili wrote:
> > On Mon, Jun 25, 2007 at 05:12:31PM +0200, Thomas Renninger wrote:
> > > On Mon, 2007-06-25 at 23:11 +0900, Mattia Dongili wrote: 
> > > > On Mon, Jun 25, 2007 at 04:01:34PM +0200, Thomas Renninger wrote:
> > > > > On Mon, 2007-06-25 at 06:34 -0700, bugme-daemon@bugzilla.kernel.org
> > > > > wrote:
> > > > > > http://bugzilla.kernel.org/show_bug.cgi?id=8671
> > > > > > 
> > > > > 
> > > > > > ------- Comment #1 from malattia@linux.it  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?
> > > > > > 
> > > > > 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.
> > > > 
> > > > > 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.
> > > 
> > > 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...
> > 
> > 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...
...
> Index: linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-2.6.22-rc6.orig/drivers/cpufreq/cpufreq.c
> +++ linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
> @@ -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) {
> @@ -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 <malattia@linux.it>
---
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9375095..21933e2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -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;
-- 
mattia
:wq!

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

[bugme-daemon]

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





------- Comment #6 from malattia@linux.it  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


-- 
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.

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

[bugme-daemon]

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


malattia@linux.it changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |malattia@linux.it




------- Comment #7 from malattia@linux.it  2007-06-25 23:28 -------
Markus, does the attached patch fixes it for you?


-- 
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.

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

[Thomas Renninger]

On Tue, 2007-06-26 at 15:22 +0900, Mattia Dongili wrote:
> On Mon, Jun 25, 2007 at 09:00:22PM +0200, Thomas Renninger wrote:
> > On Tue, 2007-06-26 at 00:34 +0900, Mattia Dongili wrote:
> > > On Mon, Jun 25, 2007 at 05:12:31PM +0200, Thomas Renninger wrote:
> > > > On Mon, 2007-06-25 at 23:11 +0900, Mattia Dongili wrote: 
> > > > > On Mon, Jun 25, 2007 at 04:01:34PM +0200, Thomas Renninger wrote:
> > > > > > On Mon, 2007-06-25 at 06:34 -0700, bugme-daemon@bugzilla.kernel.org
> > > > > > wrote:
> > > > > > > http://bugzilla.kernel.org/show_bug.cgi?id=8671
> > > > > > > 
> > > > > > 
> > > > > > > ------- Comment #1 from malattia@linux.it  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?
> > > > > > > 
> > > > > > 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.
> > > > > 
> > > > > > 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.
> > > > 
> > > > 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...
> > > 
> > > 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...
> ...
> > Index: linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
> > ===================================================================
> > --- linux-2.6.22-rc6.orig/drivers/cpufreq/cpufreq.c
> > +++ linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
> > @@ -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) {
> > @@ -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 <malattia@linux.it>
> ---
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 9375095..21933e2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -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).
>  
****************************************
>  #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);
> +		}
> +	}
****************************************
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?

>  	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;
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.

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

   Thomas

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

[bugme-daemon]

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





------- Comment #8 from linux@markus-schaub.de  2007-06-26 04:39 -------
No, the patch doesn't change anything (tested on 2.6.22-rc6).


-- 
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.

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

[Mattia Dongili]

On Tue, Jun 26, 2007 at 12:37:53PM +0200, Thomas Renninger wrote:
> On Tue, 2007-06-26 at 15:22 +0900, Mattia Dongili wrote:
> > On Mon, Jun 25, 2007 at 09:00:22PM +0200, Thomas Renninger wrote:
> > > On Tue, 2007-06-26 at 00:34 +0900, Mattia Dongili wrote:
> > > > On Mon, Jun 25, 2007 at 05:12:31PM +0200, Thomas Renninger wrote:
> > > > > On Mon, 2007-06-25 at 23:11 +0900, Mattia Dongili wrote: 
> > > > > > On Mon, Jun 25, 2007 at 04:01:34PM +0200, Thomas Renninger wrote:
> > > > > > > On Mon, 2007-06-25 at 06:34 -0700, bugme-daemon@bugzilla.kernel.org
> > > > > > > wrote:
> > > > > > > > http://bugzilla.kernel.org/show_bug.cgi?id=8671
> > > > > > > > 
> > > > > > > 
> > > > > > > > ------- Comment #1 from malattia@linux.it  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?
> > > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > > 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.
> > > > > 
> > > > > 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...
> > > > 
> > > > 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...
> > ...
> > > Index: linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
> > > ===================================================================
> > > --- linux-2.6.22-rc6.orig/drivers/cpufreq/cpufreq.c
> > > +++ linux-2.6.22-rc6/drivers/cpufreq/cpufreq.c
> > > @@ -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) {
> > > @@ -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 <malattia@linux.it>
> > ---
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 9375095..21933e2 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -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.

> ****************************************
> >  #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);
> > +		}
> > +	}
> ****************************************
> 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.

> 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?

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
-- 
mattia
:wq!

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

[bugme-daemon]

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





------- Comment #9 from trenn@suse.de  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.


-- 
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.

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

[bugme-daemon]

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





------- Comment #10 from linux@markus-schaub.de  2007-06-26 06:37 -------
Yepp, the patch from Thomas works.


-- 
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.

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

[Thomas Renninger]

On Tue, 2007-06-26 at 06:33 -0700, bugme-daemon@bugzilla.kernel.org
wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=8671

> ------- Comment #10 from linux@markus-schaub.de  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

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

[bugme-daemon]

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


malattia@linux.it changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #11881|0                           |1
        is obsolete|                            |




------- Comment #11 from malattia@linux.it  2007-06-26 07:16 -------
(From update of attachment 11881)
looks like Tomas had a better (and working) fix


-- 
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.

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

[Dave Jones]

On Tue, Jun 26, 2007 at 03:57:08PM +0200, Thomas Renninger wrote:
 > On Tue, 2007-06-26 at 06:33 -0700, bugme-daemon@bugzilla.kernel.org
 > wrote:
 > > http://bugzilla.kernel.org/show_bug.cgi?id=8671
 > 
 > > ------- Comment #10 from linux@markus-schaub.de  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

-- 
http://www.codemonkey.org.uk

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

[Thomas Renninger]

On Tue, 2007-06-26 at 16:31 -0400, Dave Jones wrote:
> On Tue, Jun 26, 2007 at 03:57:08PM +0200, Thomas Renninger wrote:
>  > On Tue, 2007-06-26 at 06:33 -0700, bugme-daemon@bugzilla.kernel.org
>  > wrote:
>  > > http://bugzilla.kernel.org/show_bug.cgi?id=8671
>  > 
>  > > ------- Comment #10 from linux@markus-schaub.de  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 ?
> 
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

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

[Mattia Dongili]

On Wed, Jun 27, 2007 at 11:28:06AM +0200, Thomas Renninger wrote:
> On Tue, 2007-06-26 at 16:31 -0400, Dave Jones wrote:
> > On Tue, Jun 26, 2007 at 03:57:08PM +0200, Thomas Renninger wrote:
> >  > On Tue, 2007-06-26 at 06:33 -0700, bugme-daemon@bugzilla.kernel.org
> >  > wrote:
> >  > > http://bugzilla.kernel.org/show_bug.cgi?id=8671
> >  > 
> >  > > ------- Comment #10 from linux@markus-schaub.de  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 ?
> > 
> 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?

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? :)
-- 
mattia
:wq!

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

[bugme-daemon]

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


bunk@stusta.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bunk@stusta.de
             Status|NEW                         |RESOLVED
         Resolution|                            |DUPLICATE




------- Comment #12 from bunk@stusta.de  2007-06-30 16:57 -------


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


-- 
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.

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

[bugme-daemon]

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


bunk@stusta.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|DUPLICATE                   |




------- Comment #13 from bunk@stusta.de  2007-06-30 17:04 -------
Sorry, wrong field.


-- 
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.

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

[bugme-daemon]

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


bunk@stusta.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
OtherBugsDependingO|                            |7216
              nThis|                            |




-- 
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.

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

[bugme-daemon]

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





------- Comment #14 from rjwysocki@sisk.pl  2007-08-08 10:03 -------
(In reply to comment #9)
> Created an attachment (id=11884)
 --> (http://bugzilla.kernel.org/attachment.cgi?id=11884&action=view) [details]
> Restore previously used governor on a hot-replugged CPU

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


-- 
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.

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

[bugme-daemon]

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





------- Comment #15 from davej@codemonkey.org.uk  2007-08-08 21:42 -------
It's in Linus' tree for 2.6.23


-- 
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.

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

[bugme-daemon]

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


bunk@kernel.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |CLOSED
         Resolution|                            |CODE_FIX




-- 
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.