* [PATCH 2.6.8-rc2] speedstep SMT support
@ 2004-07-28 14:31 Christian Hoelbling
2004-07-28 17:20 ` Dominik Brodowski
0 siblings, 1 reply; 6+ messages in thread
From: Christian Hoelbling @ 2004-07-28 14:31 UTC (permalink / raw)
To: cpufreq
[-- Attachment #1: Type: text/plain, Size: 774 bytes --]
hi list,
i have noticed that the most recent vanilla kernel oopses on a P4M-HT
(because only one CPU is registered in sysfs but 2 are informed of a
change of state). also there are some returns out of subroutines before
set_cpus_allowed() is performed to restore the mask prior to entering
the subroutine (this should not matter on uniprocessor systems, but
still...).
this patch should fix these 2 issues. for the first one, it registerers
all logical cpu's and a tiny modification is made in cpufreq.c to
perform a policy change on all siblings.
i noticed, that this should be superseeded by the patches that dominik
has submitted over a month ago. sorry, since i didn't see them in the
kernel i did my own thing. maybe they are usefull still.
christian
[-- Attachment #2: speedstep_patch.2.6.8-rc2.diff --]
[-- Type: text/x-patch, Size: 5492 bytes --]
diff --unified --recursive --new-file linux-2.6.8-rc2/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.8-rc2-speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
--- linux-2.6.8-rc2/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c 2004-07-26 10:13:07.000000000 +0000
+++ linux-2.6.8-rc2-speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c 2004-07-28 11:10:09.000000000 +0000
@@ -67,6 +67,7 @@
/**
* speedstep_set_state - set the SpeedStep state
* @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
+ * @notify: whether to call cpufreq_notify_transition for CPU speed changes
*
* Tries to change the SpeedStep state.
*/
@@ -239,22 +240,16 @@
unsigned int target_freq,
unsigned int relation)
{
- unsigned int newstate = 0;
+ unsigned int newstate = 0;
struct cpufreq_freqs freqs;
cpumask_t cpus_allowed, affected_cpu_map;
int i;
+
if (cpufreq_frequency_table_target(policy, &speedstep_freqs[0], target_freq, relation, &newstate))
return -EINVAL;
- /* no transition necessary */
- if (freqs.old == freqs.new)
- return 0;
-
- freqs.old = speedstep_get_processor_frequency(speedstep_processor);
- freqs.new = speedstep_freqs[newstate].frequency;
- freqs.cpu = policy->cpu;
-
+ /* switch to physical CPU where state is to be changed*/
cpus_allowed = current->cpus_allowed;
/* only run on CPU to be set, or on its sibling */
@@ -263,22 +258,33 @@
#else
affected_cpu_map = cpumask_of_cpu(policy->cpu);
#endif
+
+ set_cpus_allowed(current, affected_cpu_map);
+ freqs.old = speedstep_get_processor_frequency(speedstep_processor);
+ set_cpus_allowed(current, cpus_allowed);
+
+ freqs.new = speedstep_freqs[newstate].frequency;
+ freqs.cpu = policy->cpu;
+
+ /* no transition necessary */
+ if (freqs.old == freqs.new) {
+ set_cpus_allowed(current, cpus_allowed);
+ return 0;
+ }
for_each_cpu_mask(i, affected_cpu_map) {
- freqs.cpu = i;
+ freqs.cpu = i;
cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
}
- /* switch to physical CPU where state is to be changed */
+ /* do the transition */
set_cpus_allowed(current, affected_cpu_map);
-
speedstep_set_state(newstate);
-
- /* allow to be run on all CPUs */
set_cpus_allowed(current, cpus_allowed);
+ /* notifiers */
for_each_cpu_mask(i, affected_cpu_map) {
- freqs.cpu = i;
+ freqs.cpu = i;
cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
}
@@ -305,11 +311,6 @@
unsigned int speed;
cpumask_t cpus_allowed,affected_cpu_map;
-
- /* capability check */
- if (policy->cpu != 0) /* FIXME: better support for SMT in cpufreq core. Up until then, it's better to register only one CPU */
- return -ENODEV;
-
/* only run on CPU to be set, or on its sibling */
cpus_allowed = current->cpus_allowed;
#ifdef CONFIG_SMP
@@ -317,21 +318,22 @@
#else
affected_cpu_map = cpumask_of_cpu(policy->cpu);
#endif
- set_cpus_allowed(current, affected_cpu_map);
/* detect low and high frequency */
+ set_cpus_allowed(current, affected_cpu_map);
result = speedstep_get_freqs(speedstep_processor,
&speedstep_freqs[SPEEDSTEP_LOW].frequency,
&speedstep_freqs[SPEEDSTEP_HIGH].frequency,
&speedstep_set_state);
- if (result) {
- set_cpus_allowed(current, cpus_allowed);
+ set_cpus_allowed(current, cpus_allowed);
+ if (result)
return result;
- }
/* get current speed setting */
+ set_cpus_allowed(current, affected_cpu_map);
speed = speedstep_get_processor_frequency(speedstep_processor);
set_cpus_allowed(current, cpus_allowed);
+
if (!speed)
return -EIO;
@@ -362,7 +364,20 @@
static unsigned int speedstep_get(unsigned int cpu)
{
- return speedstep_get_processor_frequency(speedstep_processor);
+ unsigned int speed;
+ cpumask_t cpus_allowed,affected_cpu_map;
+
+ /* only run on CPU to be set, or on its sibling */
+ cpus_allowed = current->cpus_allowed;
+#ifdef CONFIG_SMP
+ affected_cpu_map = cpu_sibling_map[cpu];
+#else
+ affected_cpu_map = cpumask_of_cpu(cpu);
+#endif
+ set_cpus_allowed(current, affected_cpu_map);
+ speed=speedstep_get_processor_frequency(speedstep_processor);
+ set_cpus_allowed(current, cpus_allowed);
+ return speed;
}
static struct freq_attr* speedstep_attr[] = {
diff --unified --recursive --new-file linux-2.6.8-rc2/drivers/cpufreq/cpufreq.c linux-2.6.8-rc2-speedstep/drivers/cpufreq/cpufreq.c
--- linux-2.6.8-rc2/drivers/cpufreq/cpufreq.c 2004-07-26 10:13:07.000000000 +0000
+++ linux-2.6.8-rc2-speedstep/drivers/cpufreq/cpufreq.c 2004-07-28 12:35:52.000000000 +0000
@@ -218,7 +218,7 @@
/**
- * store_scaling_governor - store policy for the specified CPU
+ * store_scaling_governor - store policy for the specified CPU and all of its siblings
*/
static ssize_t store_scaling_governor (struct cpufreq_policy * policy,
const char *buf, size_t count)
@@ -226,6 +226,8 @@
unsigned int ret = -EINVAL;
char str_governor[16];
struct cpufreq_policy new_policy;
+ cpumask_t affected_cpu_map;
+ int i;
ret = cpufreq_get_policy(&new_policy, policy->cpu);
if (ret)
@@ -238,8 +240,15 @@
if (cpufreq_parse_governor(str_governor, &new_policy.policy, &new_policy.governor))
return -EINVAL;
+#ifdef CONFIG_SMP
+ affected_cpu_map = cpu_sibling_map[new_policy.cpu];
+ for_each_cpu_mask(i, affected_cpu_map) {
+ new_policy.cpu = i;
+ ret = ret & cpufreq_set_policy(&new_policy);
+ }
+#else
ret = cpufreq_set_policy(&new_policy);
-
+#endif
return ret ? ret : count;
}
[-- Attachment #3: Type: text/plain, Size: 143 bytes --]
_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2.6.8-rc2] speedstep SMT support
2004-07-28 14:31 [PATCH 2.6.8-rc2] speedstep SMT support Christian Hoelbling
@ 2004-07-28 17:20 ` Dominik Brodowski
2004-07-29 12:19 ` Christian Hoelbling
0 siblings, 1 reply; 6+ messages in thread
From: Dominik Brodowski @ 2004-07-28 17:20 UTC (permalink / raw)
To: Christian.Hoelbling; +Cc: cpufreq
On Wed, Jul 28, 2004 at 02:31:58PM +0000, Christian Hoelbling wrote:
> i have noticed that the most recent vanilla kernel oopses on a P4M-HT
> (because only one CPU is registered in sysfs but 2 are informed of a
> change of state).
Yes, please see my patch sent to this list on July 19th adressing this
issue. Hopefully it gets into 2.6.8.
> this patch should fix these 2 issues. for the first one, it registerers
> all logical cpu's
I don't like this: as both siblings can only be changed at the same time,
and all changes affect both siblings, only one instance should be
registered. The current cpufreq core lacks proper support for that, but --
as you have seen -- I'm working on it. Until then, I'd prefer to work around
the issues, and simply not register a second instance.
> maybe they are usefull still.
Let's see:
> --- linux-2.6.8-rc2/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c 2004-07-26 10:13:07.000000000 +0000
> +++ linux-2.6.8-rc2-speedstep/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c 2004-07-28 11:10:09.000000000 +0000
> @@ -67,6 +67,7 @@
> /**
> * speedstep_set_state - set the SpeedStep state
> * @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
> + * @notify: whether to call cpufreq_notify_transition for CPU speed changes
There's no "notify" argument to speedstep_set_state, AFAICS.
> + set_cpus_allowed(current, affected_cpu_map);
> + freqs.old = speedstep_get_processor_frequency(speedstep_processor);
> + set_cpus_allowed(current, cpus_allowed);
I agree that this is "cleaner"... as there's only one physical CPU and both
[or even more...] siblings run at the same speed, it shouldn't matter. As
it's cleaner, though, I like this change. However, why not simply call
speedstep_get(cpu), which needs the same change [and you already did the
change there, too...]. Saving 1 line of code ;-)
> + /* no transition necessary */
> + if (freqs.old == freqs.new) {
> + set_cpus_allowed(current, cpus_allowed);
No need to do that, you already did that above.
>
> for_each_cpu_mask(i, affected_cpu_map) {
> - freqs.cpu = i;
> + freqs.cpu = i;
Whitespace is corrected the "wrong" way: two tabs are preferred.
> + /* notifiers */
> for_each_cpu_mask(i, affected_cpu_map) {
> - freqs.cpu = i;
> + freqs.cpu = i;
Same here.
> -
> - /* capability check */
> - if (policy->cpu != 0) /* FIXME: better support for SMT in cpufreq core. Up until then, it's better to register only one CPU */
> - return -ENODEV;
> -
Please don't remove this, as stated above. Please use my workaround patch
instead.
> /* get current speed setting */
> + set_cpus_allowed(current, affected_cpu_map);
> speed = speedstep_get_processor_frequency(speedstep_processor);
> set_cpus_allowed(current, cpus_allowed);
Please use speedstep_get() here as well, as it'll save an additional two
lines of code ;-) ... no, that's not the real reason: it is that if one
piece of code already provides that functionality, doubling it should be
avoided to remove maintenance overhead. And so it'll simplify me writing the
proper SMT handling...
> static unsigned int speedstep_get(unsigned int cpu)
> {
> - return speedstep_get_processor_frequency(speedstep_processor);
> + unsigned int speed;
> + cpumask_t cpus_allowed,affected_cpu_map;
> +
> + /* only run on CPU to be set, or on its sibling */
> + cpus_allowed = current->cpus_allowed;
> +#ifdef CONFIG_SMP
> + affected_cpu_map = cpu_sibling_map[cpu];
> +#else
> + affected_cpu_map = cpumask_of_cpu(cpu);
> +#endif
> + set_cpus_allowed(current, affected_cpu_map);
> + speed=speedstep_get_processor_frequency(speedstep_processor);
> + set_cpus_allowed(current, cpus_allowed);
> + return speed;
Looks good, as stated above.
> static struct freq_attr* speedstep_attr[] = {
> diff --unified --recursive --new-file linux-2.6.8-rc2/drivers/cpufreq/cpufreq.c linux-2.6.8-rc2-speedstep/drivers/cpufreq/cpufreq.c
> --- linux-2.6.8-rc2/drivers/cpufreq/cpufreq.c 2004-07-26 10:13:07.000000000 +0000
> +++ linux-2.6.8-rc2-speedstep/drivers/cpufreq/cpufreq.c 2004-07-28 12:35:52.000000000 +0000
> @@ -218,7 +218,7 @@
>
>
> /**
> - * store_scaling_governor - store policy for the specified CPU
> + * store_scaling_governor - store policy for the specified CPU and all of its siblings
> */
> static ssize_t store_scaling_governor (struct cpufreq_policy * policy,
> const char *buf, size_t count)
> @@ -226,6 +226,8 @@
> unsigned int ret = -EINVAL;
> char str_governor[16];
> struct cpufreq_policy new_policy;
> + cpumask_t affected_cpu_map;
> + int i;
>
> ret = cpufreq_get_policy(&new_policy, policy->cpu);
> if (ret)
> @@ -238,8 +240,15 @@
> if (cpufreq_parse_governor(str_governor, &new_policy.policy, &new_policy.governor))
> return -EINVAL;
>
> +#ifdef CONFIG_SMP
> + affected_cpu_map = cpu_sibling_map[new_policy.cpu];
> + for_each_cpu_mask(i, affected_cpu_map) {
> + new_policy.cpu = i;
> + ret = ret & cpufreq_set_policy(&new_policy);
> + }
> +#else
This I'd prefer not to be merged:
a) CONFIG_SMP doesn't mean cpu_sibling_map exist, e.g. on non-x86 architectures
b) it "emulates" sibling-awareness, though the governors are not.
Instead, I'd prefer if we keep up with the "only-register-one-CPU" in the
speedstep-ich case until a proper SMT patchkit is applied.
However, for the other changes: could you consider my opinions, maybe update it
and resend it, please?
Thanks,
Dominik
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2.6.8-rc2] speedstep SMT support
2004-07-28 17:20 ` Dominik Brodowski
@ 2004-07-29 12:19 ` Christian Hoelbling
2004-07-29 16:34 ` Dominik Brodowski
0 siblings, 1 reply; 6+ messages in thread
From: Christian Hoelbling @ 2004-07-29 12:19 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: cpufreq
hi and thanks alot for your comments!
Dominik Brodowski wrote:
>
>This I'd prefer not to be merged:
>a) CONFIG_SMP doesn't mean cpu_sibling_map exist, e.g. on non-x86 architectures
>b) it "emulates" sibling-awareness, though the governors are not.
>
>
agree on both of these
>Instead, I'd prefer if we keep up with the "only-register-one-CPU" in the
>speedstep-ich case until a proper SMT patchkit is applied.
>
>However, for the other changes: could you consider my opinions, maybe update it
>and resend it, please?
>
>
>
i will gladly do. however, i am a bit afraid of missing some patch -
could you send me a diff of your source tree to a recent vanilla source
so i know where to start from? or is there some repository somewhere
that i don't know about where the cpufreq development sources are kept?
thanks alot,
chris
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.8-rc2] speedstep SMT support
2004-07-29 12:19 ` Christian Hoelbling
@ 2004-07-29 16:34 ` Dominik Brodowski
2004-07-30 11:36 ` Christian Hoelbling
0 siblings, 1 reply; 6+ messages in thread
From: Dominik Brodowski @ 2004-07-29 16:34 UTC (permalink / raw)
To: holbling; +Cc: cpufreq
On Thu, Jul 29, 2004 at 12:19:00PM +0000, Christian Hoelbling wrote:
> hi and thanks alot for your comments!
You're welcome -- and thanks for your contributions.
> i will gladly do. however, i am a bit afraid of missing some patch -
> could you send me a diff of your source tree to a recent vanilla source
> so i know where to start from? or is there some repository somewhere
> that i don't know about where the cpufreq development sources are kept?
The current[*] diff between what's in Linus' tree and what's in the "cpufreq
development sources" a.k.a. cpufreq-bk is available at
http://www.codemonkey.org.uk/projects/bitkeeper/cpufreq/
Without cc'ing this list I'll provide you with a cpufreq-updates-since-2.6.7
diff in just a moment...
Dominik
[*] updated daily... so an offset of 23:59 is possible...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.8-rc2] speedstep SMT support
2004-07-29 16:34 ` Dominik Brodowski
@ 2004-07-30 11:36 ` Christian Hoelbling
2004-07-30 15:46 ` Dominik Brodowski
0 siblings, 1 reply; 6+ messages in thread
From: Christian Hoelbling @ 2004-07-30 11:36 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: cpufreq
[-- Attachment #1: Type: text/plain, Size: 198 bytes --]
hi,
here is the new patch w.r.t. the latest cpufreq-development snapshot
(thanks for pointing me to it!). it should make speedstep-ich SMP save
(altough that is hypothetical for now).
chris
[-- Attachment #2: speedstep_SMP.diff --]
[-- Type: text/x-patch, Size: 2143 bytes --]
diff --unified --recursive --new-file linux-2.6.8-rc2-cpufreq/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c linux-2.6.8-rc2-cpufreq.my/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
--- linux-2.6.8-rc2-cpufreq/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c 2004-07-26 10:13:07.000000000 +0000
+++ linux-2.6.8-rc2-cpufreq.my/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c 2004-07-30 11:00:59.000000000 +0000
@@ -247,14 +247,14 @@
if (cpufreq_frequency_table_target(policy, &speedstep_freqs[0], target_freq, relation, &newstate))
return -EINVAL;
+ freqs.old = speedstep_get(policy->cpu);
+ freqs.new = speedstep_freqs[newstate].frequency;
+ freqs.cpu = policy->cpu;
+
/* no transition necessary */
if (freqs.old == freqs.new)
return 0;
- freqs.old = speedstep_get_processor_frequency(speedstep_processor);
- freqs.new = speedstep_freqs[newstate].frequency;
- freqs.cpu = policy->cpu;
-
cpus_allowed = current->cpus_allowed;
/* only run on CPU to be set, or on its sibling */
@@ -324,14 +324,13 @@
&speedstep_freqs[SPEEDSTEP_LOW].frequency,
&speedstep_freqs[SPEEDSTEP_HIGH].frequency,
&speedstep_set_state);
+ set_cpus_allowed(current, cpus_allowed);
if (result) {
- set_cpus_allowed(current, cpus_allowed);
return result;
}
/* get current speed setting */
- speed = speedstep_get_processor_frequency(speedstep_processor);
- set_cpus_allowed(current, cpus_allowed);
+ speed = speedstep_get(policy->cpu);
if (!speed)
return -EIO;
@@ -362,7 +361,20 @@
static unsigned int speedstep_get(unsigned int cpu)
{
- return speedstep_get_processor_frequency(speedstep_processor);
+ unsigned int speed;
+ cpumask_t cpus_allowed,affected_cpu_map;
+
+ /* only run on CPU to be set, or on its sibling */
+ cpus_allowed = current->cpus_allowed;
+#ifdef CONFIG_SMP
+ affected_cpu_map = cpu_sibling_map[cpu];
+#else
+ affected_cpu_map = cpumask_of_cpu(cpu);
+#endif
+ set_cpus_allowed(current, affected_cpu_map);
+ speed=speedstep_get_processor_frequency(speedstep_processor);
+ set_cpus_allowed(current, cpus_allowed);
+ return speed;
}
static struct freq_attr* speedstep_attr[] = {
[-- Attachment #3: Type: text/plain, Size: 143 bytes --]
_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2.6.8-rc2] speedstep SMT support
2004-07-30 11:36 ` Christian Hoelbling
@ 2004-07-30 15:46 ` Dominik Brodowski
0 siblings, 0 replies; 6+ messages in thread
From: Dominik Brodowski @ 2004-07-30 15:46 UTC (permalink / raw)
To: Christian.Hoelbling, davej; +Cc: cpufreq
On Fri, Jul 30, 2004 at 11:36:34AM +0000, Christian Hoelbling wrote:
> hi,
>
> here is the new patch w.r.t. the latest cpufreq-development snapshot
> (thanks for pointing me to it!). it should make speedstep-ich SMP save
> (altough that is hypothetical for now).
Patch looks good, so: Dave, could you please merge it?
Thanks,
Dominik
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-07-30 15:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-07-28 14:31 [PATCH 2.6.8-rc2] speedstep SMT support Christian Hoelbling
2004-07-28 17:20 ` Dominik Brodowski
2004-07-29 12:19 ` Christian Hoelbling
2004-07-29 16:34 ` Dominik Brodowski
2004-07-30 11:36 ` Christian Hoelbling
2004-07-30 15:46 ` Dominik Brodowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.