[PATCH v6 0/5] cpufreq: handle SW coordinated CPUs

[PATCH v6 0/5] cpufreq: handle SW coordinated CPUs

[Fabio Baltieri]

Hello Rafael,

this is the v6 for the cpufreq SW coordinated CPU bug fix, that I was
holding for -rc1 as agreed.

Differences from v5:
- removed dangling special case in dbs_timer_init
- rebased on top of v3.8-rc1

Would you consider this set for -next?

Thanks,
Fabio


Fabio Baltieri (4):
  cpufreq: star/stop cpufreq timers on cpu hotplug
  cpufreq: ondemand: call dbs_check_cpu only when necessary
  cpufreq: conservative: call dbs_check_cpu only when necessary
  cpufreq: ondemand: use all CPUs in update_sampling_rate

Rickard Andersson (1):
  cpufreq: handle SW coordinated CPUs

 drivers/cpufreq/cpufreq_conservative.c | 46 ++++++++++++++--
 drivers/cpufreq/cpufreq_governor.c     | 98 +++++++++++++++++++++++++++++++---
 drivers/cpufreq/cpufreq_governor.h     |  2 +
 drivers/cpufreq/cpufreq_ondemand.c     | 62 ++++++++++++++++-----
 4 files changed, 185 insertions(+), 23 deletions(-)

-- 
1.7.12.1

[PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Fabio Baltieri]

From: Rickard Andersson <rickard.andersson@stericsson.com>

This patch fixes a bug that occurred when we had load on a secondary CPU
and the primary CPU was sleeping. Only one sampling timer was spawned
and it was spawned as a deferred timer on the primary CPU, so when a
secondary CPU had a change in load this was not detected by the cpufreq
governor (both ondemand and conservative).

This patch make sure that deferred timers are run on all CPUs in the
case of software controlled CPUs that run on the same frequency.

Signed-off-by: Rickard Andersson <rickard.andersson@stericsson.com>
Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c |  3 ++-
 drivers/cpufreq/cpufreq_governor.c     | 44 +++++++++++++++++++++++++++++-----
 drivers/cpufreq/cpufreq_governor.h     |  1 +
 drivers/cpufreq/cpufreq_ondemand.c     |  3 ++-
 4 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 64ef737..b9d7f14 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -122,7 +122,8 @@ static void cs_dbs_timer(struct work_struct *work)
 
 	dbs_check_cpu(&cs_dbs_data, cpu);
 
-	schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay);
+	schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,
+			delay);
 	mutex_unlock(&dbs_info->cdbs.timer_mutex);
 }
 
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 6c5f1d3..b0e4506 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -161,13 +161,23 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 }
 EXPORT_SYMBOL_GPL(dbs_check_cpu);
 
+bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs)
+{
+	struct cpufreq_policy *policy = cdbs->cur_policy;
+
+	return cpumask_weight(policy->cpus) > 1;
+}
+EXPORT_SYMBOL_GPL(dbs_sw_coordinated_cpus);
+
 static inline void dbs_timer_init(struct dbs_data *dbs_data,
-		struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
+				  struct cpu_dbs_common_info *cdbs,
+				  unsigned int sampling_rate,
+				  int cpu)
 {
 	int delay = delay_for_sampling_rate(sampling_rate);
+	struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu);
 
-	INIT_DEFERRABLE_WORK(&cdbs->work, dbs_data->gov_dbs_timer);
-	schedule_delayed_work_on(cdbs->cpu, &cdbs->work, delay);
+	schedule_delayed_work_on(cpu, &cdbs_local->work, delay);
 }
 
 static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
@@ -217,6 +227,10 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 			if (ignore_nice)
 				j_cdbs->prev_cpu_nice =
 					kcpustat_cpu(j).cpustat[CPUTIME_NICE];
+
+			mutex_init(&j_cdbs->timer_mutex);
+			INIT_DEFERRABLE_WORK(&j_cdbs->work,
+					     dbs_data->gov_dbs_timer);
 		}
 
 		/*
@@ -275,15 +289,33 @@ second_time:
 		}
 		mutex_unlock(&dbs_data->mutex);
 
-		mutex_init(&cpu_cdbs->timer_mutex);
-		dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
+		if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
+			for_each_cpu(j, policy->cpus) {
+				struct cpu_dbs_common_info *j_cdbs;
+
+				j_cdbs = dbs_data->get_cpu_cdbs(j);
+				dbs_timer_init(dbs_data, j_cdbs,
+					       *sampling_rate, j);
+			}
+		} else {
+			dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu);
+		}
 		break;
 
 	case CPUFREQ_GOV_STOP:
 		if (dbs_data->governor == GOV_CONSERVATIVE)
 			cs_dbs_info->enable = 0;
 
-		dbs_timer_exit(cpu_cdbs);
+		if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
+			for_each_cpu(j, policy->cpus) {
+				struct cpu_dbs_common_info *j_cdbs;
+
+				j_cdbs = dbs_data->get_cpu_cdbs(j);
+				dbs_timer_exit(j_cdbs);
+			}
+		} else {
+			dbs_timer_exit(cpu_cdbs);
+		}
 
 		mutex_lock(&dbs_data->mutex);
 		mutex_destroy(&cpu_cdbs->timer_mutex);
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index f661654..5bf6fb8 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -171,6 +171,7 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate)
 
 u64 get_cpu_idle_time(unsigned int cpu, u64 *wall);
 void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
+bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs);
 int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 		struct cpufreq_policy *policy, unsigned int event);
 #endif /* _CPUFREQ_GOVERNER_H */
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 7731f7c..93bb56d 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -243,7 +243,8 @@ static void od_dbs_timer(struct work_struct *work)
 		}
 	}
 
-	schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay);
+	schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,
+			delay);
 	mutex_unlock(&dbs_info->cdbs.timer_mutex);
 }
 
-- 
1.7.12.1

[PATCH 2/5] cpufreq: star/stop cpufreq timers on cpu hotplug

[Fabio Baltieri]

Add a CPU notifier to start and stop individual core timers on CPU
hotplug events when running on CPUs with SW coordinated frequency.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.c | 51 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index b0e4506..e881250 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -25,9 +25,12 @@
 #include <linux/tick.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
+#include <linux/cpu.h>
 
 #include "cpufreq_governor.h"
 
+static DEFINE_PER_CPU(struct dbs_data *, cpu_cur_dbs);
+
 static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
 {
 	u64 idle_time;
@@ -185,6 +188,46 @@ static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
 	cancel_delayed_work_sync(&cdbs->work);
 }
 
+static int __cpuinit cpu_callback(struct notifier_block *nfb,
+		unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+	struct device *cpu_dev = get_cpu_device(cpu);
+	struct dbs_data *dbs_data = per_cpu(cpu_cur_dbs, cpu);
+	struct cpu_dbs_common_info *cpu_cdbs = dbs_data->get_cpu_cdbs(cpu);
+	unsigned int sampling_rate;
+
+	if (dbs_data->governor == GOV_CONSERVATIVE) {
+		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
+		sampling_rate = cs_tuners->sampling_rate;
+	} else {
+		struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+		sampling_rate = od_tuners->sampling_rate;
+	}
+
+	if (cpu_dev) {
+		switch (action) {
+		case CPU_ONLINE:
+		case CPU_ONLINE_FROZEN:
+		case CPU_DOWN_FAILED:
+		case CPU_DOWN_FAILED_FROZEN:
+			dbs_timer_init(dbs_data, cpu_cdbs,
+					sampling_rate, cpu);
+			break;
+		case CPU_DOWN_PREPARE:
+		case CPU_DOWN_PREPARE_FROZEN:
+			dbs_timer_exit(cpu_cdbs);
+			break;
+		}
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block __refdata ondemand_cpu_notifier = {
+	.notifier_call = cpu_callback,
+};
+
 int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 		struct cpufreq_policy *policy, unsigned int event)
 {
@@ -296,7 +339,11 @@ second_time:
 				j_cdbs = dbs_data->get_cpu_cdbs(j);
 				dbs_timer_init(dbs_data, j_cdbs,
 					       *sampling_rate, j);
+
+				per_cpu(cpu_cur_dbs, j) = dbs_data;
 			}
+
+			register_hotcpu_notifier(&ondemand_cpu_notifier);
 		} else {
 			dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu);
 		}
@@ -307,11 +354,15 @@ second_time:
 			cs_dbs_info->enable = 0;
 
 		if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
