[PATCH 1/3] workqueue: define mod_scheduled_delayed_work[_on]() helpers

[PATCH 1/3] workqueue: define mod_scheduled_delayed_work[_on]() helpers

[Viresh Kumar]

The only difference between schedule_delayed_work[_on]() and
queue_delayed_work[_on]() is the workqueue, work is scheduled on. We may need to
modify the delay for works queued with schedule_delayed_work[_on]() calls and
thus adding these helpers.

First users of these new helpers is cpufreq governors which need to modify the
delay for its works.

Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/workqueue.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 2b58905..864c2b3 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -412,6 +412,7 @@ extern bool schedule_delayed_work_on(int cpu, struct delayed_work *work,
 extern bool schedule_delayed_work(struct delayed_work *work,
 				  unsigned long delay);
 extern int schedule_on_each_cpu(work_func_t func);
+
 extern int keventd_up(void);
 
 int execute_in_process_context(work_func_t fn, struct execute_work *);
@@ -465,6 +466,11 @@ static inline long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
 long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg);
 #endif /* CONFIG_SMP */
 
+#define mod_scheduled_delayed_work_on(cpu, dwork, delay)	\
+	mod_delayed_work_on(cpu, system_wq, dwork, delay)
+#define mod_scheduled_delayed_work(dwork, delay)		\
+	mod_delayed_work(system_wq, dwork, delay)
+
 #ifdef CONFIG_FREEZER
 extern void freeze_workqueues_begin(void);
 extern bool freeze_workqueues_busy(void);
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/3] cpufreq: ondemand: Don't update sample_type if we don't evaluate load again

[Viresh Kumar]

Because we have per cpu timer now, we check if we need to evaluate load again or
not (In case it is recently evaluated). Here the 2nd cpu which got timer
interrupt updates core_dbs_info->sample_type irrespective of load evaluation is
required or not. Which is wrong as the first cpu is dependent on this variable
set to an older value.

Moreover it would be best in this case to schedule 2nd cpu's timer to
sampling_rate instead of freq_lo or hi as that must be managed by the other cpu.
In case the other cpu idles in between then also we wouldn't loose much power.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_ondemand.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index f3eb26c..a815c88 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -223,34 +223,32 @@ static void od_dbs_timer(struct work_struct *work)
 	unsigned int cpu = dbs_info->cdbs.cur_policy->cpu;
 	struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu(od_cpu_dbs_info,
 			cpu);
-	int delay, sample_type = core_dbs_info->sample_type;
-	bool eval_load;
+	int delay = 0, sample_type = core_dbs_info->sample_type;
 
 	mutex_lock(&core_dbs_info->cdbs.timer_mutex);
-	eval_load = need_load_eval(&core_dbs_info->cdbs,
-			od_tuners.sampling_rate);
+	if (!need_load_eval(&core_dbs_info->cdbs, od_tuners.sampling_rate))
+		goto max_delay;
 
 	/* Common NORMAL_SAMPLE setup */
 	core_dbs_info->sample_type = OD_NORMAL_SAMPLE;
 	if (sample_type == OD_SUB_SAMPLE) {
 		delay = core_dbs_info->freq_lo_jiffies;
-		if (eval_load)
-			__cpufreq_driver_target(core_dbs_info->cdbs.cur_policy,
-						core_dbs_info->freq_lo,
-						CPUFREQ_RELATION_H);
+		__cpufreq_driver_target(core_dbs_info->cdbs.cur_policy,
+				core_dbs_info->freq_lo, CPUFREQ_RELATION_H);
 	} else {
-		if (eval_load)
-			dbs_check_cpu(&od_dbs_data, cpu);
+		dbs_check_cpu(&od_dbs_data, cpu);
 		if (core_dbs_info->freq_lo) {
 			/* Setup timer for SUB_SAMPLE */
 			core_dbs_info->sample_type = OD_SUB_SAMPLE;
 			delay = core_dbs_info->freq_hi_jiffies;
-		} else {
-			delay = delay_for_sampling_rate(od_tuners.sampling_rate
-						* core_dbs_info->rate_mult);
 		}
 	}
 
