[PATCH 2.6.8-rc2] speedstep SMT support

[PATCH 2.6.8-rc2] speedstep SMT support

[Christian Hoelbling]

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

Re: [PATCH 2.6.8-rc2] speedstep SMT support

[Dominik Brodowski]

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

Re: [PATCH 2.6.8-rc2] speedstep SMT support

[Christian Hoelbling]

  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

Re: [PATCH 2.6.8-rc2] speedstep SMT support

[Dominik Brodowski]

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

Re: [PATCH 2.6.8-rc2] speedstep SMT support

[Christian Hoelbling]

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

Re: [PATCH 2.6.8-rc2] speedstep SMT support

[Dominik Brodowski]

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