+			unregister_hotcpu_notifier(&ondemand_cpu_notifier);
+
 			for_each_cpu(j, policy->cpus) {
 				struct cpu_dbs_common_info *j_cdbs;
 
 				j_cdbs = dbs_data->get_cpu_cdbs(j);
 				dbs_timer_exit(j_cdbs);
+
+				per_cpu(cpu_cur_dbs, j) = NULL;
 			}
 		} else {
 			dbs_timer_exit(cpu_cdbs);
-- 
1.7.12.1

[PATCH 3/5] cpufreq: ondemand: call dbs_check_cpu only when necessary

[Fabio Baltieri]

Modify ondemand timer to not resample CPU utilization if recently
sampled from another SW coordinated core.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.c |  3 ++
 drivers/cpufreq/cpufreq_governor.h |  1 +
 drivers/cpufreq/cpufreq_ondemand.c | 58 +++++++++++++++++++++++++++++++-------
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index e881250..e5a3711 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -333,6 +333,9 @@ second_time:
 		mutex_unlock(&dbs_data->mutex);
 
 		if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
+			/* Initiate timer time stamp */
+			cpu_cdbs->time_stamp = ktime_get();
+
 			for_each_cpu(j, policy->cpus) {
 				struct cpu_dbs_common_info *j_cdbs;
 
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 5bf6fb8..aaf073d 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -82,6 +82,7 @@ struct cpu_dbs_common_info {
 	 * the governor or limits.
 	 */
 	struct mutex timer_mutex;
+	ktime_t time_stamp;
 };
 
 struct od_cpu_dbs_info_s {
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 93bb56d..13ceb3c 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -216,23 +216,23 @@ static void od_check_cpu(int cpu, unsigned int load_freq)
 	}
 }
 
-static void od_dbs_timer(struct work_struct *work)
+static void od_timer_update(struct od_cpu_dbs_info_s *dbs_info, bool sample,
+			    struct delayed_work *dw)
 {
-	struct od_cpu_dbs_info_s *dbs_info =
-		container_of(work, struct od_cpu_dbs_info_s, cdbs.work.work);
 	unsigned int cpu = dbs_info->cdbs.cpu;
 	int delay, sample_type = dbs_info->sample_type;
 
-	mutex_lock(&dbs_info->cdbs.timer_mutex);
-
 	/* Common NORMAL_SAMPLE setup */
 	dbs_info->sample_type = OD_NORMAL_SAMPLE;
 	if (sample_type == OD_SUB_SAMPLE) {
 		delay = dbs_info->freq_lo_jiffies;
-		__cpufreq_driver_target(dbs_info->cdbs.cur_policy,
-			dbs_info->freq_lo, CPUFREQ_RELATION_H);
+		if (sample)
+			__cpufreq_driver_target(dbs_info->cdbs.cur_policy,
+						dbs_info->freq_lo,
+						CPUFREQ_RELATION_H);
 	} else {
-		dbs_check_cpu(&od_dbs_data, cpu);
+		if (sample)
+			dbs_check_cpu(&od_dbs_data, cpu);
 		if (dbs_info->freq_lo) {
 			/* Setup timer for SUB_SAMPLE */
 			dbs_info->sample_type = OD_SUB_SAMPLE;
@@ -243,11 +243,49 @@ static void od_dbs_timer(struct work_struct *work)
 		}
 	}
 
-	schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,
-			delay);
+	schedule_delayed_work_on(smp_processor_id(), dw, delay);
+}
+
+static void od_timer_coordinated(struct od_cpu_dbs_info_s *dbs_info_local,
+				 struct delayed_work *dw)
+{
+	struct od_cpu_dbs_info_s *dbs_info;
+	ktime_t time_now;
+	s64 delta_us;
+	bool sample = true;
+
+	/* use leader CPU's dbs_info */
+	dbs_info = &per_cpu(od_cpu_dbs_info, dbs_info_local->cdbs.cpu);
+	mutex_lock(&dbs_info->cdbs.timer_mutex);
+
+	time_now = ktime_get();
+	delta_us = ktime_us_delta(time_now, dbs_info->cdbs.time_stamp);
+
+	/* Do nothing if we recently have sampled */
+	if (delta_us < (s64)(od_tuners.sampling_rate / 2))
+		sample = false;
+	else
+		dbs_info->cdbs.time_stamp = time_now;
+
+	od_timer_update(dbs_info, sample, dw);
 	mutex_unlock(&dbs_info->cdbs.timer_mutex);
 }
 
+static void od_dbs_timer(struct work_struct *work)
+{
+	struct delayed_work *dw = to_delayed_work(work);
+	struct od_cpu_dbs_info_s *dbs_info =
+		container_of(work, struct od_cpu_dbs_info_s, cdbs.work.work);
+
+	if (dbs_sw_coordinated_cpus(&dbs_info->cdbs)) {
+		od_timer_coordinated(dbs_info, dw);
+	} else {
+		mutex_lock(&dbs_info->cdbs.timer_mutex);
+		od_timer_update(dbs_info, true, dw);
+		mutex_unlock(&dbs_info->cdbs.timer_mutex);
+	}
+}
+
 /************************** sysfs interface ************************/
 
 static ssize_t show_sampling_rate_min(struct kobject *kobj,
-- 
1.7.12.1

[PATCH 4/5] cpufreq: conservative: call dbs_check_cpu only when necessary

[Fabio Baltieri]

Modify conservative timer to not resample CPU utilization if recently
sampled from another SW coordinated core.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 47 +++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index b9d7f14..5d8e894 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -111,22 +111,57 @@ static void cs_check_cpu(int cpu, unsigned int load)
 	}
 }
 
-static void cs_dbs_timer(struct work_struct *work)
+static void cs_timer_update(struct cs_cpu_dbs_info_s *dbs_info, bool sample,
+			    struct delayed_work *dw)
 {
-	struct cs_cpu_dbs_info_s *dbs_info = container_of(work,
-			struct cs_cpu_dbs_info_s, cdbs.work.work);
 	unsigned int cpu = dbs_info->cdbs.cpu;
 	int delay = delay_for_sampling_rate(cs_tuners.sampling_rate);
 
+	if (sample)
+		dbs_check_cpu(&cs_dbs_data, cpu);
+
+	schedule_delayed_work_on(smp_processor_id(), dw, delay);
+}
+
+static void cs_timer_coordinated(struct cs_cpu_dbs_info_s *dbs_info_local,
+				 struct delayed_work *dw)
+{
+	struct cs_cpu_dbs_info_s *dbs_info;
+	ktime_t time_now;
+	s64 delta_us;
+	bool sample = true;
+
+	/* use leader CPU's dbs_info */
+	dbs_info = &per_cpu(cs_cpu_dbs_info, dbs_info_local->cdbs.cpu);
 	mutex_lock(&dbs_info->cdbs.timer_mutex);
 
-	dbs_check_cpu(&cs_dbs_data, cpu);
+	time_now = ktime_get();
+	delta_us = ktime_us_delta(time_now, dbs_info->cdbs.time_stamp);
 
-	schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,
-			delay);
+	/* Do nothing if we recently have sampled */
+	if (delta_us < (s64)(cs_tuners.sampling_rate / 2))
+		sample = false;
+	else
+		dbs_info->cdbs.time_stamp = time_now;
+
+	cs_timer_update(dbs_info, sample, dw);
 	mutex_unlock(&dbs_info->cdbs.timer_mutex);
 }
 
+static void cs_dbs_timer(struct work_struct *work)
+{
+	struct delayed_work *dw = to_delayed_work(work);
+	struct cs_cpu_dbs_info_s *dbs_info = container_of(work,
+			struct cs_cpu_dbs_info_s, cdbs.work.work);
+
+	if (dbs_sw_coordinated_cpus(&dbs_info->cdbs)) {
+		cs_timer_coordinated(dbs_info, dw);
+	} else {
+		mutex_lock(&dbs_info->cdbs.timer_mutex);
+		cs_timer_update(dbs_info, true, dw);
+		mutex_unlock(&dbs_info->cdbs.timer_mutex);
+	}
+}
 static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 		void *data)
 {
-- 
1.7.12.1

[PATCH 5/5] cpufreq: ondemand: use all CPUs in update_sampling_rate

[Fabio Baltieri]

Modify update_sampling_rate() to check, and eventually immediately
schedule, all CPU's do_dbs_timer delayed work.

This is required in case of software coordinated CPUs, as we now have a
separate delayed work for each CPU.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 drivers/cpufreq/cpufreq_ondemand.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 13ceb3c..1017b90 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -326,7 +326,7 @@ static void update_sampling_rate(unsigned int new_rate)
 			cpufreq_cpu_put(policy);
 			continue;
 		}
-		dbs_info = &per_cpu(od_cpu_dbs_info, policy->cpu);
+		dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
 		cpufreq_cpu_put(policy);
 
 		mutex_lock(&dbs_info->cdbs.timer_mutex);
@@ -345,8 +345,7 @@ static void update_sampling_rate(unsigned int new_rate)
 			cancel_delayed_work_sync(&dbs_info->cdbs.work);
 			mutex_lock(&dbs_info->cdbs.timer_mutex);
 
-			schedule_delayed_work_on(dbs_info->cdbs.cpu,
-					&dbs_info->cdbs.work,
+			schedule_delayed_work_on(cpu, &dbs_info->cdbs.work,
 					usecs_to_jiffies(new_rate));
 
 		}
-- 
1.7.12.1

Re: [PATCH 2/5] cpufreq: star/stop cpufreq timers on cpu hotplug

[Viresh Kumar]

Hi Fabio,

Sorry for waking up very late :)

The reason why i am starting this thread again is due to problem
reported by Joseph,
with latest linux-next/master branch (which contains few big patches
from me :) ):

Reboot is giving following to him:

* Will now halt
[ 193.756068] Disabling non-boot CPUs...
[ 193.760088] BUG: scheduling while atomic: halt/780/0x00000002
[ 193.765845] Modules linked in: brcmfmac brcmutil
[ 193.770613] [<c0014990>] (unwind_backtrace+0x0/0xf8) from [<c0049510>]
(__schedule_bug+0x44/0x5c)
[ 193.779548] [<c0049510>] (__schedule_bug+0x44/0x5c) from [<c04fa320>]
(__schedule+0x688/0x6ec)
[ 193.788206] [<c04fa320>] (__schedule+0x688/0x6ec) from [<c04fa75c>]
(schedule_preempt_disabled+0x24/0x34)
[ 193.797811] [<c04fa75c>] (schedule_preempt_disabled+0x24/0x34) from
[<c04f916c>] (__mutex_lock_slowpath+0x170/0x34c)
[ 193.808367] [<c04f916c>] (__mutex_lock_slowpath+0x170/0x34c) from
[<c04f9354>] (mutex_lock+0xc/0x24)
[ 193.817554] [<c04f9354>] (mutex_lock+0xc/0x24) from [<c04f1cdc>]
(unregister_cpu_notifier+0xc/0x24)
[ 193.826640] [<c04f1cdc>] (unregister_cpu_notifier+0xc/0x24) from
[<c033a8a4>] (cpufreq_governor_dbs+0x118/0x614)
[ 193.836866] [<c033a8a4>] (cpufreq_governor_dbs+0x118/0x614) from
[<c033747c>] (__cpufreq_governor+0x58/0xc0)
[ 193.846737] [<c033747c>] (__cpufreq_governor+0x58/0xc0) from
[<c0339104>] (__cpufreq_remove_dev.clone.7+0x58/0x320)
[ 193.857207] [<c0339104>] (__cpufreq_remove_dev.clone.7+0x58/0x320)
from [<c04f7958>] (cpufreq_cpu_callback+0x8c/0x9c)
[ 193.867850] [<c04f7958>] (cpufreq_cpu_callback+0x8c/0x9c) from
[<c0044f4c>] (notifier_call_chain+0x44/0x84)
[ 193.877623] [<c0044f4c>] (notifier_call_chain+0x44/0x84) from
[<c0026e24>] (__cpu_notify+0x2c/0x48)
[ 193.886704] [<c0026e24>] (__cpu_notify+0x2c/0x48) from [<c04f1b08>]
(_cpu_down+0xb0/0x23c)
[ 193.895004] [<c04f1b08>] (_cpu_down+0xb0/0x23c) from [<c00270ec>]
(disable_nonboot_cpus+0x68/0x104)
[ 193.904089] [<c00270ec>] (disable_nonboot_cpus+0x68/0x104) from
[<c0034fbc>] (kernel_power_off+0x24/0x48)
[ 193.913688] [<c0034fbc>] (kernel_power_off+0x24/0x48) from
[<c0035810>] (sys_reboot+0x104/0x1e0)
[ 193.922517] [<c0035810>] (sys_reboot+0x104/0x1e0) from [<c000e520>]
(ret_fast_syscall+0x0/0x30)


And the crash log show this patch of yours somewhere :)

First question: Is this patch still required? Because following patch from me is
sending a STOP/START to governors on cpu hot-[un]plug ?

commit dbcb63407c095af73f3464767e00902cdee55e8b
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Sat Jan 12 05:14:39 2013 +0000

    cpufreq: Notify governors when cpus are hot-[un]plugged


For me, the answer is NO.

Over that, i tried these patches on ARM bigLITTLE TC2 (3 A7's  and 2 A15's) and
my system wasn't booting at all for ondemand governor. Reverting your patch does
fix the issue:

[    2.613573] arm_big_little: bL_cpufreq_init: Initialized, cpu: 0, cluster 0
[    2.635436] arm_big_little: bL_cpufreq_init: Initialized, cpu: 2, cluster 1
[   23.650184] INFO: rcu_sched self-detected stall on CPU { 0}
(t=2100 jiffies g=4294967088 c=4294967087 q=10)
[   23.679664] Backtrace for cpu 0 (current):
[   23.680239] INFO: rcu_sched detected stalls on CPUs/tasks: { 0}
(detected by 2, t=2103 jiffies, g=4294967088, c=4294967087, q=10)
[   23.726839] [<c0014020>] (unwind_backtrace+0x0/0xf8) from
[<c0012f60>] (smp_send_all_cpu_backtrace+0x60/0xcc)
[   23.756545] [<c0012f60>] (smp_send_all_cpu_backtrace+0x60/0xcc)
from [<c008232c>] (rcu_pending+0x2c8/0x63c)
[   23.785731] [<c008232c>] (rcu_pending+0x2c8/0x63c) from
[<c0083854>] (rcu_check_callbacks+0x7c/0x148)
[   23.813358] [<c0083854>] (rcu_check_callbacks+0x7c/0x148) from
[<c002d1e0>] (update_process_times+0x38/0x68)
[   23.842808] [<c002d1e0>] (update_process_times+0x38/0x68) from
[<c0066b00>] (tick_sched_handle+0x48/0x54)
[   23.871472] [<c0066b00>] (tick_sched_handle+0x48/0x54) from
[<c0066d6c>] (tick_sched_timer+0x44/0x74)
[   23.899098] [<c0066d6c>] (tick_sched_timer+0x44/0x74) from
[<c00416d8>] (__run_hrtimer+0x84/0x1b4)
[   23.925940] [<c00416d8>] (__run_hrtimer+0x84/0x1b4) from
[<c004224c>] (hrtimer_interrupt+0x108/0x2d4)
[   23.953563] [<c004224c>] (hrtimer_interrupt+0x108/0x2d4) from
[<c0013990>] (arch_timer_handler_virt+0x30/0x38)
[   23.983530] [<c0013990>] (arch_timer_handler_virt+0x30/0x38) from
[<c007e41c>] (handle_percpu_devid_irq+0x74/0x110)
[   24.014799] [<c007e41c>] (handle_percpu_devid_irq+0x74/0x110) from
[<c007ac8c>] (generic_handle_irq+0x20/0x30)
[   24.044765] [<c007ac8c>] (generic_handle_irq+0x20/0x30) from
[<c000e9f4>] (handle_IRQ+0x38/0x94)
[   24.071085] [<c000e9f4>] (handle_IRQ+0x38/0x94) from [<c0008570>]
(gic_handle_irq+0x28/0x5c)
[   24.096362] [<c0008570>] (gic_handle_irq+0x28/0x5c) from
[<c000dd80>] (__irq_svc+0x40/0x50)
[   24.121374] Exception stack(0xef047ec0 to 0xef047f08)
[   24.136492] 7ec0: 00000000 c0591780 00000000 c0591798 c0591780
c05d5d60 00000000 c0593280
[   24.160986] 7ee0: c0552c98 c02b1df4 ef046000 00000000 c059179c
ef047f08 c03b5f84 c0043698
[   24.185477] 7f00: a0000113 ffffffff
[   24.195910] [<c000dd80>] (__irq_svc+0x40/0x50) from [<c0043698>]
(raw_notifier_chain_register+0x24/0x54)
[   24.224314] [<c0043698>] (raw_notifier_chain_register+0x24/0x54)
from [<c03b5f84>] (register_cpu_notifier+0x18/0x2c)
[   24.255843] [<c03b5f84>] (register_cpu_notifier+0x18/0x2c) from
[<c02ab1b0>] (cpufreq_register_driver+0x134/0x190)
[   24.286852] [<c02ab1b0>] (cpufreq_register_driver+0x134/0x190) from
[<c02b1930>] (bL_cpufreq_register+0x68/0xd4)
[   24.317337] [<c02b1930>] (bL_cpufreq_register+0x68/0xd4) from
[<c0008740>] (do_one_initcall+0x110/0x178)
[   24.345738] [<c0008740>] (do_one_initcall+0x110/0x178) from
[<c03b5994>] (kernel_init+0x194/0x330)
[   24.372577] [<c03b5994>] (kernel_init+0x194/0x330) from
[<c000e1d8>] (ret_from_fork+0x14/0x3c)
[   24.398371]


