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

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

[Fabio Baltieri]

Hello Rafael,

this is the v7 for the cpufreq SW coordinated CPU bug fix, developed
after the discussion with Viresh about some issues that appeared after
the merge of both mine and his patches.

Changes from v6:

* Dropped the cpu hotplug patch, as it was solving part of problem
addressed by Viresh patch:

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

but was behaving erratically when both patches were applied together.

* In the initial patch, cleaned up cpufreq_governor_dbs and
dbs_timer_{init,exit} to remove non necessary variables and special case
initialization, as the additional data provided on that code was not
necessary since v6.  The code is a bit cleaner now.

Would you consider replacing my patches currently on your pm-cpufreq-next
branch with this series?  There should not be any conflicts with existing
patches.  Let me know if you prefer incremental patches instead.

Thanks,
Fabio

Fabio Baltieri (3):
  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     | 34 ++++++++++++++-----
 drivers/cpufreq/cpufreq_governor.h     |  2 ++
 drivers/cpufreq/cpufreq_ondemand.c     | 62 +++++++++++++++++++++++++++-------
 4 files changed, 119 insertions(+), 25 deletions(-)

-- 
1.7.12.1

[PATCH v7 1/4] 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     | 31 +++++++++++++++++++++++--------
 drivers/cpufreq/cpufreq_governor.h     |  1 +
 drivers/cpufreq/cpufreq_ondemand.c     |  3 ++-
 4 files changed, 28 insertions(+), 10 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..8393d6e 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -161,17 +161,27 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 }
 EXPORT_SYMBOL_GPL(dbs_check_cpu);
 
-static inline void dbs_timer_init(struct dbs_data *dbs_data,
-		struct cpu_dbs_common_info *cdbs, unsigned int sampling_rate)
+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, int cpu,
+				  unsigned int sampling_rate)
 {
 	int delay = delay_for_sampling_rate(sampling_rate);
+	struct cpu_dbs_common_info *cdbs = 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->work, delay);
 }
 
-static inline void dbs_timer_exit(struct cpu_dbs_common_info *cdbs)
+static inline void dbs_timer_exit(struct dbs_data *dbs_data, int cpu)
 {
+	struct cpu_dbs_common_info *cdbs = dbs_data->get_cpu_cdbs(cpu);
+
 	cancel_delayed_work_sync(&cdbs->work);
 }
 
@@ -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,16 @@ second_time:
 		}
 		mutex_unlock(&dbs_data->mutex);
 
-		mutex_init(&cpu_cdbs->timer_mutex);
-		dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
+		for_each_cpu(j, policy->cpus)
+			dbs_timer_init(dbs_data, j, *sampling_rate);
 		break;
 
 	case CPUFREQ_GOV_STOP:
 		if (dbs_data->governor == GOV_CONSERVATIVE)
 			cs_dbs_info->enable = 0;
 
-		dbs_timer_exit(cpu_cdbs);
+		for_each_cpu(j, policy->cpus)
+			dbs_timer_exit(dbs_data, j);
 
 		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 v7 2/4] 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 8393d6e..46f96a4 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -289,6 +289,9 @@ second_time:
 		}
 		mutex_unlock(&dbs_data->mutex);
 
+		/* Initiate timer time stamp */
+		cpu_cdbs->time_stamp = ktime_get();
+
 		for_each_cpu(j, policy->cpus)
 			dbs_timer_init(dbs_data, j, *sampling_rate);
 		break;
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 v7 3/4] 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 v7 4/4] 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 v7 1/4] cpufreq: handle SW coordinated CPUs

[Viresh Kumar]

Hi Fabio,

I know Rafael has asked you to send only the incremental patch, but i
am interested in reviewing whole series again, as i haven't reviewed
earlier stuff :)

I believe you are required to send patches to patches@linaro.org too :)

On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@linaro.org> 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.

Its really a good point. I wondered earlier why does interactive governor
has per-cpu timer. BTW, you can check how interactive governor is handling
this requirement. That might improve these drivers too.

> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> +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);

I believe this routine should be rather present in cpufreq core code,
as their might
be other users of it. Its really not related to dbs or governors.

