Patchwork cpufreq fix timer teardown in conservative governor (2.6.30-rc2)

login
register
mail settings
Submitter Mathieu Desnoyers
Date April 24, 2009, 4:35 a.m.
Message ID <20090424043546.GB8091@Krystal>
Download mbox | patch
Permalink /patch/19753/
State New, archived
Headers show

Comments

Mathieu Desnoyers - April 24, 2009, 4:35 a.m.
The problem is that dbs_timer_exit() uses cancel_delayed_work() when it should
use cancel_delayed_work_sync(). cancel_delayed_work() does not wait for the
workqueue handler to exit.

The ondemand governor does not seem to be affected because the
"if (!dbs_info->enable)" check at the beginning of the workqueue handler returns
immediately without rescheduling the work. The conservative governor in
2.6.30-rc has the same check as the ondemand governor, which makes things
usually run smoothly. However, if the governor is quickly stopped and then
started, this could lead to the following race :

dbs_enable could be reenabled and multiple do_dbs_timer handlers would run.
This is why a synchronized teardown is required.

The following patch applies to 2.6.30-rc2.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: gregkh@suse.de
CC: stable@kernel.org
CC: cpufreq@vger.kernel.org
CC: Ingo Molnar <mingo@elte.hu>
CC: rjw@sisk.pl
CC: Ben Slusky <sluskyb@paranoiacs.org>
CC: Dave Jones <davej@redhat.com>
---
 drivers/cpufreq/cpufreq_conservative.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Len Brown - April 24, 2009, 6:18 a.m.
Somebody please remind me why we are spending effort to 
maintain the conservative governor instead of deleting it.

thanks,
Len Brown, Intel Open Source Technology Center

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathieu Desnoyers - April 24, 2009, 4:12 p.m.
* Len Brown (lenb@kernel.org) wrote:
> Somebody please remind me why we are spending effort to 
> maintain the conservative governor instead of deleting it.
> 

Documentation/cpu-freq/governors.txt

"The CPUfreq governor "conservative", much like the "ondemand"
governor, sets the CPU depending on the current usage.  It differs in
behaviour in that it gracefully increases and decreases the CPU speed
rather than jumping to max speed the moment there is any load on the
CPU.  This behaviour more suitable in a battery powered environment."

So better battery usage seems to be the reason why conservative lives.

Mathieu


> thanks,
> Len Brown, Intel Open Source Technology Center
>
Henrique de Moraes Holschuh - April 26, 2009, 2:47 p.m.
On Fri, 24 Apr 2009, Mathieu Desnoyers wrote:
> * Len Brown (lenb@kernel.org) wrote:
> > Somebody please remind me why we are spending effort to 
> > maintain the conservative governor instead of deleting it.
> 
> Documentation/cpu-freq/governors.txt
> 
> "The CPUfreq governor "conservative", much like the "ondemand"
> governor, sets the CPU depending on the current usage.  It differs in
> behaviour in that it gracefully increases and decreases the CPU speed
> rather than jumping to max speed the moment there is any load on the
> CPU.  This behaviour more suitable in a battery powered environment."
> 
> So better battery usage seems to be the reason why conservative lives.

Yeah, but the question is: is it really better in practice? race-to-idle
works better with ondemand.  Note: that needs to be answered not just for
the current crop of mobile processors, but also for at least stuff as old as
the Pentium M and Pentium 4 M.

What it _does_ help, is in broken !@#$ hardware that makes a lot of noise
due to "singing capacitors" if you use ondemand (because conservative will
make less noise as it causes more smooth transitions).  NOHZ helped a great
deal there, too.

I don't know if there are battery environments where a harsher work profile
by the CPU are a bad idea.  If there are any, conservative will also help
there.
Mathieu Desnoyers - April 26, 2009, 4:31 p.m.
* Henrique de Moraes Holschuh (hmh@hmh.eng.br) wrote:
> On Fri, 24 Apr 2009, Mathieu Desnoyers wrote:
> > * Len Brown (lenb@kernel.org) wrote:
> > > Somebody please remind me why we are spending effort to 
> > > maintain the conservative governor instead of deleting it.
> > 
> > Documentation/cpu-freq/governors.txt
> > 
> > "The CPUfreq governor "conservative", much like the "ondemand"
> > governor, sets the CPU depending on the current usage.  It differs in
> > behaviour in that it gracefully increases and decreases the CPU speed
> > rather than jumping to max speed the moment there is any load on the
> > CPU.  This behaviour more suitable in a battery powered environment."
> > 
> > So better battery usage seems to be the reason why conservative lives.
> 
> Yeah, but the question is: is it really better in practice? race-to-idle
> works better with ondemand.  Note: that needs to be answered not just for
> the current crop of mobile processors, but also for at least stuff as old as
> the Pentium M and Pentium 4 M.
> 
> What it _does_ help, is in broken !@#$ hardware that makes a lot of noise
> due to "singing capacitors" if you use ondemand (because conservative will
> make less noise as it causes more smooth transitions).  NOHZ helped a great
> deal there, too.
> 

I effectively have such singing capacitors on my laptop, and I still
have good ears.

> I don't know if there are battery environments where a harsher work profile
> by the CPU are a bad idea.  If there are any, conservative will also help
> there.
> 

Good question. We might also consider that anyway code duplication
between ondemand and conservative is a bad thing. Those look so similar
that part of them should probably be merged, with a sysfs flag and
kernel parameter to select the behavior.

Mathieu


> -- 
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh

Patch

Index: linux-2.6-lttng/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-2.6-lttng.orig/drivers/cpufreq/cpufreq_conservative.c	2009-04-23 23:22:15.000000000 -0400
+++ linux-2.6-lttng/drivers/cpufreq/cpufreq_conservative.c	2009-04-23 23:24:38.000000000 -0400
@@ -91,6 +91,9 @@  static unsigned int dbs_enable;	/* numbe
  * (like __cpufreq_driver_target()) is being called with dbs_mutex taken, then
  * cpu_hotplug lock should be taken before that. Note that cpu_hotplug lock
  * is recursive for the same process. -Venki
+ * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the dbs_mutex, because it
+ * would deadlock with cancel_delayed_work_sync(), which is needed for proper
+ * raceless workqueue teardown.
  */
 static DEFINE_MUTEX(dbs_mutex);
 
@@ -542,7 +545,7 @@  static inline void dbs_timer_init(struct
 static inline void dbs_timer_exit(struct cpu_dbs_info_s *dbs_info)
 {
 	dbs_info->enable = 0;
-	cancel_delayed_work(&dbs_info->work);
+	cancel_delayed_work_sync(&dbs_info->work);
 }
 
 static int cpufreq_governor_dbs(struct cpufreq_policy *policy,