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

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

[shin, jacob]

[-- Attachment #1: Type: text/plain, Size: 3604 bytes --]

cpufreq: Fix handling for CPU hotplug

This patch adds proper logic to cpufreq driver in order to handle
CPU Hotplug.

When CPUs go on/offline, the affected CPUs data, cpufreq_policy->cpus,
is not updated properly. This causes sysfs directories and symlinks to
be in an incorrect state after few CPU on/offlines.

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

---

--- linux.orig/drivers/cpufreq/cpufreq.c	2006-02-24 04:46:24.000000000 -0600
+++ linux/drivers/cpufreq/cpufreq.c	2006-02-27 12:48:33.000000000 -0600
@@ -6,6 +6,8 @@
  *
  *  Oct 2005 - Ashok Raj <ashok.raj@intel.com>
  *         		Added handling for CPU hotplug
+ *  Feb 2006 - Jacob Shin <jacob.shin@amd.com>
+ *                      Fix handling for CPU hotplug -- affected CPUs
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -573,8 +575,12 @@ static int cpufreq_add_dev (struct sys_d
 	struct cpufreq_policy new_policy;
 	struct cpufreq_policy *policy;
 	struct freq_attr **drv_attr;
+	struct sys_device *cpu_sys_dev;
 	unsigned long flags;
 	unsigned int j;
+#ifdef CONFIG_SMP
+	struct cpufreq_policy *managed_policy;
+#endif
 
 	if (cpu_is_offline(cpu))
 		return 0;
@@ -587,8 +593,7 @@ static int cpufreq_add_dev (struct sys_d
 	 * CPU because it is in the same boat. */
 	policy = cpufreq_cpu_get(cpu);
 	if (unlikely(policy)) {
-		dprintk("CPU already managed, adding link\n");
-		sysfs_create_link(&sys_dev->kobj, &policy->kobj, "cpufreq");
+		cpufreq_cpu_put(policy);
 		cpufreq_debug_enable_ratelimit();
 		return 0;
 	}
@@ -623,6 +628,32 @@ static int cpufreq_add_dev (struct sys_d
 		goto err_out;
 	}
 
+#ifdef CONFIG_SMP
+	for_each_cpu_mask(j, policy->cpus) {
+		if (cpu == j)
+			continue;
+
+		/* check for existing affected CPUs.  They may not be aware
+		 * of it due to CPU Hotplug.
+		 */
+		managed_policy = cpufreq_cpu_get(j);
+		if (unlikely(managed_policy)) {
+			spin_lock_irqsave(&cpufreq_driver_lock, flags);
+			managed_policy->cpus = policy->cpus;
+			cpufreq_cpu_data[cpu] = managed_policy;
+			spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+			dprintk("CPU already managed, adding link\n");
+			sysfs_create_link(&sys_dev->kobj,
+					  &managed_policy->kobj, "cpufreq");
+
+			cpufreq_debug_enable_ratelimit();
+			mutex_unlock(&policy->lock);
+			ret = 0;
+			goto err_out_driver_exit; /* call driver->exit() */
+		}
+	}
+#endif
 	memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));
 
 	/* prepare interface data */
@@ -650,6 +681,21 @@ static int cpufreq_add_dev (struct sys_d
 	for_each_cpu_mask(j, policy->cpus)
 		cpufreq_cpu_data[j] = policy;
 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+	/* symlink affected CPUs */
+	for_each_cpu_mask(j, policy->cpus) {
+		if (j == cpu)
+			continue;
+		if (!cpu_online(j))
+			continue;
+		
+		dprintk("CPU already managed, adding link\n");
+		cpufreq_cpu_get(cpu);
+		cpu_sys_dev = get_cpu_sysdev(j);
+		sysfs_create_link(&cpu_sys_dev->kobj, &policy->kobj,
+				  "cpufreq");
+	}
+
 	policy->governor = NULL; /* to assure that the starting sequence is
 				  * run in cpufreq_set_policy */
 	mutex_unlock(&policy->lock);
@@ -728,6 +774,7 @@ static int cpufreq_remove_dev (struct sy
 	 */
 	if (unlikely(cpu != data->cpu)) {
 		dprintk("removing link\n");
+		cpu_clear(cpu, data->cpus);
 		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 		sysfs_remove_link(&sys_dev->kobj, "cpufreq");
 		cpufreq_cpu_put(data);

[-- Attachment #2: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

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

[Andi Kleen]

On Tuesday 28 February 2006 02:54, shin, jacob wrote:
> cpufreq: Fix handling for CPU hotplug
> 
> This patch adds proper logic to cpufreq driver in order to handle
> CPU Hotplug.
> 
> When CPUs go on/offline, the affected CPUs data, cpufreq_policy->cpus,
> is not updated properly. This causes sysfs directories and symlinks to
> be in an incorrect state after few CPU on/offlines.

Looks good to me. I assume that's an 2.6.16 candidate too.

Dave, can you submit them?

-Andi

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

[Pallipadi, Venkatesh]

 
Questions:

>-----Original Message-----
>From: cpufreq-bounces@lists.linux.org.uk 
>[mailto:cpufreq-bounces@lists.linux.org.uk] On Behalf Of shin, jacob
>Sent: Monday, February 27, 2006 5:55 PM
>@@ -623,6 +628,32 @@ static int cpufreq_add_dev (struct sys_d
> 		goto err_out;
> 	}
> 
>+#ifdef CONFIG_SMP
>+	for_each_cpu_mask(j, policy->cpus) {
>+		if (cpu == j)
>+			continue;
>+
>+		/* check for existing affected CPUs.  They may 
>not be aware
>+		 * of it due to CPU Hotplug.
>+		 */
>+		managed_policy = cpufreq_cpu_get(j);

I see multiple cpufreq_cpu_get() without cpmplementing 
cpufreq_cpu_put(). Is it intentional?


>+		if (unlikely(managed_policy)) {
>+			spin_lock_irqsave(&cpufreq_driver_lock, flags);
>+			managed_policy->cpus = policy->cpus;
>+			cpufreq_cpu_data[cpu] = managed_policy;
>+			
>spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
>+
>+			dprintk("CPU already managed, adding link\n");
>+			sysfs_create_link(&sys_dev->kobj,
>+					  
>&managed_policy->kobj, "cpufreq");
>+
>+			cpufreq_debug_enable_ratelimit();
>+			mutex_unlock(&policy->lock);
>+			ret = 0;
>+			goto err_out_driver_exit; /* call 
>driver->exit() */

Why is this going through the error path here. Is the call to exit()
required?


Thanks,
Venki

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

[shin, jacob]

On Tuesday, February 28, 2006 7:23 PM Pallipadi, Venkatesh wrote:

> Questions:

Thanks for your questions/comments.  Getting cpufreq driver to
play nicely with CPU Hotplug is not a trivial matter, especially
when affected cpus data is concerned.  If you have another
simple/elegant/"un-intrusive" solution, I would be glad to discuss
it further.

> I see multiple cpufreq_cpu_get() without cpmplementing
> cpufreq_cpu_put(). Is it intentional?

There are complementing cpufreq_cpu_put calls in the remove
function.

This is the code path in which the main cpufreq directory is created
for the first time, and symlinks all of the affected cpus. This is
in order to deal with CPU Hotplug.  Consider the case where the main
CPU that owns the actual sys directory goes off line.  All of its
affected cpus' symlinks will be unlinked, then the data destroyed.
When that main CPU comes online again, it needs to symlink all of
its affected cpus that were already online before him.

> Why is this going through the error path here. Is the call to exit()
> required?

Note that the return code is 0.  This is the case where cpufreq
driver registers a new driver through driver->init, however finds
that the CPU is already managed by someone that was online before
him.  The existing policy cannot predict what other CPUs that
will come after, due to CPU Hotplug.  Therefore, whenever a new
policy is registered through driver->init, we must check if it is
already managed, otherwise a few CPU on/offlines will confuse
the cpufreq driver's data.  When we find that the CPU is already
managed, we do not need to keep its data around, and we also
need to call driver->exit.

Thanks,

-Jacob Shin
AMD, Inc.

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

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: shin, jacob [mailto:jacob.shin@amd.com] 
>On Tuesday, February 28, 2006 7:23 PM Pallipadi, Venkatesh wrote:
>
>> Questions:
>
>Thanks for your questions/comments.  Getting cpufreq driver to
>play nicely with CPU Hotplug is not a trivial matter, especially
>when affected cpus data is concerned.  If you have another
>simple/elegant/"un-intrusive" solution, I would be glad to discuss
>it further.
>

I am sorry if I came across as saying that this is a trivial patch. 
I never intended that. Having looked at cpufreq code for couple of 
years, I know better than that :-). I was just trying to understand 
the patch and review the patch...

>> I see multiple cpufreq_cpu_get() without cpmplementing
>> cpufreq_cpu_put(). Is it intentional?
>
>There are complementing cpufreq_cpu_put calls in the remove
>function.

So, when main CPU comes back offline, there are these affected CPUs 
that are not aware of the main CPU. So, all these other CPUs would 
have already done cpu_get() already. Now, inside this for loop, we 
do an cpu_get for these affected_cpus bunch and then link the new CPU
with affected CPUs. Right?
The concern I have is, this cpu_get() for managed_cpus outside the loop 
will end up being an extra cpu_get() without cpu_put().

>> Why is this going through the error path here. Is the call to exit()
>> required?
>
>Note that the return code is 0.  This is the case where cpufreq
>driver registers a new driver through driver->init, however finds
>that the CPU is already managed by someone that was online before
>him.  The existing policy cannot predict what other CPUs that
>will come after, due to CPU Hotplug.  Therefore, whenever a new
>policy is registered through driver->init, we must check if it is
>already managed, otherwise a few CPU on/offlines will confuse
>the cpufreq driver's data.  When we find that the CPU is already
>managed, we do not need to keep its data around, and we also
>need to call driver->exit.
>

Ok. Understood this path now.

Thanks,
Venki

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

[shin, jacob]

On Tuesday, February 28, 2006 11:19 PM Pallipadi, Venkatesh wrote:

> I am sorry if I came across as saying that this is a trivial patch.
> I never intended that. Having looked at cpufreq code for couple of
> years, I know better than that :-). I was just trying to understand
> the patch and review the patch...

Oh, you didn't at all.. I guess what I was trying to say was that..
I did put some time and thought into this solution, and to me this
was the best way to solve the problem without causing too much havoc.
;-)

> So, when main CPU comes back offline, there are these affected CPUs
> that are not aware of the main CPU. So, all these other CPUs would
> have already done cpu_get() already. Now, inside this for loop, we
> do an cpu_get for these affected_cpus bunch and then link the new CPU
> with affected CPUs. Right?
> The concern I have is, this cpu_get() for managed_cpus outside the loop
> will end up being an extra cpu_get() without cpu_put().

The very first cpu_get will not have succeeded if we reach
this part of the code.  If it did succeed, then it would have
cpu_put() and return (see line 31 of the patch).  The cpu_get()
inside the loop will succeed iff we find a affected cpu that
is already managed.  In that case, it will symlink and exit
out of the loop on the first one it finds.

The logic here goes like this:

Consider CPU0 and CPU1, where their frequencies are tied together.
Both of them are offline in the beginning.  Now, lets bring CPU0
online first.  cpufreq driver creates sys directory for CPU0.  At
this point in time, CPU0's affected cpus mask must only have CPU0
set.  Because it is the only CPU online at this time.  Now, lets
bring CPU1 online, cpufreq will begin the initialization process,
calling driver->init.  However, now we have a dilemma, CPU1 now
says that its affected cpus are CPU0 and CPU1, however, CPU0 is 
already managed!

Therefore, the code path here will find out if any of the
affected cpus of CPU1 is already managed, finds CPU0, updates
CPU0's affected cpus mask, symlinks CPU1 to CPU0, then exits
through the error handling path. (it has to free memory not used
anymore)

The most important thing to note here is that CPU0 at its boot
time, does not know about CPU1.

Thanks!

-Jacob Shin
AMD, Inc.

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

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: shin, jacob [mailto:jacob.shin@amd.com] 
>
>The logic here goes like this:
>
>Consider CPU0 and CPU1, where their frequencies are tied together.
>Both of them are offline in the beginning.  Now, lets bring CPU0
>online first.  cpufreq driver creates sys directory for CPU0.  At
>this point in time, CPU0's affected cpus mask must only have CPU0
>set.  Because it is the only CPU online at this time.  Now, lets
>bring CPU1 online, cpufreq will begin the initialization process,
>calling driver->init.  However, now we have a dilemma, CPU1 now
>says that its affected cpus are CPU0 and CPU1, however, CPU0 is 
>already managed!
>
>Therefore, the code path here will find out if any of the
>affected cpus of CPU1 is already managed, finds CPU0, updates
>CPU0's affected cpus mask, symlinks CPU1 to CPU0, then exits
>through the error handling path. (it has to free memory not used
>anymore)
>
>The most important thing to note here is that CPU0 at its boot
>time, does not know about CPU1.
>

Thanks for the detailed explanation. I understood most of the code. 
Was only worried whether there is an extra get for CPU 1 in above case, 
just before 0 gets linked to 1. Now, I am convinced that is 
it not the case. 

Thanks again,
Venki

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

[Andi Kleen]

On Thursday 02 March 2006 02:53, Pallipadi, Venkatesh wrote:

> Thanks for the detailed explanation. I understood most of the code. 
> Was only worried whether there is an extra get for CPU 1 in above case, 
> just before 0 gets linked to 1. Now, I am convinced that is 
> it not the case. 

Thanks for the review, Venki. Do you think the patches are suitable
for 2.6.16? 

-Andi

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

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Andi Kleen [mailto:ak@suse.de] 
>Sent: Wednesday, March 01, 2006 5:59 PM
>To: Pallipadi, Venkatesh
>Cc: shin, jacob; cpufreq@lists.linux.org.uk; Langsdorf, Mark; 
>davej@redhat.com
>Subject: Re: [PATCH] [2/2] cpufreq: Fix handling for CPU hotplug
>
>On Thursday 02 March 2006 02:53, Pallipadi, Venkatesh wrote:
>
>> Thanks for the detailed explanation. I understood most of the code. 
>> Was only worried whether there is an extra get for CPU 1 in 
>above case, 
>> just before 0 gets linked to 1. Now, I am convinced that is 
>> it not the case. 
>
>Thanks for the review, Venki. Do you think the patches are suitable
>for 2.6.16? 
>

Yes. I think the patch is OK for 2.6.16.

Thanks,
Venki