My ideas about the name of routine then:
- policy_is_shared()
- or something better you have :)

> @@ -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);

That doesn't look the right place for doing it. With this change we
will initialize
mutex and work for all cpus whenever cpufreq_governor_dbs(GOV_START) is
called. We really want to do it just before second_time: label, which will do
it only when this is called for the very first time or cpu.

That's what i thought initially :)

Then i looked at my own work and realized something else :).. Now, because
we stop/start governors on every cpu movement, this routine is called only once.

What you can do:
- Create a single for_each_cpu() in GOV_START path
- Get rid of dbs_data->enable routine as we don't need to store the
number of calls
  for GOV_START.

>                 }
>
>                 /*
> @@ -275,15 +289,16 @@ second_time:
>                 }
>                 mutex_unlock(&dbs_data->mutex);
>
> -               mutex_init(&cpu_cdbs->timer_mutex);
> -               dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
> +               for_each_cpu(j, policy->cpus)
> +                       dbs_timer_init(dbs_data, j, *sampling_rate);

Keep this too in the same for_each_cpu() loop and hence get to older
param types of dbs_timer_init(), i.e. don't pass cpu as we already have
j_cdbs in our loop.

>                 break;
>
>         case CPUFREQ_GOV_STOP:
>                 if (dbs_data->governor == GOV_CONSERVATIVE)
>                         cs_dbs_info->enable = 0;
>
> -               dbs_timer_exit(cpu_cdbs);
> +               for_each_cpu(j, policy->cpus)
> +                       dbs_timer_exit(dbs_data, j);
>
>                 mutex_lock(&dbs_data->mutex);
>                 mutex_destroy(&cpu_cdbs->timer_mutex);

Re: [PATCH v7 2/4] cpufreq: ondemand: call dbs_check_cpu only when necessary

[Viresh Kumar]

On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> 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>

I might be wrong but i have some real concerns over this patch.

This is what i believe is your idea:
- because a cpu can sleep, create timer per cpu
- then check load again only when no other cpu in policy->cpus has
  checked load within sampling time interval.

Correct?

> ---
>  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 8393d6e..46f96a4 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -289,6 +289,9 @@ second_time:
>                 }
>                 mutex_unlock(&dbs_data->mutex);
>
> +               /* Initiate timer time stamp */
> +               cpu_cdbs->time_stamp = ktime_get();

We have updated time_stamp only for policy->cpu's cdbs.

>                 for_each_cpu(j, policy->cpus)
>                         dbs_timer_init(dbs_data, j, *sampling_rate);
>                 break;

> 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);
> +}

All good until now.

> +
> +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;
> +

--start--

> +       /* 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;
> +

--end--

Instead of two routines (this and the one below), we can have one and can
put the above code in the if (coordinated cpus case). That will save some
redundant code.

Another issue that i see is: Current routine will be called from timer handler
of every cpu and so above calculations will happen for every cpu. Here, you
are taking the diff of last timestamp of cpu "x" with cpu "x" current timestamp,
but what you should have really done is the diff b/w current timestamp with
the timestamp of last change on any cpu in policy->cpus.

Over that, timestamp for all cpu's isn't initialized early in the code
at GOV_START.

Am i correct?

> +       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);
> +       }
> +}
> +

Re: [PATCH v7 3/4] cpufreq: conservative: call dbs_check_cpu only when necessary

[Viresh Kumar]

On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> 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>

We are again doing the same mistake which i fixed with:

commit 4471a34f9a1f2da220272e823bdb8e8fa83a7661
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Fri Oct 26 00:47:42 2012 +0200

    cpufreq: governors: remove redundant code

    Initially ondemand governor was written and then using its code conservative
    governor is written. It used a lot of code from ondemand governor,
but copy of
    code was created instead of using the same routines from both
governors. Which
    increased code redundancy, which is difficult to manage.

    This patch is an attempt to move common part of both the governors to
    cpufreq_governor.c file to come over above mentioned issues.

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

[Fabio Baltieri]

Hi Viresh,

On Wed, Jan 30, 2013 at 08:32:38PM +0530, Viresh Kumar wrote:
> Hi Fabio,
> 
> I know Rafael has asked you to send only the incremental patch, but i
> am interested in reviewing whole series again, as i haven't reviewed
> earlier stuff :)

Sure, take your chance.

[internal note]
> I believe you are required to send patches to patches@linaro.org too :)

I already have patches@linaro.org in BCC, all my patches are there.
[/internal note]

> On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@linaro.org> 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.
> 
> Its really a good point. I wondered earlier why does interactive governor
> has per-cpu timer. BTW, you can check how interactive governor is handling
> this requirement. That might improve these drivers too.
> 
> > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> > +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);
> 
> I believe this routine should be rather present in cpufreq core code,
> as their might
> be other users of it. Its really not related to dbs or governors.
> 
> My ideas about the name of routine then:
> - policy_is_shared()
> - or something better you have :)

So you are suggesting to rethink this function to be related to policy
rather than dbs... this may as well become an inline in cpufreq.h, as:

static inline bool policy_is_shared(struct cpufreq_policy *policy)
{
        return cpumask_weight(policy->cpus) > 1;
}

I'm not sure about the name through, I like mentioning sw coordination in it
because that's the comment in the declaration:

        cpumask_var_t           cpus;   /* CPUs requiring sw coordination */
        cpumask_var_t           related_cpus; /* CPUs with any coordination */