On Thu, Dec 27, 2012 at 8:25 PM, Fabio Baltieri
<fabio.baltieri@linaro.org> wrote:
> Add a CPU notifier to start and stop individual core timers on CPU
> hotplug events when running on CPUs with SW coordinated frequency.
>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_governor.c | 51 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index b0e4506..e881250 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -25,9 +25,12 @@
>  #include <linux/tick.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
> +#include <linux/cpu.h>
>
>  #include "cpufreq_governor.h"
>
> +static DEFINE_PER_CPU(struct dbs_data *, cpu_cur_dbs);
> +
>  static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
>  {
>         u64 idle_time;
> @@ -185,6 +188,46 @@ static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
>         cancel_delayed_work_sync(&cdbs->work);
>  }
>
> +static int __cpuinit cpu_callback(struct notifier_block *nfb,
> +               unsigned long action, void *hcpu)
> +{
> +       unsigned int cpu = (unsigned long)hcpu;
> +       struct device *cpu_dev = get_cpu_device(cpu);
> +       struct dbs_data *dbs_data = per_cpu(cpu_cur_dbs, cpu);
> +       struct cpu_dbs_common_info *cpu_cdbs = dbs_data->get_cpu_cdbs(cpu);
> +       unsigned int sampling_rate;
> +
> +       if (dbs_data->governor == GOV_CONSERVATIVE) {
> +               struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
> +               sampling_rate = cs_tuners->sampling_rate;
> +       } else {
> +               struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> +               sampling_rate = od_tuners->sampling_rate;
> +       }
> +
> +       if (cpu_dev) {
> +               switch (action) {
> +               case CPU_ONLINE:
> +               case CPU_ONLINE_FROZEN:
> +               case CPU_DOWN_FAILED:
> +               case CPU_DOWN_FAILED_FROZEN:
> +                       dbs_timer_init(dbs_data, cpu_cdbs,
> +                                       sampling_rate, cpu);
> +                       break;
> +               case CPU_DOWN_PREPARE:
> +               case CPU_DOWN_PREPARE_FROZEN:
> +                       dbs_timer_exit(cpu_cdbs);
> +                       break;
> +               }
> +       }
> +
> +       return NOTIFY_OK;
> +}
> +
> +static struct notifier_block __refdata ondemand_cpu_notifier = {

Over that, why is it called ondemand here ?

> +       .notifier_call = cpu_callback,
> +};
> +
>  int cpufreq_governor_dbs(struct dbs_data *dbs_data,
>                 struct cpufreq_policy *policy, unsigned int event)
>  {
> @@ -296,7 +339,11 @@ second_time:
>                                 j_cdbs = dbs_data->get_cpu_cdbs(j);
>                                 dbs_timer_init(dbs_data, j_cdbs,
>                                                *sampling_rate, j);
> +
> +                               per_cpu(cpu_cur_dbs, j) = dbs_data;
>                         }
> +
> +                       register_hotcpu_notifier(&ondemand_cpu_notifier);
>                 } else {
>                         dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu);
>                 }
> @@ -307,11 +354,15 @@ second_time:
>                         cs_dbs_info->enable = 0;
>
>                 if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
> +                       unregister_hotcpu_notifier(&ondemand_cpu_notifier);
> +
>                         for_each_cpu(j, policy->cpus) {
>                                 struct cpu_dbs_common_info *j_cdbs;
>
>                                 j_cdbs = dbs_data->get_cpu_cdbs(j);
>                                 dbs_timer_exit(j_cdbs);
> +
> +                               per_cpu(cpu_cur_dbs, j) = NULL;
>                         }
>                 } else {
>                         dbs_timer_exit(cpu_cdbs);

--
viresh

Re: [PATCH 2/5] cpufreq: star/stop cpufreq timers on cpu hotplug

[Joseph Lo]

On Wed, 2013-01-30 at 12:52 +0800, Viresh Kumar wrote:
> Hi Fabio,
> 
> Sorry for waking up very late :)
> 
> The reason why i am starting this thread again is due to problem
> reported by Joseph,
> with latest linux-next/master branch (which contains few big patches
> from me :) ):
> 
After Viresh point me out what patch may cause the issue that I met
below, I also try to just revert this patch on linux-next. Then
everything that may trigger to disable_nonboot_cpus or hotplug back to
normal. So just reverting this patch also fix the issues I saw.
FYI.

Thanks,
Joseph

> Reboot is giving following to him:
> 
> * Will now halt
> [ 193.756068] Disabling non-boot CPUs...
> [ 193.760088] BUG: scheduling while atomic: halt/780/0x00000002
> [ 193.765845] Modules linked in: brcmfmac brcmutil
> [ 193.770613] [<c0014990>] (unwind_backtrace+0x0/0xf8) from [<c0049510>]
> (__schedule_bug+0x44/0x5c)
> [ 193.779548] [<c0049510>] (__schedule_bug+0x44/0x5c) from [<c04fa320>]
> (__schedule+0x688/0x6ec)
> [ 193.788206] [<c04fa320>] (__schedule+0x688/0x6ec) from [<c04fa75c>]
> (schedule_preempt_disabled+0x24/0x34)
> [ 193.797811] [<c04fa75c>] (schedule_preempt_disabled+0x24/0x34) from
> [<c04f916c>] (__mutex_lock_slowpath+0x170/0x34c)
> [ 193.808367] [<c04f916c>] (__mutex_lock_slowpath+0x170/0x34c) from
> [<c04f9354>] (mutex_lock+0xc/0x24)
> [ 193.817554] [<c04f9354>] (mutex_lock+0xc/0x24) from [<c04f1cdc>]
> (unregister_cpu_notifier+0xc/0x24)
> [ 193.826640] [<c04f1cdc>] (unregister_cpu_notifier+0xc/0x24) from
> [<c033a8a4>] (cpufreq_governor_dbs+0x118/0x614)
> [ 193.836866] [<c033a8a4>] (cpufreq_governor_dbs+0x118/0x614) from
> [<c033747c>] (__cpufreq_governor+0x58/0xc0)
> [ 193.846737] [<c033747c>] (__cpufreq_governor+0x58/0xc0) from
> [<c0339104>] (__cpufreq_remove_dev.clone.7+0x58/0x320)
> [ 193.857207] [<c0339104>] (__cpufreq_remove_dev.clone.7+0x58/0x320)
> from [<c04f7958>] (cpufreq_cpu_callback+0x8c/0x9c)
> [ 193.867850] [<c04f7958>] (cpufreq_cpu_callback+0x8c/0x9c) from
> [<c0044f4c>] (notifier_call_chain+0x44/0x84)
> [ 193.877623] [<c0044f4c>] (notifier_call_chain+0x44/0x84) from
> [<c0026e24>] (__cpu_notify+0x2c/0x48)
> [ 193.886704] [<c0026e24>] (__cpu_notify+0x2c/0x48) from [<c04f1b08>]
> (_cpu_down+0xb0/0x23c)
> [ 193.895004] [<c04f1b08>] (_cpu_down+0xb0/0x23c) from [<c00270ec>]
> (disable_nonboot_cpus+0x68/0x104)
> [ 193.904089] [<c00270ec>] (disable_nonboot_cpus+0x68/0x104) from
> [<c0034fbc>] (kernel_power_off+0x24/0x48)
> [ 193.913688] [<c0034fbc>] (kernel_power_off+0x24/0x48) from
> [<c0035810>] (sys_reboot+0x104/0x1e0)
> [ 193.922517] [<c0035810>] (sys_reboot+0x104/0x1e0) from [<c000e520>]
> (ret_fast_syscall+0x0/0x30)
> 
> 
> And the crash log show this patch of yours somewhere :)
> 
> First question: Is this patch still required? Because following patch from me is
> sending a STOP/START to governors on cpu hot-[un]plug ?
> 
> commit dbcb63407c095af73f3464767e00902cdee55e8b
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sat Jan 12 05:14:39 2013 +0000
> 
>     cpufreq: Notify governors when cpus are hot-[un]plugged
> 
> 
> For me, the answer is NO.
> 
> Over that, i tried these patches on ARM bigLITTLE TC2 (3 A7's  and 2 A15's) and
> my system wasn't booting at all for ondemand governor. Reverting your patch does
> fix the issue:
> 
> [    2.613573] arm_big_little: bL_cpufreq_init: Initialized, cpu: 0, cluster 0
> [    2.635436] arm_big_little: bL_cpufreq_init: Initialized, cpu: 2, cluster 1
> [   23.650184] INFO: rcu_sched self-detected stall on CPU { 0}
> (t=2100 jiffies g=4294967088 c=4294967087 q=10)
> [   23.679664] Backtrace for cpu 0 (current):
> [   23.680239] INFO: rcu_sched detected stalls on CPUs/tasks: { 0}
> (detected by 2, t=2103 jiffies, g=4294967088, c=4294967087, q=10)
> [   23.726839] [<c0014020>] (unwind_backtrace+0x0/0xf8) from
> [<c0012f60>] (smp_send_all_cpu_backtrace+0x60/0xcc)
> [   23.756545] [<c0012f60>] (smp_send_all_cpu_backtrace+0x60/0xcc)
> from [<c008232c>] (rcu_pending+0x2c8/0x63c)
> [   23.785731] [<c008232c>] (rcu_pending+0x2c8/0x63c) from
> [<c0083854>] (rcu_check_callbacks+0x7c/0x148)
> [   23.813358] [<c0083854>] (rcu_check_callbacks+0x7c/0x148) from
> [<c002d1e0>] (update_process_times+0x38/0x68)
> [   23.842808] [<c002d1e0>] (update_process_times+0x38/0x68) from
> [<c0066b00>] (tick_sched_handle+0x48/0x54)
> [   23.871472] [<c0066b00>] (tick_sched_handle+0x48/0x54) from
> [<c0066d6c>] (tick_sched_timer+0x44/0x74)
> [   23.899098] [<c0066d6c>] (tick_sched_timer+0x44/0x74) from
> [<c00416d8>] (__run_hrtimer+0x84/0x1b4)
> [   23.925940] [<c00416d8>] (__run_hrtimer+0x84/0x1b4) from
> [<c004224c>] (hrtimer_interrupt+0x108/0x2d4)
> [   23.953563] [<c004224c>] (hrtimer_interrupt+0x108/0x2d4) from
> [<c0013990>] (arch_timer_handler_virt+0x30/0x38)
> [   23.983530] [<c0013990>] (arch_timer_handler_virt+0x30/0x38) from
> [<c007e41c>] (handle_percpu_devid_irq+0x74/0x110)
> [   24.014799] [<c007e41c>] (handle_percpu_devid_irq+0x74/0x110) from
> [<c007ac8c>] (generic_handle_irq+0x20/0x30)
> [   24.044765] [<c007ac8c>] (generic_handle_irq+0x20/0x30) from
> [<c000e9f4>] (handle_IRQ+0x38/0x94)
> [   24.071085] [<c000e9f4>] (handle_IRQ+0x38/0x94) from [<c0008570>]
> (gic_handle_irq+0x28/0x5c)
> [   24.096362] [<c0008570>] (gic_handle_irq+0x28/0x5c) from
> [<c000dd80>] (__irq_svc+0x40/0x50)
> [   24.121374] Exception stack(0xef047ec0 to 0xef047f08)
> [   24.136492] 7ec0: 00000000 c0591780 00000000 c0591798 c0591780
> c05d5d60 00000000 c0593280
> [   24.160986] 7ee0: c0552c98 c02b1df4 ef046000 00000000 c059179c
> ef047f08 c03b5f84 c0043698
> [   24.185477] 7f00: a0000113 ffffffff
> [   24.195910] [<c000dd80>] (__irq_svc+0x40/0x50) from [<c0043698>]
> (raw_notifier_chain_register+0x24/0x54)
> [   24.224314] [<c0043698>] (raw_notifier_chain_register+0x24/0x54)
> from [<c03b5f84>] (register_cpu_notifier+0x18/0x2c)
> [   24.255843] [<c03b5f84>] (register_cpu_notifier+0x18/0x2c) from
> [<c02ab1b0>] (cpufreq_register_driver+0x134/0x190)
> [   24.286852] [<c02ab1b0>] (cpufreq_register_driver+0x134/0x190) from
> [<c02b1930>] (bL_cpufreq_register+0x68/0xd4)
> [   24.317337] [<c02b1930>] (bL_cpufreq_register+0x68/0xd4) from
> [<c0008740>] (do_one_initcall+0x110/0x178)
> [   24.345738] [<c0008740>] (do_one_initcall+0x110/0x178) from
> [<c03b5994>] (kernel_init+0x194/0x330)
> [   24.372577] [<c03b5994>] (kernel_init+0x194/0x330) from
> [<c000e1d8>] (ret_from_fork+0x14/0x3c)
> [   24.398371]
> 
> 
> On Thu, Dec 27, 2012 at 8:25 PM, Fabio Baltieri
> <fabio.baltieri@linaro.org> wrote:
> > Add a CPU notifier to start and stop individual core timers on CPU
> > hotplug events when running on CPUs with SW coordinated frequency.
> >
> > Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> > ---
> >  drivers/cpufreq/cpufreq_governor.c | 51 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> > index b0e4506..e881250 100644
> > --- a/drivers/cpufreq/cpufreq_governor.c
> > +++ b/drivers/cpufreq/cpufreq_governor.c
> > @@ -25,9 +25,12 @@
> >  #include <linux/tick.h>
> >  #include <linux/types.h>
> >  #include <linux/workqueue.h>
> > +#include <linux/cpu.h>
> >
> >  #include "cpufreq_governor.h"
> >
> > +static DEFINE_PER_CPU(struct dbs_data *, cpu_cur_dbs);
> > +
> >  static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
> >  {
> >         u64 idle_time;
> > @@ -185,6 +188,46 @@ static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
> >         cancel_delayed_work_sync(&cdbs->work);
> >  }
> >
> > +static int __cpuinit cpu_callback(struct notifier_block *nfb,
> > +               unsigned long action, void *hcpu)
> > +{
> > +       unsigned int cpu = (unsigned long)hcpu;
> > +       struct device *cpu_dev = get_cpu_device(cpu);
> > +       struct dbs_data *dbs_data = per_cpu(cpu_cur_dbs, cpu);
> > +       struct cpu_dbs_common_info *cpu_cdbs = dbs_data->get_cpu_cdbs(cpu);
> > +       unsigned int sampling_rate;
> > +
> > +       if (dbs_data->governor == GOV_CONSERVATIVE) {
> > +               struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
> > +               sampling_rate = cs_tuners->sampling_rate;
> > +       } else {
> > +               struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> > +               sampling_rate = od_tuners->sampling_rate;
> > +       }
> > +
> > +       if (cpu_dev) {
> > +               switch (action) {
> > +               case CPU_ONLINE:
> > +               case CPU_ONLINE_FROZEN:
> > +               case CPU_DOWN_FAILED:
> > +               case CPU_DOWN_FAILED_FROZEN:
> > +                       dbs_timer_init(dbs_data, cpu_cdbs,
> > +                                       sampling_rate, cpu);
> > +                       break;
> > +               case CPU_DOWN_PREPARE:
> > +               case CPU_DOWN_PREPARE_FROZEN:
> > +                       dbs_timer_exit(cpu_cdbs);
> > +                       break;
> > +               }
> > +       }
> > +
> > +       return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block __refdata ondemand_cpu_notifier = {
> 
> Over that, why is it called ondemand here ?
> 
> > +       .notifier_call = cpu_callback,
> > +};
> > +
> >  int cpufreq_governor_dbs(struct dbs_data *dbs_data,
> >                 struct cpufreq_policy *policy, unsigned int event)
> >  {
> > @@ -296,7 +339,11 @@ second_time:
> >                                 j_cdbs = dbs_data->get_cpu_cdbs(j);
> >                                 dbs_timer_init(dbs_data, j_cdbs,
> >                                                *sampling_rate, j);
> > +
> > +                               per_cpu(cpu_cur_dbs, j) = dbs_data;
> >                         }
> > +
> > +                       register_hotcpu_notifier(&ondemand_cpu_notifier);
> >                 } else {
> >                         dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu);
> >                 }
> > @@ -307,11 +354,15 @@ second_time:
> >                         cs_dbs_info->enable = 0;
> >
> >                 if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
> > +                       unregister_hotcpu_notifier(&ondemand_cpu_notifier);
> > +
> >                         for_each_cpu(j, policy->cpus) {
> >                                 struct cpu_dbs_common_info *j_cdbs;
> >
> >                                 j_cdbs = dbs_data->get_cpu_cdbs(j);
> >                                 dbs_timer_exit(j_cdbs);
> > +
> > +                               per_cpu(cpu_cur_dbs, j) = NULL;
> >                         }
> >                 } else {
> >                         dbs_timer_exit(cpu_cdbs);
> 
> --
> viresh

Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Viresh Kumar]

