[RFC] cpufreq: Fix issues with hot-[un]plugging of cpus

[Viresh Kumar]

Hi Rafael and other cpufreq masters,

I am really not sure if this patch is a HACK or a valid solution :)
Please have a look.

Setup:
-----
- ARM big LITTLE Platform with 5 cpus. cpus 2, 3 & 4 share clock and hence are
  part of policy->cpus. cpus 0 & 1 share clock but they aren't discussed here.
- Interactive governor: I know its not mainlined and will never be. Problem
  should be same with all the governors we have. So, problem is not really
  governor dependent. I have added few prints at start/stop cases of governor,
  to see what policy->cpus is set to.

Problem Statement:
-----------------
There are many issues :(

1) I couldn't understand what we really want the value of policy->cpus be? Only
   the online cpus sharing clock? or all the possible cpus sharing clock?

   When we offline a cpu, we remove it from cpus field: cpumask_clear_cpu(cpu, data->cpus);
   
   Which shows that it is mask of online cpus only. But there are many places
   checking if a cpu is online or not (cpu in policy->cpus) in cpufreq.c. Which
   shows something else.

2) Kernel crashes when hot-unplugging cpus

   When we do: for i in 2 3 4; do echo 0 > /sys/devices/system/cpu/cpu$i/online; done
   We get:

   cpufreq_governor_interactive: CPUFREQ_GOV_STOP cpu 2 policy->cpus 2-4
   cpufreq_vexpress: CPUFreq for CPU 3 initialized
   cpufreq_governor_interactive: CPUFREQ_GOV_START cpu 3 policy->cpus 2-4
   CPU2: shutdown
   cpufreq_governor_interactive: CPUFREQ_GOV_STOP cpu 3 policy->cpus 3-4
   cpufreq_vexpress: CPUFreq for CPU 4 initialized
   cpufreq_governor_interactive: CPUFREQ_GOV_START cpu 4 policy->cpus 2-4

   And then crash from interactive governor.

   Note the following lines:
   ------------------------
   cpufreq_governor_interactive: CPUFREQ_GOV_STOP cpu 2 policy->cpus 2-4
   cpufreq_governor_interactive: CPUFREQ_GOV_START cpu 3 policy->cpus 2-4
   cpufreq_governor_interactive: CPUFREQ_GOV_STOP cpu 3 policy->cpus 3-4
   cpufreq_governor_interactive: CPUFREQ_GOV_START cpu 4 policy->cpus 2-4
   
   So the governor is always started for CPUs 2 through 4 despite the fact
   that CPU 2 is removed. This makes the governor unstable due to wrong info
   passed.

   FIX: keep only online cpus in policy->cpus
   ---

3) With above issue solved, i tried to run the same stuff again and look what i
   got:

   cpufreq_governor_interactive: CPUFREQ_GOV_STOP cpu 2 policy->cpus 2-4
   cpufreq_vexpress: CPUFreq for CPU 3 initialized
   cpufreq_governor_interactive: CPUFREQ_GOV_START cpu 3 policy->cpus 2-4
   CPU2: shutdown

a) Because cpu 2 is still not completely hot unplugged and is still present in
   cpu_online_mask.
b) If we try to remove cpu 3, governor wouldn't be notified of it, because cpu 3
   wasn't policy->cpu.

   FIX: Stop/Start governor on every non policy->cpu removal
   ---

4) Hot unplug is fine now, but not hotplug :(
   for i in 2 3 4; do echo 1 > /sys/devices/system/cpu/cpu$i/online; done

   CPU2: Booted secondary processor
   cpufreq_vexpress: CPUFreq for CPU 2 initialized
   cpufreq_governor_interactive: CPUFREQ_GOV_START cpu 2 policy->cpus 2
   CPU3: Booted secondary processor
   cpufreq_vexpress: CPUFreq for CPU 3 initialized
   CPU4: Booted secondary processor
   cpufreq_vexpress: CPUFreq for CPU 4 initialized

   Hmmm, Governor is not informed about 3 and 4. :(

   FIX: Stop/Start governor on every managed_policy cpu, i.e. cpu for which
   ---  policy struct exists


Please share your comments, i know there would be many :)

Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
---
 drivers/cpufreq/cpufreq.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 0c3e29a..69bd739 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -749,11 +749,16 @@ static int cpufreq_add_dev_policy(unsigned int cpu,
 				return -EBUSY;
 			}
 
+			__cpufreq_governor(managed_policy, CPUFREQ_GOV_STOP);
+
 			spin_lock_irqsave(&cpufreq_driver_lock, flags);
 			cpumask_copy(managed_policy->cpus, policy->cpus);
 			per_cpu(cpufreq_cpu_data, cpu) = managed_policy;
 			spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+			__cpufreq_governor(managed_policy, CPUFREQ_GOV_START);
+			__cpufreq_governor(managed_policy, CPUFREQ_GOV_LIMITS);
+
 			pr_debug("CPU already managed, adding link\n");
 			ret = sysfs_create_link(&dev->kobj,
 						&managed_policy->kobj,
@@ -968,6 +973,13 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 		pr_debug("initialization failed\n");
 		goto err_unlock_policy;
 	}
+
+	/*
+	 * affected cpus must always be the one, which are online. We aren't
+	 * managing offline cpus here.
+	 */
+	cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
+
 	policy->user_policy.min = policy->min;
 	policy->user_policy.max = policy->max;
 
@@ -1057,8 +1069,13 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 	 */
 	if (unlikely(cpu != data->cpu)) {
 		pr_debug("removing link\n");
+		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
 		cpumask_clear_cpu(cpu, data->cpus);
 		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+		__cpufreq_governor(data, CPUFREQ_GOV_START);
+		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+
 		kobj = &dev->kobj;
 		cpufreq_cpu_put(data);
 		unlock_policy_rwsem_write(cpu);
-- 
1.7.12.rc2.18.g61b472e


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/