And those two are already confusing as a starting point.

Anyway, this sounds fine to me.  If you think this is useful I can send
a patch, or feel free to include it in your patches if you plan to do
further cleanup work on this code.

/me tries to also keep that ->cpu field in mind.

> > @@ -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);
> 
> That doesn't look the right place for doing it. With this change we
> will initialize
> mutex and work for all cpus whenever cpufreq_governor_dbs(GOV_START) is
> called. We really want to do it just before second_time: label, which will do
> it only when this is called for the very first time or cpu.
> 
> That's what i thought initially :)
> 
> Then i looked at my own work and realized something else :).. Now, because
> we stop/start governors on every cpu movement, this routine is called only once.
> 
> What you can do:
> - Create a single for_each_cpu() in GOV_START path
> - Get rid of dbs_data->enable routine as we don't need to store the
> number of calls
>   for GOV_START.
> 
> >                 }
> >
> >                 /*
> > @@ -275,15 +289,16 @@ second_time:
> >                 }
> >                 mutex_unlock(&dbs_data->mutex);
> >
> > -               mutex_init(&cpu_cdbs->timer_mutex);
> > -               dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
> > +               for_each_cpu(j, policy->cpus)
> > +                       dbs_timer_init(dbs_data, j, *sampling_rate);
> 
> Keep this too in the same for_each_cpu() loop and hence get to older
> param types of dbs_timer_init(), i.e. don't pass cpu as we already have
> j_cdbs in our loop.

Ok, I can try to implement this if you help testing the patch on your bL
system. :-)

Fabio

> >                 break;
> >
> >         case CPUFREQ_GOV_STOP:
> >                 if (dbs_data->governor == GOV_CONSERVATIVE)
> >                         cs_dbs_info->enable = 0;
> >
> > -               dbs_timer_exit(cpu_cdbs);
> > +               for_each_cpu(j, policy->cpus)
> > +                       dbs_timer_exit(dbs_data, j);
> >
> >                 mutex_lock(&dbs_data->mutex);
> >                 mutex_destroy(&cpu_cdbs->timer_mutex);

-- 
Fabio Baltieri

Re: [PATCH v7 2/4] cpufreq: ondemand: call dbs_check_cpu only when necessary

[Fabio Baltieri]

On Wed, Jan 30, 2013 at 09:21:41PM +0530, Viresh Kumar wrote:
> On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> > 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>
> 
> I might be wrong but i have some real concerns over this patch.
> 
> This is what i believe is your idea:
> - because a cpu can sleep, create timer per cpu
> - then check load again only when no other cpu in policy->cpus has
>   checked load within sampling time interval.
> 
> Correct?

Yes.

> > ---
> >  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 8393d6e..46f96a4 100644
> > --- a/drivers/cpufreq/cpufreq_governor.c
> > +++ b/drivers/cpufreq/cpufreq_governor.c
> > @@ -289,6 +289,9 @@ second_time:
> >                 }
> >                 mutex_unlock(&dbs_data->mutex);
> >
> > +               /* Initiate timer time stamp */
> > +               cpu_cdbs->time_stamp = ktime_get();
> 
> We have updated time_stamp only for policy->cpu's cdbs.