I am starting to follow cpufreq patches religiously now and so have to come
back to this old thread due to some crash we got :)

Its still not pushed upstream, so better to get it resolved before 3.9.

On Thu, Dec 27, 2012 at 8:25 PM, Fabio Baltieri
<fabio.baltieri@linaro.org> wrote:

> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c

>  static inline void dbs_timer_init(struct dbs_data *dbs_data,
> -               struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
> +                                 struct cpu_dbs_common_info *cdbs,
> +                                 unsigned int sampling_rate,
> +                                 int cpu)
>  {
>         int delay = delay_for_sampling_rate(sampling_rate);
> +       struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu);

I couldn't understand the real need for this, as it should really give
back the same
pointer pointed out by: cdbs and hence no need of cpu in params too..

I may be wrong here :)

>
> -       INIT_DEFERRABLE_WORK(&cdbs->work, dbs_data->gov_dbs_timer);
> -       schedule_delayed_work_on(cdbs->cpu, &cdbs->work, delay);
> +       schedule_delayed_work_on(cpu, &cdbs_local->work, delay);
>  }
>
>  static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
> @@ -217,6 +227,10 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
>                         if (ignore_nice)
>                                 j_cdbs->prev_cpu_nice =
>                                         kcpustat_cpu(j).cpustat[CPUTIME_NICE];
> +
> +                       mutex_init(&j_cdbs->timer_mutex);
> +                       INIT_DEFERRABLE_WORK(&j_cdbs->work,
> +                                            dbs_data->gov_dbs_timer);
>                 }
>
>                 /*
> @@ -275,15 +289,33 @@ second_time:
>                 }
>                 mutex_unlock(&dbs_data->mutex);
>
> -               mutex_init(&cpu_cdbs->timer_mutex);
> -               dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
> +               if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
> +                       for_each_cpu(j, policy->cpus) {
> +                               struct cpu_dbs_common_info *j_cdbs;
> +
> +                               j_cdbs = dbs_data->get_cpu_cdbs(j);
> +                               dbs_timer_init(dbs_data, j_cdbs,
> +                                              *sampling_rate, j);
> +                       }
> +               } else {
> +                       dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu);
> +               }

do you really need this else part? In case of uniprocessor systems also, the if
block should be enough. Isn't it?

>                 break;
>
>         case CPUFREQ_GOV_STOP:
>                 if (dbs_data->governor == GOV_CONSERVATIVE)
>                         cs_dbs_info->enable = 0;
>
> -               dbs_timer_exit(cpu_cdbs);
> +               if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
> +                       for_each_cpu(j, policy->cpus) {
> +                               struct cpu_dbs_common_info *j_cdbs;
> +
> +                               j_cdbs = dbs_data->get_cpu_cdbs(j);
> +                               dbs_timer_exit(j_cdbs);
> +                       }
> +               } else {
> +                       dbs_timer_exit(cpu_cdbs);
> +               }

ditto.

>                 mutex_lock(&dbs_data->mutex);
>                 mutex_destroy(&cpu_cdbs->timer_mutex);
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index f661654..5bf6fb8 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -171,6 +171,7 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate)
>
>  u64 get_cpu_idle_time(unsigned int cpu, u64 *wall);
>  void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
> +bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs);
>  int cpufreq_governor_dbs(struct dbs_data *dbs_data,
>                 struct cpufreq_policy *policy, unsigned int event);
>  #endif /* _CPUFREQ_GOVERNER_H */
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 7731f7c..93bb56d 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -243,7 +243,8 @@ static void od_dbs_timer(struct work_struct *work)
>                 }
>         }
>
> -       schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay);
> +       schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,
> +                       delay);
>         mutex_unlock(&dbs_info->cdbs.timer_mutex);
>  }
>
> --
> 1.7.12.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Fabio Baltieri]

