I've had it with this stuff. For months, we've had various warnings
popping up from this code (which was clearly half-baked at best when it
went in).
Until someone steps up who actually gives a damn about fixing it, can
we just rip this crap out so I stop getting mails from users who couldn't
care less about CPU hotplug anyway?
Dave
The hotplug CPU locking in cpufreq is hurrendous.
No-one seems to care enough to fix it, so just remove it
so that the 99.9% of the real world users of this code can
use cpufreq without being bothered by warnings.
Signed-off-by: Dave Jones <[email protected]>
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 86e69b7..ce99804 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -400,12 +400,10 @@ (struct cpufreq_policy * policy, const c
if (ret != 1) \
return -EINVAL; \
\
- lock_cpu_hotplug(); \
mutex_lock(&policy->lock); \
ret = __cpufreq_set_policy(policy, &new_policy); \
policy->user_policy.object = policy->object; \
mutex_unlock(&policy->lock); \
- unlock_cpu_hotplug(); \
\
return ret ? ret : count; \
}
@@ -461,8 +459,6 @@ static ssize_t store_scaling_governor (s
if (cpufreq_parse_governor(str_governor, &new_policy.policy, &new_policy.governor))
return -EINVAL;
- lock_cpu_hotplug();
-
/* Do not use cpufreq_set_policy here or the user_policy.max
will be wrongly overridden */
mutex_lock(&policy->lock);
@@ -472,8 +468,6 @@ static ssize_t store_scaling_governor (s
policy->user_policy.governor = policy->governor;
mutex_unlock(&policy->lock);
- unlock_cpu_hotplug();
-
return ret ? ret : count;
}
@@ -1235,7 +1229,6 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
*********************************************************************/
-/* Must be called with lock_cpu_hotplug held */
int __cpufreq_driver_target(struct cpufreq_policy *policy,
unsigned int target_freq,
unsigned int relation)
@@ -1261,13 +1254,11 @@ int cpufreq_driver_target(struct cpufreq
if (!policy)
return -EINVAL;
- lock_cpu_hotplug();
mutex_lock(&policy->lock);
ret = __cpufreq_driver_target(policy, target_freq, relation);
mutex_unlock(&policy->lock);
- unlock_cpu_hotplug();
cpufreq_cpu_put(policy);
return ret;
@@ -1275,7 +1266,6 @@ int cpufreq_driver_target(struct cpufreq
EXPORT_SYMBOL_GPL(cpufreq_driver_target);
/*
- * Locking: Must be called with the lock_cpu_hotplug() lock held
* when "event" is CPUFREQ_GOV_LIMITS
*/
@@ -1364,9 +1354,6 @@ int cpufreq_get_policy(struct cpufreq_po
EXPORT_SYMBOL(cpufreq_get_policy);
-/*
- * Locking: Must be called with the lock_cpu_hotplug() lock held
- */
static int __cpufreq_set_policy(struct cpufreq_policy *data, struct cpufreq_policy *policy)
{
int ret = 0;
@@ -1466,8 +1453,6 @@ int cpufreq_set_policy(struct cpufreq_po
if (!data)
return -EINVAL;
- lock_cpu_hotplug();
-
/* lock this CPU */
mutex_lock(&data->lock);
@@ -1479,7 +1464,6 @@ int cpufreq_set_policy(struct cpufreq_po
mutex_unlock(&data->lock);
- unlock_cpu_hotplug();
cpufreq_cpu_put(data);
return ret;
@@ -1503,7 +1487,6 @@ int cpufreq_update_policy(unsigned int c
if (!data)
return -ENODEV;
- lock_cpu_hotplug();
mutex_lock(&data->lock);
dprintk("updating policy for CPU %u\n", cpu);
@@ -1529,7 +1512,6 @@ int cpufreq_update_policy(unsigned int c
ret = __cpufreq_set_policy(data, &policy);
mutex_unlock(&data->lock);
- unlock_cpu_hotplug();
cpufreq_cpu_put(data);
return ret;
}
diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index c4c578d..528d16e 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -423,14 +423,12 @@ static void dbs_check_cpu(int cpu)
static void do_dbs_timer(void *data)
{
int i;
- lock_cpu_hotplug();
mutex_lock(&dbs_mutex);
for_each_online_cpu(i)
dbs_check_cpu(i);
schedule_delayed_work(&dbs_work,
usecs_to_jiffies(dbs_tuners_ins.sampling_rate));
mutex_unlock(&dbs_mutex);
- unlock_cpu_hotplug();
}
static inline void dbs_timer_init(void)
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index bf8aa45..e3bc0b1 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -424,9 +424,7 @@ static void do_dbs_timer(void *data)
INIT_WORK(&dbs_info->work, do_dbs_timer, (void *)DBS_NORMAL_SAMPLE);
if (!dbs_tuners_ins.powersave_bias ||
(unsigned long) data == DBS_NORMAL_SAMPLE) {
- lock_cpu_hotplug();
dbs_check_cpu(dbs_info);
- unlock_cpu_hotplug();
if (dbs_info->freq_lo) {
/* Setup timer for SUB_SAMPLE */
INIT_WORK(&dbs_info->work, do_dbs_timer,
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index c2ecc59..9926997 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -366,12 +366,10 @@ __exit cpufreq_stats_exit(void)
cpufreq_unregister_notifier(¬ifier_trans_block,
CPUFREQ_TRANSITION_NOTIFIER);
unregister_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
- lock_cpu_hotplug();
for_each_online_cpu(cpu) {
cpufreq_stat_cpu_callback(&cpufreq_stat_cpu_notifier, CPU_DEAD,
(void *)(long)cpu);
}
- unlock_cpu_hotplug();
}
MODULE_AUTHOR ("Zou Nan hai <[email protected]>");
diff --git a/drivers/cpufreq/cpufreq_userspace.c b/drivers/cpufreq/cpufreq_userspace.c
index a06c204..29ada96 100644
--- a/drivers/cpufreq/cpufreq_userspace.c
+++ b/drivers/cpufreq/cpufreq_userspace.c
@@ -71,7 +71,6 @@ static int cpufreq_set(unsigned int freq
dprintk("cpufreq_set for cpu %u, freq %u kHz\n", policy->cpu, freq);
- lock_cpu_hotplug();
mutex_lock(&userspace_mutex);
if (!cpu_is_managed[policy->cpu])
goto err;
@@ -94,7 +93,6 @@ static int cpufreq_set(unsigned int freq
err:
mutex_unlock(&userspace_mutex);
- unlock_cpu_hotplug();
return ret;
}
--
http://www.codemonkey.org.uk
On Wed, 1 Nov 2006, Dave Jones wrote:
>
> I've had it with this stuff. For months, we've had various warnings
> popping up from this code (which was clearly half-baked at best when it
> went in).
>
> Until someone steps up who actually gives a damn about fixing it, can
> we just rip this crap out so I stop getting mails from users who couldn't
> care less about CPU hotplug anyway?
Hmm. People _have_ given a damn, and I think you were even cc'd.
Did you take a look at the 5-patch (or was it 6?) series by Gautham R
Shenoy <[email protected]>? I'm cc'ing him, in case you weren't on the
original list, and he should talk to you ;)
Right now, for 2.6.19, I'd prefer to not touch that mess unless there are
known conditions that actually cause more problems than just stupid
warnings..
Linus
Hi,
On Wednesday, 1 November 2006 23:59, Dave Jones wrote:
> I've had it with this stuff. For months, we've had various warnings
> popping up from this code (which was clearly half-baked at best when it
> went in).
>
> Until someone steps up who actually gives a damn about fixing it, can
> we just rip this crap out so I stop getting mails from users who couldn't
> care less about CPU hotplug anyway?
Won't there be any problems with suspend on SMP vs cpufreq if this stuff is
removed?
Rafael
On Thu, 2 Nov 2006, Rafael J. Wysocki wrote:
>
> Won't there be any problems with suspend on SMP vs cpufreq if this stuff is
> removed?
Well, at least traditionally, the hotplug locking has caused more problems
than it has fixed, but at least right _now_ it seems to work.
At least for the one machine I tend to test on (Core Duo Mac Mini) a
suspend currently works at least once (which is not saying a lot,
especially as the X server I use right now is broken and doesn't bring the
screen back. Oh, well).
Linus
On Wed, Nov 01, 2006 at 03:09:52PM -0800, Linus Torvalds wrote:
> Hmm. People _have_ given a damn, and I think you were even cc'd.
You're right. In my defense, that stuff arrived the day I went
on vacation for two weeks, and I subsequently forgot all about it.
Looking back over that thread though, a few people seemed to pick a
number of holes in the patches, and there are some real gems in that
thread like.
> Really, the hotplug locking rules are fairly simple-
>
> 1. If you are in cpu hotplug callback path, don't take any lock.
Which is just great, as afair, the cpufreq locks were there _before_
someone liberally sprinkled lock_cpu_hotplug() everywhere.
> Right now, for 2.6.19, I'd prefer to not touch that mess unless there are
> known conditions that actually cause more problems than just stupid
> warnings..
>From what I can tell from looking at that thread back in August,
it went on for a while with a number of people picking holes in the
proposed patches, but there wasn't any reposted after that, and
certainly nothing that ended up in -mm.
_something_ needs to be done. If someone wants to fix it, great, but
until we see something mergable, we're left in this half-assed state
which is freaking people out.
Dave
--
http://www.codemonkey.org.uk
On Wed, 1 Nov 2006 15:09:52 -0800 (PST)
Linus Torvalds <[email protected]> wrote:
>
>
> On Wed, 1 Nov 2006, Dave Jones wrote:
> >
> > I've had it with this stuff. For months, we've had various warnings
> > popping up from this code (which was clearly half-baked at best when it
> > went in).
> >
> > Until someone steps up who actually gives a damn about fixing it, can
> > we just rip this crap out so I stop getting mails from users who couldn't
> > care less about CPU hotplug anyway?
>
> Hmm. People _have_ given a damn, and I think you were even cc'd.
I don't think Gautham cares about cpufreq-vs-hotplug per-se. He cares
about the crappy cpufreq code and the warnings it emits.
> Did you take a look at the 5-patch (or was it 6?) series by Gautham R
> Shenoy <[email protected]>? I'm cc'ing him, in case you weren't on the
> original list, and he should talk to you ;)
Gautham's work is "add lots of complex machinery so cpufreq's existing crap
works as it was supposed to". We end up with complex machinery as well as
crappy cpufreq.
The alternative is to rip all that stuff out of cpufreq and then go back
and reimplement cpufreq cpu-hotplug safety from scratch.
> Right now, for 2.6.19, I'd prefer to not touch that mess unless there are
> known conditions that actually cause more problems than just stupid
> warnings..
afaik the warnings are the only symptom.
On Wed, Nov 01, 2006 at 04:17:23PM -0800, Andrew Morton wrote:
> On Wed, 1 Nov 2006 15:09:52 -0800 (PST)
> Linus Torvalds <[email protected]> wrote:
>
> >
> >
> > On Wed, 1 Nov 2006, Dave Jones wrote:
> > >
> > > I've had it with this stuff. For months, we've had various warnings
> > > popping up from this code (which was clearly half-baked at best when it
> > > went in).
> > >
> > > Until someone steps up who actually gives a damn about fixing it, can
> > > we just rip this crap out so I stop getting mails from users who couldn't
> > > care less about CPU hotplug anyway?
> >
> > Hmm. People _have_ given a damn, and I think you were even cc'd.
>
> I don't think Gautham cares about cpufreq-vs-hotplug per-se. He cares
I *do* care about cpufreq-vs-hotplug. This was infact the main motivation to
fix the whole thing!
> about the crappy cpufreq code and the warnings it emits.
Cpufreq code is crappy with respect to hotplug "locking". The
reason may be because of the fact that cpufreq was written before
cpu-hotplug was. I'm not sure, I need to check my history books!
I'm sure, as you pointed out in one of the earlier threads, folks have
misunderstood lock_cpu_hotplug and have sprinkled it everywhere
they could. Which was not good. I think the name "lock_cpu_hotplug"
is to blame.
Anyway,the way I see it, lock_cpu_hotplug was a mechanism provided to ensure
that cpu's don't vanish/appear during some critical operation which uses the
"online cpus" state information.
So, it was upto the subsystems to make use of it responsibly.
In case of cpufreq, the "perfect" (and the only) place to "lock" hot-cpu state
is inside __cpufreq_driver_target around the call cpufreq_driver->target just
before changing frequencies on a set of cpus.
But unfortunately, the existing cpufreq-design does not allow this :(
> > Did you take a look at the 5-patch (or was it 6?) series by Gautham R
> > Shenoy <[email protected]>? I'm cc'ing him, in case you weren't on the
> > original list, and he should talk to you ;)
>
> Gautham's work is "add lots of complex machinery so cpufreq's existing crap
> works as it was supposed to". We end up with complex machinery as well as
> crappy cpufreq.
> The alternative is to rip all that stuff out of cpufreq and then go back
> and reimplement cpufreq cpu-hotplug safety from scratch.
My concern was interdependent subsystem calling lock_cpu_hotplug recursively.
cpufreq-workqueue interdependency was the only example for such an
interdependency, though it could be avoided with the right design of cpufreq.
> > Right now, for 2.6.19, I'd prefer to not touch that mess unless there are
> > known conditions that actually cause more problems than just stupid
> > warnings..
>
> afaik the warnings are the only symptom.
IMHO, we do need the feature to postpone a cpu-hotplug operation.
Question is how do we provide this feature?
a) Globally as it was (and still is!) done by "lock_cpu_hotplug"
OR
b) Per-subsystem-wise as done by workqueue_mutex in kernel/workqueue.c.
I have been experimenting with both these approaches, and they both
seem to work just fine for me.
Thanks and Regards
gautham.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"
On Wed, Nov 01, 2006 at 03:21:09PM -0800, Linus Torvalds wrote:
>
>
> On Thu, 2 Nov 2006, Rafael J. Wysocki wrote:
> >
> > Won't there be any problems with suspend on SMP vs cpufreq if this stuff is
> > removed?
>
> Well, at least traditionally, the hotplug locking has caused more problems
> than it has fixed, but at least right _now_ it seems to work.
if I am not completely mistaken I also need this for proper
suspend/resume on my X60s (CoreDUO machine) and THIS is one of
the things that DO work.
gruss
mlo
--
Dipl.-Ing. Martin Lorenz
They that can give up essential liberty
to obtain a little temporary safety
deserve neither liberty nor safety.
Benjamin Franklin
please encrypt your mail to me
GnuPG key-ID: F1AAD37D
get it here:
http://blackhole.pca.dfn.de:11371/pks/lookup?op=get&search=0xF1AAD37D
ICQ UIN: 33588107
Dave,
On Wed, Nov 01, 2006 at 06:32:50PM -0500, Dave Jones wrote:
> On Wed, Nov 01, 2006 at 03:09:52PM -0800, Linus Torvalds wrote:
>
> > Hmm. People _have_ given a damn, and I think you were even cc'd.
>
> You're right. In my defense, that stuff arrived the day I went
> on vacation for two weeks, and I subsequently forgot all about it.
> Looking back over that thread though, a few people seemed to pick a
> number of holes in the patches, and there are some real gems in that
> thread like.
Have you looked at this patchset - http://lkml.org/lkml/2006/10/26/65 ?
This is the latest patchset posted last week and I haven't seen any
comments on it.
> > Really, the hotplug locking rules are fairly simple-
> >
> > 1. If you are in cpu hotplug callback path, don't take any lock.
>
> Which is just great, as afair, the cpufreq locks were there _before_
> someone liberally sprinkled lock_cpu_hotplug() everywhere.
This one has a major *cleanup* of cpufreq code including removel
of unncessary lock_cpu_hotplug() from cpufreq.
>
> From what I can tell from looking at that thread back in August,
> it went on for a while with a number of people picking holes in the
> proposed patches, but there wasn't any reposted after that, and
> certainly nothing that ended up in -mm.
>
> _something_ needs to be done. If someone wants to fix it, great, but
> until we see something mergable, we're left in this half-assed state
> which is freaking people out.
There are two approaches - Implicit hotplug callback order-based locking
as Andrew has done or keep the current cpu hotplug "lock" semantics
and just use a better lock (RCU-based) with cpu-local access in
the fast path. Gautham's patchset does the latter. lock_cpu_hotplug() is
a misnomer, we should probably use get_cpu_hotplug() and
put_cpu_hotplug() there.
Thanks
Dipankar
Andrew,
On Wed, Nov 01, 2006 at 04:17:23PM -0800, Andrew Morton wrote:
> On Wed, 1 Nov 2006 15:09:52 -0800 (PST)
> Linus Torvalds <[email protected]> wrote:
>
> Gautham's work is "add lots of complex machinery so cpufreq's existing crap
> works as it was supposed to". We end up with complex machinery as well as
> crappy cpufreq.
>
> The alternative is to rip all that stuff out of cpufreq and then go back
> and reimplement cpufreq cpu-hotplug safety from scratch.
No matter what we do with cpufreq, cpufreq needs to protect itself against
the cpu going away. I am not too confident about the long term viability of the
implicit callback order-based locking. Sooner or later, someone
will add another complex per-cpu subsystem with calls to another
per-cpu subsystem and will violate locking order between the two
subsystems.
IMO, the right thing to do would be to convert lock_cpu_hotplug() to
a get_cpu_hotplug()/put_cpu_hotplug() type semantics and use a more
scalable implementation underneath (as opposed to a global
semaphore/mutex).
We have had some discussion on taking a look at the overall design
of the cpufreq and its cpu drivers themselves, but we need
solve this issue in the short term.
Thanks
Dipankar
On Fri, 3 Nov 2006, Dipankar Sarma wrote:
>
> IMO, the right thing to do would be to convert lock_cpu_hotplug() to
> a get_cpu_hotplug()/put_cpu_hotplug() type semantics and use a more
> scalable implementation underneath (as opposed to a global
> semaphore/mutex).
Yes, I think Gautham's patch-series basically did exactly that, no? Except
it kept the old name.
Linus
On Thu, Nov 02, 2006 at 11:30:20AM -0800, Linus Torvalds wrote:
>
>
> On Fri, 3 Nov 2006, Dipankar Sarma wrote:
> >
> > IMO, the right thing to do would be to convert lock_cpu_hotplug() to
> > a get_cpu_hotplug()/put_cpu_hotplug() type semantics and use a more
> > scalable implementation underneath (as opposed to a global
> > semaphore/mutex).
>
> Yes, I think Gautham's patch-series basically did exactly that, no? Except
> it kept the old name.
Yes, atleast that is the idea - turn it into a scalable refcount model
as opposed to a global lock.
Thanks
Dipankar