[PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[shin, jacob]

powernow-k8: Let cpufreq driver handle affected CPUs

Let the cpufreq driver manage AMD Dual-Core CPUs being tied together.

Since cpufreq driver's affected CPUs data, cpufreq_policy->cpus, already
knows about which cores are tied together, powernow driver does not have
keep its internal data for every core.  (even a pointer.. it will never
be called on)  Telling cpufreq driver about cpu_core_map at init time is
sufficient.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>

---

--- linux.orig/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	2006-02-27 06:40:23.000000000 -0600
+++ linux/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	2006-02-27 09:53:10.000000000 -0600
@@ -45,7 +45,7 @@
 
 #define PFX "powernow-k8: "
 #define BFX PFX "BIOS error: "
-#define VERSION "version 1.60.0"
+#define VERSION "version 1.60.1"
 #include "powernow-k8.h"
 
 /* serialize freq changes  */
@@ -909,7 +909,6 @@ static int powernowk8_target(struct cpuf
 	u32 checkvid = data->currvid;
 	unsigned int newstate;
 	int ret = -EIO;
-	int i;
 
 	/* only run on specific CPU from here on */
 	oldmask = current->cpus_allowed;
@@ -957,10 +956,8 @@ static int powernowk8_target(struct cpuf
 	}
 
 	/* Update all the fid/vids of our siblings */
-	for_each_cpu_mask(i, cpu_core_map[pol->cpu]) {
-		powernow_data[i]->currvid = data->currvid;
-		powernow_data[i]->currfid = data->currfid;
-	}	
+	data->currvid = data->currvid;
+	data->currfid = data->currfid;
 	up(&fidvid_sem);
 
 	pol->cur = find_khz_freq_from_fid(data->currfid);
@@ -984,7 +981,7 @@ static int __cpuinit powernowk8_cpu_init
 {
 	struct powernow_k8_data *data;
 	cpumask_t oldmask = CPU_MASK_ALL;
-	int rc, i;
+	int rc;
 
 	if (!cpu_online(pol->cpu))
 		return -ENODEV;
@@ -1070,9 +1067,7 @@ static int __cpuinit powernowk8_cpu_init
 	printk("cpu_init done, current fid 0x%x, vid 0x%x\n",
 	       data->currfid, data->currvid);
 
-	for_each_cpu_mask(i, cpu_core_map[pol->cpu]) {
-		powernow_data[i] = data;
-	}
+	powernow_data[pol->cpu] = data;
 
 	return 0;

Re: [PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[Andi Kleen]

On Tuesday 28 February 2006 02:54, shin, jacob wrote:
> powernow-k8: Let cpufreq driver handle affected CPUs
> 
> Let the cpufreq driver manage AMD Dual-Core CPUs being tied together.
> 
> Since cpufreq driver's affected CPUs data, cpufreq_policy->cpus, already
> knows about which cores are tied together, powernow driver does not have
> keep its internal data for every core.  (even a pointer.. it will never
> be called on)  Telling cpufreq driver about cpu_core_map at init time is
> sufficient.

Nice - it even simplifies the code.  And makes sense.

How much regression testing did you do? Is it well tested enough for 2.6.16? 

-Andi

RE: [PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[shin, jacob]

On Tuesday, February 28, 2006 6:16 AM Andi Kleen wrote:

> How much regression testing did you do? Is it well tested enough for 2.6.16?

I tested on machine running 2 Dual-Core AMD Opteron processors.
I tried 1P, SMP, SMP + Hotplug configurations on x86_64 side.
i386 should be fine too, since we share the powernow code.

These changes to should really be transparent, since it simply
does away with keeping track of data that we never use.

I am confidant enough to push for 2.6.16.

Thanks!

-Jacob Shin
AMD, Inc.

Re: [PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[Dave Jones]

On Mon, Feb 27, 2006 at 07:54:36PM -0600, shin, jacob wrote:
 > +	data->currvid = data->currvid;
 > +	data->currfid = data->currfid;

ummm ?

		Dave

RE: [PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[shin, jacob]

oops,
meant to minus those two lines,
will resubmit very soon.

-----Original Message-----
From: Dave Jones [mailto:davej@redhat.com] 
Sent: Tuesday, February 28, 2006 10:23 AM
To: shin, jacob
Cc: Andi Kleen; cpufreq@lists.linux.org.uk; Langsdorf, Mark
Subject: Re: [PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

On Mon, Feb 27, 2006 at 07:54:36PM -0600, shin, jacob wrote:
 > +	data->currvid = data->currvid;
 > +	data->currfid = data->currfid;

ummm ?

		Dave

RE: [PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[shin, jacob]

On Tuesday, February 28, 2006 10:23 AM Dave Jones wrote:
> On Mon, Feb 27, 2006 at 07:54:36PM -0600, shin, jacob wrote:
>  > +	data->currvid = data->currvid;
>  > +	data->currfid = data->currfid;

Sorry about that, here we go again..

powernow-k8: Let cpufreq driver handle affected CPUs

Let the cpufreq driver manage AMD Dual-Core CPUs being tied together.

Since cpufreq driver's affected CPUs data, cpufreq_policy->cpus, already
knows about which cores are tied together, powernow driver does not have
keep its internal data for every core.  (even a pointer.. it will never
be called on)  Telling cpufreq driver about cpu_core_map at init time is
sufficient.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>

---

--- linux.orig/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	2006-02-27 06:40:23.000000000 -0600
+++ linux/arch/i386/kernel/cpu/cpufreq/powernow-k8.c	2006-02-28 04:38:30.000000000 -0600
@@ -45,7 +45,7 @@
 
 #define PFX "powernow-k8: "
 #define BFX PFX "BIOS error: "
-#define VERSION "version 1.60.0"
+#define VERSION "version 1.60.1"
 #include "powernow-k8.h"
 
 /* serialize freq changes  */
@@ -909,7 +909,6 @@ static int powernowk8_target(struct cpuf
 	u32 checkvid = data->currvid;
 	unsigned int newstate;
 	int ret = -EIO;
-	int i;
 
 	/* only run on specific CPU from here on */
 	oldmask = current->cpus_allowed;
@@ -955,12 +954,6 @@ static int powernowk8_target(struct cpuf
 		up(&fidvid_sem);
 		goto err_out;
 	}
-
-	/* Update all the fid/vids of our siblings */
-	for_each_cpu_mask(i, cpu_core_map[pol->cpu]) {
-		powernow_data[i]->currvid = data->currvid;
-		powernow_data[i]->currfid = data->currfid;
-	}	
 	up(&fidvid_sem);
 
 	pol->cur = find_khz_freq_from_fid(data->currfid);
@@ -984,7 +977,7 @@ static int __cpuinit powernowk8_cpu_init
 {
 	struct powernow_k8_data *data;
 	cpumask_t oldmask = CPU_MASK_ALL;
-	int rc, i;
+	int rc;
 
 	if (!cpu_online(pol->cpu))
 		return -ENODEV;
@@ -1070,9 +1063,7 @@ static int __cpuinit powernowk8_cpu_init
 	printk("cpu_init done, current fid 0x%x, vid 0x%x\n",
 	       data->currfid, data->currvid);
 
-	for_each_cpu_mask(i, cpu_core_map[pol->cpu]) {
-		powernow_data[i] = data;
-	}
+	powernow_data[pol->cpu] = data;
 
 	return 0;
 

RE: [PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[shin, jacob]

On Tuesday, February 28, 2006 10:51 AM shin, jacob wrote:
> On Tuesday, February 28, 2006 10:23 AM Dave Jones wrote:
>> On Mon, Feb 27, 2006 at 07:54:36PM -0600, shin, jacob wrote:
>>  > +	data->currvid = data->currvid;
>>  > +	data->currfid = data->currfid;
> 
> Sorry about that, here we go again..

The test results should still be valid.  But will test again
regardless.

-jacob

RE: [PATCH] [1/2] cpufreq: Fix handling for CPU hotplug

[shin, jacob]

On Tuesday, February 28, 2006 10:54 AM shin, jacob wrote:

> The test results should still be valid.  But will test again
> regardless.

Did more testing w/ the updated patch.  Everything still looks
good as it should.. w/ or w/o hotplug, smp.. etc.

again, the changes should be transparent the the rest of the
world.

Thanks!

-jacob