Yes, see below.

> >                 for_each_cpu(j, policy->cpus)
> >                         dbs_timer_init(dbs_data, j, *sampling_rate);
> >                 break;
> 
> > 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);
> > +}
> 
> All good until now.
> 
> > +
> > +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;
> > +
> 
> --start--
> 
> > +       /* 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;
> > +
> 
> --end--
> 
> Instead of two routines (this and the one below), we can have one and can
> put the above code in the if (coordinated cpus case). That will save some
> redundant code.

Ok but then you have two dbs_infos mixing up in the same code and it
start to become harder to read (first version was with a single function
I think).

> Another issue that i see is: Current routine will be called from timer handler
> of every cpu and so above calculations will happen for every cpu. Here, you
> are taking the diff of last timestamp of cpu "x" with cpu "x" current timestamp,
> but what you should have really done is the diff b/w current timestamp with
> the timestamp of last change on any cpu in policy->cpus.

Isn't that how it works now?  The current cpu ktime is not checked
against its own, but against the "leader" cpu (dbs_info_local->cdbs.cpu),
that's why it's initialized only for the first.

Maybe I should have used dbs_info_leader/dbs_info instead of
dbs_info_local/dbs_info.

> Over that, timestamp for all cpu's isn't initialized early in the code
> at GOV_START.
> 
> Am i correct?

As above.

Fabio

> > +       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);
> > +       }
> > +}
> > +

-- 
Fabio Baltieri

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

[Viresh Kumar]

On 30 January 2013 21:53, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> On Wed, Jan 30, 2013 at 08:32:38PM +0530, Viresh Kumar wrote:

>> I believe this routine should be rather present in cpufreq core code,
>> as their might
>> be other users of it. Its really not related to dbs or governors.
>>
>> My ideas about the name of routine then:
>> - policy_is_shared()
>> - or something better you have :)
>
> So you are suggesting to rethink this function to be related to policy
> rather than dbs... this may as well become an inline in cpufreq.h, as:
>
> static inline bool policy_is_shared(struct cpufreq_policy *policy)
> {
>         return cpumask_weight(policy->cpus) > 1;
> }

Perfect.

> I'm not sure about the name through, I like mentioning sw coordination in it
> because that's the comment in the declaration:
>
>         cpumask_var_t           cpus;   /* CPUs requiring sw coordination */
>         cpumask_var_t           related_cpus; /* CPUs with any coordination */
>
> And those two are already confusing as a starting point.

I will fix these comments with a patch of mine.

> Anyway, this sounds fine to me.  If you think this is useful I can send
> a patch, or feel free to include it in your patches if you plan to do
> further cleanup work on this code.
>
> /me tries to also keep that ->cpu field in mind.

You can make it part of your patchsets v8.

Re: [PATCH v7 3/4] cpufreq: conservative: call dbs_check_cpu only when necessary

[Fabio Baltieri]

On Wed, Jan 30, 2013 at 09:23:22PM +0530, Viresh Kumar wrote:
> On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> > 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>
> 
> We are again doing the same mistake which i fixed with:
> 
> commit 4471a34f9a1f2da220272e823bdb8e8fa83a7661
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Fri Oct 26 00:47:42 2012 +0200
> 
>     cpufreq: governors: remove redundant code

Can't argue with this, but the two had some subdle differences (namely
th two dbs_info structures) and I opted to not mess up forcing some
non-obvious common code.

Feel free to suggest a strategy.

Thanks,
Fabio

-- 
Fabio Baltieri

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

[Fabio Baltieri]