Hello Viresh,

On Wed, Jan 30, 2013 at 12:33:40PM +0530, Viresh Kumar wrote:
> I am starting to follow cpufreq patches religiously now and so have to come
> back to this old thread due to some crash we got :)
> 
> Its still not pushed upstream, so better to get it resolved before 3.9.

Definitely, that's what we have -next for!

> On Thu, Dec 27, 2012 at 8:25 PM, Fabio Baltieri
> <fabio.baltieri@linaro.org> wrote:
> 
> > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> 
> >  static inline void dbs_timer_init(struct dbs_data *dbs_data,
> > -               struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
> > +                                 struct cpu_dbs_common_info *cdbs,
> > +                                 unsigned int sampling_rate,
> > +                                 int cpu)
> >  {
> >         int delay = delay_for_sampling_rate(sampling_rate);
> > +       struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu);
> 
> I couldn't understand the real need for this, as it should really give
> back the same
> pointer pointed out by: cdbs and hence no need of cpu in params too..
> 
> I may be wrong here :)

You are actually right.  This comes from the first version of the patch
(I basically rewrote it after the common code rafactoring), and cdbs was
meant to be always the one for the master CPU while cpu should indicate
the one being initialized.  Then the thing turned out as:

A - I dropped the code specific for master cdbs here as it was already
there on another code path following the rafactoring.
B - I passed j_cdbs = dbs_data->get_cpu_cdbs(j) in the init cycle while
it was really meant to be get_cpu_cdbs(cpu).