+max_delay:
+	if (!delay)
+		delay = delay_for_sampling_rate(od_tuners.sampling_rate
+				* core_dbs_info->rate_mult);
+
 	schedule_delayed_work_on(smp_processor_id(), dw, delay);
 	mutex_unlock(&core_dbs_info->cdbs.timer_mutex);
 }
-- 
1.7.12.rc2.18.g61b472e

[PATCH 3/3] cpufreq: governors: Avoid unnecessary per cpu timer interrupts

[Viresh Kumar]

Following patch has introduced per cpu timers or works for ondemand and
conservative governors.

	commit 2abfa876f1117b0ab45f191fb1f82c41b1cbc8fe
	Author: Rickard Andersson <rickard.andersson@stericsson.com>
	Date:   Thu Dec 27 14:55:38 2012 +0000

	    cpufreq: handle SW coordinated CPUs

This causes additional unnecessary interrupts on all cpus when the load is
recently evaluated by any other cpu. i.e. When load is recently evaluated by cpu
x, we don't really need any other cpu to evaluate this load again for the next
sampling_rate time.

Some sort of code is present to avoid that but we are still getting timer
interrupts for all cpus. A good way of avoiding this would be to modify delays
for all cpus (policy->cpus) whenever any cpu has evaluated load.

This patch does this change and some related code cleanup.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 10 ++++++---
 drivers/cpufreq/cpufreq_governor.c     | 38 ++++++++++++++++++++++++----------
 drivers/cpufreq/cpufreq_governor.h     |  2 ++
 drivers/cpufreq/cpufreq_ondemand.c     | 13 +++++++-----
 4 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 4fd0006..de5f66a 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -113,19 +113,23 @@ static void cs_check_cpu(int cpu, unsigned int load)
 
 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);
 	unsigned int cpu = dbs_info->cdbs.cur_policy->cpu;
 	struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu(cs_cpu_dbs_info,
 			cpu);
 	int delay = delay_for_sampling_rate(cs_tuners.sampling_rate);
+	bool modify_all = true;
 
 	mutex_lock(&core_dbs_info->cdbs.timer_mutex);
-	if (need_load_eval(&core_dbs_info->cdbs, cs_tuners.sampling_rate))
+	if (!need_load_eval(&core_dbs_info->cdbs, cs_tuners.sampling_rate))
+		modify_all = false;
+	else
 		dbs_check_cpu(&cs_dbs_data, cpu);
 
-	schedule_delayed_work_on(smp_processor_id(), dw, delay);
+	gov_queue_work(&cs_dbs_data, dbs_info->cdbs.cur_policy, delay,
+			modify_all);
+
 	mutex_unlock(&core_dbs_info->cdbs.timer_mutex);
 }
 
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 5a76086..f0747f1 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -161,20 +161,37 @@ 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, int cpu,
-				  unsigned int sampling_rate)
+static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
+		unsigned int delay)
 {
-	int delay = delay_for_sampling_rate(sampling_rate);
 	struct cpu_dbs_common_info *cdbs = dbs_data->get_cpu_cdbs(cpu);
 
-	schedule_delayed_work_on(cpu, &cdbs->work, delay);
+	mod_scheduled_delayed_work_on(cpu, &cdbs->work, delay);
 }
 
-static inline void dbs_timer_exit(struct dbs_data *dbs_data, int cpu)
+void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
+		unsigned int delay, bool all_cpus)
 {
-	struct cpu_dbs_common_info *cdbs = dbs_data->get_cpu_cdbs(cpu);
+	int i;
+
+	if (!all_cpus) {
+		__gov_queue_work(smp_processor_id(), dbs_data, delay);
+	} else {
+		for_each_cpu(i, policy->cpus)
+			__gov_queue_work(i, dbs_data, delay);
+	}
+}
+EXPORT_SYMBOL_GPL(gov_queue_work);
+
+void gov_cancel_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy)
+{
+	struct cpu_dbs_common_info *cdbs;
+	int i;
 
-	cancel_delayed_work_sync(&cdbs->work);
+	for_each_cpu(i, policy->cpus) {
+		cdbs = dbs_data->get_cpu_cdbs(i);
+		cancel_delayed_work_sync(&cdbs->work);
+	}
 }
 
 /* Will return if we need to evaluate cpu load again or not */