On Wed, Jan 30, 2013 at 10:21:03PM +0530, Viresh Kumar wrote:
> > I'm not sure about the name through, I like mentioning sw coordination in it
> > because that's the comment in the declaration:
> >
> >         cpumask_var_t           cpus;   /* CPUs requiring sw coordination */
> >         cpumask_var_t           related_cpus; /* CPUs with any coordination */
> >
> > And those two are already confusing as a starting point.
> 
> I will fix these comments with a patch of mine.

Great!

> 
> > Anyway, this sounds fine to me.  If you think this is useful I can send
> > a patch, or feel free to include it in your patches if you plan to do
> > further cleanup work on this code.
> >
> > /me tries to also keep that ->cpu field in mind.
> 
> You can make it part of your patchsets v8.

I'm not sending a v8 as Rafael already asked for incremental, but I'll
send a patch with that soon.

Thanks,
Fabio

-- 
Fabio Baltieri

Re: [PATCH v7 2/4] cpufreq: ondemand: call dbs_check_cpu only when necessary

[Viresh Kumar]

On 30 January 2013 22:16, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> Isn't that how it works now?  The current cpu ktime is not checked
> against its own, but against the "leader" cpu (dbs_info_local->cdbs.cpu),
> that's why it's initialized only for the first.
>
> Maybe I should have used dbs_info_leader/dbs_info instead of
> dbs_info_local/dbs_info.

This routine is called as wq handler. Which will recover dbs_info from work
using container_of. Which would give dbs_info_local for the cpu j.

Then we will execute below code.

+       /* use leader CPU's dbs_info */
+       dbs_info = &per_cpu(od_cpu_dbs_info, dbs_info_local->cdbs.cpu);

dbs_info_local->cdbs.cpu was uninitialized for all cpus except policy->cpu.
And so, might be initialized with 0 as its a global variable... But if you
offline cpu 0 and online it back, then policy->cpu would be 1 and this logic,
which worked by mistake will fail.

+       mutex_lock(&dbs_info->cdbs.timer_mutex);
+
+       time_now = ktime_get();
+       delta_us = ktime_us_delta(time_now, dbs_info->cdbs.time_stamp);

and so as this.

Correct?

Re: [PATCH v7 3/4] cpufreq: conservative: call dbs_check_cpu only when necessary

[Viresh Kumar]

On 30 January 2013 22:23, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> On Wed, Jan 30, 2013 at 09:23:22PM +0530, Viresh Kumar wrote:

> Can't argue with this, but the two had some subdle differences (namely
> th two dbs_info structures) and I opted to not mess up forcing some
> non-obvious common code.
>
> Feel free to suggest a strategy.

Just check the patch i mentioned, go ahead with something similar. There
is a lot of code in my original patch which removed redundancy to a big level.

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

[Rafael J. Wysocki]

On Wednesday, January 30, 2013 05:57:39 PM Fabio Baltieri wrote:
> On Wed, Jan 30, 2013 at 10:21:03PM +0530, Viresh Kumar wrote:
> > > I'm not sure about the name through, I like mentioning sw coordination in it
> > > because that's the comment in the declaration:
> > >
> > >         cpumask_var_t           cpus;   /* CPUs requiring sw coordination */
> > >         cpumask_var_t           related_cpus; /* CPUs with any coordination */
> > >
> > > And those two are already confusing as a starting point.
> > 
> > I will fix these comments with a patch of mine.
> 
> Great!
> 
> > 
> > > Anyway, this sounds fine to me.  If you think this is useful I can send
> > > a patch, or feel free to include it in your patches if you plan to do
> > > further cleanup work on this code.
> > >
> > > /me tries to also keep that ->cpu field in mind.
> > 
> > You can make it part of your patchsets v8.
> 
> I'm not sending a v8 as Rafael already asked for incremental, but I'll
> send a patch with that soon.

Yes, please.

Thanks,
Rafael


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

Re: [PATCH v7 2/4] cpufreq: ondemand: call dbs_check_cpu only when necessary

[Fabio Baltieri]

Hello Viresh,

