Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1322161 > unrolled thread
| Started by | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| First post | 2016-01-30 00:00 +0100 |
| Last post | 2016-02-07 16:40 +0100 |
| Articles | 20 on this page of 21 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 3/3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-30 00:00 +0100
[Update][PATCH 3/3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-03 02:20 +0100
Re: [Update][PATCH 3/3] cpufreq: governor: Replace timers with utilization update callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 06:00 +0100
Re: [Update][PATCH 3/3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-04 12:00 +0100
[PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 02:30 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 08:00 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-05 14:40 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 16:10 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-06 00:20 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 10:20 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 15:50 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-08 03:10 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-08 13:00 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-08 14:00 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-08 14:50 +0100
Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-06 00:10 +0100
[PATCH 3/3 v4] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-06 04:50 +0100
Re: [PATCH 3/3 v4] cpufreq: governor: Replace timers with utilization update callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 10:30 +0100
Re: [PATCH 3/3 v4] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 15:40 +0100
[PATCH 3/3 v5] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 16:00 +0100
Re: [PATCH 3/3 v5] cpufreq: governor: Replace timers with utilization update callbacks Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 16:40 +0100
Page 1 of 2 [1] 2 Next page →
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-01-30 00:00 +0100 |
| Subject | [PATCH 3/3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qWqbf-1BM-1@gated-at.bofh.it> |
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Instead of using a per-CPU deferrable timer for queuing up governor
work items, register a utilization update callback that will be
invoked from the scheduler on utilization changes.
The sampling rate is still the same as what was used for the
deferrable timers and the added irq_work overhead should be offset by
the eliminated timers overhead, so in theory the functional impact of
this patch should not be significant.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpufreq/cpufreq_conservative.c | 6 -
drivers/cpufreq/cpufreq_governor.c | 129 +++++++++++++++------------------
drivers/cpufreq/cpufreq_governor.h | 13 ++-
drivers/cpufreq/cpufreq_ondemand.c | 25 +++---
4 files changed, 81 insertions(+), 92 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -18,6 +18,7 @@
#define _CPUFREQ_GOVERNOR_H
#include <linux/atomic.h>
+#include <linux/irq_work.h>
#include <linux/cpufreq.h>
#include <linux/kernel_stat.h>
#include <linux/module.h>
@@ -139,7 +140,9 @@ struct cpu_common_dbs_info {
struct mutex timer_mutex;
ktime_t time_stamp;
+ s64 sample_delay_ns;
atomic_t skip_work;
+ struct irq_work irq_work;
struct work_struct work;
};
@@ -155,7 +158,8 @@ struct cpu_dbs_info {
* wake-up from idle.
*/
unsigned int prev_load;
- struct timer_list timer;
+ u64 last_sample_time;
+ struct update_util_data update_util;
struct cpu_common_dbs_info *shared;
};
@@ -212,8 +216,7 @@ struct common_dbs_data {
struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
void *(*get_cpu_dbs_info_s)(int cpu);
- unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy,
- bool modify_all);
+ unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
void (*gov_check_cpu)(int cpu, unsigned int load);
int (*init)(struct dbs_data *dbs_data, bool notify);
void (*exit)(struct dbs_data *dbs_data, bool notify);
@@ -270,8 +273,8 @@ static ssize_t show_sampling_rate_min_go
}
extern struct mutex cpufreq_governor_lock;
-
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us);
void gov_cancel_work(struct cpu_common_dbs_info *shared);
void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
int cpufreq_governor_dbs(struct cpufreq_policy *policy,
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -128,10 +128,10 @@ void dbs_check_cpu(struct dbs_data *dbs_
* dropped down. So we perform the copy only once, upon the
* first wake-up from idle.)
*
- * Detecting this situation is easy: the governor's deferrable
- * timer would not have fired during CPU-idle periods. Hence
- * an unusually large 'wall_time' (as compared to the sampling
- * rate) indicates this scenario.
+ * Detecting this situation is easy: the governor's utilization
+ * update handler would not have run during CPU-idle periods.
+ * Hence, an unusually large 'wall_time' (as compared to the
+ * sampling rate) indicates this scenario.
*
* prev_load can be zero in two cases and we must recalculate it
* for both cases:
@@ -161,21 +161,26 @@ void dbs_check_cpu(struct dbs_data *dbs_
}
EXPORT_SYMBOL_GPL(dbs_check_cpu);
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us)
{
+ struct cpufreq_policy *policy = shared->policy;
struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int cpu;
+ shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
+ shared->time_stamp = ktime_get();
+
for_each_cpu(cpu, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
- cdbs->timer.expires = jiffies + delay;
- add_timer_on(&cdbs->timer, cpu);
+ struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+
+ cdbs->last_sample_time = 0;
+ cpufreq_set_update_util_data(cpu, &cdbs->update_util);
}
}
-EXPORT_SYMBOL_GPL(gov_add_timers);
+EXPORT_SYMBOL_GPL(gov_set_update_util);
-static inline void gov_cancel_timers(struct cpufreq_policy *policy)
+static inline void gov_clear_update_util_data(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
struct cpu_dbs_info *cdbs;
@@ -183,51 +188,26 @@ static inline void gov_cancel_timers(str
for_each_cpu(i, policy->cpus) {
cdbs = dbs_data->cdata->get_cpu_cdbs(i);
- del_timer_sync(&cdbs->timer);
+ cpufreq_set_update_util_data(i, NULL);
}
+ synchronize_rcu();
}
void gov_cancel_work(struct cpu_common_dbs_info *shared)
{
- /* Tell dbs_timer_handler() to skip queuing up work items. */
+ /* Tell dbs_update_util_handler() to skip queuing up work items. */
atomic_inc(&shared->skip_work);
/*
- * If dbs_timer_handler() is already running, it may not notice the
- * incremented skip_work, so wait for it to complete to prevent its work
- * item from being queued up after the cancel_work_sync() below.
- */
- gov_cancel_timers(shared->policy);
- /*
- * In case dbs_timer_handler() managed to run and spawn a work item
- * before the timers have been canceled, wait for that work item to
- * complete and then cancel all of the timers set up by it. If
- * dbs_timer_handler() runs again at that point, it will see the
- * positive value of skip_work and won't spawn any more work items.
+ * If dbs_update_util_handler() is already running, it may not notice
+ * the incremented skip_work, so wait for it to complete to prevent its
+ * work item from being queued up after the cancel_work_sync() below.
*/
+ gov_clear_update_util_data(shared->policy);
cancel_work_sync(&shared->work);
- gov_cancel_timers(shared->policy);
atomic_set(&shared->skip_work, 0);
}
EXPORT_SYMBOL_GPL(gov_cancel_work);
-/* Will return if we need to evaluate cpu load again or not */
-static bool need_load_eval(struct cpu_common_dbs_info *shared,
- unsigned int sampling_rate)
-{
- if (policy_is_shared(shared->policy)) {
- ktime_t time_now = ktime_get();
- s64 delta_us = ktime_us_delta(time_now, shared->time_stamp);
-
- /* Do nothing if we recently have sampled */
- if (delta_us < (s64)(sampling_rate / 2))
- return false;
- else
- shared->time_stamp = time_now;
- }
-
- return true;
-}
-
static void dbs_work_handler(struct work_struct *work)
{
struct cpu_common_dbs_info *shared = container_of(work, struct
@@ -235,14 +215,10 @@ static void dbs_work_handler(struct work
struct cpufreq_policy *policy;
struct dbs_data *dbs_data;
unsigned int sampling_rate, delay;
- bool eval_load;
policy = shared->policy;
dbs_data = policy->governor_data;
- /* Kill all timers */
- gov_cancel_timers(policy);
-
if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
@@ -253,37 +229,53 @@ static void dbs_work_handler(struct work
sampling_rate = od_tuners->sampling_rate;
}
- eval_load = need_load_eval(shared, sampling_rate);
-
/*
- * Make sure cpufreq_governor_limits() isn't evaluating load in
+ * Make sure cpufreq_governor_limits() isn't evaluating load or the
+ * ondemand governor isn't reading the time stamp and sampling rate in
* parallel.
*/
mutex_lock(&shared->timer_mutex);
- delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
+ delay = dbs_data->cdata->gov_dbs_timer(policy);
+ shared->sample_delay_ns = jiffies_to_nsecs(delay);
+ shared->time_stamp = ktime_get();
mutex_unlock(&shared->timer_mutex);
+ smp_mb__before_atomic();
atomic_dec(&shared->skip_work);
+}
+
+static void dbs_irq_work(struct irq_work *irq_work)
+{
+ struct cpu_common_dbs_info *shared;
- gov_add_timers(policy, delay);
+ shared = container_of(irq_work, struct cpu_common_dbs_info, irq_work);
+ schedule_work(&shared->work);
}
-static void dbs_timer_handler(unsigned long data)
+static void dbs_update_util_handler(struct update_util_data *data, u64 time,
+ unsigned long util, unsigned long max)
{
- struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
+ struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
struct cpu_common_dbs_info *shared = cdbs->shared;
/*
- * Timer handler may not be allowed to queue the work at the moment,
- * because:
- * - Another timer handler has done that
- * - We are stopping the governor
- * - Or we are updating the sampling rate of the ondemand governor
+ * The work may not be allowed to be queued up right now.
+ * Possible reasons:
+ * - Work has already been queued up or is in progress.
+ * - The governor is being stopped.
+ * - It is too early (too little time from the previous sample).
*/
- if (atomic_inc_return(&shared->skip_work) > 1)
- atomic_dec(&shared->skip_work);
- else
- queue_work(system_wq, &shared->work);
+ if (atomic_inc_return(&shared->skip_work) == 1) {
+ u64 delta_ns;
+
+ delta_ns = time - cdbs->last_sample_time;
+ if ((s64)delta_ns >= shared->sample_delay_ns) {
+ cdbs->last_sample_time = time;
+ irq_work_queue_on(&shared->irq_work, smp_processor_id());
+ return;
+ }
+ }
+ atomic_dec(&shared->skip_work);
}
static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -462,9 +454,6 @@ static int cpufreq_governor_start(struct
io_busy = od_tuners->io_is_busy;
}
- shared->policy = policy;
- shared->time_stamp = ktime_get();
-
for_each_cpu(j, policy->cpus) {
struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
unsigned int prev_load;
@@ -480,10 +469,10 @@ static int cpufreq_governor_start(struct
if (ignore_nice)
j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
- __setup_timer(&j_cdbs->timer, dbs_timer_handler,
- (unsigned long)j_cdbs,
- TIMER_DEFERRABLE | TIMER_IRQSAFE);
+ j_cdbs->update_util.func = dbs_update_util_handler;
}
+ shared->policy = policy;
+ init_irq_work(&shared->irq_work, dbs_irq_work);
if (cdata->governor == GOV_CONSERVATIVE) {
struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -500,7 +489,7 @@ static int cpufreq_governor_start(struct
od_ops->powersave_bias_init_cpu(cpu);
}
- gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
+ gov_set_update_util(shared, sampling_rate);
return 0;
}
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -191,7 +191,7 @@ static void od_check_cpu(int cpu, unsign
}
}
-static unsigned int od_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
unsigned int cpu = policy->cpu;
@@ -200,9 +200,6 @@ static unsigned int od_dbs_timer(struct
struct od_dbs_tuners *od_tuners = dbs_data->tuners;
int delay = 0, sample_type = dbs_info->sample_type;
- if (!modify_all)
- goto max_delay;
-
/* Common NORMAL_SAMPLE setup */
dbs_info->sample_type = OD_NORMAL_SAMPLE;
if (sample_type == OD_SUB_SAMPLE) {
@@ -218,7 +215,6 @@ static unsigned int od_dbs_timer(struct
}
}
-max_delay:
if (!delay)
delay = delay_for_sampling_rate(od_tuners->sampling_rate
* dbs_info->rate_mult);
@@ -264,7 +260,7 @@ static void update_sampling_rate(struct
struct od_cpu_dbs_info_s *dbs_info;
struct cpu_dbs_info *cdbs;
struct cpu_common_dbs_info *shared;
- unsigned long next_sampling, appointed_at;
+ ktime_t next_sampling, appointed_at;
dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
cdbs = &dbs_info->cdbs;
@@ -292,16 +288,19 @@ static void update_sampling_rate(struct
continue;
/*
- * Checking this for any CPU should be fine, timers for all of
- * them are scheduled together.
+ * Checking this for any CPU sharing the policy should be fine,
+ * they are all scheduled to sample at the same time.
*/
- next_sampling = jiffies + usecs_to_jiffies(new_rate);
- appointed_at = dbs_info->cdbs.timer.expires;
+ next_sampling = ktime_add_us(ktime_get(), new_rate);
- if (time_before(next_sampling, appointed_at)) {
- gov_cancel_work(shared);
- gov_add_timers(policy, usecs_to_jiffies(new_rate));
+ mutex_lock(&shared->timer_mutex);
+ appointed_at = ktime_add_ns(shared->time_stamp,
+ shared->sample_delay_ns);
+ mutex_unlock(&shared->timer_mutex);
+ if (ktime_before(next_sampling, appointed_at)) {
+ gov_cancel_work(shared);
+ gov_set_update_util(shared, new_rate);
}
}
Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -115,14 +115,12 @@ static void cs_check_cpu(int cpu, unsign
}
}
-static unsigned int cs_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
- if (modify_all)
- dbs_check_cpu(dbs_data, policy->cpu);
-
+ dbs_check_cpu(dbs_data, policy->cpu);
return delay_for_sampling_rate(cs_tuners->sampling_rate);
}
[toc] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-03 02:20 +0100 |
| Subject | [Update][PATCH 3/3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qXUgW-3rX-1@gated-at.bofh.it> |
| In reply to | #1322161 |
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] cpufreq: governor: Replace timers with utilization update callbacks
Instead of using a per-CPU deferrable timer for queuing up governor
work items, register a utilization update callback that will be
invoked from the scheduler on utilization changes.
The sampling rate is still the same as what was used for the
deferrable timers and the added irq_work overhead should be offset by
the eliminated timers overhead, so in theory the functional impact of
this patch should not be significant.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
I realized that the previous version of this patch didn't remove some code
that wasn't necessary any more, so here's an update.
---
drivers/cpufreq/cpufreq_conservative.c | 6 -
drivers/cpufreq/cpufreq_governor.c | 136 ++++++++++++++-------------------
drivers/cpufreq/cpufreq_governor.h | 13 +--
drivers/cpufreq/cpufreq_ondemand.c | 25 ++----
4 files changed, 83 insertions(+), 97 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -18,6 +18,7 @@
#define _CPUFREQ_GOVERNOR_H
#include <linux/atomic.h>
+#include <linux/irq_work.h>
#include <linux/cpufreq.h>
#include <linux/kernel_stat.h>
#include <linux/module.h>
@@ -139,7 +140,9 @@ struct cpu_common_dbs_info {
struct mutex timer_mutex;
ktime_t time_stamp;
+ s64 sample_delay_ns;
atomic_t skip_work;
+ struct irq_work irq_work;
struct work_struct work;
};
@@ -155,7 +158,8 @@ struct cpu_dbs_info {
* wake-up from idle.
*/
unsigned int prev_load;
- struct timer_list timer;
+ u64 last_sample_time;
+ struct update_util_data update_util;
struct cpu_common_dbs_info *shared;
};
@@ -212,8 +216,7 @@ struct common_dbs_data {
struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
void *(*get_cpu_dbs_info_s)(int cpu);
- unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy,
- bool modify_all);
+ unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
void (*gov_check_cpu)(int cpu, unsigned int load);
int (*init)(struct dbs_data *dbs_data, bool notify);
void (*exit)(struct dbs_data *dbs_data, bool notify);
@@ -270,8 +273,8 @@ static ssize_t show_sampling_rate_min_go
}
extern struct mutex cpufreq_governor_lock;
-
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us);
void gov_cancel_work(struct cpu_common_dbs_info *shared);
void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
int cpufreq_governor_dbs(struct cpufreq_policy *policy,
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -128,10 +128,10 @@ void dbs_check_cpu(struct dbs_data *dbs_
* dropped down. So we perform the copy only once, upon the
* first wake-up from idle.)
*
- * Detecting this situation is easy: the governor's deferrable
- * timer would not have fired during CPU-idle periods. Hence
- * an unusually large 'wall_time' (as compared to the sampling
- * rate) indicates this scenario.
+ * Detecting this situation is easy: the governor's utilization
+ * update handler would not have run during CPU-idle periods.
+ * Hence, an unusually large 'wall_time' (as compared to the
+ * sampling rate) indicates this scenario.
*
* prev_load can be zero in two cases and we must recalculate it
* for both cases:
@@ -161,73 +161,50 @@ void dbs_check_cpu(struct dbs_data *dbs_
}
EXPORT_SYMBOL_GPL(dbs_check_cpu);
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us)
{
+ struct cpufreq_policy *policy = shared->policy;
struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int cpu;
+ shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
+ shared->time_stamp = ktime_get();
+
for_each_cpu(cpu, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
- cdbs->timer.expires = jiffies + delay;
- add_timer_on(&cdbs->timer, cpu);
+ struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+
+ cdbs->last_sample_time = 0;
+ cpufreq_set_update_util_data(cpu, &cdbs->update_util);
}
}
-EXPORT_SYMBOL_GPL(gov_add_timers);
+EXPORT_SYMBOL_GPL(gov_set_update_util);
-static inline void gov_cancel_timers(struct cpufreq_policy *policy)
+static inline void gov_clear_update_util(struct cpufreq_policy *policy)
{
- struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int i;
- for_each_cpu(i, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(i);
- del_timer_sync(&cdbs->timer);
- }
+ for_each_cpu(i, policy->cpus)
+ cpufreq_set_update_util_data(i, NULL);
+
+ synchronize_rcu();
}
void gov_cancel_work(struct cpu_common_dbs_info *shared)
{
- /* Tell dbs_timer_handler() to skip queuing up work items. */
+ /* Tell dbs_update_util_handler() to skip queuing up work items. */
atomic_inc(&shared->skip_work);
/*
- * If dbs_timer_handler() is already running, it may not notice the
- * incremented skip_work, so wait for it to complete to prevent its work
- * item from being queued up after the cancel_work_sync() below.
- */
- gov_cancel_timers(shared->policy);
- /*
- * In case dbs_timer_handler() managed to run and spawn a work item
- * before the timers have been canceled, wait for that work item to
- * complete and then cancel all of the timers set up by it. If
- * dbs_timer_handler() runs again at that point, it will see the
- * positive value of skip_work and won't spawn any more work items.
+ * If dbs_update_util_handler() is already running, it may not notice
+ * the incremented skip_work, so wait for it to complete to prevent its
+ * work item from being queued up after the cancel_work_sync() below.
*/
+ gov_clear_update_util(shared->policy);
cancel_work_sync(&shared->work);
- gov_cancel_timers(shared->policy);
atomic_set(&shared->skip_work, 0);
}
EXPORT_SYMBOL_GPL(gov_cancel_work);
-/* Will return if we need to evaluate cpu load again or not */
-static bool need_load_eval(struct cpu_common_dbs_info *shared,
- unsigned int sampling_rate)
-{
- if (policy_is_shared(shared->policy)) {
- ktime_t time_now = ktime_get();
- s64 delta_us = ktime_us_delta(time_now, shared->time_stamp);
-
- /* Do nothing if we recently have sampled */
- if (delta_us < (s64)(sampling_rate / 2))
- return false;
- else
- shared->time_stamp = time_now;
- }
-
- return true;
-}
-
static void dbs_work_handler(struct work_struct *work)
{
struct cpu_common_dbs_info *shared = container_of(work, struct
@@ -235,14 +212,10 @@ static void dbs_work_handler(struct work
struct cpufreq_policy *policy;
struct dbs_data *dbs_data;
unsigned int sampling_rate, delay;
- bool eval_load;
policy = shared->policy;
dbs_data = policy->governor_data;
- /* Kill all timers */
- gov_cancel_timers(policy);
-
if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
@@ -253,37 +226,53 @@ static void dbs_work_handler(struct work
sampling_rate = od_tuners->sampling_rate;
}
- eval_load = need_load_eval(shared, sampling_rate);
-
/*
- * Make sure cpufreq_governor_limits() isn't evaluating load in
+ * Make sure cpufreq_governor_limits() isn't evaluating load or the
+ * ondemand governor isn't reading the time stamp and sampling rate in
* parallel.
*/
mutex_lock(&shared->timer_mutex);
- delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
+ delay = dbs_data->cdata->gov_dbs_timer(policy);
+ shared->sample_delay_ns = jiffies_to_nsecs(delay);
+ shared->time_stamp = ktime_get();
mutex_unlock(&shared->timer_mutex);
+ smp_mb__before_atomic();
atomic_dec(&shared->skip_work);
+}
- gov_add_timers(policy, delay);
+static void dbs_irq_work(struct irq_work *irq_work)
+{
+ struct cpu_common_dbs_info *shared;
+
+ shared = container_of(irq_work, struct cpu_common_dbs_info, irq_work);
+ schedule_work(&shared->work);
}
-static void dbs_timer_handler(unsigned long data)
+static void dbs_update_util_handler(struct update_util_data *data, u64 time,
+ unsigned long util, unsigned long max)
{
- struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
+ struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
struct cpu_common_dbs_info *shared = cdbs->shared;
/*
- * Timer handler may not be allowed to queue the work at the moment,
- * because:
- * - Another timer handler has done that
- * - We are stopping the governor
- * - Or we are updating the sampling rate of the ondemand governor
+ * The work may not be allowed to be queued up right now.
+ * Possible reasons:
+ * - Work has already been queued up or is in progress.
+ * - The governor is being stopped.
+ * - It is too early (too little time from the previous sample).
*/
- if (atomic_inc_return(&shared->skip_work) > 1)
- atomic_dec(&shared->skip_work);
- else
- queue_work(system_wq, &shared->work);
+ if (atomic_inc_return(&shared->skip_work) == 1) {
+ u64 delta_ns;
+
+ delta_ns = time - cdbs->last_sample_time;
+ if ((s64)delta_ns >= shared->sample_delay_ns) {
+ cdbs->last_sample_time = time;
+ irq_work_queue_on(&shared->irq_work, smp_processor_id());
+ return;
+ }
+ }
+ atomic_dec(&shared->skip_work);
}
static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -467,9 +456,6 @@ static int cpufreq_governor_start(struct
io_busy = od_tuners->io_is_busy;
}
- shared->policy = policy;
- shared->time_stamp = ktime_get();
-
for_each_cpu(j, policy->cpus) {
struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
unsigned int prev_load;
@@ -485,10 +471,10 @@ static int cpufreq_governor_start(struct
if (ignore_nice)
j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
- __setup_timer(&j_cdbs->timer, dbs_timer_handler,
- (unsigned long)j_cdbs,
- TIMER_DEFERRABLE | TIMER_IRQSAFE);
+ j_cdbs->update_util.func = dbs_update_util_handler;
}
+ shared->policy = policy;
+ init_irq_work(&shared->irq_work, dbs_irq_work);
if (cdata->governor == GOV_CONSERVATIVE) {
struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -505,7 +491,7 @@ static int cpufreq_governor_start(struct
od_ops->powersave_bias_init_cpu(cpu);
}
- gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
+ gov_set_update_util(shared, sampling_rate);
return 0;
}
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -191,7 +191,7 @@ static void od_check_cpu(int cpu, unsign
}
}
-static unsigned int od_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
unsigned int cpu = policy->cpu;
@@ -200,9 +200,6 @@ static unsigned int od_dbs_timer(struct
struct od_dbs_tuners *od_tuners = dbs_data->tuners;
int delay = 0, sample_type = dbs_info->sample_type;
- if (!modify_all)
- goto max_delay;
-
/* Common NORMAL_SAMPLE setup */
dbs_info->sample_type = OD_NORMAL_SAMPLE;
if (sample_type == OD_SUB_SAMPLE) {
@@ -218,7 +215,6 @@ static unsigned int od_dbs_timer(struct
}
}
-max_delay:
if (!delay)
delay = delay_for_sampling_rate(od_tuners->sampling_rate
* dbs_info->rate_mult);
@@ -264,7 +260,7 @@ static void update_sampling_rate(struct
struct od_cpu_dbs_info_s *dbs_info;
struct cpu_dbs_info *cdbs;
struct cpu_common_dbs_info *shared;
- unsigned long next_sampling, appointed_at;
+ ktime_t next_sampling, appointed_at;
dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
cdbs = &dbs_info->cdbs;
@@ -292,16 +288,19 @@ static void update_sampling_rate(struct
continue;
/*
- * Checking this for any CPU should be fine, timers for all of
- * them are scheduled together.
+ * Checking this for any CPU sharing the policy should be fine,
+ * they are all scheduled to sample at the same time.
*/
- next_sampling = jiffies + usecs_to_jiffies(new_rate);
- appointed_at = dbs_info->cdbs.timer.expires;
+ next_sampling = ktime_add_us(ktime_get(), new_rate);
- if (time_before(next_sampling, appointed_at)) {
- gov_cancel_work(shared);
- gov_add_timers(policy, usecs_to_jiffies(new_rate));
+ mutex_lock(&shared->timer_mutex);
+ appointed_at = ktime_add_ns(shared->time_stamp,
+ shared->sample_delay_ns);
+ mutex_unlock(&shared->timer_mutex);
+ if (ktime_before(next_sampling, appointed_at)) {
+ gov_cancel_work(shared);
+ gov_set_update_util(shared, new_rate);
}
}
Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -115,14 +115,12 @@ static void cs_check_cpu(int cpu, unsign
}
}
-static unsigned int cs_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
- if (modify_all)
- dbs_check_cpu(dbs_data, policy->cpu);
-
+ dbs_check_cpu(dbs_data, policy->cpu);
return delay_for_sampling_rate(cs_tuners->sampling_rate);
}
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-04 06:00 +0100 |
| Subject | Re: [Update][PATCH 3/3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qYkbo-3OE-5@gated-at.bofh.it> |
| In reply to | #1324797 |
On 03-02-16, 02:16, Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> -void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
> +void gov_set_update_util(struct cpu_common_dbs_info *shared,
> + unsigned int delay_us)
> {
> + struct cpufreq_policy *policy = shared->policy;
> struct dbs_data *dbs_data = policy->governor_data;
> - struct cpu_dbs_info *cdbs;
> int cpu;
>
> + shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
> + shared->time_stamp = ktime_get();
> +
> for_each_cpu(cpu, policy->cpus) {
> - cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> - cdbs->timer.expires = jiffies + delay;
> - add_timer_on(&cdbs->timer, cpu);
> + struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> +
> + cdbs->last_sample_time = 0;
> + cpufreq_set_update_util_data(cpu, &cdbs->update_util);
Why no synchronize_rcu() here? This can be called from ondemand
governor on sampling-rate updates ..
> }
> }
> -EXPORT_SYMBOL_GPL(gov_add_timers);
> +EXPORT_SYMBOL_GPL(gov_set_update_util);
>
> -static inline void gov_cancel_timers(struct cpufreq_policy *policy)
> +static inline void gov_clear_update_util(struct cpufreq_policy *policy)
> {
> - struct dbs_data *dbs_data = policy->governor_data;
> - struct cpu_dbs_info *cdbs;
> int i;
>
> - for_each_cpu(i, policy->cpus) {
> - cdbs = dbs_data->cdata->get_cpu_cdbs(i);
> - del_timer_sync(&cdbs->timer);
> - }
> + for_each_cpu(i, policy->cpus)
> + cpufreq_set_update_util_data(i, NULL);
> +
> + synchronize_rcu();
> }
>
> void gov_cancel_work(struct cpu_common_dbs_info *shared)
> {
> - /* Tell dbs_timer_handler() to skip queuing up work items. */
> + /* Tell dbs_update_util_handler() to skip queuing up work items. */
> atomic_inc(&shared->skip_work);
> /*
> - * If dbs_timer_handler() is already running, it may not notice the
> - * incremented skip_work, so wait for it to complete to prevent its work
> - * item from being queued up after the cancel_work_sync() below.
> - */
> - gov_cancel_timers(shared->policy);
> - /*
> - * In case dbs_timer_handler() managed to run and spawn a work item
> - * before the timers have been canceled, wait for that work item to
> - * complete and then cancel all of the timers set up by it. If
> - * dbs_timer_handler() runs again at that point, it will see the
> - * positive value of skip_work and won't spawn any more work items.
> + * If dbs_update_util_handler() is already running, it may not notice
> + * the incremented skip_work, so wait for it to complete to prevent its
> + * work item from being queued up after the cancel_work_sync() below.
> */
> + gov_clear_update_util(shared->policy);
> cancel_work_sync(&shared->work);
How are we sure that the irq-work can't be pending at this point of
time, which will queue the above works again ?
> - gov_cancel_timers(shared->policy);
> atomic_set(&shared->skip_work, 0);
> }
> EXPORT_SYMBOL_GPL(gov_cancel_work);
>
> -/* Will return if we need to evaluate cpu load again or not */
> -static bool need_load_eval(struct cpu_common_dbs_info *shared,
> - unsigned int sampling_rate)
> -{
> - if (policy_is_shared(shared->policy)) {
> - ktime_t time_now = ktime_get();
> - s64 delta_us = ktime_us_delta(time_now, shared->time_stamp);
> -
> - /* Do nothing if we recently have sampled */
> - if (delta_us < (s64)(sampling_rate / 2))
> - return false;
> - else
> - shared->time_stamp = time_now;
> - }
> -
> - return true;
> -}
> -
> static void dbs_work_handler(struct work_struct *work)
> {
> struct cpu_common_dbs_info *shared = container_of(work, struct
> @@ -235,14 +212,10 @@ static void dbs_work_handler(struct work
> struct cpufreq_policy *policy;
> struct dbs_data *dbs_data;
> unsigned int sampling_rate, delay;
> - bool eval_load;
>
> policy = shared->policy;
> dbs_data = policy->governor_data;
>
> - /* Kill all timers */
> - gov_cancel_timers(policy);
> -
> if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
> struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
>
> @@ -253,37 +226,53 @@ static void dbs_work_handler(struct work
> sampling_rate = od_tuners->sampling_rate;
> }
>
> - eval_load = need_load_eval(shared, sampling_rate);
> -
> /*
> - * Make sure cpufreq_governor_limits() isn't evaluating load in
> + * Make sure cpufreq_governor_limits() isn't evaluating load or the
> + * ondemand governor isn't reading the time stamp and sampling rate in
> * parallel.
> */
> mutex_lock(&shared->timer_mutex);
> - delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
> + delay = dbs_data->cdata->gov_dbs_timer(policy);
> + shared->sample_delay_ns = jiffies_to_nsecs(delay);
> + shared->time_stamp = ktime_get();
> mutex_unlock(&shared->timer_mutex);
>
> + smp_mb__before_atomic();
And why is this required exactly ? Maybe a comment as well to clarify
this as this isn't obvious ?
> atomic_dec(&shared->skip_work);
> +}
>
> - gov_add_timers(policy, delay);
> +static void dbs_irq_work(struct irq_work *irq_work)
> +{
> + struct cpu_common_dbs_info *shared;
> +
> + shared = container_of(irq_work, struct cpu_common_dbs_info, irq_work);
> + schedule_work(&shared->work);
> }
>
> -static void dbs_timer_handler(unsigned long data)
> +static void dbs_update_util_handler(struct update_util_data *data, u64 time,
> + unsigned long util, unsigned long max)
> {
> - struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
> + struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
> struct cpu_common_dbs_info *shared = cdbs->shared;
>
> /*
> - * Timer handler may not be allowed to queue the work at the moment,
> - * because:
> - * - Another timer handler has done that
> - * - We are stopping the governor
> - * - Or we are updating the sampling rate of the ondemand governor
> + * The work may not be allowed to be queued up right now.
> + * Possible reasons:
> + * - Work has already been queued up or is in progress.
> + * - The governor is being stopped.
> + * - It is too early (too little time from the previous sample).
> */
> - if (atomic_inc_return(&shared->skip_work) > 1)
> - atomic_dec(&shared->skip_work);
> - else
> - queue_work(system_wq, &shared->work);
> + if (atomic_inc_return(&shared->skip_work) == 1) {
> + u64 delta_ns;
> +
> + delta_ns = time - cdbs->last_sample_time;
> + if ((s64)delta_ns >= shared->sample_delay_ns) {
> + cdbs->last_sample_time = time;
> + irq_work_queue_on(&shared->irq_work, smp_processor_id());
> + return;
> + }
> + }
> + atomic_dec(&shared->skip_work);
> }
>
> static void set_sampling_rate(struct dbs_data *dbs_data,
> @@ -467,9 +456,6 @@ static int cpufreq_governor_start(struct
> io_busy = od_tuners->io_is_busy;
> }
>
> - shared->policy = policy;
> - shared->time_stamp = ktime_get();
> -
> for_each_cpu(j, policy->cpus) {
> struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
> unsigned int prev_load;
> @@ -485,10 +471,10 @@ static int cpufreq_governor_start(struct
> if (ignore_nice)
> j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
>
> - __setup_timer(&j_cdbs->timer, dbs_timer_handler,
> - (unsigned long)j_cdbs,
> - TIMER_DEFERRABLE | TIMER_IRQSAFE);
> + j_cdbs->update_util.func = dbs_update_util_handler;
> }
> + shared->policy = policy;
> + init_irq_work(&shared->irq_work, dbs_irq_work);
>
> if (cdata->governor == GOV_CONSERVATIVE) {
> struct cs_cpu_dbs_info_s *cs_dbs_info =
> @@ -505,7 +491,7 @@ static int cpufreq_governor_start(struct
> od_ops->powersave_bias_init_cpu(cpu);
> }
>
> - gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
> + gov_set_update_util(shared, sampling_rate);
> return 0;
> }
>
> Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
> +++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
> @@ -191,7 +191,7 @@ static void od_check_cpu(int cpu, unsign
> }
> }
>
> -static unsigned int od_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
> +static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
> {
> struct dbs_data *dbs_data = policy->governor_data;
> unsigned int cpu = policy->cpu;
> @@ -200,9 +200,6 @@ static unsigned int od_dbs_timer(struct
> struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> int delay = 0, sample_type = dbs_info->sample_type;
Perhaps, the delay = 0 can be dropped now and ...
>
> - if (!modify_all)
> - goto max_delay;
> -
> /* Common NORMAL_SAMPLE setup */
> dbs_info->sample_type = OD_NORMAL_SAMPLE;
> if (sample_type == OD_SUB_SAMPLE) {
> @@ -218,7 +215,6 @@ static unsigned int od_dbs_timer(struct
> }
> }
>
> -max_delay:
> if (!delay)
> delay = delay_for_sampling_rate(od_tuners->sampling_rate
> * dbs_info->rate_mult);
^^ can be moved to the else part of above block ..
> @@ -264,7 +260,7 @@ static void update_sampling_rate(struct
> struct od_cpu_dbs_info_s *dbs_info;
> struct cpu_dbs_info *cdbs;
> struct cpu_common_dbs_info *shared;
> - unsigned long next_sampling, appointed_at;
> + ktime_t next_sampling, appointed_at;
>
> dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> cdbs = &dbs_info->cdbs;
> @@ -292,16 +288,19 @@ static void update_sampling_rate(struct
> continue;
>
> /*
> - * Checking this for any CPU should be fine, timers for all of
> - * them are scheduled together.
> + * Checking this for any CPU sharing the policy should be fine,
> + * they are all scheduled to sample at the same time.
> */
> - next_sampling = jiffies + usecs_to_jiffies(new_rate);
> - appointed_at = dbs_info->cdbs.timer.expires;
> + next_sampling = ktime_add_us(ktime_get(), new_rate);
>
> - if (time_before(next_sampling, appointed_at)) {
> - gov_cancel_work(shared);
> - gov_add_timers(policy, usecs_to_jiffies(new_rate));
> + mutex_lock(&shared->timer_mutex);
Why is taking this lock important here ?
> + appointed_at = ktime_add_ns(shared->time_stamp,
Also I failed to understand why we need time_stamp variable at all?
Why can't we use last_sample_time ?
> + shared->sample_delay_ns);
> + mutex_unlock(&shared->timer_mutex);
>
> + if (ktime_before(next_sampling, appointed_at)) {
> + gov_cancel_work(shared);
> + gov_set_update_util(shared, new_rate);
You don't need to a complete update here, the pointers are all fine.
> }
> }
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-04 12:00 +0100 |
| Subject | Re: [Update][PATCH 3/3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qYpNM-7uB-19@gated-at.bofh.it> |
| In reply to | #1326400 |
On Thursday, February 04, 2016 10:19:59 AM Viresh Kumar wrote:
> On 03-02-16, 02:16, Rafael J. Wysocki wrote:
> > Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> > -void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
> > +void gov_set_update_util(struct cpu_common_dbs_info *shared,
> > + unsigned int delay_us)
> > {
> > + struct cpufreq_policy *policy = shared->policy;
> > struct dbs_data *dbs_data = policy->governor_data;
> > - struct cpu_dbs_info *cdbs;
> > int cpu;
> >
> > + shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
> > + shared->time_stamp = ktime_get();
> > +
> > for_each_cpu(cpu, policy->cpus) {
> > - cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> > - cdbs->timer.expires = jiffies + delay;
> > - add_timer_on(&cdbs->timer, cpu);
> > + struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> > +
> > + cdbs->last_sample_time = 0;
> > + cpufreq_set_update_util_data(cpu, &cdbs->update_util);
>
> Why no synchronize_rcu() here?
Because it is not needed. This always changes a NULL pointer into a non-NULL.
> This can be called from ondemand governor on sampling-rate updates ..
But that calls gov_cancel_work() before, right?
>
> > }
> > }
> > -EXPORT_SYMBOL_GPL(gov_add_timers);
> > +EXPORT_SYMBOL_GPL(gov_set_update_util);
> >
> > -static inline void gov_cancel_timers(struct cpufreq_policy *policy)
> > +static inline void gov_clear_update_util(struct cpufreq_policy *policy)
> > {
> > - struct dbs_data *dbs_data = policy->governor_data;
> > - struct cpu_dbs_info *cdbs;
> > int i;
> >
> > - for_each_cpu(i, policy->cpus) {
> > - cdbs = dbs_data->cdata->get_cpu_cdbs(i);
> > - del_timer_sync(&cdbs->timer);
> > - }
> > + for_each_cpu(i, policy->cpus)
> > + cpufreq_set_update_util_data(i, NULL);
> > +
> > + synchronize_rcu();
> > }
> >
> > void gov_cancel_work(struct cpu_common_dbs_info *shared)
> > {
> > - /* Tell dbs_timer_handler() to skip queuing up work items. */
> > + /* Tell dbs_update_util_handler() to skip queuing up work items. */
> > atomic_inc(&shared->skip_work);
> > /*
> > - * If dbs_timer_handler() is already running, it may not notice the
> > - * incremented skip_work, so wait for it to complete to prevent its work
> > - * item from being queued up after the cancel_work_sync() below.
> > - */
> > - gov_cancel_timers(shared->policy);
> > - /*
> > - * In case dbs_timer_handler() managed to run and spawn a work item
> > - * before the timers have been canceled, wait for that work item to
> > - * complete and then cancel all of the timers set up by it. If
> > - * dbs_timer_handler() runs again at that point, it will see the
> > - * positive value of skip_work and won't spawn any more work items.
> > + * If dbs_update_util_handler() is already running, it may not notice
> > + * the incremented skip_work, so wait for it to complete to prevent its
> > + * work item from being queued up after the cancel_work_sync() below.
> > */
> > + gov_clear_update_util(shared->policy);
> > cancel_work_sync(&shared->work);
>
> How are we sure that the irq-work can't be pending at this point of
> time, which will queue the above works again ?
Good point. The irq_work has to be waited for here too.
> > - gov_cancel_timers(shared->policy);
> > atomic_set(&shared->skip_work, 0);
> > }
> > EXPORT_SYMBOL_GPL(gov_cancel_work);
> >
> > -/* Will return if we need to evaluate cpu load again or not */
> > -static bool need_load_eval(struct cpu_common_dbs_info *shared,
> > - unsigned int sampling_rate)
> > -{
> > - if (policy_is_shared(shared->policy)) {
> > - ktime_t time_now = ktime_get();
> > - s64 delta_us = ktime_us_delta(time_now, shared->time_stamp);
> > -
> > - /* Do nothing if we recently have sampled */
> > - if (delta_us < (s64)(sampling_rate / 2))
> > - return false;
> > - else
> > - shared->time_stamp = time_now;
> > - }
> > -
> > - return true;
> > -}
> > -
> > static void dbs_work_handler(struct work_struct *work)
> > {
> > struct cpu_common_dbs_info *shared = container_of(work, struct
> > @@ -235,14 +212,10 @@ static void dbs_work_handler(struct work
> > struct cpufreq_policy *policy;
> > struct dbs_data *dbs_data;
> > unsigned int sampling_rate, delay;
> > - bool eval_load;
> >
> > policy = shared->policy;
> > dbs_data = policy->governor_data;
> >
> > - /* Kill all timers */
> > - gov_cancel_timers(policy);
> > -
> > if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
> > struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
> >
> > @@ -253,37 +226,53 @@ static void dbs_work_handler(struct work
> > sampling_rate = od_tuners->sampling_rate;
> > }
> >
> > - eval_load = need_load_eval(shared, sampling_rate);
> > -
> > /*
> > - * Make sure cpufreq_governor_limits() isn't evaluating load in
> > + * Make sure cpufreq_governor_limits() isn't evaluating load or the
> > + * ondemand governor isn't reading the time stamp and sampling rate in
> > * parallel.
> > */
> > mutex_lock(&shared->timer_mutex);
> > - delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
> > + delay = dbs_data->cdata->gov_dbs_timer(policy);
> > + shared->sample_delay_ns = jiffies_to_nsecs(delay);
> > + shared->time_stamp = ktime_get();
> > mutex_unlock(&shared->timer_mutex);
> >
> > + smp_mb__before_atomic();
>
> And why is this required exactly ? Maybe a comment as well to clarify
> this as this isn't obvious ?
OK, you have a point.
This relies on the atomic_dec() below to happen after sample_delay_ns has
been updated, to prevent dbs_update_util_handler() from using a stale
value.
> > atomic_dec(&shared->skip_work);
> > +}
> >
> > - gov_add_timers(policy, delay);
> > +static void dbs_irq_work(struct irq_work *irq_work)
> > +{
> > + struct cpu_common_dbs_info *shared;
> > +
> > + shared = container_of(irq_work, struct cpu_common_dbs_info, irq_work);
> > + schedule_work(&shared->work);
> > }
> >
> > -static void dbs_timer_handler(unsigned long data)
> > +static void dbs_update_util_handler(struct update_util_data *data, u64 time,
> > + unsigned long util, unsigned long max)
> > {
> > - struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
> > + struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
> > struct cpu_common_dbs_info *shared = cdbs->shared;
> >
> > /*
> > - * Timer handler may not be allowed to queue the work at the moment,
> > - * because:
> > - * - Another timer handler has done that
> > - * - We are stopping the governor
> > - * - Or we are updating the sampling rate of the ondemand governor
> > + * The work may not be allowed to be queued up right now.
> > + * Possible reasons:
> > + * - Work has already been queued up or is in progress.
> > + * - The governor is being stopped.
> > + * - It is too early (too little time from the previous sample).
> > */
> > - if (atomic_inc_return(&shared->skip_work) > 1)
> > - atomic_dec(&shared->skip_work);
> > - else
> > - queue_work(system_wq, &shared->work);
> > + if (atomic_inc_return(&shared->skip_work) == 1) {
> > + u64 delta_ns;
> > +
> > + delta_ns = time - cdbs->last_sample_time;
> > + if ((s64)delta_ns >= shared->sample_delay_ns) {
> > + cdbs->last_sample_time = time;
> > + irq_work_queue_on(&shared->irq_work, smp_processor_id());
> > + return;
> > + }
> > + }
> > + atomic_dec(&shared->skip_work);
> > }
> >
> > static void set_sampling_rate(struct dbs_data *dbs_data,
> > @@ -467,9 +456,6 @@ static int cpufreq_governor_start(struct
> > io_busy = od_tuners->io_is_busy;
> > }
> >
> > - shared->policy = policy;
> > - shared->time_stamp = ktime_get();
> > -
> > for_each_cpu(j, policy->cpus) {
> > struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
> > unsigned int prev_load;
> > @@ -485,10 +471,10 @@ static int cpufreq_governor_start(struct
> > if (ignore_nice)
> > j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
> >
> > - __setup_timer(&j_cdbs->timer, dbs_timer_handler,
> > - (unsigned long)j_cdbs,
> > - TIMER_DEFERRABLE | TIMER_IRQSAFE);
> > + j_cdbs->update_util.func = dbs_update_util_handler;
> > }
> > + shared->policy = policy;
> > + init_irq_work(&shared->irq_work, dbs_irq_work);
> >
> > if (cdata->governor == GOV_CONSERVATIVE) {
> > struct cs_cpu_dbs_info_s *cs_dbs_info =
> > @@ -505,7 +491,7 @@ static int cpufreq_governor_start(struct
> > od_ops->powersave_bias_init_cpu(cpu);
> > }
> >
> > - gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
> > + gov_set_update_util(shared, sampling_rate);
> > return 0;
> > }
> >
> > Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
> > +++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
> > @@ -191,7 +191,7 @@ static void od_check_cpu(int cpu, unsign
> > }
> > }
> >
> > -static unsigned int od_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
> > +static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
> > {
> > struct dbs_data *dbs_data = policy->governor_data;
> > unsigned int cpu = policy->cpu;
> > @@ -200,9 +200,6 @@ static unsigned int od_dbs_timer(struct
> > struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> > int delay = 0, sample_type = dbs_info->sample_type;
>
> Perhaps, the delay = 0 can be dropped now and ...
>
> >
> > - if (!modify_all)
> > - goto max_delay;
> > -
> > /* Common NORMAL_SAMPLE setup */
> > dbs_info->sample_type = OD_NORMAL_SAMPLE;
> > if (sample_type == OD_SUB_SAMPLE) {
> > @@ -218,7 +215,6 @@ static unsigned int od_dbs_timer(struct
> > }
> > }
> >
> > -max_delay:
> > if (!delay)
> > delay = delay_for_sampling_rate(od_tuners->sampling_rate
> > * dbs_info->rate_mult);
>
> ^^ can be moved to the else part of above block ..
Both this and the above are valid observation, but those changes should be
made in a follow-up patch IMO.
> > @@ -264,7 +260,7 @@ static void update_sampling_rate(struct
> > struct od_cpu_dbs_info_s *dbs_info;
> > struct cpu_dbs_info *cdbs;
> > struct cpu_common_dbs_info *shared;
> > - unsigned long next_sampling, appointed_at;
> > + ktime_t next_sampling, appointed_at;
> >
> > dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> > cdbs = &dbs_info->cdbs;
> > @@ -292,16 +288,19 @@ static void update_sampling_rate(struct
> > continue;
> >
> > /*
> > - * Checking this for any CPU should be fine, timers for all of
> > - * them are scheduled together.
> > + * Checking this for any CPU sharing the policy should be fine,
> > + * they are all scheduled to sample at the same time.
> > */
> > - next_sampling = jiffies + usecs_to_jiffies(new_rate);
> > - appointed_at = dbs_info->cdbs.timer.expires;
> > + next_sampling = ktime_add_us(ktime_get(), new_rate);
> >
> > - if (time_before(next_sampling, appointed_at)) {
> > - gov_cancel_work(shared);
> > - gov_add_timers(policy, usecs_to_jiffies(new_rate));
> > + mutex_lock(&shared->timer_mutex);
>
> Why is taking this lock important here ?
Because this reads both time_stamp and sample_delay_ns and uses them in
a computation. If they happen to be out of sync, this surely isn't right.
> > + appointed_at = ktime_add_ns(shared->time_stamp,
>
> Also I failed to understand why we need time_stamp variable at all?
> Why can't we use last_sample_time ?
Because the time base for last_sample_time may be different, so comparing it
to the return value of ktime_get() may not lead to correct decisions, so to
speak.
> > + shared->sample_delay_ns);
> > + mutex_unlock(&shared->timer_mutex);
> >
> > + if (ktime_before(next_sampling, appointed_at)) {
> > + gov_cancel_work(shared);
> > + gov_set_update_util(shared, new_rate);
>
> You don't need to a complete update here, the pointers are all fine.
I do, but that's not because of the pointers.
Effectively, I need to change sample_delay_ns and that's the most startghtforward
way to do that safely.
It may not be the most efficient, but this is not a fast path anyway.
> > }
> > }
>
>
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-05 02:30 +0100 |
| Subject | [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qYDnJ-1Cc-13@gated-at.bofh.it> |
| In reply to | #1324797 |
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Instead of using a per-CPU deferrable timer for queuing up governor
work items, register a utilization update callback that will be
invoked from the scheduler on utilization changes.
The sampling rate is still the same as what was used for the
deferrable timers and the added irq_work overhead should be offset by
the eliminated timers overhead, so in theory the functional impact of
this patch should not be significant.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
The v3 addresses some review comments from Viresh and a couple of issues found
by me. Changes from the previous version:
- Synchronize gov_cancel_work() with the (new) irq_work properly.
- Add a comment about the (new) memory barrier.
- Move samle_delay_ns to "shared" (struct cpu_common_dbs_info) so it is the
same for all policy CPUs (without this modification we may end up taking
samples too often).
- Drop some more unused code (in dbs_work_handler()).
Thanks,
Rafael
---
drivers/cpufreq/cpufreq_conservative.c | 6 -
drivers/cpufreq/cpufreq_governor.c | 157 ++++++++++++++-------------------
drivers/cpufreq/cpufreq_governor.h | 15 ++-
drivers/cpufreq/cpufreq_ondemand.c | 25 ++---
4 files changed, 95 insertions(+), 108 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -18,6 +18,8 @@
#define _CPUFREQ_GOVERNOR_H
#include <linux/atomic.h>
+#include <linux/irq_work.h>
+#include <linux/completion.h>
#include <linux/cpufreq.h>
#include <linux/kernel_stat.h>
#include <linux/module.h>
@@ -139,7 +141,11 @@ struct cpu_common_dbs_info {
struct mutex timer_mutex;
ktime_t time_stamp;
+ u64 last_sample_time;
+ s64 sample_delay_ns;
atomic_t skip_work;
+ struct irq_work irq_work;
+ struct completion irq_work_done;
struct work_struct work;
};
@@ -155,7 +161,7 @@ struct cpu_dbs_info {
* wake-up from idle.
*/
unsigned int prev_load;
- struct timer_list timer;
+ struct update_util_data update_util;
struct cpu_common_dbs_info *shared;
};
@@ -212,8 +218,7 @@ struct common_dbs_data {
struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
void *(*get_cpu_dbs_info_s)(int cpu);
- unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy,
- bool modify_all);
+ unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
void (*gov_check_cpu)(int cpu, unsigned int load);
int (*init)(struct dbs_data *dbs_data, bool notify);
void (*exit)(struct dbs_data *dbs_data, bool notify);
@@ -270,8 +275,8 @@ static ssize_t show_sampling_rate_min_go
}
extern struct mutex cpufreq_governor_lock;
-
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us);
void gov_cancel_work(struct cpu_common_dbs_info *shared);
void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
int cpufreq_governor_dbs(struct cpufreq_policy *policy,
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -128,10 +128,10 @@ void dbs_check_cpu(struct dbs_data *dbs_
* dropped down. So we perform the copy only once, upon the
* first wake-up from idle.)
*
- * Detecting this situation is easy: the governor's deferrable
- * timer would not have fired during CPU-idle periods. Hence
- * an unusually large 'wall_time' (as compared to the sampling
- * rate) indicates this scenario.
+ * Detecting this situation is easy: the governor's utilization
+ * update handler would not have run during CPU-idle periods.
+ * Hence, an unusually large 'wall_time' (as compared to the
+ * sampling rate) indicates this scenario.
*
* prev_load can be zero in two cases and we must recalculate it
* for both cases:
@@ -161,129 +161,116 @@ void dbs_check_cpu(struct dbs_data *dbs_
}
EXPORT_SYMBOL_GPL(dbs_check_cpu);
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us)
{
+ struct cpufreq_policy *policy = shared->policy;
struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int cpu;
+ shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
+ shared->time_stamp = ktime_get();
+ shared->last_sample_time = 0;
+
for_each_cpu(cpu, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
- cdbs->timer.expires = jiffies + delay;
- add_timer_on(&cdbs->timer, cpu);
+ struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+
+ cpufreq_set_update_util_data(cpu, &cdbs->update_util);
}
}
-EXPORT_SYMBOL_GPL(gov_add_timers);
+EXPORT_SYMBOL_GPL(gov_set_update_util);
-static inline void gov_cancel_timers(struct cpufreq_policy *policy)
+static inline void gov_clear_update_util(struct cpufreq_policy *policy)
{
- struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int i;
- for_each_cpu(i, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(i);
- del_timer_sync(&cdbs->timer);
- }
+ for_each_cpu(i, policy->cpus)
+ cpufreq_set_update_util_data(i, NULL);
+
+ synchronize_rcu();
}
void gov_cancel_work(struct cpu_common_dbs_info *shared)
{
- /* Tell dbs_timer_handler() to skip queuing up work items. */
+ /* Tell dbs_update_util_handler() to skip queuing up work items. */
atomic_inc(&shared->skip_work);
/*
- * If dbs_timer_handler() is already running, it may not notice the
- * incremented skip_work, so wait for it to complete to prevent its work
- * item from being queued up after the cancel_work_sync() below.
- */
- gov_cancel_timers(shared->policy);
- /*
- * In case dbs_timer_handler() managed to run and spawn a work item
- * before the timers have been canceled, wait for that work item to
- * complete and then cancel all of the timers set up by it. If
- * dbs_timer_handler() runs again at that point, it will see the
- * positive value of skip_work and won't spawn any more work items.
+ * If dbs_update_util_handler() is already running, it may not notice
+ * the incremented skip_work, so wait for it to complete to prevent its
+ * work item from being queued up after the cancel_work_sync() below.
*/
+ gov_clear_update_util(shared->policy);
+ wait_for_completion(&shared->irq_work_done);
cancel_work_sync(&shared->work);
- gov_cancel_timers(shared->policy);
atomic_set(&shared->skip_work, 0);
}
EXPORT_SYMBOL_GPL(gov_cancel_work);
-/* Will return if we need to evaluate cpu load again or not */
-static bool need_load_eval(struct cpu_common_dbs_info *shared,
- unsigned int sampling_rate)
-{
- if (policy_is_shared(shared->policy)) {
- ktime_t time_now = ktime_get();
- s64 delta_us = ktime_us_delta(time_now, shared->time_stamp);
-
- /* Do nothing if we recently have sampled */
- if (delta_us < (s64)(sampling_rate / 2))
- return false;
- else
- shared->time_stamp = time_now;
- }
-
- return true;
-}
-
static void dbs_work_handler(struct work_struct *work)
{
struct cpu_common_dbs_info *shared = container_of(work, struct
cpu_common_dbs_info, work);
struct cpufreq_policy *policy;
struct dbs_data *dbs_data;
- unsigned int sampling_rate, delay;
- bool eval_load;
+ unsigned int delay;
policy = shared->policy;
dbs_data = policy->governor_data;
- /* Kill all timers */
- gov_cancel_timers(policy);
-
- if (dbs_data->cdata->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;
- }
-
- eval_load = need_load_eval(shared, sampling_rate);
-
/*
- * Make sure cpufreq_governor_limits() isn't evaluating load in
+ * Make sure cpufreq_governor_limits() isn't evaluating load or the
+ * ondemand governor isn't reading the time stamp and sampling rate in
* parallel.
*/
mutex_lock(&shared->timer_mutex);
- delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
+ delay = dbs_data->cdata->gov_dbs_timer(policy);
+ shared->sample_delay_ns = jiffies_to_nsecs(delay);
+ shared->time_stamp = ktime_get();
mutex_unlock(&shared->timer_mutex);
+ /*
+ * If the atomic operation below is reordered with respect to the
+ * sample delay modification, the utilization update handler may end
+ * up using a stale sample delay value.
+ */
+ smp_mb__before_atomic();
atomic_dec(&shared->skip_work);
+}
- gov_add_timers(policy, delay);
+static void dbs_irq_work(struct irq_work *irq_work)
+{
+ struct cpu_common_dbs_info *shared;
+
+ shared = container_of(irq_work, struct cpu_common_dbs_info, irq_work);
+ schedule_work(&shared->work);
+ complete(&shared->irq_work_done);
}
-static void dbs_timer_handler(unsigned long data)
+static void dbs_update_util_handler(struct update_util_data *data, u64 time,
+ unsigned long util, unsigned long max)
{
- struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
+ struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
struct cpu_common_dbs_info *shared = cdbs->shared;
/*
- * Timer handler may not be allowed to queue the work at the moment,
- * because:
- * - Another timer handler has done that
- * - We are stopping the governor
- * - Or we are updating the sampling rate of the ondemand governor
+ * The work may not be allowed to be queued up right now.
+ * Possible reasons:
+ * - Work has already been queued up or is in progress.
+ * - The governor is being stopped.
+ * - It is too early (too little time from the previous sample).
*/
- if (atomic_inc_return(&shared->skip_work) > 1)
- atomic_dec(&shared->skip_work);
- else
- queue_work(system_wq, &shared->work);
+ if (atomic_inc_return(&shared->skip_work) == 1) {
+ u64 delta_ns;
+
+ delta_ns = time - shared->last_sample_time;
+ if ((s64)delta_ns >= shared->sample_delay_ns) {
+ shared->last_sample_time = time;
+ reinit_completion(&shared->irq_work_done);
+ irq_work_queue_on(&shared->irq_work, smp_processor_id());
+ return;
+ }
+ }
+ atomic_dec(&shared->skip_work);
}
static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -467,9 +454,6 @@ static int cpufreq_governor_start(struct
io_busy = od_tuners->io_is_busy;
}
- shared->policy = policy;
- shared->time_stamp = ktime_get();
-
for_each_cpu(j, policy->cpus) {
struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
unsigned int prev_load;
@@ -485,10 +469,11 @@ static int cpufreq_governor_start(struct
if (ignore_nice)
j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
- __setup_timer(&j_cdbs->timer, dbs_timer_handler,
- (unsigned long)j_cdbs,
- TIMER_DEFERRABLE | TIMER_IRQSAFE);
+ j_cdbs->update_util.func = dbs_update_util_handler;
}
+ shared->policy = policy;
+ init_irq_work(&shared->irq_work, dbs_irq_work);
+ init_completion(&shared->irq_work_done);
if (cdata->governor == GOV_CONSERVATIVE) {
struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -505,7 +490,7 @@ static int cpufreq_governor_start(struct
od_ops->powersave_bias_init_cpu(cpu);
}
- gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
+ gov_set_update_util(shared, sampling_rate);
return 0;
}
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -191,7 +191,7 @@ static void od_check_cpu(int cpu, unsign
}
}
-static unsigned int od_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
unsigned int cpu = policy->cpu;
@@ -200,9 +200,6 @@ static unsigned int od_dbs_timer(struct
struct od_dbs_tuners *od_tuners = dbs_data->tuners;
int delay = 0, sample_type = dbs_info->sample_type;
- if (!modify_all)
- goto max_delay;
-
/* Common NORMAL_SAMPLE setup */
dbs_info->sample_type = OD_NORMAL_SAMPLE;
if (sample_type == OD_SUB_SAMPLE) {
@@ -218,7 +215,6 @@ static unsigned int od_dbs_timer(struct
}
}
-max_delay:
if (!delay)
delay = delay_for_sampling_rate(od_tuners->sampling_rate
* dbs_info->rate_mult);
@@ -264,7 +260,7 @@ static void update_sampling_rate(struct
struct od_cpu_dbs_info_s *dbs_info;
struct cpu_dbs_info *cdbs;
struct cpu_common_dbs_info *shared;
- unsigned long next_sampling, appointed_at;
+ ktime_t next_sampling, appointed_at;
dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
cdbs = &dbs_info->cdbs;
@@ -292,16 +288,19 @@ static void update_sampling_rate(struct
continue;
/*
- * Checking this for any CPU should be fine, timers for all of
- * them are scheduled together.
+ * Checking this for any CPU sharing the policy should be fine,
+ * they are all scheduled to sample at the same time.
*/
- next_sampling = jiffies + usecs_to_jiffies(new_rate);
- appointed_at = dbs_info->cdbs.timer.expires;
+ next_sampling = ktime_add_us(ktime_get(), new_rate);
- if (time_before(next_sampling, appointed_at)) {
- gov_cancel_work(shared);
- gov_add_timers(policy, usecs_to_jiffies(new_rate));
+ mutex_lock(&shared->timer_mutex);
+ appointed_at = ktime_add_ns(shared->time_stamp,
+ shared->sample_delay_ns);
+ mutex_unlock(&shared->timer_mutex);
+ if (ktime_before(next_sampling, appointed_at)) {
+ gov_cancel_work(shared);
+ gov_set_update_util(shared, new_rate);
}
}
Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -115,14 +115,12 @@ static void cs_check_cpu(int cpu, unsign
}
}
-static unsigned int cs_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
- if (modify_all)
- dbs_check_cpu(dbs_data, policy->cpu);
-
+ dbs_check_cpu(dbs_data, policy->cpu);
return delay_for_sampling_rate(cs_tuners->sampling_rate);
}
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-05 08:00 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qYIx4-57t-5@gated-at.bofh.it> |
| In reply to | #1327389 |
Will suck some more blood, sorry about that :)
On 05-02-16, 02:28, Rafael J. Wysocki wrote:
> The v3 addresses some review comments from Viresh and a couple of issues found
> by me. Changes from the previous version:
> - Synchronize gov_cancel_work() with the (new) irq_work properly.
> - Add a comment about the (new) memory barrier.
> - Move samle_delay_ns to "shared" (struct cpu_common_dbs_info) so it is the
sample_delay_ns was already there, you moved last_sample_time instead :)
> @@ -139,7 +141,11 @@ struct cpu_common_dbs_info {
> struct mutex timer_mutex;
>
> ktime_t time_stamp;
> + u64 last_sample_time;
> + s64 sample_delay_ns;
> atomic_t skip_work;
> + struct irq_work irq_work;
Just for my understanding, why can't we schedule a normal work directly? Is it
because of scheduler's hotpath and queue_work() is slow?
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> +void gov_set_update_util(struct cpu_common_dbs_info *shared,
> + unsigned int delay_us)
> {
> + struct cpufreq_policy *policy = shared->policy;
> struct dbs_data *dbs_data = policy->governor_data;
> - struct cpu_dbs_info *cdbs;
> int cpu;
>
> + shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
> + shared->time_stamp = ktime_get();
> + shared->last_sample_time = 0;
Calling this routine from update_sampling_rate() is still wrong. Because that
will also make last_sample_time = 0, which means that we will schedule the
irq-work on the next util update.
We surely didn't wanted that to happen, isn't it ?
> for_each_cpu(cpu, policy->cpus) {
> - cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> - cdbs->timer.expires = jiffies + delay;
> - add_timer_on(&cdbs->timer, cpu);
> + struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
> +
> + cpufreq_set_update_util_data(cpu, &cdbs->update_util);
> }
> }
> -EXPORT_SYMBOL_GPL(gov_add_timers);
> +EXPORT_SYMBOL_GPL(gov_set_update_util);
> void gov_cancel_work(struct cpu_common_dbs_info *shared)
> {
> - /* Tell dbs_timer_handler() to skip queuing up work items. */
> + /* Tell dbs_update_util_handler() to skip queuing up work items. */
> atomic_inc(&shared->skip_work);
> /*
> - * If dbs_timer_handler() is already running, it may not notice the
> - * incremented skip_work, so wait for it to complete to prevent its work
> - * item from being queued up after the cancel_work_sync() below.
> - */
> - gov_cancel_timers(shared->policy);
> - /*
> - * In case dbs_timer_handler() managed to run and spawn a work item
> - * before the timers have been canceled, wait for that work item to
> - * complete and then cancel all of the timers set up by it. If
> - * dbs_timer_handler() runs again at that point, it will see the
> - * positive value of skip_work and won't spawn any more work items.
> + * If dbs_update_util_handler() is already running, it may not notice
> + * the incremented skip_work, so wait for it to complete to prevent its
> + * work item from being queued up after the cancel_work_sync() below.
> */
> + gov_clear_update_util(shared->policy);
> + wait_for_completion(&shared->irq_work_done);
I may be wrong, but isn't running irq_work_sync() enough here instead ?
> cancel_work_sync(&shared->work);
> - gov_cancel_timers(shared->policy);
> atomic_set(&shared->skip_work, 0);
> }
> EXPORT_SYMBOL_GPL(gov_cancel_work);
> Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
> @@ -264,7 +260,7 @@ static void update_sampling_rate(struct
> struct od_cpu_dbs_info_s *dbs_info;
> struct cpu_dbs_info *cdbs;
> struct cpu_common_dbs_info *shared;
> - unsigned long next_sampling, appointed_at;
> + ktime_t next_sampling, appointed_at;
>
> dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> cdbs = &dbs_info->cdbs;
> @@ -292,16 +288,19 @@ static void update_sampling_rate(struct
> continue;
>
> /*
> - * Checking this for any CPU should be fine, timers for all of
> - * them are scheduled together.
> + * Checking this for any CPU sharing the policy should be fine,
> + * they are all scheduled to sample at the same time.
> */
> - next_sampling = jiffies + usecs_to_jiffies(new_rate);
> - appointed_at = dbs_info->cdbs.timer.expires;
> + next_sampling = ktime_add_us(ktime_get(), new_rate);
>
> - if (time_before(next_sampling, appointed_at)) {
> - gov_cancel_work(shared);
> - gov_add_timers(policy, usecs_to_jiffies(new_rate));
> + mutex_lock(&shared->timer_mutex);
> + appointed_at = ktime_add_ns(shared->time_stamp,
> + shared->sample_delay_ns);
> + mutex_unlock(&shared->timer_mutex);
>
> + if (ktime_before(next_sampling, appointed_at)) {
> + gov_cancel_work(shared);
> + gov_set_update_util(shared, new_rate);
So, I don't think we need to call these heavy routines at all here. Just use the
above timer_mutex to update time_stamp and sample_delay_ns.
Over that, that particular change might turn out to be a big big bonus for us.
Why would we be taking the od_dbs_cdata.mutex in this routine anymore ? We
aren't removing/adding timers anymore, just update the sample_delay_ns and there
shouldn't be any races. Ofcourse you need to use the same timer_mutex in util's
handler as well around sample_delay_ns, I believe.
And that will also kill the circular dependency lockdep we have been chasing
badly :)
Or am I being over excited here ? :(
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-05 14:40 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qYOMb-P6-43@gated-at.bofh.it> |
| In reply to | #1327506 |
On Fri, Feb 5, 2016 at 7:50 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Will suck some more blood, sorry about that :)
>
> On 05-02-16, 02:28, Rafael J. Wysocki wrote:
>> The v3 addresses some review comments from Viresh and a couple of issues found
>> by me. Changes from the previous version:
>> - Synchronize gov_cancel_work() with the (new) irq_work properly.
>> - Add a comment about the (new) memory barrier.
>> - Move samle_delay_ns to "shared" (struct cpu_common_dbs_info) so it is the
>
> sample_delay_ns was already there, you moved last_sample_time instead :)
>
>> @@ -139,7 +141,11 @@ struct cpu_common_dbs_info {
>> struct mutex timer_mutex;
>>
>> ktime_t time_stamp;
>> + u64 last_sample_time;
>> + s64 sample_delay_ns;
>> atomic_t skip_work;
>> + struct irq_work irq_work;
>
> Just for my understanding, why can't we schedule a normal work directly? Is it
> because of scheduler's hotpath and queue_work() is slow?
No, that's not the reason.
That path can't call wake_up_process() as it may be holding the locks
this would have attempted to grab.
That said it is hot too. For example, ktime_get() may be too slow to
be called from it on some systems.
>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
>> +void gov_set_update_util(struct cpu_common_dbs_info *shared,
>> + unsigned int delay_us)
>> {
>> + struct cpufreq_policy *policy = shared->policy;
>> struct dbs_data *dbs_data = policy->governor_data;
>> - struct cpu_dbs_info *cdbs;
>> int cpu;
>>
>> + shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
>> + shared->time_stamp = ktime_get();
>> + shared->last_sample_time = 0;
>
> Calling this routine from update_sampling_rate() is still wrong. Because that
> will also make last_sample_time = 0, which means that we will schedule the
> irq-work on the next util update.
That isn't a problem, though.
This is the case when the new rate is smaller than the old one and we
want it to take effect immediately. Taking the next sample
immediately in that case is not going to hurt anyone.
And this observation actually leads to some interesting realization
about update_sampling_rate() (see below).
> We surely didn't wanted that to happen, isn't it ?
No, it isn't. :-)
>
>> for_each_cpu(cpu, policy->cpus) {
>> - cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
>> - cdbs->timer.expires = jiffies + delay;
>> - add_timer_on(&cdbs->timer, cpu);
>> + struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
>> +
>> + cpufreq_set_update_util_data(cpu, &cdbs->update_util);
>> }
>> }
>> -EXPORT_SYMBOL_GPL(gov_add_timers);
>> +EXPORT_SYMBOL_GPL(gov_set_update_util);
>
>> void gov_cancel_work(struct cpu_common_dbs_info *shared)
>> {
>> - /* Tell dbs_timer_handler() to skip queuing up work items. */
>> + /* Tell dbs_update_util_handler() to skip queuing up work items. */
>> atomic_inc(&shared->skip_work);
>> /*
>> - * If dbs_timer_handler() is already running, it may not notice the
>> - * incremented skip_work, so wait for it to complete to prevent its work
>> - * item from being queued up after the cancel_work_sync() below.
>> - */
>> - gov_cancel_timers(shared->policy);
>> - /*
>> - * In case dbs_timer_handler() managed to run and spawn a work item
>> - * before the timers have been canceled, wait for that work item to
>> - * complete and then cancel all of the timers set up by it. If
>> - * dbs_timer_handler() runs again at that point, it will see the
>> - * positive value of skip_work and won't spawn any more work items.
>> + * If dbs_update_util_handler() is already running, it may not notice
>> + * the incremented skip_work, so wait for it to complete to prevent its
>> + * work item from being queued up after the cancel_work_sync() below.
>> */
>> + gov_clear_update_util(shared->policy);
>> + wait_for_completion(&shared->irq_work_done);
>
> I may be wrong, but isn't running irq_work_sync() enough here instead ?
Yes, it is.
I assumed that it would only check if the irq_work is running at the
moment for some reason, but that's not the case.
>> cancel_work_sync(&shared->work);
>> - gov_cancel_timers(shared->policy);
>> atomic_set(&shared->skip_work, 0);
>> }
>> EXPORT_SYMBOL_GPL(gov_cancel_work);
>
>> Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
>> @@ -264,7 +260,7 @@ static void update_sampling_rate(struct
>> struct od_cpu_dbs_info_s *dbs_info;
>> struct cpu_dbs_info *cdbs;
>> struct cpu_common_dbs_info *shared;
>> - unsigned long next_sampling, appointed_at;
>> + ktime_t next_sampling, appointed_at;
>>
>> dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
>> cdbs = &dbs_info->cdbs;
>> @@ -292,16 +288,19 @@ static void update_sampling_rate(struct
>> continue;
>>
>> /*
>> - * Checking this for any CPU should be fine, timers for all of
>> - * them are scheduled together.
>> + * Checking this for any CPU sharing the policy should be fine,
>> + * they are all scheduled to sample at the same time.
>> */
>> - next_sampling = jiffies + usecs_to_jiffies(new_rate);
>> - appointed_at = dbs_info->cdbs.timer.expires;
>> + next_sampling = ktime_add_us(ktime_get(), new_rate);
>>
>> - if (time_before(next_sampling, appointed_at)) {
>> - gov_cancel_work(shared);
>> - gov_add_timers(policy, usecs_to_jiffies(new_rate));
>> + mutex_lock(&shared->timer_mutex);
>> + appointed_at = ktime_add_ns(shared->time_stamp,
>> + shared->sample_delay_ns);
>> + mutex_unlock(&shared->timer_mutex);
>>
>> + if (ktime_before(next_sampling, appointed_at)) {
>> + gov_cancel_work(shared);
>> + gov_set_update_util(shared, new_rate);
>
> So, I don't think we need to call these heavy routines at all here. Just use the
> above timer_mutex to update time_stamp and sample_delay_ns.
Well, the concern was that sample_delay_ns might not be updated
atomically on 32-bit architectures and that might be a problem for
dbs_update_util_handler(). However, this really isn't a problem,
because dbs_update_util_handler() only decides whether or not to take
a sample *this* time. If it sees a semi-update value of
sample_delay_ns, that value will be either too small or too big, so it
will either skip the sample unnecessarily or take it immediately and
none of these is a real problem. It doesn't hurt to take the sample
immediately at this point (as stated earlier) and if it is skipped, it
will be taken on the next attempt when the update has been completed
(which would have happened anyway had the update been atomic).
> Over that, that particular change might turn out to be a big big bonus for us.
> Why would we be taking the od_dbs_cdata.mutex in this routine anymore ? We
> aren't removing/adding timers anymore, just update the sample_delay_ns and there
> shouldn't be any races.
That's a very good point.
The only concern is that this function walks the entire collection of
cpu_dbs_infos and that's potentially racing with anything that updates
those.
> Of course you need to use the same timer_mutex in util's
> handler as well around sample_delay_ns, I believe.
That can't take any mutexes. It might only take a raw spinlock if
really needed.
> And that will also kill the circular dependency lockdep we have been chasing
> badly :)
>
> Or am I being over excited here ? :(
Not really. I think you're on the right track.
Before we drop the lock from here, though, we need to audit the code
for any possible races carefully.
Anyway, I'll send an update of the $subject patch later today when I
have a chance to run it through some test.
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-05 16:10 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qYQbg-1UO-29@gated-at.bofh.it> |
| In reply to | #1327695 |
On Fri, Feb 5, 2016 at 7:06 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
>>> @@ -264,7 +260,7 @@ static void update_sampling_rate(struct
>>> struct od_cpu_dbs_info_s *dbs_info;
>>> struct cpu_dbs_info *cdbs;
>>> struct cpu_common_dbs_info *shared;
>>> - unsigned long next_sampling, appointed_at;
>>> + ktime_t next_sampling, appointed_at;
>>>
>>> dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
>>> cdbs = &dbs_info->cdbs;
>>> @@ -292,16 +288,19 @@ static void update_sampling_rate(struct
>>> continue;
>>>
>>> /*
>>> - * Checking this for any CPU should be fine, timers for all of
>>> - * them are scheduled together.
>>> + * Checking this for any CPU sharing the policy should be fine,
>>> + * they are all scheduled to sample at the same time.
>>> */
>>> - next_sampling = jiffies + usecs_to_jiffies(new_rate);
>>> - appointed_at = dbs_info->cdbs.timer.expires;
>>> + next_sampling = ktime_add_us(ktime_get(), new_rate);
>>>
>>> - if (time_before(next_sampling, appointed_at)) {
>>> - gov_cancel_work(shared);
>>> - gov_add_timers(policy, usecs_to_jiffies(new_rate));
>>> + mutex_lock(&shared->timer_mutex);
>>> + appointed_at = ktime_add_ns(shared->time_stamp,
>>> + shared->sample_delay_ns);
>>> + mutex_unlock(&shared->timer_mutex);
>>>
>>> + if (ktime_before(next_sampling, appointed_at)) {
>>> + gov_cancel_work(shared);
>>> + gov_set_update_util(shared, new_rate);
>>
>> So, I don't think we need to call these heavy routines at all here. Just use the
>> above timer_mutex to update time_stamp and sample_delay_ns.
>
> Well, the concern was that sample_delay_ns might not be updated
> atomically on 32-bit architectures and that might be a problem for
> dbs_update_util_handler(). However, this really isn't a problem,
> because dbs_update_util_handler() only decides whether or not to take
> a sample *this* time. If it sees a semi-update value of
> sample_delay_ns, that value will be either too small or too big, so it
> will either skip the sample unnecessarily or take it immediately and
> none of these is a real problem. It doesn't hurt to take the sample
> immediately at this point (as stated earlier) and if it is skipped, it
> will be taken on the next attempt when the update has been completed
> (which would have happened anyway had the update been atomic).
Okay, how about this then.
We do some computations here and based on them, conditionally want to
update sample_delay_ns. Because there is no penalty now, in terms of
removing/adding timers/wq, etc, why shouldn't we simply update the
sample_delay_ns everytime without any checks? That would mean that the
change of sampling rate is effective immediately, what can be better than that?
Also, we should do the same from update-sampling-rate of conservative
governor as well.
Just kill all this complex, unwanted code and make life simple.
> The only concern is that this function walks the entire collection of
> cpu_dbs_infos and that's potentially racing with anything that updates
> those.
Yeah, but fixing this race shall be easier than other crazy things we are
looking to do with kobjects :)
> That can't take any mutexes. It might only take a raw spinlock if
> really needed.
That's doable as well :)
> Before we drop the lock from here, though, we need to audit the code
> for any possible races carefully.
I did bit of that this morning, and there weren't any serious issues as
as far as I could see :)
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-06 00:20 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qYXPs-6RV-17@gated-at.bofh.it> |
| In reply to | #1327830 |
On Friday, February 05, 2016 08:17:56 PM Viresh Kumar wrote:
> On Fri, Feb 5, 2016 at 7:06 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> >>> Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
> >>> @@ -264,7 +260,7 @@ static void update_sampling_rate(struct
> >>> struct od_cpu_dbs_info_s *dbs_info;
> >>> struct cpu_dbs_info *cdbs;
> >>> struct cpu_common_dbs_info *shared;
> >>> - unsigned long next_sampling, appointed_at;
> >>> + ktime_t next_sampling, appointed_at;
> >>>
> >>> dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> >>> cdbs = &dbs_info->cdbs;
> >>> @@ -292,16 +288,19 @@ static void update_sampling_rate(struct
> >>> continue;
> >>>
> >>> /*
> >>> - * Checking this for any CPU should be fine, timers for all of
> >>> - * them are scheduled together.
> >>> + * Checking this for any CPU sharing the policy should be fine,
> >>> + * they are all scheduled to sample at the same time.
> >>> */
> >>> - next_sampling = jiffies + usecs_to_jiffies(new_rate);
> >>> - appointed_at = dbs_info->cdbs.timer.expires;
> >>> + next_sampling = ktime_add_us(ktime_get(), new_rate);
> >>>
> >>> - if (time_before(next_sampling, appointed_at)) {
> >>> - gov_cancel_work(shared);
> >>> - gov_add_timers(policy, usecs_to_jiffies(new_rate));
> >>> + mutex_lock(&shared->timer_mutex);
> >>> + appointed_at = ktime_add_ns(shared->time_stamp,
> >>> + shared->sample_delay_ns);
> >>> + mutex_unlock(&shared->timer_mutex);
> >>>
> >>> + if (ktime_before(next_sampling, appointed_at)) {
> >>> + gov_cancel_work(shared);
> >>> + gov_set_update_util(shared, new_rate);
> >>
> >> So, I don't think we need to call these heavy routines at all here. Just use the
> >> above timer_mutex to update time_stamp and sample_delay_ns.
> >
> > Well, the concern was that sample_delay_ns might not be updated
> > atomically on 32-bit architectures and that might be a problem for
> > dbs_update_util_handler(). However, this really isn't a problem,
> > because dbs_update_util_handler() only decides whether or not to take
> > a sample *this* time. If it sees a semi-update value of
> > sample_delay_ns, that value will be either too small or too big, so it
> > will either skip the sample unnecessarily or take it immediately and
> > none of these is a real problem. It doesn't hurt to take the sample
> > immediately at this point (as stated earlier) and if it is skipped, it
> > will be taken on the next attempt when the update has been completed
> > (which would have happened anyway had the update been atomic).
>
> Okay, how about this then.
>
> We do some computations here and based on them, conditionally want to
> update sample_delay_ns. Because there is no penalty now, in terms of
> removing/adding timers/wq, etc, why shouldn't we simply update the
> sample_delay_ns everytime without any checks? That would mean that the
> change of sampling rate is effective immediately, what can be better than that?
Yes, we can do that.
There is a small concern about updating in parallel with dbs_work_handler()
in which case we may overwrite the (hopefully already correct) sample_delay_ns
value that it has just written, but then it will be corrected next time we
take a sample, so it shouldn't be a big deal.
OK, I'll update the patch to do that.
> Also, we should do the same from update-sampling-rate of conservative
> governor as well.
Let's just not change the whole world in one patch, OK?
> Just kill all this complex, unwanted code and make life simple.
>
> > The only concern is that this function walks the entire collection of
> > cpu_dbs_infos and that's potentially racing with anything that updates
> > those.
>
> Yeah, but fixing this race shall be easier than other crazy things we are
> looking to do with kobjects :)
Yes, I agree.
> > That can't take any mutexes. It might only take a raw spinlock if
> > really needed.
>
> That's doable as well :)
>
> > Before we drop the lock from here, though, we need to audit the code
> > for any possible races carefully.
>
> I did bit of that this morning, and there weren't any serious issues as
> as far as I could see :)
The case I'm mostly concerned about is when update_sampling_rate() looks
at a CPU with a policy completely unrelated to the dbs_data it was called
for. In that case the "shared" object may just go away from under it at
any time while it is looking at that object in theory.
The existing code has this problem AFAICS and the reason why we don't see
any breakage from it right now is because the granularity of cdata->mutex
is really coarse.
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-07 10:20 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZtFE-42X-11@gated-at.bofh.it> |
| In reply to | #1328126 |
On 06-02-16, 00:10, Rafael J. Wysocki wrote: > On Friday, February 05, 2016 08:17:56 PM Viresh Kumar wrote: > > Okay, how about this then. > > > > We do some computations here and based on them, conditionally want to > > update sample_delay_ns. Because there is no penalty now, in terms of > > removing/adding timers/wq, etc, why shouldn't we simply update the > > sample_delay_ns everytime without any checks? That would mean that the > > change of sampling rate is effective immediately, what can be better than that? > > Yes, we can do that. > > There is a small concern about updating in parallel with dbs_work_handler() > in which case we may overwrite the (hopefully already correct) sample_delay_ns > value that it has just written, but then it will be corrected next time we > take a sample, so it shouldn't be a big deal. > > OK, I'll update the patch to do that. Great. > > Also, we should do the same from update-sampling-rate of conservative > > governor as well. > > Let's just not change the whole world in one patch, OK? Yeah, I wasn't asking to update in the same patch, but just that we should do that as well. > > I did bit of that this morning, and there weren't any serious issues as > > as far as I could see :) > > The case I'm mostly concerned about is when update_sampling_rate() looks > at a CPU with a policy completely unrelated to the dbs_data it was called > for. In that case the "shared" object may just go away from under it at > any time while it is looking at that object in theory. Right, a way (ofcourse we should try find something better) is to move that update to a separate work item, just as I did it in my patch.. But, I am quite sure we can get that fixed. -- viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-07 15:50 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZyOZ-7ts-3@gated-at.bofh.it> |
| In reply to | #1328448 |
On Sunday, February 07, 2016 02:40:40 PM Viresh Kumar wrote: > On 06-02-16, 00:10, Rafael J. Wysocki wrote: > > On Friday, February 05, 2016 08:17:56 PM Viresh Kumar wrote: > > > Okay, how about this then. > > > > > > We do some computations here and based on them, conditionally want to > > > update sample_delay_ns. Because there is no penalty now, in terms of > > > removing/adding timers/wq, etc, why shouldn't we simply update the > > > sample_delay_ns everytime without any checks? That would mean that the > > > change of sampling rate is effective immediately, what can be better than that? > > > > Yes, we can do that. > > > > There is a small concern about updating in parallel with dbs_work_handler() > > in which case we may overwrite the (hopefully already correct) sample_delay_ns > > value that it has just written, but then it will be corrected next time we > > take a sample, so it shouldn't be a big deal. > > > > OK, I'll update the patch to do that. > > Great. > > > > Also, we should do the same from update-sampling-rate of conservative > > > governor as well. > > > > Let's just not change the whole world in one patch, OK? > > Yeah, I wasn't asking to update in the same patch, but just that we > should do that as well. > > > > I did bit of that this morning, and there weren't any serious issues as > > > as far as I could see :) > > > > The case I'm mostly concerned about is when update_sampling_rate() looks > > at a CPU with a policy completely unrelated to the dbs_data it was called > > for. In that case the "shared" object may just go away from under it at > > any time while it is looking at that object in theory. > > Right, a way (ofcourse we should try find something better) is to move > that update to a separate work item, just as I did it in my patch.. No, it isn't. Trying to do it asynchronously will only lead to more concurrency-related issues. > But, I am quite sure we can get that fixed. What we need to do, is to make it possible for update_sampling_rate() to walk all of the cpu_dbs_infos and look at what their policy_dbs fields point to safely. After my cleanup patches it does that under dbs_data_mutex and that works, because this mutex is also held around *any* updates of struct cpu_dbs_info anywhere. However, the cpu_dbs_infos themselves are actually static, so they can be accessed at any time. It looks like, then, we may just need to add a lock to each of them to ensure that the policy_dbs thing won't go away suddenly and we may not need dbs_data_mutex in there any more. Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-08 03:10 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZJr4-6O0-3@gated-at.bofh.it> |
| In reply to | #1328496 |
On Sunday, February 07, 2016 03:43:20 PM Rafael J. Wysocki wrote:
> On Sunday, February 07, 2016 02:40:40 PM Viresh Kumar wrote:
> > On 06-02-16, 00:10, Rafael J. Wysocki wrote:
> > > On Friday, February 05, 2016 08:17:56 PM Viresh Kumar wrote:
> > > > Okay, how about this then.
> > > >
> > > > We do some computations here and based on them, conditionally want to
> > > > update sample_delay_ns. Because there is no penalty now, in terms of
> > > > removing/adding timers/wq, etc, why shouldn't we simply update the
> > > > sample_delay_ns everytime without any checks? That would mean that the
> > > > change of sampling rate is effective immediately, what can be better than that?
> > >
> > > Yes, we can do that.
> > >
> > > There is a small concern about updating in parallel with dbs_work_handler()
> > > in which case we may overwrite the (hopefully already correct) sample_delay_ns
> > > value that it has just written, but then it will be corrected next time we
> > > take a sample, so it shouldn't be a big deal.
> > >
> > > OK, I'll update the patch to do that.
> >
> > Great.
> >
> > > > Also, we should do the same from update-sampling-rate of conservative
> > > > governor as well.
> > >
> > > Let's just not change the whole world in one patch, OK?
> >
> > Yeah, I wasn't asking to update in the same patch, but just that we
> > should do that as well.
> >
> > > > I did bit of that this morning, and there weren't any serious issues as
> > > > as far as I could see :)
> > >
> > > The case I'm mostly concerned about is when update_sampling_rate() looks
> > > at a CPU with a policy completely unrelated to the dbs_data it was called
> > > for. In that case the "shared" object may just go away from under it at
> > > any time while it is looking at that object in theory.
> >
> > Right, a way (ofcourse we should try find something better) is to move
> > that update to a separate work item, just as I did it in my patch..
>
> No, it isn't. Trying to do it asynchronously will only lead to more
> concurrency-related issues.
>
> > But, I am quite sure we can get that fixed.
>
> What we need to do, is to make it possible for update_sampling_rate()
> to walk all of the cpu_dbs_infos and look at what their policy_dbs
> fields point to safely.
>
> After my cleanup patches it does that under dbs_data_mutex and that works,
> because this mutex is also held around *any* updates of struct cpu_dbs_info
> anywhere.
>
> However, the cpu_dbs_infos themselves are actually static, so they can be
> accessed at any time. It looks like, then, we may just need to add a lock to
> each of them to ensure that the policy_dbs thing won't go away suddenly and
> we may not need dbs_data_mutex in there any more.
Moreover, update_sampling_rate() doesn't need to walk the cpu_dbs_infos,
it may walk policies instead. Like after the (untested) appended patch.
Then, if we have a governor_data_lock in struct policy, we can use that
to protect policy_dbs while it is being access there and we're done.
I'll try to prototype something along these lines tomorrow.
Thanks,
Rafael
---
drivers/cpufreq/cpufreq_ondemand.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -254,34 +254,23 @@ static void update_sampling_rate(struct
cpumask_copy(&cpumask, cpu_online_mask);
for_each_cpu(cpu, &cpumask) {
- struct cpufreq_policy *policy;
- struct od_cpu_dbs_info_s *dbs_info;
- struct cpu_dbs_info *cdbs;
+ struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
struct policy_dbs_info *policy_dbs;
- dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
- cdbs = &dbs_info->cdbs;
- policy_dbs = cdbs->policy_dbs;
-
- /*
- * A valid policy_dbs and policy_dbs->policy means governor
- * hasn't stopped or exited yet.
- */
- if (!policy_dbs || !policy_dbs->policy)
+ if (!policy)
continue;
- policy = policy_dbs->policy;
-
/* clear all CPUs of this policy */
cpumask_andnot(&cpumask, &cpumask, policy->cpus);
+ policy_dbs = policy->governor_data;
/*
* Update sampling rate for CPUs whose policy is governed by
* dbs_data. In case of governor_per_policy, only a single
* policy will be governed by dbs_data, otherwise there can be
* multiple policies that are governed by the same dbs_data.
*/
- if (dbs_data == policy_dbs->dbs_data) {
+ if (policy_dbs && policy_dbs->dbs_data == dbs_data) {
mutex_lock(&policy_dbs->timer_mutex);
/*
* On 32-bit architectures this may race with the
@@ -304,6 +293,8 @@ static void update_sampling_rate(struct
gov_update_sample_delay(policy_dbs, new_rate);
mutex_unlock(&policy_dbs->timer_mutex);
}
+
+ cpufreq_cpu_put(policy);
}
mutex_unlock(&dbs_data_mutex);
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-08 13:00 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZSE1-4HT-1@gated-at.bofh.it> |
| In reply to | #1328696 |
On 08-02-16, 03:08, Rafael J. Wysocki wrote: > Moreover, update_sampling_rate() doesn't need to walk the cpu_dbs_infos, > it may walk policies instead. Like after the (untested) appended patch. > > Then, if we have a governor_data_lock in struct policy, we can use that > to protect policy_dbs while it is being access there and we're done. > > I'll try to prototype something along these lines tomorrow. I have solved that in a different way, and dropped the lock from update_sampling_rate(). Please see if that looks good. -- viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-08 14:00 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZTA6-5ma-21@gated-at.bofh.it> |
| In reply to | #1328965 |
On Mon, Feb 8, 2016 at 12:52 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 08-02-16, 03:08, Rafael J. Wysocki wrote: >> Moreover, update_sampling_rate() doesn't need to walk the cpu_dbs_infos, >> it may walk policies instead. Like after the (untested) appended patch. >> >> Then, if we have a governor_data_lock in struct policy, we can use that >> to protect policy_dbs while it is being access there and we're done. >> >> I'll try to prototype something along these lines tomorrow. > > I have solved that in a different way, and dropped the lock from > update_sampling_rate(). Please see if that looks good. Well, almost. I like the list approach, but you need to be careful about it. Let me comment more on the patches in the series. I have a gut feeling that my idea of walking policies will end up being simpler in the end, but let's see. :-) Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-08 14:50 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZUmu-5Zc-27@gated-at.bofh.it> |
| In reply to | #1329020 |
On Mon, Feb 8, 2016 at 1:52 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Mon, Feb 8, 2016 at 12:52 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> On 08-02-16, 03:08, Rafael J. Wysocki wrote: >>> Moreover, update_sampling_rate() doesn't need to walk the cpu_dbs_infos, >>> it may walk policies instead. Like after the (untested) appended patch. >>> >>> Then, if we have a governor_data_lock in struct policy, we can use that >>> to protect policy_dbs while it is being access there and we're done. >>> >>> I'll try to prototype something along these lines tomorrow. >> >> I have solved that in a different way, and dropped the lock from >> update_sampling_rate(). Please see if that looks good. > > Well, almost. > > I like the list approach, but you need to be careful about it. Let me > comment more on the patches in the series. > > I have a gut feeling that my idea of walking policies will end up > being simpler in the end, but let's see. :-) Well, my gut feeling seems to have been incorrect, as often happens. Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-06 00:10 +0100 |
| Subject | Re: [PATCH 3/3 v3] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qYXFN-6Ni-25@gated-at.bofh.it> |
| In reply to | #1327695 |
On Friday, February 05, 2016 02:36:54 PM Rafael J. Wysocki wrote:
> On Fri, Feb 5, 2016 at 7:50 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Will suck some more blood, sorry about that :)
> >
> > On 05-02-16, 02:28, Rafael J. Wysocki wrote:
> >> The v3 addresses some review comments from Viresh and a couple of issues found
> >> by me. Changes from the previous version:
> >> - Synchronize gov_cancel_work() with the (new) irq_work properly.
> >> - Add a comment about the (new) memory barrier.
> >> - Move samle_delay_ns to "shared" (struct cpu_common_dbs_info) so it is the
> >
> > sample_delay_ns was already there, you moved last_sample_time instead :)
> >
> >> @@ -139,7 +141,11 @@ struct cpu_common_dbs_info {
> >> struct mutex timer_mutex;
> >>
> >> ktime_t time_stamp;
> >> + u64 last_sample_time;
> >> + s64 sample_delay_ns;
> >> atomic_t skip_work;
> >> + struct irq_work irq_work;
> >
> > Just for my understanding, why can't we schedule a normal work directly? Is it
> > because of scheduler's hotpath and queue_work() is slow?
>
> No, that's not the reason.
>
> That path can't call wake_up_process() as it may be holding the locks
> this would have attempted to grab.
My answer wasn't really to the point here.
Among other things, the scheduler path cannot use normal spinlocks. It can
only use raw spinlocks and this means no work queuing from it.
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-06 04:50 +0100 |
| Subject | [PATCH 3/3 v4] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZ22J-1rp-1@gated-at.bofh.it> |
| In reply to | #1327389 |
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Instead of using a per-CPU deferrable timer for queuing up governor
work items, register a utilization update callback that will be
invoked from the scheduler on utilization changes.
The sampling rate is still the same as what was used for the
deferrable timers and the added irq_work overhead should be offset by
the eliminated timers overhead, so in theory the functional impact of
this patch should not be significant.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
Updated after the recent discussion with Viresh.
Changes from v3:
- The completion used for irq_work synchronization replaced with irq_work_sync()
in gov_cancel_work().
- update_sampling_rate() now modifies shared->sample_delay_ns for all CPUs
where it matters directly with a big fat comment explaining why this is
actually OK.
- The above means the time_stamp field in struct cpu_common_dbs_info is not
necessary any more, so it is dropped.
- A build error for !CONFIG_SMP is addressed (hopefully effectively).
This version was lightly tested on an x86 laptop.
Thanks!
---
drivers/cpufreq/cpufreq_conservative.c | 6 -
drivers/cpufreq/cpufreq_governor.c | 164 +++++++++++++++------------------
drivers/cpufreq/cpufreq_governor.h | 19 ++-
drivers/cpufreq/cpufreq_ondemand.c | 43 ++++----
4 files changed, 112 insertions(+), 120 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -18,6 +18,7 @@
#define _CPUFREQ_GOVERNOR_H
#include <linux/atomic.h>
+#include <linux/irq_work.h>
#include <linux/cpufreq.h>
#include <linux/kernel_stat.h>
#include <linux/module.h>
@@ -138,11 +139,19 @@ struct cpu_common_dbs_info {
*/
struct mutex timer_mutex;
- ktime_t time_stamp;
+ u64 last_sample_time;
+ s64 sample_delay_ns;
atomic_t skip_work;
+ struct irq_work irq_work;
struct work_struct work;
};
+static inline void gov_update_sample_delay(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us)
+{
+ shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
+}
+
/* Per cpu structures */
struct cpu_dbs_info {
u64 prev_cpu_idle;
@@ -155,7 +164,7 @@ struct cpu_dbs_info {
* wake-up from idle.
*/
unsigned int prev_load;
- struct timer_list timer;
+ struct update_util_data update_util;
struct cpu_common_dbs_info *shared;
};
@@ -212,8 +221,7 @@ struct common_dbs_data {
struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
void *(*get_cpu_dbs_info_s)(int cpu);
- unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy,
- bool modify_all);
+ unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
void (*gov_check_cpu)(int cpu, unsigned int load);
int (*init)(struct dbs_data *dbs_data, bool notify);
void (*exit)(struct dbs_data *dbs_data, bool notify);
@@ -270,9 +278,6 @@ static ssize_t show_sampling_rate_min_go
}
extern struct mutex cpufreq_governor_lock;
-
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
-void gov_cancel_work(struct cpu_common_dbs_info *shared);
void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
int cpufreq_governor_dbs(struct cpufreq_policy *policy,
struct common_dbs_data *cdata, unsigned int event);
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -128,10 +128,10 @@ void dbs_check_cpu(struct dbs_data *dbs_
* dropped down. So we perform the copy only once, upon the
* first wake-up from idle.)
*
- * Detecting this situation is easy: the governor's deferrable
- * timer would not have fired during CPU-idle periods. Hence
- * an unusually large 'wall_time' (as compared to the sampling
- * rate) indicates this scenario.
+ * Detecting this situation is easy: the governor's utilization
+ * update handler would not have run during CPU-idle periods.
+ * Hence, an unusually large 'wall_time' (as compared to the
+ * sampling rate) indicates this scenario.
*
* prev_load can be zero in two cases and we must recalculate it
* for both cases:
@@ -161,72 +161,48 @@ void dbs_check_cpu(struct dbs_data *dbs_
}
EXPORT_SYMBOL_GPL(dbs_check_cpu);
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us)
{
+ struct cpufreq_policy *policy = shared->policy;
struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int cpu;
+ gov_update_sample_delay(shared, delay_us);
+ shared->last_sample_time = 0;
+
for_each_cpu(cpu, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
- cdbs->timer.expires = jiffies + delay;
- add_timer_on(&cdbs->timer, cpu);
+ struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+
+ cpufreq_set_update_util_data(cpu, &cdbs->update_util);
}
}
-EXPORT_SYMBOL_GPL(gov_add_timers);
+EXPORT_SYMBOL_GPL(gov_set_update_util);
-static inline void gov_cancel_timers(struct cpufreq_policy *policy)
+static inline void gov_clear_update_util(struct cpufreq_policy *policy)
{
- struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int i;
- for_each_cpu(i, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(i);
- del_timer_sync(&cdbs->timer);
- }
+ for_each_cpu(i, policy->cpus)
+ cpufreq_set_update_util_data(i, NULL);
+
+ synchronize_rcu();
}
-void gov_cancel_work(struct cpu_common_dbs_info *shared)
+static void gov_cancel_work(struct cpu_common_dbs_info *shared)
{
- /* Tell dbs_timer_handler() to skip queuing up work items. */
+ /* Tell dbs_update_util_handler() to skip queuing up work items. */
atomic_inc(&shared->skip_work);
/*
- * If dbs_timer_handler() is already running, it may not notice the
- * incremented skip_work, so wait for it to complete to prevent its work
- * item from being queued up after the cancel_work_sync() below.
- */
- gov_cancel_timers(shared->policy);
- /*
- * In case dbs_timer_handler() managed to run and spawn a work item
- * before the timers have been canceled, wait for that work item to
- * complete and then cancel all of the timers set up by it. If
- * dbs_timer_handler() runs again at that point, it will see the
- * positive value of skip_work and won't spawn any more work items.
+ * If dbs_update_util_handler() is already running, it may not notice
+ * the incremented skip_work, so wait for it to complete to prevent its
+ * work item from being queued up after the cancel_work_sync() below.
*/
+ gov_clear_update_util(shared->policy);
+ irq_work_sync(&shared->irq_work);
cancel_work_sync(&shared->work);
- gov_cancel_timers(shared->policy);
atomic_set(&shared->skip_work, 0);
}
-EXPORT_SYMBOL_GPL(gov_cancel_work);
-
-/* Will return if we need to evaluate cpu load again or not */
-static bool need_load_eval(struct cpu_common_dbs_info *shared,
- unsigned int sampling_rate)
-{
- if (policy_is_shared(shared->policy)) {
- ktime_t time_now = ktime_get();
- s64 delta_us = ktime_us_delta(time_now, shared->time_stamp);
-
- /* Do nothing if we recently have sampled */
- if (delta_us < (s64)(sampling_rate / 2))
- return false;
- else
- shared->time_stamp = time_now;
- }
-
- return true;
-}
static void dbs_work_handler(struct work_struct *work)
{
@@ -234,56 +210,69 @@ static void dbs_work_handler(struct work
cpu_common_dbs_info, work);
struct cpufreq_policy *policy;
struct dbs_data *dbs_data;
- unsigned int sampling_rate, delay;
- bool eval_load;
+ unsigned int delay;
policy = shared->policy;
dbs_data = policy->governor_data;
- /* Kill all timers */
- gov_cancel_timers(policy);
-
- if (dbs_data->cdata->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;
- }
-
- eval_load = need_load_eval(shared, sampling_rate);
-
/*
- * Make sure cpufreq_governor_limits() isn't evaluating load in
- * parallel.
+ * Make sure cpufreq_governor_limits() isn't evaluating load or the
+ * ondemand governor isn't updating the sampling rate in parallel.
*/
mutex_lock(&shared->timer_mutex);
- delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
+ delay = dbs_data->cdata->gov_dbs_timer(policy);
+ shared->sample_delay_ns = jiffies_to_nsecs(delay);
mutex_unlock(&shared->timer_mutex);
+ /*
+ * If the atomic operation below is reordered with respect to the
+ * sample delay modification, the utilization update handler may end
+ * up using a stale sample delay value.
+ */
+ smp_mb__before_atomic();
atomic_dec(&shared->skip_work);
+}
+
+static void dbs_irq_work(struct irq_work *irq_work)
+{
+ struct cpu_common_dbs_info *shared;
+
+ shared = container_of(irq_work, struct cpu_common_dbs_info, irq_work);
+ schedule_work(&shared->work);
+}
- gov_add_timers(policy, delay);
+static inline void gov_queue_irq_work(struct cpu_common_dbs_info *shared)
+{
+ if (IS_ENABLED(CONFIG_SMP))
+ irq_work_queue_on(&shared->irq_work, smp_processor_id());
+ else
+ irq_work_queue(&shared->irq_work);
}
-static void dbs_timer_handler(unsigned long data)
+static void dbs_update_util_handler(struct update_util_data *data, u64 time,
+ unsigned long util, unsigned long max)
{
- struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
+ struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
struct cpu_common_dbs_info *shared = cdbs->shared;
/*
- * Timer handler may not be allowed to queue the work at the moment,
- * because:
- * - Another timer handler has done that
- * - We are stopping the governor
- * - Or we are updating the sampling rate of the ondemand governor
+ * The work may not be allowed to be queued up right now.
+ * Possible reasons:
+ * - Work has already been queued up or is in progress.
+ * - The governor is being stopped.
+ * - It is too early (too little time from the previous sample).
*/
- if (atomic_inc_return(&shared->skip_work) > 1)
- atomic_dec(&shared->skip_work);
- else
- queue_work(system_wq, &shared->work);
+ if (atomic_inc_return(&shared->skip_work) == 1) {
+ u64 delta_ns;
+
+ delta_ns = time - shared->last_sample_time;
+ if ((s64)delta_ns >= shared->sample_delay_ns) {
+ shared->last_sample_time = time;
+ gov_queue_irq_work(shared);
+ return;
+ }
+ }
+ atomic_dec(&shared->skip_work);
}
static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -467,9 +456,6 @@ static int cpufreq_governor_start(struct
io_busy = od_tuners->io_is_busy;
}
- shared->policy = policy;
- shared->time_stamp = ktime_get();
-
for_each_cpu(j, policy->cpus) {
struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
unsigned int prev_load;
@@ -485,10 +471,10 @@ static int cpufreq_governor_start(struct
if (ignore_nice)
j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
- __setup_timer(&j_cdbs->timer, dbs_timer_handler,
- (unsigned long)j_cdbs,
- TIMER_DEFERRABLE | TIMER_IRQSAFE);
+ j_cdbs->update_util.func = dbs_update_util_handler;
}
+ shared->policy = policy;
+ init_irq_work(&shared->irq_work, dbs_irq_work);
if (cdata->governor == GOV_CONSERVATIVE) {
struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -505,7 +491,7 @@ static int cpufreq_governor_start(struct
od_ops->powersave_bias_init_cpu(cpu);
}
- gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
+ gov_set_update_util(shared, sampling_rate);
return 0;
}
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -191,7 +191,7 @@ static void od_check_cpu(int cpu, unsign
}
}
-static unsigned int od_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
unsigned int cpu = policy->cpu;
@@ -200,9 +200,6 @@ static unsigned int od_dbs_timer(struct
struct od_dbs_tuners *od_tuners = dbs_data->tuners;
int delay = 0, sample_type = dbs_info->sample_type;
- if (!modify_all)
- goto max_delay;
-
/* Common NORMAL_SAMPLE setup */
dbs_info->sample_type = OD_NORMAL_SAMPLE;
if (sample_type == OD_SUB_SAMPLE) {
@@ -218,7 +215,6 @@ static unsigned int od_dbs_timer(struct
}
}
-max_delay:
if (!delay)
delay = delay_for_sampling_rate(od_tuners->sampling_rate
* dbs_info->rate_mult);
@@ -264,7 +260,6 @@ static void update_sampling_rate(struct
struct od_cpu_dbs_info_s *dbs_info;
struct cpu_dbs_info *cdbs;
struct cpu_common_dbs_info *shared;
- unsigned long next_sampling, appointed_at;
dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
cdbs = &dbs_info->cdbs;
@@ -288,20 +283,28 @@ static void update_sampling_rate(struct
* policy will be governed by dbs_data, otherwise there can be
* multiple policies that are governed by the same dbs_data.
*/
- if (dbs_data != policy->governor_data)
- continue;
-
- /*
- * Checking this for any CPU should be fine, timers for all of
- * them are scheduled together.
- */
- next_sampling = jiffies + usecs_to_jiffies(new_rate);
- appointed_at = dbs_info->cdbs.timer.expires;
-
- if (time_before(next_sampling, appointed_at)) {
- gov_cancel_work(shared);
- gov_add_timers(policy, usecs_to_jiffies(new_rate));
-
+ if (dbs_data == policy->governor_data) {
+ mutex_lock(&shared->timer_mutex);
+ /*
+ * On 32-bit architectures this may race with the
+ * sample_delay_ns read in dbs_update_util_handler(),
+ * but that really doesn't matter. If the read returns
+ * a value that's too big, the sample will be skipped,
+ * but the next invocation of dbs_update_util_handler()
+ * (when the update has been completed) will take a
+ * sample. If the returned value is too small, the
+ * sample will be taken immediately, but that isn't a
+ * problem, as we want the new rate to take effect
+ * immediately anyway.
+ *
+ * If this runs in parallel with dbs_work_handler(), we
+ * may end up overwriting the sample_delay_ns value that
+ * it has just written, but the difference should not be
+ * too big and it will be corrected next time a sample
+ * is taken, so it shouldn't be significant.
+ */
+ gov_update_sample_delay(shared, new_rate);
+ mutex_unlock(&shared->timer_mutex);
}
}
Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -115,14 +115,12 @@ static void cs_check_cpu(int cpu, unsign
}
}
-static unsigned int cs_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
- if (modify_all)
- dbs_check_cpu(dbs_data, policy->cpu);
-
+ dbs_check_cpu(dbs_data, policy->cpu);
return delay_for_sampling_rate(cs_tuners->sampling_rate);
}
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-07 10:30 +0100 |
| Subject | Re: [PATCH 3/3 v4] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZtPj-46p-9@gated-at.bofh.it> |
| In reply to | #1328204 |
On 06-02-16, 04:40, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Instead of using a per-CPU deferrable timer for queuing up governor > work items, register a utilization update callback that will be > invoked from the scheduler on utilization changes. > > The sampling rate is still the same as what was used for the > deferrable timers and the added irq_work overhead should be offset by > the eliminated timers overhead, so in theory the functional impact of > this patch should not be significant. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > Updated after the recent discussion with Viresh. > > Changes from v3: > - The completion used for irq_work synchronization replaced with irq_work_sync() > in gov_cancel_work(). > - update_sampling_rate() now modifies shared->sample_delay_ns for all CPUs > where it matters directly with a big fat comment explaining why this is > actually OK. > - The above means the time_stamp field in struct cpu_common_dbs_info is not > necessary any more, so it is dropped. > - A build error for !CONFIG_SMP is addressed (hopefully effectively). > > This version was lightly tested on an x86 laptop. Awesome work Rafael, this looks really good now. Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-07 15:40 +0100 |
| Subject | Re: [PATCH 3/3 v4] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZyFj-7pJ-3@gated-at.bofh.it> |
| In reply to | #1328453 |
On Sunday, February 07, 2016 02:50:19 PM Viresh Kumar wrote: > On 06-02-16, 04:40, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Instead of using a per-CPU deferrable timer for queuing up governor > > work items, register a utilization update callback that will be > > invoked from the scheduler on utilization changes. > > > > The sampling rate is still the same as what was used for the > > deferrable timers and the added irq_work overhead should be offset by > > the eliminated timers overhead, so in theory the functional impact of > > this patch should not be significant. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > > > Updated after the recent discussion with Viresh. > > > > Changes from v3: > > - The completion used for irq_work synchronization replaced with irq_work_sync() > > in gov_cancel_work(). > > - update_sampling_rate() now modifies shared->sample_delay_ns for all CPUs > > where it matters directly with a big fat comment explaining why this is > > actually OK. > > - The above means the time_stamp field in struct cpu_common_dbs_info is not > > necessary any more, so it is dropped. > > - A build error for !CONFIG_SMP is addressed (hopefully effectively). > > > > This version was lightly tested on an x86 laptop. > > Awesome work Rafael, this looks really good now. > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> Thanks! I have one small update, though. Namely, it is more logical to initialize irq_work along with doing INIT_WORK() on the main work item. I'll send it in a while. Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-07 16:00 +0100 |
| Subject | [PATCH 3/3 v5] cpufreq: governor: Replace timers with utilization update callbacks |
| Message-ID | <qZyYG-7x5-7@gated-at.bofh.it> |
| In reply to | #1328204 |
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Instead of using a per-CPU deferrable timer for queuing up governor
work items, register a utilization update callback that will be
invoked from the scheduler on utilization changes.
The sampling rate is still the same as what was used for the
deferrable timers and the added irq_work overhead should be offset by
the eliminated timers overhead, so in theory the functional impact of
this patch should not be significant.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Changes from v4:
- Moved init_irq_work() to alloc_common_dbs_info() so it is done along with
the INIT_WORK() on the main work structure (which seems more logical to me).
- Added the ACK from Viresh (in the hope that it still applied).
Thanks,
Rafael
---
drivers/cpufreq/cpufreq_conservative.c | 6 -
drivers/cpufreq/cpufreq_governor.c | 164 +++++++++++++++------------------
drivers/cpufreq/cpufreq_governor.h | 19 ++-
drivers/cpufreq/cpufreq_ondemand.c | 43 ++++----
4 files changed, 112 insertions(+), 120 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -18,6 +18,7 @@
#define _CPUFREQ_GOVERNOR_H
#include <linux/atomic.h>
+#include <linux/irq_work.h>
#include <linux/cpufreq.h>
#include <linux/kernel_stat.h>
#include <linux/module.h>
@@ -138,11 +139,19 @@ struct cpu_common_dbs_info {
*/
struct mutex timer_mutex;
- ktime_t time_stamp;
+ u64 last_sample_time;
+ s64 sample_delay_ns;
atomic_t skip_work;
+ struct irq_work irq_work;
struct work_struct work;
};
+static inline void gov_update_sample_delay(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us)
+{
+ shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
+}
+
/* Per cpu structures */
struct cpu_dbs_info {
u64 prev_cpu_idle;
@@ -155,7 +164,7 @@ struct cpu_dbs_info {
* wake-up from idle.
*/
unsigned int prev_load;
- struct timer_list timer;
+ struct update_util_data update_util;
struct cpu_common_dbs_info *shared;
};
@@ -212,8 +221,7 @@ struct common_dbs_data {
struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
void *(*get_cpu_dbs_info_s)(int cpu);
- unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy,
- bool modify_all);
+ unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
void (*gov_check_cpu)(int cpu, unsigned int load);
int (*init)(struct dbs_data *dbs_data, bool notify);
void (*exit)(struct dbs_data *dbs_data, bool notify);
@@ -270,9 +278,6 @@ static ssize_t show_sampling_rate_min_go
}
extern struct mutex cpufreq_governor_lock;
-
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
-void gov_cancel_work(struct cpu_common_dbs_info *shared);
void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
int cpufreq_governor_dbs(struct cpufreq_policy *policy,
struct common_dbs_data *cdata, unsigned int event);
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -128,10 +128,10 @@ void dbs_check_cpu(struct dbs_data *dbs_
* dropped down. So we perform the copy only once, upon the
* first wake-up from idle.)
*
- * Detecting this situation is easy: the governor's deferrable
- * timer would not have fired during CPU-idle periods. Hence
- * an unusually large 'wall_time' (as compared to the sampling
- * rate) indicates this scenario.
+ * Detecting this situation is easy: the governor's utilization
+ * update handler would not have run during CPU-idle periods.
+ * Hence, an unusually large 'wall_time' (as compared to the
+ * sampling rate) indicates this scenario.
*
* prev_load can be zero in two cases and we must recalculate it
* for both cases:
@@ -161,72 +161,48 @@ void dbs_check_cpu(struct dbs_data *dbs_
}
EXPORT_SYMBOL_GPL(dbs_check_cpu);
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+ unsigned int delay_us)
{
+ struct cpufreq_policy *policy = shared->policy;
struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int cpu;
+ gov_update_sample_delay(shared, delay_us);
+ shared->last_sample_time = 0;
+
for_each_cpu(cpu, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
- cdbs->timer.expires = jiffies + delay;
- add_timer_on(&cdbs->timer, cpu);
+ struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+
+ cpufreq_set_update_util_data(cpu, &cdbs->update_util);
}
}
-EXPORT_SYMBOL_GPL(gov_add_timers);
+EXPORT_SYMBOL_GPL(gov_set_update_util);
-static inline void gov_cancel_timers(struct cpufreq_policy *policy)
+static inline void gov_clear_update_util(struct cpufreq_policy *policy)
{
- struct dbs_data *dbs_data = policy->governor_data;
- struct cpu_dbs_info *cdbs;
int i;
- for_each_cpu(i, policy->cpus) {
- cdbs = dbs_data->cdata->get_cpu_cdbs(i);
- del_timer_sync(&cdbs->timer);
- }
+ for_each_cpu(i, policy->cpus)
+ cpufreq_set_update_util_data(i, NULL);
+
+ synchronize_rcu();
}
-void gov_cancel_work(struct cpu_common_dbs_info *shared)
+static void gov_cancel_work(struct cpu_common_dbs_info *shared)
{
- /* Tell dbs_timer_handler() to skip queuing up work items. */
+ /* Tell dbs_update_util_handler() to skip queuing up work items. */
atomic_inc(&shared->skip_work);
/*
- * If dbs_timer_handler() is already running, it may not notice the
- * incremented skip_work, so wait for it to complete to prevent its work
- * item from being queued up after the cancel_work_sync() below.
- */
- gov_cancel_timers(shared->policy);
- /*
- * In case dbs_timer_handler() managed to run and spawn a work item
- * before the timers have been canceled, wait for that work item to
- * complete and then cancel all of the timers set up by it. If
- * dbs_timer_handler() runs again at that point, it will see the
- * positive value of skip_work and won't spawn any more work items.
+ * If dbs_update_util_handler() is already running, it may not notice
+ * the incremented skip_work, so wait for it to complete to prevent its
+ * work item from being queued up after the cancel_work_sync() below.
*/
+ gov_clear_update_util(shared->policy);
+ irq_work_sync(&shared->irq_work);
cancel_work_sync(&shared->work);
- gov_cancel_timers(shared->policy);
atomic_set(&shared->skip_work, 0);
}
-EXPORT_SYMBOL_GPL(gov_cancel_work);
-
-/* Will return if we need to evaluate cpu load again or not */
-static bool need_load_eval(struct cpu_common_dbs_info *shared,
- unsigned int sampling_rate)
-{
- if (policy_is_shared(shared->policy)) {
- ktime_t time_now = ktime_get();
- s64 delta_us = ktime_us_delta(time_now, shared->time_stamp);
-
- /* Do nothing if we recently have sampled */
- if (delta_us < (s64)(sampling_rate / 2))
- return false;
- else
- shared->time_stamp = time_now;
- }
-
- return true;
-}
static void dbs_work_handler(struct work_struct *work)
{
@@ -234,56 +210,69 @@ static void dbs_work_handler(struct work
cpu_common_dbs_info, work);
struct cpufreq_policy *policy;
struct dbs_data *dbs_data;
- unsigned int sampling_rate, delay;
- bool eval_load;
+ unsigned int delay;
policy = shared->policy;
dbs_data = policy->governor_data;
- /* Kill all timers */
- gov_cancel_timers(policy);
-
- if (dbs_data->cdata->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;
- }
-
- eval_load = need_load_eval(shared, sampling_rate);
-
/*
- * Make sure cpufreq_governor_limits() isn't evaluating load in
- * parallel.
+ * Make sure cpufreq_governor_limits() isn't evaluating load or the
+ * ondemand governor isn't updating the sampling rate in parallel.
*/
mutex_lock(&shared->timer_mutex);
- delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
+ delay = dbs_data->cdata->gov_dbs_timer(policy);
+ shared->sample_delay_ns = jiffies_to_nsecs(delay);
mutex_unlock(&shared->timer_mutex);
+ /*
+ * If the atomic operation below is reordered with respect to the
+ * sample delay modification, the utilization update handler may end
+ * up using a stale sample delay value.
+ */
+ smp_mb__before_atomic();
atomic_dec(&shared->skip_work);
+}
+
+static void dbs_irq_work(struct irq_work *irq_work)
+{
+ struct cpu_common_dbs_info *shared;
+
+ shared = container_of(irq_work, struct cpu_common_dbs_info, irq_work);
+ schedule_work(&shared->work);
+}
- gov_add_timers(policy, delay);
+static inline void gov_queue_irq_work(struct cpu_common_dbs_info *shared)
+{
+ if (IS_ENABLED(CONFIG_SMP))
+ irq_work_queue_on(&shared->irq_work, smp_processor_id());
+ else
+ irq_work_queue(&shared->irq_work);
}
-static void dbs_timer_handler(unsigned long data)
+static void dbs_update_util_handler(struct update_util_data *data, u64 time,
+ unsigned long util, unsigned long max)
{
- struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
+ struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
struct cpu_common_dbs_info *shared = cdbs->shared;
/*
- * Timer handler may not be allowed to queue the work at the moment,
- * because:
- * - Another timer handler has done that
- * - We are stopping the governor
- * - Or we are updating the sampling rate of the ondemand governor
+ * The work may not be allowed to be queued up right now.
+ * Possible reasons:
+ * - Work has already been queued up or is in progress.
+ * - The governor is being stopped.
+ * - It is too early (too little time from the previous sample).
*/
- if (atomic_inc_return(&shared->skip_work) > 1)
- atomic_dec(&shared->skip_work);
- else
- queue_work(system_wq, &shared->work);
+ if (atomic_inc_return(&shared->skip_work) == 1) {
+ u64 delta_ns;
+
+ delta_ns = time - shared->last_sample_time;
+ if ((s64)delta_ns >= shared->sample_delay_ns) {
+ shared->last_sample_time = time;
+ gov_queue_irq_work(shared);
+ return;
+ }
+ }
+ atomic_dec(&shared->skip_work);
}
static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -315,6 +304,7 @@ static int alloc_common_dbs_info(struct
mutex_init(&shared->timer_mutex);
atomic_set(&shared->skip_work, 0);
+ init_irq_work(&shared->irq_work, dbs_irq_work);
INIT_WORK(&shared->work, dbs_work_handler);
return 0;
}
@@ -467,9 +457,6 @@ static int cpufreq_governor_start(struct
io_busy = od_tuners->io_is_busy;
}
- shared->policy = policy;
- shared->time_stamp = ktime_get();
-
for_each_cpu(j, policy->cpus) {
struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
unsigned int prev_load;
@@ -485,10 +472,9 @@ static int cpufreq_governor_start(struct
if (ignore_nice)
j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
- __setup_timer(&j_cdbs->timer, dbs_timer_handler,
- (unsigned long)j_cdbs,
- TIMER_DEFERRABLE | TIMER_IRQSAFE);
+ j_cdbs->update_util.func = dbs_update_util_handler;
}
+ shared->policy = policy;
if (cdata->governor == GOV_CONSERVATIVE) {
struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -505,7 +491,7 @@ static int cpufreq_governor_start(struct
od_ops->powersave_bias_init_cpu(cpu);
}
- gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
+ gov_set_update_util(shared, sampling_rate);
return 0;
}
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -191,7 +191,7 @@ static void od_check_cpu(int cpu, unsign
}
}
-static unsigned int od_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
unsigned int cpu = policy->cpu;
@@ -200,9 +200,6 @@ static unsigned int od_dbs_timer(struct
struct od_dbs_tuners *od_tuners = dbs_data->tuners;
int delay = 0, sample_type = dbs_info->sample_type;
- if (!modify_all)
- goto max_delay;
-
/* Common NORMAL_SAMPLE setup */
dbs_info->sample_type = OD_NORMAL_SAMPLE;
if (sample_type == OD_SUB_SAMPLE) {
@@ -218,7 +215,6 @@ static unsigned int od_dbs_timer(struct
}
}
-max_delay:
if (!delay)
delay = delay_for_sampling_rate(od_tuners->sampling_rate
* dbs_info->rate_mult);
@@ -264,7 +260,6 @@ static void update_sampling_rate(struct
struct od_cpu_dbs_info_s *dbs_info;
struct cpu_dbs_info *cdbs;
struct cpu_common_dbs_info *shared;
- unsigned long next_sampling, appointed_at;
dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
cdbs = &dbs_info->cdbs;
@@ -288,20 +283,28 @@ static void update_sampling_rate(struct
* policy will be governed by dbs_data, otherwise there can be
* multiple policies that are governed by the same dbs_data.
*/
- if (dbs_data != policy->governor_data)
- continue;
-
- /*
- * Checking this for any CPU should be fine, timers for all of
- * them are scheduled together.
- */
- next_sampling = jiffies + usecs_to_jiffies(new_rate);
- appointed_at = dbs_info->cdbs.timer.expires;
-
- if (time_before(next_sampling, appointed_at)) {
- gov_cancel_work(shared);
- gov_add_timers(policy, usecs_to_jiffies(new_rate));
-
+ if (dbs_data == policy->governor_data) {
+ mutex_lock(&shared->timer_mutex);
+ /*
+ * On 32-bit architectures this may race with the
+ * sample_delay_ns read in dbs_update_util_handler(),
+ * but that really doesn't matter. If the read returns
+ * a value that's too big, the sample will be skipped,
+ * but the next invocation of dbs_update_util_handler()
+ * (when the update has been completed) will take a
+ * sample. If the returned value is too small, the
+ * sample will be taken immediately, but that isn't a
+ * problem, as we want the new rate to take effect
+ * immediately anyway.
+ *
+ * If this runs in parallel with dbs_work_handler(), we
+ * may end up overwriting the sample_delay_ns value that
+ * it has just written, but the difference should not be
+ * too big and it will be corrected next time a sample
+ * is taken, so it shouldn't be significant.
+ */
+ gov_update_sample_delay(shared, new_rate);
+ mutex_unlock(&shared->timer_mutex);
}
}
Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -115,14 +115,12 @@ static void cs_check_cpu(int cpu, unsign
}
}
-static unsigned int cs_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
{
struct dbs_data *dbs_data = policy->governor_data;
struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
- if (modify_all)
- dbs_check_cpu(dbs_data, policy->cpu);
-
+ dbs_check_cpu(dbs_data, policy->cpu);
return delay_for_sampling_rate(cs_tuners->sampling_rate);
}
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web