speedstep SMT support
PATCH 1/1 speedstep SMT support
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
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +40 -25
@@ -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[] = {
drivers/cpufreq/cpufreq.c +11 -2
@@ -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; }
_______________________________________________
Cpufreq mailing list
[email protected]
http://www.linux.org.uk/mailman/listinfo/cpufreq
Quoted 3 lines · Christian Hoelbling
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.
Quoted 2 lines
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.
Quoted 1 line
maybe they are usefull still.
Let's see:
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c line 70
Quoted 7 lines
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +1
-0@@ -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.
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c line 264
Quoted 3 lines
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +3
-0+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 ;-)
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c line 271
Quoted 3 lines
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +3
-0+/* no transition necessary */ +if (freqs.old == freqs.new) { +set_cpus_allowed (current, cpus_allowed);
No need to do that, you already did that above.
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c line 276
Quoted 4 lines
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +1
-1for_each_cpu_mask (i, affected_cpu_map) { - freqs.cpu = i; + freqs.cpu = i;
Whitespace is corrected the "wrong" way: two tabs are preferred.
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c line 287
Quoted 4 lines
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +2
-1+/* notifiers */ for_each_cpu_mask (i, affected_cpu_map) { - freqs.cpu = i; + freqs.cpu = i;
Same here.
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c line 312
Quoted 5 lines
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +0
-5- -/* 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.
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c line 335
Quoted 4 lines
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +1
-0/* 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...
Quoted 7 lines
static unsigned int
speedstep_get(unsigned int cpu){arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +4
-1-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 */
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c line 380
Quoted 10 lines
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +10
-0+ 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.
Quoted 23 lines
static struct freq_attr* speedstep_attr[] = {
diff --unified --recursive --new-filelinux-2.6.8-rc2/drivers/cpufreq/cpufreq.clinux-2.6.8-rc2-speedstep/drivers/cpufreq/cpufreq.cdrivers/cpufreq/cpufreq.c +3
-1@@ -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)
drivers/cpufreq/cpufreq.c line 249
Quoted 10 lines
drivers/cpufreq/cpufreq.c +7
-0@@ -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
hi and thanks alot for your comments!
Quoted 5 lines · Dominik Brodowski
This I'd prefer not to be merged: a)
CONFIG_SMPdoesn't meancpu_sibling_mapexist, e.g. on non-x86 architectures
b) it "emulates" sibling-awareness, though the governors are not.
agree on both of these
Quoted 8 lines
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
Quoted 1 line · Christian Hoelbling
hi and thanks alot for your comments!
You're welcome -- and thanks for your contributions.
Quoted 4 lines
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...
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
arch/i386/kernel/cpu/cpufreq/speedstep-ich.c +20 -8
@@ -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[] = {
_______________________________________________
Cpufreq mailing list
[email protected]
http://www.linux.org.uk/mailman/listinfo/cpufreq
Quoted 5 lines · Christian Hoelbling
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