@@ -301,16 +318,15 @@ unlock:
 		/* Initiate timer time stamp */
 		cpu_cdbs->time_stamp = ktime_get();
 
-		for_each_cpu(j, policy->cpus)
-			dbs_timer_init(dbs_data, j, *sampling_rate);
+		gov_queue_work(dbs_data, policy,
+				delay_for_sampling_rate(*sampling_rate), true);
 		break;
 
 	case CPUFREQ_GOV_STOP:
 		if (dbs_data->governor == GOV_CONSERVATIVE)
 			cs_dbs_info->enable = 0;
 
-		for_each_cpu(j, policy->cpus)
-			dbs_timer_exit(dbs_data, j);
+		gov_cancel_work(dbs_data, policy);
 
 		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 d2ac911..8c0116e 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -175,4 +175,6 @@ bool need_load_eval(struct cpu_dbs_common_info *cdbs,
 		unsigned int sampling_rate);
 int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 		struct cpufreq_policy *policy, unsigned int event);
+void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
+		unsigned int delay, bool all_cpus);
 #endif /* _CPUFREQ_GOVERNER_H */
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index a815c88..7d9cfdb 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -217,17 +217,19 @@ static void od_check_cpu(int cpu, unsigned int load_freq)
 
 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);
 	unsigned int cpu = dbs_info->cdbs.cur_policy->cpu;
 	struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu(od_cpu_dbs_info,
 			cpu);
 	int delay = 0, sample_type = core_dbs_info->sample_type;
+	bool modify_all = true;
 
 	mutex_lock(&core_dbs_info->cdbs.timer_mutex);
-	if (!need_load_eval(&core_dbs_info->cdbs, od_tuners.sampling_rate))
+	if (!need_load_eval(&core_dbs_info->cdbs, od_tuners.sampling_rate)) {
+		modify_all = false;
 		goto max_delay;
+	}
 
 	/* Common NORMAL_SAMPLE setup */
 	core_dbs_info->sample_type = OD_NORMAL_SAMPLE;
@@ -249,7 +251,8 @@ max_delay:
 		delay = delay_for_sampling_rate(od_tuners.sampling_rate
 				* core_dbs_info->rate_mult);
 
-	schedule_delayed_work_on(smp_processor_id(), dw, delay);
+	gov_queue_work(&od_dbs_data, dbs_info->cdbs.cur_policy, delay,
+			modify_all);
 	mutex_unlock(&core_dbs_info->cdbs.timer_mutex);
 }
 
@@ -312,8 +315,8 @@ 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(cpu, &dbs_info->cdbs.work,
-					usecs_to_jiffies(new_rate));
+			gov_queue_work(&od_dbs_data, dbs_info->cdbs.cur_policy,
+					usecs_to_jiffies(new_rate), true);
 
 		}
 		mutex_unlock(&dbs_info->cdbs.timer_mutex);
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 1/3] workqueue: define mod_scheduled_delayed_work[_on]() helpers

[Viresh Kumar]

On 27 February 2013 16:59, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The only difference between schedule_delayed_work[_on]() and
> queue_delayed_work[_on]() is the workqueue, work is scheduled on. We may need to
> modify the delay for works queued with schedule_delayed_work[_on]() calls and
> thus adding these helpers.
>
> First users of these new helpers is cpufreq governors which need to modify the
> delay for its works.
>
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

btw, this patchset is pushed here:

http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/cpufreq-fix-gov-wakeups

Re: [PATCH 1/3] workqueue: define mod_scheduled_delayed_work[_on]() helpers

[Tejun Heo]