> >
> > -       INIT_DEFERRABLE_WORK(&cdbs->work, dbs_data->gov_dbs_timer);
> > -       schedule_delayed_work_on(cdbs->cpu, &cdbs->work, delay);
> > +       schedule_delayed_work_on(cpu, &cdbs_local->work, delay);
> >  }
> >
> >  static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
> > @@ -217,6 +227,10 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
> >                         if (ignore_nice)
> >                                 j_cdbs->prev_cpu_nice =
> >                                         kcpustat_cpu(j).cpustat[CPUTIME_NICE];
> > +
> > +                       mutex_init(&j_cdbs->timer_mutex);
> > +                       INIT_DEFERRABLE_WORK(&j_cdbs->work,
> > +                                            dbs_data->gov_dbs_timer);
> >                 }
> >
> >                 /*
> > @@ -275,15 +289,33 @@ second_time:
> >                 }
> >                 mutex_unlock(&dbs_data->mutex);
> >
> > -               mutex_init(&cpu_cdbs->timer_mutex);
> > -               dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
> > +               if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
> > +                       for_each_cpu(j, policy->cpus) {
> > +                               struct cpu_dbs_common_info *j_cdbs;
> > +
> > +                               j_cdbs = dbs_data->get_cpu_cdbs(j);
> > +                               dbs_timer_init(dbs_data, j_cdbs,
> > +                                              *sampling_rate, j);
> > +                       }
> > +               } else {
> > +                       dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu);
> > +               }
> 
> do you really need this else part? In case of uniprocessor systems also, the if
> block should be enough. Isn't it?

Same reason, get_cpu_cdbs(j) was meant to be get_cpu_cdbs(cpu) but
that's not used anymore in the last version of the patch, and the same
for the last hunk.

I'll send a patch to clean this up, thanks for spotting it!

Fabio

> >                 break;
> >
> >         case CPUFREQ_GOV_STOP:
> >                 if (dbs_data->governor == GOV_CONSERVATIVE)
> >                         cs_dbs_info->enable = 0;
> >
> > -               dbs_timer_exit(cpu_cdbs);
> > +               if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
> > +                       for_each_cpu(j, policy->cpus) {
> > +                               struct cpu_dbs_common_info *j_cdbs;
> > +
> > +                               j_cdbs = dbs_data->get_cpu_cdbs(j);
> > +                               dbs_timer_exit(j_cdbs);
> > +                       }
> > +               } else {
> > +                       dbs_timer_exit(cpu_cdbs);
> > +               }
> 
> ditto.

-- 
Fabio Baltieri

Re: [PATCH 2/5] cpufreq: star/stop cpufreq timers on cpu hotplug

[Fabio Baltieri]

Hi Viresh,

On Wed, Jan 30, 2013 at 10:22:37AM +0530, Viresh Kumar wrote:
> Hi Fabio,
> 
> Sorry for waking up very late :)
> 
> The reason why i am starting this thread again is due to problem
> reported by Joseph,
> with latest linux-next/master branch (which contains few big patches
> from me :) ):
> 
> Reboot is giving following to him:
> 
> * Will now halt
> [ 193.756068] Disabling non-boot CPUs...
> [ 193.760088] BUG: scheduling while atomic: halt/780/0x00000002
> [ 193.765845] Modules linked in: brcmfmac brcmutil
> [ 193.770613] [<c0014990>] (unwind_backtrace+0x0/0xf8) from [<c0049510>]
> (__schedule_bug+0x44/0x5c)
> [ 193.779548] [<c0049510>] (__schedule_bug+0x44/0x5c) from [<c04fa320>]
> (__schedule+0x688/0x6ec)
> [ 193.788206] [<c04fa320>] (__schedule+0x688/0x6ec) from [<c04fa75c>]
> (schedule_preempt_disabled+0x24/0x34)
> [ 193.797811] [<c04fa75c>] (schedule_preempt_disabled+0x24/0x34) from
> [<c04f916c>] (__mutex_lock_slowpath+0x170/0x34c)
> [ 193.808367] [<c04f916c>] (__mutex_lock_slowpath+0x170/0x34c) from
> [<c04f9354>] (mutex_lock+0xc/0x24)
> [ 193.817554] [<c04f9354>] (mutex_lock+0xc/0x24) from [<c04f1cdc>]
> (unregister_cpu_notifier+0xc/0x24)
> [ 193.826640] [<c04f1cdc>] (unregister_cpu_notifier+0xc/0x24) from
> [<c033a8a4>] (cpufreq_governor_dbs+0x118/0x614)
> [ 193.836866] [<c033a8a4>] (cpufreq_governor_dbs+0x118/0x614) from
> [<c033747c>] (__cpufreq_governor+0x58/0xc0)
> [ 193.846737] [<c033747c>] (__cpufreq_governor+0x58/0xc0) from
> [<c0339104>] (__cpufreq_remove_dev.clone.7+0x58/0x320)
> [ 193.857207] [<c0339104>] (__cpufreq_remove_dev.clone.7+0x58/0x320)
> from [<c04f7958>] (cpufreq_cpu_callback+0x8c/0x9c)
> [ 193.867850] [<c04f7958>] (cpufreq_cpu_callback+0x8c/0x9c) from
> [<c0044f4c>] (notifier_call_chain+0x44/0x84)
> [ 193.877623] [<c0044f4c>] (notifier_call_chain+0x44/0x84) from
> [<c0026e24>] (__cpu_notify+0x2c/0x48)
> [ 193.886704] [<c0026e24>] (__cpu_notify+0x2c/0x48) from [<c04f1b08>]
> (_cpu_down+0xb0/0x23c)
> [ 193.895004] [<c04f1b08>] (_cpu_down+0xb0/0x23c) from [<c00270ec>]
> (disable_nonboot_cpus+0x68/0x104)
> [ 193.904089] [<c00270ec>] (disable_nonboot_cpus+0x68/0x104) from
> [<c0034fbc>] (kernel_power_off+0x24/0x48)
> [ 193.913688] [<c0034fbc>] (kernel_power_off+0x24/0x48) from
> [<c0035810>] (sys_reboot+0x104/0x1e0)
> [ 193.922517] [<c0035810>] (sys_reboot+0x104/0x1e0) from [<c000e520>]
> (ret_fast_syscall+0x0/0x30)
> 
> 
> And the crash log show this patch of yours somewhere :)

It looks like the two patches clashed togher quite badly... :-)

On Wed, Jan 30, 2013 at 10:22:37AM +0530, Viresh Kumar wrote:
[...]
> First question: Is this patch still required? Because following patch
> from me is
> sending a STOP/START to governors on cpu hot-[un]plug ?
> 
> commit dbcb63407c095af73f3464767e00902cdee55e8b
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sat Jan 12 05:14:39 2013 +0000
> 
>     cpufreq: Notify governors when cpus are hot-[un]plugged
> 
> For me, the answer is NO.

I confirm that your patch handles correctly the problem solved by this
one so I agree on dropping mine.

Rafael, this is screwing up a bit on bisection for cpu hotplug problems
so I'm sending a v7 with the cleanup on first patch and this one
dropped if you are ok with rebasing your pm-cpufreq-next.  Let me know
if you prefer me to just send a revert + cleanup patch instead.

Thanks,
Fabio

-- 
Fabio Baltieri

Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Fabio Baltieri]

On Wed, Jan 30, 2013 at 10:14:53AM +0100, Fabio Baltieri wrote:
> Hello Viresh,
> 
> On Wed, Jan 30, 2013 at 12:33:40PM +0530, Viresh Kumar wrote:
> > I am starting to follow cpufreq patches religiously now and so have to come
> > back to this old thread due to some crash we got :)
> > 
> > Its still not pushed upstream, so better to get it resolved before 3.9.
> 
> Definitely, that's what we have -next for!
> 
> > On Thu, Dec 27, 2012 at 8:25 PM, Fabio Baltieri
> > <fabio.baltieri@linaro.org> wrote:
> > 
> > > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> > 
> > >  static inline void dbs_timer_init(struct dbs_data *dbs_data,
> > > -               struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
> > > +                                 struct cpu_dbs_common_info *cdbs,
> > > +                                 unsigned int sampling_rate,
> > > +                                 int cpu)
> > >  {
> > >         int delay = delay_for_sampling_rate(sampling_rate);
> > > +       struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu);
> > 
> > I couldn't understand the real need for this, as it should really give
> > back the same
> > pointer pointed out by: cdbs and hence no need of cpu in params too..

Small sidenote, actually what I'm going to drop here i *cdbs, as I need
cpu for schedule_delayed_work_on and can't use cdbs->cpu for that as
it's the master's one.

Fabio

> > 
> > I may be wrong here :)
> 
> You are actually right.  This comes from the first version of the patch
> (I basically rewrote it after the common code rafactoring), and cdbs was
> meant to be always the one for the master CPU while cpu should indicate
> the one being initialized.  Then the thing turned out as:
> 
> A - I dropped the code specific for master cdbs here as it was already
> there on another code path following the rafactoring.
> B - I passed j_cdbs = dbs_data->get_cpu_cdbs(j) in the init cycle while
> it was really meant to be get_cpu_cdbs(cpu).
> 
> > >
> > > -       INIT_DEFERRABLE_WORK(&cdbs->work, dbs_data->gov_dbs_timer);
> > > -       schedule_delayed_work_on(cdbs->cpu, &cdbs->work, delay);
> > > +       schedule_delayed_work_on(cpu, &cdbs_local->work, delay);
> > >  }

-- 
Fabio Baltieri

Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Viresh Kumar]

On 30 January 2013 16:34, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> Small sidenote, actually what I'm going to drop here i *cdbs, as I need
> cpu for schedule_delayed_work_on and can't use cdbs->cpu for that as
> it's the master's one.

I can't find code which would do j_cdbs->cpu = j and so j_cdbs->cpu is
un-initialized. So, if that's true, you can initialize that and drop
cpu param too.

Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Fabio Baltieri]

On Wed, Jan 30, 2013 at 04:47:58PM +0530, Viresh Kumar wrote:
> On 30 January 2013 16:34, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> > Small sidenote, actually what I'm going to drop here i *cdbs, as I need
> > cpu for schedule_delayed_work_on and can't use cdbs->cpu for that as
> > it's the master's one.
> 
> I can't find code which would do j_cdbs->cpu = j and so j_cdbs->cpu is
> un-initialized. So, if that's true, you can initialize that and drop
> cpu param too.

Right, it looks like cdbs->cpu is not really needed anymore.  Anyway I'm
quite happy with how it came out passing cpu number, code is a bit more
compact, so I'm sending that version and than we can discuss if recycle
cdbs->cpu or just drop it.

Fabio

-- 
Fabio Baltieri

Re: [PATCH 2/5] cpufreq: star/stop cpufreq timers on cpu hotplug

[Rafael J. Wysocki]

On Wednesday, January 30, 2013 11:44:32 AM Fabio Baltieri wrote:
> Hi Viresh,
> 
> On Wed, Jan 30, 2013 at 10:22:37AM +0530, Viresh Kumar wrote:
> > Hi Fabio,
> > 
> > Sorry for waking up very late :)
> > 
> > The reason why i am starting this thread again is due to problem
> > reported by Joseph,
> > with latest linux-next/master branch (which contains few big patches
> > from me :) ):
> > 
> > Reboot is giving following to him:
> > 
> > * Will now halt
> > [ 193.756068] Disabling non-boot CPUs...
> > [ 193.760088] BUG: scheduling while atomic: halt/780/0x00000002
> > [ 193.765845] Modules linked in: brcmfmac brcmutil
> > [ 193.770613] [<c0014990>] (unwind_backtrace+0x0/0xf8) from [<c0049510>]
> > (__schedule_bug+0x44/0x5c)
> > [ 193.779548] [<c0049510>] (__schedule_bug+0x44/0x5c) from [<c04fa320>]
> > (__schedule+0x688/0x6ec)
> > [ 193.788206] [<c04fa320>] (__schedule+0x688/0x6ec) from [<c04fa75c>]
> > (schedule_preempt_disabled+0x24/0x34)
> > [ 193.797811] [<c04fa75c>] (schedule_preempt_disabled+0x24/0x34) from
> > [<c04f916c>] (__mutex_lock_slowpath+0x170/0x34c)
> > [ 193.808367] [<c04f916c>] (__mutex_lock_slowpath+0x170/0x34c) from
> > [<c04f9354>] (mutex_lock+0xc/0x24)
> > [ 193.817554] [<c04f9354>] (mutex_lock+0xc/0x24) from [<c04f1cdc>]
> > (unregister_cpu_notifier+0xc/0x24)
> > [ 193.826640] [<c04f1cdc>] (unregister_cpu_notifier+0xc/0x24) from
> > [<c033a8a4>] (cpufreq_governor_dbs+0x118/0x614)
> > [ 193.836866] [<c033a8a4>] (cpufreq_governor_dbs+0x118/0x614) from
> > [<c033747c>] (__cpufreq_governor+0x58/0xc0)
> > [ 193.846737] [<c033747c>] (__cpufreq_governor+0x58/0xc0) from
> > [<c0339104>] (__cpufreq_remove_dev.clone.7+0x58/0x320)
> > [ 193.857207] [<c0339104>] (__cpufreq_remove_dev.clone.7+0x58/0x320)
> > from [<c04f7958>] (cpufreq_cpu_callback+0x8c/0x9c)
> > [ 193.867850] [<c04f7958>] (cpufreq_cpu_callback+0x8c/0x9c) from
> > [<c0044f4c>] (notifier_call_chain+0x44/0x84)
> > [ 193.877623] [<c0044f4c>] (notifier_call_chain+0x44/0x84) from
> > [<c0026e24>] (__cpu_notify+0x2c/0x48)
> > [ 193.886704] [<c0026e24>] (__cpu_notify+0x2c/0x48) from [<c04f1b08>]
> > (_cpu_down+0xb0/0x23c)
> > [ 193.895004] [<c04f1b08>] (_cpu_down+0xb0/0x23c) from [<c00270ec>]
> > (disable_nonboot_cpus+0x68/0x104)
> > [ 193.904089] [<c00270ec>] (disable_nonboot_cpus+0x68/0x104) from
> > [<c0034fbc>] (kernel_power_off+0x24/0x48)
> > [ 193.913688] [<c0034fbc>] (kernel_power_off+0x24/0x48) from
> > [<c0035810>] (sys_reboot+0x104/0x1e0)
> > [ 193.922517] [<c0035810>] (sys_reboot+0x104/0x1e0) from [<c000e520>]
> > (ret_fast_syscall+0x0/0x30)
> > 
> > 
> > And the crash log show this patch of yours somewhere :)
> 
> It looks like the two patches clashed togher quite badly... :-)
> 
> On Wed, Jan 30, 2013 at 10:22:37AM +0530, Viresh Kumar wrote:
> [...]
> > First question: Is this patch still required? Because following patch
> > from me is
> > sending a STOP/START to governors on cpu hot-[un]plug ?
> > 
> > commit dbcb63407c095af73f3464767e00902cdee55e8b
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Sat Jan 12 05:14:39 2013 +0000
> > 
> >     cpufreq: Notify governors when cpus are hot-[un]plugged
> > 
> > For me, the answer is NO.
> 
> I confirm that your patch handles correctly the problem solved by this
> one so I agree on dropping mine.
> 
> Rafael, this is screwing up a bit on bisection for cpu hotplug problems
> so I'm sending a v7 with the cleanup on first patch and this one
> dropped if you are ok with rebasing your pm-cpufreq-next.  Let me know
> if you prefer me to just send a revert + cleanup patch instead.

I've dropped the $subject one already and please post an incremental fix on
top of the first one in your series.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Rafael J. Wysocki]

On Wednesday, November 28, 2012 11:51:20 AM Fabio Baltieri wrote:
> Hello Rafael,

Hi,

> On Tue, Nov 27, 2012 at 11:05:52PM +0100, Rafael J. Wysocki wrote:
> > >  static inline void dbs_timer_init(struct dbs_data *dbs_data,
> > > -		struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
> > > +				  struct cpu_dbs_common_info *cdbs,
> > > +				  unsigned int sampling_rate,
> > > +				  int cpu)
> > >  {
> > >  	int delay = delay_for_sampling_rate(sampling_rate);
> > > +	struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu);
> > > +	struct od_cpu_dbs_info_s *od_dbs_info;
> > > +
> > > +	cancel_delayed_work_sync(&cdbs_local->work);
> > > +
> > > +	if (dbs_data->governor == GOV_ONDEMAND) {
> > > +		od_dbs_info = dbs_data->get_cpu_dbs_info_s(cpu);
> > > +		od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
> > > +	}
> > 
> > The patch looks good in general except for the special case above.
> > 
> > Why exactly is it necessary?
> 
> Now that you point it out... it's not!  It was part of ondemand init and
> moved in cpufreq_governor_dbs, I forgot to take it out the way.
> 
> Also, I think that cancel_delayed_work_sync can be removed too.
> 
> Should I send an updated version as soon as I get an ack for the other
> patches in the series or do you want me to wait until 3.8-rc1?

Well, if it's not very urgent, I'd prefer it to wait a bit longer,
get some more testing and so on.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Fabio Baltieri]

Hello Rafael,

On Tue, Nov 27, 2012 at 11:05:52PM +0100, Rafael J. Wysocki wrote:
> >  static inline void dbs_timer_init(struct dbs_data *dbs_data,
> > -		struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
> > +				  struct cpu_dbs_common_info *cdbs,
> > +				  unsigned int sampling_rate,
> > +				  int cpu)
> >  {
> >  	int delay = delay_for_sampling_rate(sampling_rate);
> > +	struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu);
> > +	struct od_cpu_dbs_info_s *od_dbs_info;
> > +
> > +	cancel_delayed_work_sync(&cdbs_local->work);
> > +
> > +	if (dbs_data->governor == GOV_ONDEMAND) {
> > +		od_dbs_info = dbs_data->get_cpu_dbs_info_s(cpu);
> > +		od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
> > +	}
> 
> The patch looks good in general except for the special case above.
> 
> Why exactly is it necessary?

Now that you point it out... it's not!  It was part of ondemand init and
moved in cpufreq_governor_dbs, I forgot to take it out the way.

Also, I think that cancel_delayed_work_sync can be removed too.

Should I send an updated version as soon as I get an ack for the other
patches in the series or do you want me to wait until 3.8-rc1?

Thanks,
Fabio

-- 
Fabio Baltieri

Re: [PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Rafael J. Wysocki]

On Monday, November 26, 2012 05:39:52 PM Fabio Baltieri wrote:
> From: Rickard Andersson <rickard.andersson@stericsson.com>
> 
> This patch fixes a bug that occurred when we had load on a secondary CPU
> and the primary CPU was sleeping. Only one sampling timer was spawned
> and it was spawned as a deferred timer on the primary CPU, so when a
> secondary CPU had a change in load this was not detected by the cpufreq
> governor (both ondemand and conservative).
> 
> This patch make sure that deferred timers are run on all CPUs in the
> case of software controlled CPUs that run on the same frequency.
> 
> Signed-off-by: Rickard Andersson <rickard.andersson@stericsson.com>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_conservative.c |  3 +-
>  drivers/cpufreq/cpufreq_governor.c     | 52 ++++++++++++++++++++++++++++++----
>  drivers/cpufreq/cpufreq_governor.h     |  1 +
>  drivers/cpufreq/cpufreq_ondemand.c     |  3 +-
>  4 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 64ef737..b9d7f14 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -122,7 +122,8 @@ static void cs_dbs_timer(struct work_struct *work)
>  
>  	dbs_check_cpu(&cs_dbs_data, cpu);
>  
> -	schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay);
> +	schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,
> +			delay);
>  	mutex_unlock(&dbs_info->cdbs.timer_mutex);
>  }
>  
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 6c5f1d3..a00f02d 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -161,13 +161,31 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>  }
>  EXPORT_SYMBOL_GPL(dbs_check_cpu);
>  
> +bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs)
> +{
> +	struct cpufreq_policy *policy = cdbs->cur_policy;
> +
> +	return cpumask_weight(policy->cpus) > 1;
> +}
> +EXPORT_SYMBOL_GPL(dbs_sw_coordinated_cpus);
> +
>  static inline void dbs_timer_init(struct dbs_data *dbs_data,
> -		struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
> +				  struct cpu_dbs_common_info *cdbs,
> +				  unsigned int sampling_rate,
> +				  int cpu)
>  {
>  	int delay = delay_for_sampling_rate(sampling_rate);
> +	struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu);
> +	struct od_cpu_dbs_info_s *od_dbs_info;
> +
> +	cancel_delayed_work_sync(&cdbs_local->work);
> +
> +	if (dbs_data->governor == GOV_ONDEMAND) {
> +		od_dbs_info = dbs_data->get_cpu_dbs_info_s(cpu);
> +		od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
> +	}

The patch looks good in general except for the special case above.

Why exactly is it necessary?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/5] cpufreq: handle SW coordinated CPUs

[Fabio Baltieri]

From: Rickard Andersson <rickard.andersson@stericsson.com>

This patch fixes a bug that occurred when we had load on a secondary CPU
and the primary CPU was sleeping. Only one sampling timer was spawned
and it was spawned as a deferred timer on the primary CPU, so when a
secondary CPU had a change in load this was not detected by the cpufreq
governor (both ondemand and conservative).

This patch make sure that deferred timers are run on all CPUs in the
case of software controlled CPUs that run on the same frequency.

Signed-off-by: Rickard Andersson <rickard.andersson@stericsson.com>
Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c |  3 +-
 drivers/cpufreq/cpufreq_governor.c     | 52 ++++++++++++++++++++++++++++++----
 drivers/cpufreq/cpufreq_governor.h     |  1 +
 drivers/cpufreq/cpufreq_ondemand.c     |  3 +-
 4 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 64ef737..b9d7f14 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -122,7 +122,8 @@ static void cs_dbs_timer(struct work_struct *work)
 
 	dbs_check_cpu(&cs_dbs_data, cpu);
 
-	schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay);
+	schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,
+			delay);
 	mutex_unlock(&dbs_info->cdbs.timer_mutex);
 }
 
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 6c5f1d3..a00f02d 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -161,13 +161,31 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 }
 EXPORT_SYMBOL_GPL(dbs_check_cpu);
 
+bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs)
+{
+	struct cpufreq_policy *policy = cdbs->cur_policy;
+
+	return cpumask_weight(policy->cpus) > 1;
+}
+EXPORT_SYMBOL_GPL(dbs_sw_coordinated_cpus);
+
 static inline void dbs_timer_init(struct dbs_data *dbs_data,
-		struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
+				  struct cpu_dbs_common_info *cdbs,
+				  unsigned int sampling_rate,
+				  int cpu)
 {
 	int delay = delay_for_sampling_rate(sampling_rate);
+	struct cpu_dbs_common_info *cdbs_local = dbs_data->get_cpu_cdbs(cpu);
+	struct od_cpu_dbs_info_s *od_dbs_info;
+
+	cancel_delayed_work_sync(&cdbs_local->work);
+
+	if (dbs_data->governor == GOV_ONDEMAND) {
+		od_dbs_info = dbs_data->get_cpu_dbs_info_s(cpu);
+		od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
+	}
 
-	INIT_DEFERRABLE_WORK(&cdbs->work, dbs_data->gov_dbs_timer);
-	schedule_delayed_work_on(cdbs->cpu, &cdbs->work, delay);
+	schedule_delayed_work_on(cpu, &cdbs_local->work, delay);
 }
 
 static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
@@ -217,6 +235,10 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 			if (ignore_nice)
 				j_cdbs->prev_cpu_nice =
 					kcpustat_cpu(j).cpustat[CPUTIME_NICE];
+
+			mutex_init(&j_cdbs->timer_mutex);
+			INIT_DEFERRABLE_WORK(&j_cdbs->work,
+					     dbs_data->gov_dbs_timer);
 		}
 
 		/*
@@ -275,15 +297,33 @@ second_time:
 		}
 		mutex_unlock(&dbs_data->mutex);
 
-		mutex_init(&cpu_cdbs->timer_mutex);
-		dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
+		if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
+			for_each_cpu(j, policy->cpus) {
+				struct cpu_dbs_common_info *j_cdbs;
+
+				j_cdbs = dbs_data->get_cpu_cdbs(j);
+				dbs_timer_init(dbs_data, j_cdbs,
+					       *sampling_rate, j);
+			}
+		} else {
+			dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate, cpu);
+		}
 		break;
 
 	case CPUFREQ_GOV_STOP:
 		if (dbs_data->governor == GOV_CONSERVATIVE)
 			cs_dbs_info->enable = 0;
 
-		dbs_timer_exit(cpu_cdbs);
+		if (dbs_sw_coordinated_cpus(cpu_cdbs)) {
+			for_each_cpu(j, policy->cpus) {
+				struct cpu_dbs_common_info *j_cdbs;
+
+				j_cdbs = dbs_data->get_cpu_cdbs(j);
+				dbs_timer_exit(j_cdbs);
+			}
+		} else {
+			dbs_timer_exit(cpu_cdbs);
+		}
 
 		mutex_lock(&dbs_data->mutex);
 		mutex_destroy(&cpu_cdbs->timer_mutex);
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index f661654..5bf6fb8 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -171,6 +171,7 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate)
 
 u64 get_cpu_idle_time(unsigned int cpu, u64 *wall);
 void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
+bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs);
 int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 		struct cpufreq_policy *policy, unsigned int event);
 #endif /* _CPUFREQ_GOVERNER_H */
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index cca3e9f..fe6e47c 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -239,7 +239,8 @@ static void od_dbs_timer(struct work_struct *work)
 		}
 	}
 
-	schedule_delayed_work_on(cpu, &dbs_info->cdbs.work, delay);
+	schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,
+			delay);
 	mutex_unlock(&dbs_info->cdbs.timer_mutex);
 }
 
-- 
1.7.12.1