On Wed, Jan 30, 2013 at 10:41:08PM +0530, Viresh Kumar wrote:
> On 30 January 2013 22:16, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> > Isn't that how it works now?  The current cpu ktime is not checked
> > against its own, but against the "leader" cpu (dbs_info_local->cdbs.cpu),
> > that's why it's initialized only for the first.
> >
> > Maybe I should have used dbs_info_leader/dbs_info instead of
> > dbs_info_local/dbs_info.
> 
> This routine is called as wq handler. Which will recover dbs_info from work
> using container_of. Which would give dbs_info_local for the cpu j.
> 
> Then we will execute below code.
> 
> +       /* use leader CPU's dbs_info */
> +       dbs_info = &per_cpu(od_cpu_dbs_info, dbs_info_local->cdbs.cpu);
> 
> dbs_info_local->cdbs.cpu was uninitialized for all cpus except policy->cpu.

Ok, now I see the problem: cdbs->cpu is initialized only on the leader
cpu and this is working by coincidence on my system, while
cdbs->time_stamp is initialized only on the leader cpu, and that should
be correct even when cpu hotplugging as that's reinitialized every time.

That's a fix so I'll send a patch just to set ->cpu into the
for_each_cpu cycle.

Thanks,
Fabio

> And so, might be initialized with 0 as its a global variable... But if you
> offline cpu 0 and online it back, then policy->cpu would be 1 and this logic,
> which worked by mistake will fail.
> 
> +       mutex_lock(&dbs_info->cdbs.timer_mutex);
> +
> +       time_now = ktime_get();
> +       delta_us = ktime_us_delta(time_now, dbs_info->cdbs.time_stamp);
> 
> and so as this.
> 
> Correct?
> --
> 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

-- 
Fabio Baltieri

Re: [PATCH v7 2/4] cpufreq: ondemand: call dbs_check_cpu only when necessary

[Viresh Kumar]

On 31 January 2013 14:09, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> Ok, now I see the problem: cdbs->cpu is initialized only on the leader
> cpu and this is working by coincidence on my system, while
> cdbs->time_stamp is initialized only on the leader cpu, and that should
> be correct even when cpu hotplugging as that's reinitialized every time.
>
> That's a fix so I'll send a patch just to set ->cpu into the
> for_each_cpu cycle.

That's not enough. You need to set ->cpu to j and not policy->cpu as we
discussed it earlier (params to timer init and exit.).

And so, we need to get policy->cpu somehow and get its timestamp.

Re: [PATCH v7 2/4] cpufreq: ondemand: call dbs_check_cpu only when necessary

[Fabio Baltieri]

On Thu, Jan 31, 2013 at 02:12:29PM +0530, Viresh Kumar wrote:
> On 31 January 2013 14:09, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> > Ok, now I see the problem: cdbs->cpu is initialized only on the leader
> > cpu and this is working by coincidence on my system, while
> > cdbs->time_stamp is initialized only on the leader cpu, and that should
> > be correct even when cpu hotplugging as that's reinitialized every time.
> >
> > That's a fix so I'll send a patch just to set ->cpu into the
> > for_each_cpu cycle.
> 
> That's not enough. You need to set ->cpu to j and not policy->cpu as we
> discussed it earlier (params to timer init and exit.).
> 
> And so, we need to get policy->cpu somehow and get its timestamp.

Current code uses ->cpu to track policy->cpu and get the timestamp, I
can modify it to point to the current cpu and use it in timer_init *and*
add a new field just to track leader_cpu but I don't see the benefits.
Am I missing something?

Fabio

-- 
Fabio Baltieri

Re: [PATCH v7 2/4] cpufreq: ondemand: call dbs_check_cpu only when necessary

[Viresh Kumar]

On 31 January 2013 14:36, Fabio Baltieri <fabio.baltieri@linaro.org> wrote:
> Current code uses ->cpu to track policy->cpu and get the timestamp, I
> can modify it to point to the current cpu and use it in timer_init *and*
> add a new field just to track leader_cpu but I don't see the benefits.
> Am I missing something?

Current code doesn't use cdbs->cpu for any cpu leaving the leader. So, we
can use that field to keep local cpus. And for any cpu we can get the updated
leader cpu with following:

j_cdbs->cur_policy->cpu :)