On Wed, Feb 27, 2013 at 04:59:10PM +0530, Viresh Kumar wrote:
>  int execute_in_process_context(work_func_t fn, struct execute_work *);
> @@ -465,6 +466,11 @@ static inline long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
>  long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg);
>  #endif /* CONFIG_SMP */
>  
> +#define mod_scheduled_delayed_work_on(cpu, dwork, delay)	\
> +	mod_delayed_work_on(cpu, system_wq, dwork, delay)
> +#define mod_scheduled_delayed_work(dwork, delay)		\
> +	mod_delayed_work(system_wq, dwork, delay)

So, the intention is to just let people use system_wq.  We no longer
have single system-wide workqueue and we don't wanna add different
variants matching each system wq.  schedule_work() and friends were
already there so I'm leaving those alone but I don't really want to
add another set of rather meaningless wrappers.  Please just use
system_wq with mode_delayed_work*().

Thanks.

-- 
tejun

Re: [PATCH 1/3] workqueue: define mod_scheduled_delayed_work[_on]() helpers

[Viresh Kumar]

On 27 February 2013 20:40, Tejun Heo <tj@kernel.org> wrote:
> So, the intention is to just let people use system_wq.  We no longer
> have single system-wide workqueue and we don't wanna add different
> variants matching each system wq.  schedule_work() and friends were
> already there so I'm leaving those alone but I don't really want to
> add another set of rather meaningless wrappers.  Please just use
> system_wq with mode_delayed_work*().

Even i thought the same initially but got these wrappers so that any updates
to workqueue used in schedule_work later on would simply work here as the
commiter would fix these too but fixing user drivers can be tricky.

To keep things aligned probably i should replace schedule_work() with
queue_work() and use system_wq there too..

--
viresh

Re: [PATCH 1/3] workqueue: define mod_scheduled_delayed_work[_on]() helpers

[Tejun Heo]

Hello, Viresh.

On Wed, Feb 27, 2013 at 08:44:52PM +0530, Viresh Kumar wrote:
> To keep things aligned probably i should replace schedule_work() with
> queue_work() and use system_wq there too..

Yeah, that probably is more preferable.  There are codes like the
following in kernel and they always irk me.

	if (custom_wq)
		queue_work();
	else
		schedule_work();

It's just silly.

Thanks.

-- 
tejun

Re: [PATCH 1/3] workqueue: define mod_scheduled_delayed_work[_on]() helpers

[Rafael J. Wysocki]

On Wednesday, February 27, 2013 04:59:10 PM Viresh Kumar wrote:
> The only difference between schedule_delayed_work[_on]() and
> queue_delayed_work[_on]() is the workqueue, work is scheduled on. We may need to
> modify the delay for works queued with schedule_delayed_work[_on]() calls and
> thus adding these helpers.
> 
> First users of these new helpers is cpufreq governors which need to modify the
> delay for its works.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Do I assume correctly that you have withdrawn this patch?

Rafael


> ---
>  include/linux/workqueue.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 2b58905..864c2b3 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -412,6 +412,7 @@ extern bool schedule_delayed_work_on(int cpu, struct delayed_work *work,
>  extern bool schedule_delayed_work(struct delayed_work *work,
>  				  unsigned long delay);
>  extern int schedule_on_each_cpu(work_func_t func);
> +
>  extern int keventd_up(void);
>  
>  int execute_in_process_context(work_func_t fn, struct execute_work *);
> @@ -465,6 +466,11 @@ static inline long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
>  long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg);
>  #endif /* CONFIG_SMP */
>  
> +#define mod_scheduled_delayed_work_on(cpu, dwork, delay)	\
> +	mod_delayed_work_on(cpu, system_wq, dwork, delay)
> +#define mod_scheduled_delayed_work(dwork, delay)		\
> +	mod_delayed_work(system_wq, dwork, delay)
> +
>  #ifdef CONFIG_FREEZER
>  extern void freeze_workqueues_begin(void);
>  extern bool freeze_workqueues_busy(void);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] workqueue: define mod_scheduled_delayed_work[_on]() helpers

[Viresh Kumar]

On 12 March 2013 07:28, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Do I assume correctly that you have withdrawn this patch?

Yes.