cpufreq: governor: Avoid invalid state-transitions
Viresh Kumar
maintainerv2 cover cpufreq: governor: Avoid invalid state-transitions
Hi Rafael/Preeti,
This is another attempt to fix the crashes reported by Preeti. They work quite well for me now, and I hope they would work well for Preeti as well :)
So, patches [1-7,9] are already Reviewed by Preeti.
The first 5 patches are minor cleanups, 6th & 7th try to optimize few things to make code less complex.
Patches 8-10 actually solve (or try to solve :)) the synchronization problems, or the crashes I was getting.
V1->V2:
- 7/11 is dropped and only 8/11 is updated, which is 8/10 now.
- Avoid taking the same mutex in both cpufreq_governor_dbs() and work-handler as that has given us some locdeps, classic ABBA stuff.
- And so timer_mutex, responsible for synchronizing governor work-handlers is kept as is.
- Later patches are almost same with minor updates.
- Invalid state-transitions are sensed better now with improved checks.
- I have run enough tests on my exynos dual core board and failed to crash at all. Not that I wanted to crash :)
Rebased over pm/bleeeding-edge.
Viresh Kumar (10):
cpufreq: governor: Name delayed-work as dwork
cpufreq: governor: Drop unused field 'cpu'
cpufreq: governor: Rename 'cpu_dbs_common_info' to 'cpu_dbs_info'
cpufreq: governor: name pointer to cpu_dbs_info as 'cdbs'
cpufreq: governor: rename cur_policy as policy
cpufreq: governor: Keep single copy of information common to
policy->cpus
cpufreq: governor: split out common part of {cs|od}_dbs_timer()
cpufreq: governor: Avoid invalid states with additional checks
cpufreq: governor: Don't WARN on invalid states
cpufreq: propagate errors returned from __cpufreq_governor()
drivers/cpufreq/cpufreq.c | 22 ++-- drivers/cpufreq/cpufreq_conservative.c | 25 ++--- drivers/cpufreq/cpufreq_governor.c | 196 ++++++++++++++++++++++++--------- drivers/cpufreq/cpufreq_governor.h | 40 ++++--- drivers/cpufreq/cpufreq_ondemand.c | 67 ++++++----- 5 files changed, 220 insertions(+), 130 deletions(-)
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 1/10 cpufreq: governor: Name delayed-work as dwork
Delayed work was named as 'work' and to access work within it we do work.work. Not much readable. Rename delayed_work as 'dwork'.
--- drivers/cpufreq/cpufreq_conservative.c | 2 +- drivers/cpufreq/cpufreq_governor.c | 6 +++--- drivers/cpufreq/cpufreq_governor.h | 2 +- drivers/cpufreq/cpufreq_ondemand.c | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +1 -1
@@ -105,7 +105,7 @@ static void cs_check_cpu(int cpu, unsigned int load)static void cs_dbs_timer (struct work_struct *work) {struct cs_cpu_dbs_info_s *dbs_info =container_of (work, - struct cs_cpu_dbs_info_s, cdbs.work.work); + struct cs_cpu_dbs_info_s, cdbs.dwork.work);unsigned int cpu = dbs_info->cdbs.cur_policy->cpu;struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu (cs_cpu_dbs_info, cpu);
drivers/cpufreq/cpufreq_governor.c +3 -3
@@ -165,7 +165,7 @@ static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data, {struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu); -mod_delayed_work_on (cpu, system_wq, &cdbs->work, delay); +mod_delayed_work_on (cpu, system_wq, &cdbs->dwork, delay); } void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
@@ -204,7 +204,7 @@ static inline void gov_cancel_work(struct dbs_data *dbs_data,for_each_cpu (i, policy->cpus) { cdbs = dbs_data->cdata->get_cpu_cdbs (i); -cancel_delayed_work_sync (&cdbs->work); +cancel_delayed_work_sync (&cdbs->dwork); } }
@@ -367,7 +367,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, j_cdbs->prev_cpu_nice =kcpustat_cpu (j).cpustat[CPUTIME_NICE ];mutex_init (&j_cdbs->timer_mutex); -INIT_DEFERRABLE_WORK (&j_cdbs->work, cdata->gov_dbs_timer); +INIT_DEFERRABLE_WORK (&j_cdbs->dwork, cdata->gov_dbs_timer); }if (cdata->governor ==GOV_CONSERVATIVE ) {
drivers/cpufreq/cpufreq_governor.h +1 -1
@@ -142,7 +142,7 @@ struct cpu_dbs_common_info {*/ unsigned int prev_load;struct cpufreq_policy *cur_policy; -struct delayed_work work; +struct delayed_work dwork;/* * percpu mutex that serializes governor limit change with gov_dbs_timer * invocation. We do not want gov_dbs_timer to run when user is changing
drivers/cpufreq/cpufreq_ondemand.c +4 -4
@@ -194,7 +194,7 @@ static void od_check_cpu(int cpu, unsigned int load)static void od_dbs_timer (struct work_struct *work) {struct od_cpu_dbs_info_s *dbs_info = - container_of(work, struct od_cpu_dbs_info_s, cdbs.work.work); + container_of(work, struct od_cpu_dbs_info_s, cdbs.dwork.work);unsigned int cpu = dbs_info->cdbs.cur_policy->cpu;struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu (od_cpu_dbs_info, cpu);
@@ -275,18 +275,18 @@ static void update_sampling_rate(struct dbs_data *dbs_data,mutex_lock (&dbs_info->cdbs.timer_mutex); -if (!delayed_work_pending (&dbs_info->cdbs.work)) { +if (!delayed_work_pending (&dbs_info->cdbs.dwork)) {mutex_unlock (&dbs_info->cdbs.timer_mutex);continue ; } next_sampling = jiffies +usecs_to_jiffies (new_rate); - appointed_at = dbs_info->cdbs.work.timer.expires; + appointed_at = dbs_info->cdbs.dwork.timer.expires;if (time_before (next_sampling, appointed_at)) {mutex_unlock (&dbs_info->cdbs.timer_mutex); -cancel_delayed_work_sync (&dbs_info->cdbs.work); +cancel_delayed_work_sync (&dbs_info->cdbs.dwork);mutex_lock (&dbs_info->cdbs.timer_mutex); gov_queue_work(dbs_data, dbs_info->cdbs.cur_policy,
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 2/10 cpufreq: governor: Drop unused field 'cpu'
In reply to Viresh KumarIts not used at all, drop it.
--- drivers/cpufreq/cpufreq_governor.c | 1 - drivers/cpufreq/cpufreq_governor.h | 1 - 2 files changed, 2 deletions(-)
drivers/cpufreq/cpufreq_governor.c +0 -1
@@ -353,7 +353,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,struct cpu_dbs_common_info *j_cdbs = cdata->get_cpu_cdbs (j);unsigned int prev_load; - j_cdbs->cpu = j; j_cdbs->cur_policy = policy; j_cdbs->prev_cpu_idle =get_cpu_idle_time (j, &j_cdbs->prev_cpu_wall, io_busy);
drivers/cpufreq/cpufreq_governor.h +0 -1
@@ -130,7 +130,6 @@ static void *get_cpu_dbs_info_s(int cpu) \/* Per cpu structures */ struct cpu_dbs_common_info { -int cpu;u64 prev_cpu_idle;u64 prev_cpu_wall;u64 prev_cpu_nice;
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 3/10 cpufreq: governor: Rename 'cpu_dbs_common_info' to 'cpu_dbs_info'
In reply to Viresh KumarIts not common info to all CPUs, but a structure representing common type of cpu info to both governor types. Lets drop 'common_' from its name.
--- drivers/cpufreq/cpufreq_governor.c | 19 +++++++++---------- drivers/cpufreq/cpufreq_governor.h | 13 ++++++------- 2 files changed, 15 insertions(+), 17 deletions(-)
drivers/cpufreq/cpufreq_governor.c +9 -10
@@ -32,7 +32,7 @@ static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)void dbs_check_cpu (struct dbs_data *dbs_data,int cpu) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);struct od_dbs_tuners *od_tuners = dbs_data->tuners;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;struct cpufreq_policy *policy;
@@ -64,7 +64,7 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)/* Get Absolute Load */ for_each_cpu (j, policy->cpus) { -struct cpu_dbs_common_info *j_cdbs; +struct cpu_dbs_info *j_cdbs;u64 cur_wall_time, cur_idle_time;unsigned int idle_time, wall_time;unsigned int load;
@@ -163,7 +163,7 @@ EXPORT_SYMBOL_GPL(dbs_check_cpu);static inline void __gov_queue_work (int cpu,struct dbs_data *dbs_data,unsigned int delay) { -struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);mod_delayed_work_on (cpu, system_wq, &cdbs->dwork, delay); }
@@ -199,7 +199,7 @@ EXPORT_SYMBOL_GPL(gov_queue_work);static inline void gov_cancel_work (struct dbs_data *dbs_data,struct cpufreq_policy *policy) { -struct cpu_dbs_common_info *cdbs; +struct cpu_dbs_info *cdbs;int i;for_each_cpu (i, policy->cpus) {
@@ -209,8 +209,7 @@ static inline void gov_cancel_work(struct dbs_data *dbs_data, }/* Will return if we need to evaluate cpu load again or not */ -bool need_load_eval (struct cpu_dbs_common_info *cdbs, -unsigned int sampling_rate) +bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate) {if (policy_is_shared (cdbs->cur_policy)) {ktime_t time_now =ktime_get ();
@@ -330,7 +329,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu; -struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu);int io_busy =0 ;if (!policy->cur)
@@ -350,7 +349,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, }for_each_cpu (j, policy->cpus) { -struct cpu_dbs_common_info *j_cdbs = cdata->get_cpu_cdbs (j); +struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs (j);unsigned int prev_load; j_cdbs->cur_policy = policy;
@@ -398,7 +397,7 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy, {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu; -struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu);if (cdata->governor ==GOV_CONSERVATIVE ) { struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -418,7 +417,7 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy, {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu; -struct cpu_dbs_common_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu);if (!cpu_cdbs->cur_policy)return ;
drivers/cpufreq/cpufreq_governor.h +6 -7
@@ -109,7 +109,7 @@ store_one(_gov, file_name)/* create helper routines */ #define define_get_cpu_dbs_routines (_dbs_info) \ -static struct cpu_dbs_common_info *get_cpu_cdbs(int cpu) \ +static struct cpu_dbs_info *get_cpu_cdbs(int cpu) \ { \ return &per_cpu(_dbs_info, cpu).cdbs; \ } \
@@ -129,7 +129,7 @@ static void *get_cpu_dbs_info_s(int cpu) \*/ /* Per cpu structures */ -struct cpu_dbs_common_info { +struct cpu_dbs_info {u64 prev_cpu_idle;u64 prev_cpu_wall;u64 prev_cpu_nice;
@@ -152,7 +152,7 @@ struct cpu_dbs_common_info { };struct od_cpu_dbs_info_s { -struct cpu_dbs_common_info cdbs; +struct cpu_dbs_info cdbs;struct cpufreq_frequency_table *freq_table;unsigned int freq_lo;unsigned int freq_lo_jiffies;
@@ -162,7 +162,7 @@ struct od_cpu_dbs_info_s { };struct cs_cpu_dbs_info_s { -struct cpu_dbs_common_info cdbs; +struct cpu_dbs_info cdbs;unsigned int down_skip;unsigned int requested_freq;unsigned int enable:1 ;
@@ -203,7 +203,7 @@ struct common_dbs_data {*/ struct dbs_data *gdbs_data; -struct cpu_dbs_common_info *(*get_cpu_cdbs)(int cpu); +struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);void *(*get_cpu_dbs_info_s)(int cpu);void (*gov_dbs_timer)(struct work_struct *work);void (*gov_check_cpu)(int cpu,unsigned int load);
@@ -264,8 +264,7 @@ static ssize_t show_sampling_rate_min_gov_pol \extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct dbs_data *dbs_data,int cpu); -bool need_load_eval (struct cpu_dbs_common_info *cdbs, -unsigned int sampling_rate); +bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate);int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event); void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 4/10 cpufreq: governor: name pointer to cpu_dbs_info as 'cdbs'
In reply to Viresh KumarIt is called as 'cdbs' at most of the places and 'cpu_dbs' at others. Lets use 'cdbs' consistently for better readability.
--- drivers/cpufreq/cpufreq_governor.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
drivers/cpufreq/cpufreq_governor.c +13 -13
@@ -329,7 +329,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu; -struct cpu_dbs_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu);int io_busy =0 ;if (!policy->cur)
@@ -385,7 +385,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, }/* Initiate timer time stamp */ - cpu_cdbs->time_stamp =ktime_get (); + cdbs->time_stamp =ktime_get ();gov_queue_work (dbs_data, policy,delay_for_sampling_rate (sampling_rate), true);
@@ -397,7 +397,7 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy, {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu; -struct cpu_dbs_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu);if (cdata->governor ==GOV_CONSERVATIVE ) { struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -408,8 +408,8 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy,gov_cancel_work (dbs_data, policy); -mutex_destroy (&cpu_cdbs->timer_mutex); - cpu_cdbs->cur_policy =NULL ; +mutex_destroy (&cdbs->timer_mutex); + cdbs->cur_policy =NULL ; } static void cpufreq_governor_limits(struct cpufreq_policy *policy,
@@ -417,20 +417,20 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy, {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu; -struct cpu_dbs_info *cpu_cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); -if (!cpu_cdbs->cur_policy) +if (!cdbs->cur_policy)return ; -mutex_lock (&cpu_cdbs->timer_mutex); -if (policy->max < cpu_cdbs->cur_policy->cur) -__cpufreq_driver_target (cpu_cdbs->cur_policy, policy->max, +mutex_lock (&cdbs->timer_mutex); +if (policy->max < cdbs->cur_policy->cur) +__cpufreq_driver_target (cdbs->cur_policy, policy->max,CPUFREQ_RELATION_H ); -else if (policy->min > cpu_cdbs->cur_policy->cur) -__cpufreq_driver_target (cpu_cdbs->cur_policy, policy->min, +else if (policy->min > cdbs->cur_policy->cur) +__cpufreq_driver_target (cdbs->cur_policy, policy->min,CPUFREQ_RELATION_L );dbs_check_cpu (dbs_data, cpu); -mutex_unlock (&cpu_cdbs->timer_mutex); +mutex_unlock (&cdbs->timer_mutex); } int cpufreq_governor_dbs(struct cpufreq_policy *policy,
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 5/10 cpufreq: governor: rename cur_policy as policy
In reply to Viresh KumarJust call it 'policy', cur_policy is unnecessarily long and doesn't have any special meaning.
--- drivers/cpufreq/cpufreq_conservative.c | 10 +++++----- drivers/cpufreq/cpufreq_governor.c | 18 +++++++++--------- drivers/cpufreq/cpufreq_governor.h | 2 +- drivers/cpufreq/cpufreq_ondemand.c | 19 ++++++++++--------- 4 files changed, 25 insertions(+), 24 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +5 -5
@@ -47,7 +47,7 @@ static inline unsigned int get_freq_target(struct cs_dbs_tuners *cs_tuners,static void cs_check_cpu (int cpu,unsigned int load) {struct cs_cpu_dbs_info_s *dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.cur_policy; +struct cpufreq_policy *policy = dbs_info->cdbs.policy;struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
@@ -106,10 +106,10 @@ static void cs_dbs_timer(struct work_struct *work) {struct cs_cpu_dbs_info_s *dbs_info =container_of (work, struct cs_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.cur_policy->cpu; +unsigned int cpu = dbs_info->cdbs.policy->cpu;struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.cur_policy->governor_data; +struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;int delay =delay_for_sampling_rate (cs_tuners->sampling_rate);bool modify_all = true;
@@ -120,7 +120,7 @@ static void cs_dbs_timer(struct work_struct *work)else dbs_check_cpu (dbs_data, cpu); -gov_queue_work (dbs_data, dbs_info->cdbs.cur_policy, delay, modify_all); +gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all);mutex_unlock (&core_dbs_info->cdbs.timer_mutex); }
@@ -135,7 +135,7 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,if (!dbs_info->enable)return 0 ; - policy = dbs_info->cdbs.cur_policy; + policy = dbs_info->cdbs.policy;/* * we only care if our internally tracked freq moves outside the 'valid'
drivers/cpufreq/cpufreq_governor.c +9 -9
@@ -60,7 +60,7 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) ignore_nice = cs_tuners->ignore_nice_load; } - policy = cdbs->cur_policy; + policy = cdbs->policy;/* Get Absolute Load */ for_each_cpu (j, policy->cpus) {
@@ -211,7 +211,7 @@ static inline void gov_cancel_work(struct dbs_data *dbs_data,/* Will return if we need to evaluate cpu load again or not */ bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate) { -if (policy_is_shared (cdbs->cur_policy)) { +if (policy_is_shared (cdbs->policy)) {ktime_t time_now =ktime_get ();s64 delta_us =ktime_us_delta (time_now, cdbs->time_stamp);
@@ -352,7 +352,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs (j);unsigned int prev_load; - j_cdbs->cur_policy = policy; + j_cdbs->policy = policy; j_cdbs->prev_cpu_idle =get_cpu_idle_time (j, &j_cdbs->prev_cpu_wall, io_busy);
@@ -409,7 +409,7 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy,gov_cancel_work (dbs_data, policy);mutex_destroy (&cdbs->timer_mutex); - cdbs->cur_policy =NULL ; + cdbs->policy =NULL ; } static void cpufreq_governor_limits(struct cpufreq_policy *policy,
@@ -419,15 +419,15 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy,unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); -if (!cdbs->cur_policy) +if (!cdbs->policy)return ;mutex_lock (&cdbs->timer_mutex); -if (policy->max < cdbs->cur_policy->cur) -__cpufreq_driver_target (cdbs->cur_policy, policy->max, +if (policy->max < cdbs->policy->cur) +__cpufreq_driver_target (cdbs->policy, policy->max,CPUFREQ_RELATION_H ); -else if (policy->min > cdbs->cur_policy->cur) -__cpufreq_driver_target (cdbs->cur_policy, policy->min, +else if (policy->min > cdbs->policy->cur) +__cpufreq_driver_target (cdbs->policy, policy->min,CPUFREQ_RELATION_L );dbs_check_cpu (dbs_data, cpu);mutex_unlock (&cdbs->timer_mutex);
drivers/cpufreq/cpufreq_governor.h +1 -1
@@ -140,7 +140,7 @@ struct cpu_dbs_info {* wake-up from idle. */ unsigned int prev_load; -struct cpufreq_policy *cur_policy; +struct cpufreq_policy *policy;struct delayed_work dwork;/* * percpu mutex that serializes governor limit change with gov_dbs_timer
drivers/cpufreq/cpufreq_ondemand.c +10 -9
@@ -155,7 +155,7 @@ static void dbs_freq_increase(struct cpufreq_policy *policy, unsigned int freq)static void od_check_cpu (int cpu,unsigned int load) {struct od_cpu_dbs_info_s *dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.cur_policy; +struct cpufreq_policy *policy = dbs_info->cdbs.policy;struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners;
@@ -195,10 +195,10 @@ static void od_dbs_timer(struct work_struct *work) {struct od_cpu_dbs_info_s *dbs_info = container_of(work, struct od_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.cur_policy->cpu; +unsigned int cpu = dbs_info->cdbs.policy->cpu;struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.cur_policy->governor_data; +struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners;int delay =0 , sample_type = core_dbs_info->sample_type;bool modify_all = true;
@@ -213,8 +213,9 @@ static void od_dbs_timer(struct work_struct *work) core_dbs_info->sample_type =OD_NORMAL_SAMPLE ;if (sample_type ==OD_SUB_SAMPLE ) { delay = core_dbs_info->freq_lo_jiffies; -__cpufreq_driver_target (core_dbs_info->cdbs.cur_policy, - core_dbs_info->freq_lo,CPUFREQ_RELATION_H ); +__cpufreq_driver_target (core_dbs_info->cdbs.policy, + core_dbs_info->freq_lo, +CPUFREQ_RELATION_H ); }else {dbs_check_cpu (dbs_data, cpu);if (core_dbs_info->freq_lo) {
@@ -229,7 +230,7 @@ static void od_dbs_timer(struct work_struct *work) delay =delay_for_sampling_rate (od_tuners->sampling_rate * core_dbs_info->rate_mult); -gov_queue_work (dbs_data, dbs_info->cdbs.cur_policy, delay, modify_all); +gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all);mutex_unlock (&core_dbs_info->cdbs.timer_mutex); }
@@ -289,8 +290,8 @@ static void update_sampling_rate(struct dbs_data *dbs_data,cancel_delayed_work_sync (&dbs_info->cdbs.dwork);mutex_lock (&dbs_info->cdbs.timer_mutex); -gov_queue_work (dbs_data, dbs_info->cdbs.cur_policy, -usecs_to_jiffies (new_rate), true); +gov_queue_work (dbs_data, dbs_info->cdbs.policy, +usecs_to_jiffies (new_rate), true); }mutex_unlock (&dbs_info->cdbs.timer_mutex);
@@ -559,7 +560,7 @@ static void od_set_powersave_bias(unsigned int powersave_bias)if (cpumask_test_cpu (cpu, &done))continue ; - policy =per_cpu (od_cpu_dbs_info, cpu).cdbs.cur_policy; + policy =per_cpu (od_cpu_dbs_info, cpu).cdbs.policy;if (!policy)continue ;
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 6/10 cpufreq: governor: Keep single copy of information common to policy->cpus
In reply to Viresh KumarSome information is common to all CPUs belonging to a policy, but are kept on per-cpu basis. Lets keep that in another structure common to all policy->cpus. That will make updates/reads to that less complex and less error prone.
The memory for ccdbs is allocated/freed at INIT/EXIT, so that it we don't reallocate it for STOP/START sequence. It will be also be used (in next patch) while the governor is stopped and so must not be freed that early.
--- drivers/cpufreq/cpufreq_conservative.c | 18 ++++--- drivers/cpufreq/cpufreq_governor.c | 92 +++++++++++++++++++++++++--------- drivers/cpufreq/cpufreq_governor.h | 24 +++++---- drivers/cpufreq/cpufreq_ondemand.c | 38 +++++++------- 4 files changed, 114 insertions(+), 58 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +10 -8
@@ -47,7 +47,7 @@ static inline unsigned int get_freq_target(struct cs_dbs_tuners *cs_tuners,static void cs_check_cpu (int cpu,unsigned int load) {struct cs_cpu_dbs_info_s *dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.policy; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy;struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
@@ -106,22 +106,24 @@ static void cs_dbs_timer(struct work_struct *work) {struct cs_cpu_dbs_info_s *dbs_info =container_of (work, struct cs_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.policy->cpu; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy; +unsigned int cpu = policy->cpu;struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data; +struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;int delay =delay_for_sampling_rate (cs_tuners->sampling_rate);bool modify_all = true; -mutex_lock (&core_dbs_info->cdbs.timer_mutex); -if (!need_load_eval (&core_dbs_info->cdbs, cs_tuners->sampling_rate)) +mutex_lock (&core_dbs_info->cdbs.ccdbs->timer_mutex); +if (!need_load_eval (core_dbs_info->cdbs.ccdbs, + cs_tuners->sampling_rate)) modify_all = false;else dbs_check_cpu (dbs_data, cpu); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.timer_mutex); +gov_queue_work (dbs_data, policy, delay, modify_all); +mutex_unlock (&core_dbs_info->cdbs.ccdbs->timer_mutex); } static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
@@ -135,7 +137,7 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,if (!dbs_info->enable)return 0 ; - policy = dbs_info->cdbs.policy; + policy = dbs_info->cdbs.ccdbs->policy;/* * we only care if our internally tracked freq moves outside the 'valid'
drivers/cpufreq/cpufreq_governor.c +68 -24
203 lines not on this page · raw message
drivers/cpufreq/cpufreq_governor.h +15 -9
@@ -128,6 +128,18 @@ static void *get_cpu_dbs_info_s(int cpu) \* cs_*: Conservative governor */ +/* Common to all CPUs of a policy */ +struct cpu_common_dbs_info { +struct cpufreq_policy *policy; +/* +* percpu mutex that serializes governor limit change with gov_dbs_timer +* invocation. We do not want gov_dbs_timer to run when user is changing +* the governor or limits. +*/ +struct mutex timer_mutex; +ktime_t time_stamp; +}; +/* Per cpu structures */ struct cpu_dbs_info {u64 prev_cpu_idle;
@@ -140,15 +152,8 @@ struct cpu_dbs_info {* wake-up from idle. */ unsigned int prev_load; -struct cpufreq_policy *policy;struct delayed_work dwork; -/* -* percpu mutex that serializes governor limit change with gov_dbs_timer -* invocation. We do not want gov_dbs_timer to run when user is changing -* the governor or limits. -*/ -struct mutex timer_mutex; -ktime_t time_stamp; +struct cpu_common_dbs_info *ccdbs; };struct od_cpu_dbs_info_s {
@@ -264,7 +269,8 @@ static ssize_t show_sampling_rate_min_gov_pol \extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct dbs_data *dbs_data,int cpu); -bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate); +bool need_load_eval (struct cpu_common_dbs_info *ccdbs, +unsigned int sampling_rate);int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event); void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
drivers/cpufreq/cpufreq_ondemand.c +21 -17
@@ -155,7 +155,7 @@ static void dbs_freq_increase(struct cpufreq_policy *policy, unsigned int freq)static void od_check_cpu (int cpu,unsigned int load) {struct od_cpu_dbs_info_s *dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.policy; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy;struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners;
@@ -195,16 +195,18 @@ static void od_dbs_timer(struct work_struct *work) {struct od_cpu_dbs_info_s *dbs_info = container_of(work, struct od_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.policy->cpu; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy; +unsigned int cpu = policy->cpu;struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data; +struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners;int delay =0 , sample_type = core_dbs_info->sample_type;bool modify_all = true; -mutex_lock (&core_dbs_info->cdbs.timer_mutex); -if (!need_load_eval (&core_dbs_info->cdbs, od_tuners->sampling_rate)) { +mutex_lock (&core_dbs_info->cdbs.ccdbs->timer_mutex); +if (!need_load_eval (core_dbs_info->cdbs.ccdbs, + od_tuners->sampling_rate)) { modify_all = false; goto max_delay; }
@@ -213,8 +215,7 @@ static void od_dbs_timer(struct work_struct *work) core_dbs_info->sample_type =OD_NORMAL_SAMPLE ;if (sample_type ==OD_SUB_SAMPLE ) { delay = core_dbs_info->freq_lo_jiffies; -__cpufreq_driver_target (core_dbs_info->cdbs.policy, - core_dbs_info->freq_lo, +__cpufreq_driver_target (policy, core_dbs_info->freq_lo,CPUFREQ_RELATION_H ); }else {dbs_check_cpu (dbs_data, cpu);
@@ -230,8 +231,8 @@ static void od_dbs_timer(struct work_struct *work) delay =delay_for_sampling_rate (od_tuners->sampling_rate * core_dbs_info->rate_mult); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.timer_mutex); +gov_queue_work (dbs_data, policy, delay, modify_all); +mutex_unlock (&core_dbs_info->cdbs.ccdbs->timer_mutex); }/************************** sysfs interface ************************/
@@ -274,10 +275,10 @@ static void update_sampling_rate(struct dbs_data *dbs_data, dbs_info = &per_cpu (od_cpu_dbs_info, cpu);cpufreq_cpu_put (policy); -mutex_lock (&dbs_info->cdbs.timer_mutex); +mutex_lock (&dbs_info->cdbs.ccdbs->timer_mutex);if (!delayed_work_pending (&dbs_info->cdbs.dwork)) { -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.ccdbs->timer_mutex);continue ; }
@@ -286,15 +287,15 @@ static void update_sampling_rate(struct dbs_data *dbs_data,if (time_before (next_sampling, appointed_at)) { -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.ccdbs->timer_mutex);cancel_delayed_work_sync (&dbs_info->cdbs.dwork); -mutex_lock (&dbs_info->cdbs.timer_mutex); +mutex_lock (&dbs_info->cdbs.ccdbs->timer_mutex); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, +gov_queue_work (dbs_data, policy,usecs_to_jiffies (new_rate), true); } -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.ccdbs->timer_mutex); } }
@@ -557,13 +558,16 @@ static void od_set_powersave_bias(unsigned int powersave_bias)get_online_cpus ();for_each_online_cpu (cpu) { +struct cpu_common_dbs_info *ccdbs; +if (cpumask_test_cpu (cpu, &done))continue ; - policy =per_cpu (od_cpu_dbs_info, cpu).cdbs.policy; -if (!policy) + ccdbs =per_cpu (od_cpu_dbs_info, cpu).cdbs.ccdbs; +if (!ccdbs)continue ; + policy = ccdbs->policy;cpumask_or (&done, &done, policy->cpus);if (policy->governor != &cpufreq_gov_ondemand)
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Re: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
Quoted 321 lines · Viresh Kumar
Some information is common to all CPUs belonging to a policy, but are kept on per-cpu basis. Lets keep that in another structure common to all
policy->cpus. That will make updates/reads to that less complex and less error prone.The memory for ccdbs is allocated/freed at INIT/EXIT, so that it we don't reallocate it for STOP/START sequence. It will be also be used (in next patch) while the governor is stopped and so must not be freed that early.
Reviewed-by Preeti U Murthy <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_conservative.c | 18 ++++--- drivers/cpufreq/cpufreq_governor.c | 92 +++++++++++++++++++++++++--------- drivers/cpufreq/cpufreq_governor.h | 24 +++++---- drivers/cpufreq/cpufreq_ondemand.c | 38 +++++++------- 4 files changed, 114 insertions(+), 58 deletions(-)drivers/cpufreq/cpufreq_conservative.c +10
-8@@ -47,7 +47,7 @@ static inline unsigned int get_freq_target(struct cs_dbs_tuners *cs_tuners,static void cs_check_cpu (int cpu,unsigned int load) {struct cs_cpu_dbs_info_s *dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.policy; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy;struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;@@ -106,22 +106,24 @@ static void cs_dbs_timer(struct work_struct *work) {struct cs_cpu_dbs_info_s *dbs_info =container_of (work, struct cs_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.policy->cpu; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy; +unsigned int cpu = policy->cpu;struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data; +struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;int delay =delay_for_sampling_rate (cs_tuners->sampling_rate);bool modify_all = true; -mutex_lock (&core_dbs_info->cdbs.timer_mutex); -if (!need_load_eval (&core_dbs_info->cdbs, cs_tuners->sampling_rate)) +mutex_lock (&core_dbs_info->cdbs.ccdbs->timer_mutex); +if (!need_load_eval (core_dbs_info->cdbs.ccdbs, + cs_tuners->sampling_rate)) modify_all = false;else dbs_check_cpu (dbs_data, cpu); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.timer_mutex); +gov_queue_work (dbs_data, policy, delay, modify_all); +mutex_unlock (&core_dbs_info->cdbs.ccdbs->timer_mutex); } static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,@@ -135,7 +137,7 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,if (!dbs_info->enable)return 0 ; - policy = dbs_info->cdbs.policy; + policy = dbs_info->cdbs.ccdbs->policy;/* * we only care if our internally tracked freq moves outside the 'valid' drivers/cpufreq/cpufreq_governor.c +68
-24@@ -35,7 +35,7 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);struct od_dbs_tuners *od_tuners = dbs_data->tuners;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; -struct cpufreq_policy *policy; +struct cpufreq_policy *policy = cdbs->ccdbs->policy;unsigned int sampling_rate;unsigned int max_load =0 ;unsigned int ignore_nice;@@ -60,8 +60,6 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) ignore_nice = cs_tuners->ignore_nice_load; } - policy = cdbs->policy; -/* Get Absolute Load */ for_each_cpu (j, policy->cpus) {struct cpu_dbs_info *j_cdbs;@@ -209,17 +207,18 @@ static inline void gov_cancel_work(struct dbs_data *dbs_data, }/* Will return if we need to evaluate cpu load again or not */ -bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate) +bool need_load_eval (struct cpu_common_dbs_info *ccdbs, +unsigned int sampling_rate) { -if (policy_is_shared (cdbs->policy)) { +if (policy_is_shared (ccdbs->policy)) {ktime_t time_now =ktime_get (); -s64 delta_us =ktime_us_delta (time_now, cdbs->time_stamp); +s64 delta_us =ktime_us_delta (time_now, ccdbs->time_stamp);/* Do nothing if we recently have sampled */ if (delta_us < (s64)(sampling_rate /2 ))return false;else - cdbs->time_stamp = time_now; + ccdbs->time_stamp = time_now; }return true;@@ -238,6 +237,37 @@ static void set_sampling_rate(struct dbs_data *dbs_data, } } +static int alloc_ccdbs (struct cpufreq_policy *policy, +struct common_dbs_data *cdata) +{ +struct cpu_common_dbs_info *ccdbs; +int j; + +/* Allocate memory for the common information for policy->cpus */ + ccdbs =kzalloc (sizeof (*ccdbs),GFP_KERNEL ); +if (!ccdbs) +return -ENOMEM ; + +/* Set ccdbs for all CPUs, online+offline */ +for_each_cpu (j, policy->related_cpus) + cdata->get_cpu_cdbs (j)->ccdbs = ccdbs; + +return 0 ; +} + +static void free_ccdbs (struct cpufreq_policy *policy, +struct common_dbs_data *cdata) +{ +struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (policy->cpu); +struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs; +int j; + +for_each_cpu (j, policy->cpus) + cdata->get_cpu_cdbs (j)->ccdbs =NULL ; + +kfree (ccdbs); +} +static int cpufreq_governor_init (struct cpufreq_policy *policy,struct dbs_data *dbs_data,struct common_dbs_data *cdata)@@ -248,6 +278,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,if (dbs_data) {if (WARN_ON (have_governor_per_policy ()))return -EINVAL ; + + ret =alloc_ccdbs (policy, cdata); +if (ret) +return ret; + dbs_data->usage_count++; policy->governor_data = dbs_data;return 0 ;@@ -257,12 +292,16 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,if (!dbs_data)return -ENOMEM ; + ret =alloc_ccdbs (policy, cdata); +if (ret) + goto free_dbs_data; + dbs_data->cdata = cdata; dbs_data->usage_count =1 ; ret = cdata->init (dbs_data, !policy->governor->initialized);if (ret) - goto free_dbs_data; + goto free_ccdbs;/* policy latency is in ns. Convert it to us first */ latency = policy->cpuinfo.transition_latency /1000 ;@@ -299,6 +338,8 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy, } cdata_exit: cdata->exit (dbs_data, !policy->governor->initialized); +free_ccdbs: +free_ccdbs (policy, cdata); free_dbs_data:kfree (dbs_data);return ret;@@ -322,6 +363,8 @@ static void cpufreq_governor_exit(struct cpufreq_policy *policy, cdata->exit (dbs_data, policy->governor->initialized ==1 );kfree (dbs_data); } + +free_ccdbs (policy, cdata); } static int cpufreq_governor_start(struct cpufreq_policy *policy,@@ -330,6 +373,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,struct common_dbs_data *cdata = dbs_data->cdata;unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs;int io_busy =0 ;if (!policy->cur)@@ -348,11 +392,14 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, io_busy = od_tuners->io_is_busy; } + ccdbs->policy = policy; + ccdbs->time_stamp =ktime_get (); +mutex_init (&ccdbs->timer_mutex); +for_each_cpu (j, policy->cpus) {struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs (j);unsigned int prev_load; - j_cdbs->policy = policy; j_cdbs->prev_cpu_idle =get_cpu_idle_time (j, &j_cdbs->prev_cpu_wall, io_busy);@@ -364,7 +411,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,if (ignore_nice) j_cdbs->prev_cpu_nice =kcpustat_cpu (j).cpustat[CPUTIME_NICE ]; -mutex_init (&j_cdbs->timer_mutex);INIT_DEFERRABLE_WORK (&j_cdbs->dwork, cdata->gov_dbs_timer); }@@ -384,9 +430,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, od_ops->powersave_bias_init_cpu (cpu); } -/* Initiate timer time stamp */ - cdbs->time_stamp =ktime_get (); -gov_queue_work (dbs_data, policy,delay_for_sampling_rate (sampling_rate), true);return 0 ;@@ -398,6 +441,9 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy,struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs; + +gov_cancel_work (dbs_data, policy);if (cdata->governor ==GOV_CONSERVATIVE ) { struct cs_cpu_dbs_info_s *cs_dbs_info =@@ -406,10 +452,8 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy, cs_dbs_info->enable =0 ; } -gov_cancel_work (dbs_data, policy); - -mutex_destroy (&cdbs->timer_mutex); - cdbs->policy =NULL ; + ccdbs->policy =NULL ; +mutex_destroy (&ccdbs->timer_mutex); } static void cpufreq_governor_limits(struct cpufreq_policy *policy,@@ -419,18 +463,18 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy,unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); -if (!cdbs->policy) +if (!cdbs->ccdbs || !cdbs->ccdbs->policy)return ; -mutex_lock (&cdbs->timer_mutex); -if (policy->max < cdbs->policy->cur) -__cpufreq_driver_target (cdbs->policy, policy->max, +mutex_lock (&cdbs->ccdbs->timer_mutex); +if (policy->max < cdbs->ccdbs->policy->cur) +__cpufreq_driver_target (cdbs->ccdbs->policy, policy->max,CPUFREQ_RELATION_H ); -else if (policy->min > cdbs->policy->cur) -__cpufreq_driver_target (cdbs->policy, policy->min, +else if (policy->min > cdbs->ccdbs->policy->cur) +__cpufreq_driver_target (cdbs->ccdbs->policy, policy->min,CPUFREQ_RELATION_L );dbs_check_cpu (dbs_data, cpu); -mutex_unlock (&cdbs->timer_mutex); +mutex_unlock (&cdbs->ccdbs->timer_mutex); } int cpufreq_governor_dbs(struct cpufreq_policy *policy,drivers/cpufreq/cpufreq_governor.h +12
-0@@ -128,6 +128,18 @@ static void *get_cpu_dbs_info_s(int cpu) \* cs_*: Conservative governor */ +/* Common to all CPUs of a policy */ +struct cpu_common_dbs_info { +struct cpufreq_policy *policy; +/* +* percpu mutex that serializes governor limit change with gov_dbs_timer +* invocation. We do not want gov_dbs_timer to run when user is changing +* the governor or limits. +*/ +struct mutex timer_mutex; +ktime_t time_stamp; +}; +/* Per cpu structures */ struct cpu_dbs_info {u64 prev_cpu_idle;@@ -140,15 +152,8 @@ struct cpu_dbs_info {* wake-up from idle. */ unsigned int prev_load;
drivers/cpufreq/cpufreq_governor.h struct cpu_dbs_info { · line 156
Quoted 10 lines
drivers/cpufreq/cpufreq_governor.h +1
-8-struct cpufreq_policy *policy;struct delayed_work dwork; -/* -* percpu mutex that serializes governor limit change with gov_dbs_timer -* invocation. We do not want gov_dbs_timer to run when user is changing -* the governor or limits. -*/ -struct mutex timer_mutex; -ktime_t time_stamp; +struct cpu_common_dbs_info *ccdbs;
I don't like this new field name to be honest. It's just too cryptic to me.
Why don't you simply call it "common" or "shared", for example?
Quoted 18 lines
drivers/cpufreq/cpufreq_governor.h +2
-1};struct od_cpu_dbs_info_s {@@ -264,7 +269,8 @@ static ssize_t show_sampling_rate_min_gov_pol \extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct dbs_data *dbs_data,int cpu); -bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate); +bool need_load_eval (struct cpu_common_dbs_info *ccdbs, +unsigned int sampling_rate);int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event); void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,drivers/cpufreq/cpufreq_ondemand.c +0
-0
drivers/cpufreq/cpufreq_ondemand.c static void dbs_freq_increase(struct cpufreq_policy *policy, unsigned int freq) · line 158
Quoted 5 lines
drivers/cpufreq/cpufreq_ondemand.c +1
-1@@ -155,7 +155,7 @@ static void dbs_freq_increase(struct cpufreq_policy *policy, unsigned int freq)static void od_check_cpu (int cpu,unsigned int load) {struct od_cpu_dbs_info_s *dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.policy; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy;
If you rename ccdbs to "shared" that will look as follows:
struct cpufreq_policy *policy = dbs_info->cdbs.shared->policy;
which is way more readable to me.
Thanks,
Rafael
Re: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
In reply to Viresh KumarAm Freitag, den 19.06.2015, 17:18 +0530 schrieb Viresh Kumar:
Quoted 5 lines
Some information is common to all CPUs belonging to a policy, but are kept on per-cpu basis. Lets keep that in another structure common to all
policy->cpus. That will make updates/reads to that less complex and less error prone.
Quoted 7 lines
The memory for ccdbs is allocated/freed at INIT/EXIT, so that it we don't reallocate it for STOP/START sequence. It will be also be used (in next patch) while the governor is stopped and so must not be freed that early.
Reviewed-by Preeti U Murthy <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Sorry for being late to the party, as this has long been applied, but I stumbled upon this while looking at other stuff and wanted to express my finding.
Quoted 1 line
From a short look this change is totally bogus.
The *_dbs_timer functions are called from a workqueue on each CPU. Moving the time_stamp and the timer_mutex to a shared structure now causes a thundering herd issue on multicore systems. All workqueues wake up at the same time with all but one of the worker bouncing on the contended mutex.
Also sharing the time_stamp causes all but the first worker to acquire the mutex to not properly update their sampling information, I guess. I didn't look to deeply into this.
Regards,
Lucas
Quoted 441 lines
--- drivers/cpufreq/cpufreq_conservative.c | 18 ++++--- drivers/cpufreq/cpufreq_governor.c | 92 +++++++++++++++++++++++++--------- drivers/cpufreq/cpufreq_governor.h | 24 +++++---- drivers/cpufreq/cpufreq_ondemand.c | 38 +++++++------- 4 files changed, 114 insertions(+), 58 deletions(-)drivers/cpufreq/cpufreq_conservative.c +10
-8@@ -47,7 +47,7 @@ static inline unsigned int get_freq_target(struct cs_dbs_tuners *cs_tuners,static void cs_check_cpu (int cpu,unsigned int load) {struct cs_cpu_dbs_info_s *dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.policy; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy;struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;@@ -106,22 +106,24 @@ static void cs_dbs_timer(struct work_struct *work) {struct cs_cpu_dbs_info_s *dbs_info =container_of (work, struct cs_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.policy->cpu; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy; +unsigned int cpu = policy->cpu;struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data; +struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;int delay =delay_for_sampling_rate (cs_tuners->sampling_rate);bool modify_all = true; -mutex_lock (&core_dbs_info->cdbs.timer_mutex); -if (!need_load_eval (&core_dbs_info->cdbs, cs_tuners->sampling_rate)) +mutex_lock (&core_dbs_info->cdbs.ccdbs->timer_mutex); +if (!need_load_eval (core_dbs_info->cdbs.ccdbs, + cs_tuners->sampling_rate)) modify_all = false;else dbs_check_cpu (dbs_data, cpu); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.timer_mutex); +gov_queue_work (dbs_data, policy, delay, modify_all); +mutex_unlock (&core_dbs_info->cdbs.ccdbs->timer_mutex); } static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,@@ -135,7 +137,7 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,if (!dbs_info->enable)return 0 ; - policy = dbs_info->cdbs.policy; + policy = dbs_info->cdbs.ccdbs->policy;/* * we only care if our internally tracked freq moves outside the 'valid' drivers/cpufreq/cpufreq_governor.c +68
-24@@ -35,7 +35,7 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs (cpu);struct od_dbs_tuners *od_tuners = dbs_data->tuners;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; -struct cpufreq_policy *policy; +struct cpufreq_policy *policy = cdbs->ccdbs->policy;unsigned int sampling_rate;unsigned int max_load =0 ;unsigned int ignore_nice;@@ -60,8 +60,6 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) ignore_nice = cs_tuners->ignore_nice_load; } - policy = cdbs->policy; -/* Get Absolute Load */ for_each_cpu (j, policy->cpus) {struct cpu_dbs_info *j_cdbs;@@ -209,17 +207,18 @@ static inline void gov_cancel_work(struct dbs_data *dbs_data, }/* Will return if we need to evaluate cpu load again or not */ -bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate) +bool need_load_eval (struct cpu_common_dbs_info *ccdbs, +unsigned int sampling_rate) { -if (policy_is_shared (cdbs->policy)) { +if (policy_is_shared (ccdbs->policy)) {ktime_t time_now =ktime_get (); -s64 delta_us =ktime_us_delta (time_now, cdbs->time_stamp); +s64 delta_us =ktime_us_delta (time_now, ccdbs->time_stamp);/* Do nothing if we recently have sampled */ if (delta_us < (s64)(sampling_rate /2 ))return false;else - cdbs->time_stamp = time_now; + ccdbs->time_stamp = time_now; }return true;@@ -238,6 +237,37 @@ static void set_sampling_rate(struct dbs_data *dbs_data, } } +static int alloc_ccdbs (struct cpufreq_policy *policy, +struct common_dbs_data *cdata) +{ +struct cpu_common_dbs_info *ccdbs; +int j; + +/* Allocate memory for the common information for policy->cpus */ + ccdbs =kzalloc (sizeof (*ccdbs),GFP_KERNEL ); +if (!ccdbs) +return -ENOMEM ; + +/* Set ccdbs for all CPUs, online+offline */ +for_each_cpu (j, policy->related_cpus) + cdata->get_cpu_cdbs (j)->ccdbs = ccdbs; + +return 0 ; +} + +static void free_ccdbs (struct cpufreq_policy *policy, +struct common_dbs_data *cdata) +{ +struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (policy->cpu); +struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs; +int j; + +for_each_cpu (j, policy->cpus) + cdata->get_cpu_cdbs (j)->ccdbs =NULL ; + +kfree (ccdbs); +} +static int cpufreq_governor_init (struct cpufreq_policy *policy,struct dbs_data *dbs_data,struct common_dbs_data *cdata)@@ -248,6 +278,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,if (dbs_data) {if (WARN_ON (have_governor_per_policy ()))return -EINVAL ; + + ret =alloc_ccdbs (policy, cdata); +if (ret) +return ret; + dbs_data->usage_count++; policy->governor_data = dbs_data;return 0 ;@@ -257,12 +292,16 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,if (!dbs_data)return -ENOMEM ; + ret =alloc_ccdbs (policy, cdata); +if (ret) + goto free_dbs_data; + dbs_data->cdata = cdata; dbs_data->usage_count =1 ; ret = cdata->init (dbs_data, !policy->governor->initialized);if (ret) - goto free_dbs_data; + goto free_ccdbs;/* policy latency is in ns. Convert it to us first */ latency = policy->cpuinfo.transition_latency /1000 ;@@ -299,6 +338,8 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy, } cdata_exit: cdata->exit (dbs_data, !policy->governor->initialized); +free_ccdbs: +free_ccdbs (policy, cdata); free_dbs_data:kfree (dbs_data);return ret;@@ -322,6 +363,8 @@ static void cpufreq_governor_exit(struct cpufreq_policy *policy, cdata->exit (dbs_data, policy->governor->initialized ==1 );kfree (dbs_data); } + +free_ccdbs (policy, cdata); } static int cpufreq_governor_start(struct cpufreq_policy *policy,@@ -330,6 +373,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,struct common_dbs_data *cdata = dbs_data->cdata;unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs;int io_busy =0 ;if (!policy->cur)@@ -348,11 +392,14 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, io_busy = od_tuners->io_is_busy; } + ccdbs->policy = policy; + ccdbs->time_stamp =ktime_get (); +mutex_init (&ccdbs->timer_mutex); +for_each_cpu (j, policy->cpus) {struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs (j);unsigned int prev_load; - j_cdbs->policy = policy; j_cdbs->prev_cpu_idle =get_cpu_idle_time (j, &j_cdbs->prev_cpu_wall, io_busy);@@ -364,7 +411,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,if (ignore_nice) j_cdbs->prev_cpu_nice =kcpustat_cpu (j).cpustat[CPUTIME_NICE ]; -mutex_init (&j_cdbs->timer_mutex);INIT_DEFERRABLE_WORK (&j_cdbs->dwork, cdata->gov_dbs_timer); }@@ -384,9 +430,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, od_ops->powersave_bias_init_cpu (cpu); } -/* Initiate timer time stamp */ - cdbs->time_stamp =ktime_get (); -gov_queue_work (dbs_data, policy,delay_for_sampling_rate (sampling_rate), true);return 0 ;@@ -398,6 +441,9 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy,struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); +struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs; + +gov_cancel_work (dbs_data, policy);if (cdata->governor ==GOV_CONSERVATIVE ) { struct cs_cpu_dbs_info_s *cs_dbs_info =@@ -406,10 +452,8 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy, cs_dbs_info->enable =0 ; } -gov_cancel_work (dbs_data, policy); - -mutex_destroy (&cdbs->timer_mutex); - cdbs->policy =NULL ; + ccdbs->policy =NULL ; +mutex_destroy (&ccdbs->timer_mutex); } static void cpufreq_governor_limits(struct cpufreq_policy *policy,@@ -419,18 +463,18 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy,unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); -if (!cdbs->policy) +if (!cdbs->ccdbs || !cdbs->ccdbs->policy)return ; -mutex_lock (&cdbs->timer_mutex); -if (policy->max < cdbs->policy->cur) -__cpufreq_driver_target (cdbs->policy, policy->max, +mutex_lock (&cdbs->ccdbs->timer_mutex); +if (policy->max < cdbs->ccdbs->policy->cur) +__cpufreq_driver_target (cdbs->ccdbs->policy, policy->max,CPUFREQ_RELATION_H ); -else if (policy->min > cdbs->policy->cur) -__cpufreq_driver_target (cdbs->policy, policy->min, +else if (policy->min > cdbs->ccdbs->policy->cur) +__cpufreq_driver_target (cdbs->ccdbs->policy, policy->min,CPUFREQ_RELATION_L );dbs_check_cpu (dbs_data, cpu); -mutex_unlock (&cdbs->timer_mutex); +mutex_unlock (&cdbs->ccdbs->timer_mutex); } int cpufreq_governor_dbs(struct cpufreq_policy *policy,drivers/cpufreq/cpufreq_governor.h +15
-9@@ -128,6 +128,18 @@ static void *get_cpu_dbs_info_s(int cpu) \* cs_*: Conservative governor */ +/* Common to all CPUs of a policy */ +struct cpu_common_dbs_info { +struct cpufreq_policy *policy; +/* +* percpu mutex that serializes governor limit change with gov_dbs_timer +* invocation. We do not want gov_dbs_timer to run when user is changing +* the governor or limits. +*/ +struct mutex timer_mutex; +ktime_t time_stamp; +}; +/* Per cpu structures */ struct cpu_dbs_info {u64 prev_cpu_idle;@@ -140,15 +152,8 @@ struct cpu_dbs_info {* wake-up from idle. */ unsigned int prev_load; -struct cpufreq_policy *policy;struct delayed_work dwork; -/* -* percpu mutex that serializes governor limit change with gov_dbs_timer -* invocation. We do not want gov_dbs_timer to run when user is changing -* the governor or limits. -*/ -struct mutex timer_mutex; -ktime_t time_stamp; +struct cpu_common_dbs_info *ccdbs; };struct od_cpu_dbs_info_s {@@ -264,7 +269,8 @@ static ssize_t show_sampling_rate_min_gov_pol \extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct dbs_data *dbs_data,int cpu); -bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate); +bool need_load_eval (struct cpu_common_dbs_info *ccdbs, +unsigned int sampling_rate);int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event); void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,drivers/cpufreq/cpufreq_ondemand.c +21
-17@@ -155,7 +155,7 @@ static void dbs_freq_increase(struct cpufreq_policy *policy, unsigned int freq)static void od_check_cpu (int cpu,unsigned int load) {struct od_cpu_dbs_info_s *dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.policy; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy;struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners;@@ -195,16 +195,18 @@ static void od_dbs_timer(struct work_struct *work) {struct od_cpu_dbs_info_s *dbs_info = container_of(work, struct od_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.policy->cpu; +struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy; +unsigned int cpu = policy->cpu;struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data; +struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners;int delay =0 , sample_type = core_dbs_info->sample_type;bool modify_all = true; -mutex_lock (&core_dbs_info->cdbs.timer_mutex); -if (!need_load_eval (&core_dbs_info->cdbs, od_tuners->sampling_rate)) { +mutex_lock (&core_dbs_info->cdbs.ccdbs->timer_mutex); +if (!need_load_eval (core_dbs_info->cdbs.ccdbs, + od_tuners->sampling_rate)) { modify_all = false; goto max_delay; }@@ -213,8 +215,7 @@ static void od_dbs_timer(struct work_struct *work) core_dbs_info->sample_type =OD_NORMAL_SAMPLE ;if (sample_type ==OD_SUB_SAMPLE ) { delay = core_dbs_info->freq_lo_jiffies; -__cpufreq_driver_target (core_dbs_info->cdbs.policy, - core_dbs_info->freq_lo, +__cpufreq_driver_target (policy, core_dbs_info->freq_lo,CPUFREQ_RELATION_H ); }else {dbs_check_cpu (dbs_data, cpu);@@ -230,8 +231,8 @@ static void od_dbs_timer(struct work_struct *work) delay =delay_for_sampling_rate (od_tuners->sampling_rate * core_dbs_info->rate_mult); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.timer_mutex); +gov_queue_work (dbs_data, policy, delay, modify_all); +mutex_unlock (&core_dbs_info->cdbs.ccdbs->timer_mutex); }/************************** sysfs interface ************************/ @@ -274,10 +275,10 @@ static void update_sampling_rate(struct dbs_data *dbs_data, dbs_info = &per_cpu (od_cpu_dbs_info, cpu);cpufreq_cpu_put (policy); -mutex_lock (&dbs_info->cdbs.timer_mutex); +mutex_lock (&dbs_info->cdbs.ccdbs->timer_mutex);if (!delayed_work_pending (&dbs_info->cdbs.dwork)) { -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.ccdbs->timer_mutex);continue ; }@@ -286,15 +287,15 @@ static void update_sampling_rate(struct dbs_data *dbs_data,if (time_before (next_sampling, appointed_at)) { -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.ccdbs->timer_mutex);cancel_delayed_work_sync (&dbs_info->cdbs.dwork); -mutex_lock (&dbs_info->cdbs.timer_mutex); +mutex_lock (&dbs_info->cdbs.ccdbs->timer_mutex); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, +gov_queue_work (dbs_data, policy,usecs_to_jiffies (new_rate), true); } -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.ccdbs->timer_mutex); } }@@ -557,13 +558,16 @@ static void od_set_powersave_bias(unsigned int powersave_bias)get_online_cpus ();for_each_online_cpu (cpu) { +struct cpu_common_dbs_info *ccdbs; +if (cpumask_test_cpu (cpu, &done))continue ; - policy =per_cpu (od_cpu_dbs_info, cpu).cdbs.policy; -if (!policy) + ccdbs =per_cpu (od_cpu_dbs_info, cpu).cdbs.ccdbs; +if (!ccdbs)continue ; + policy = ccdbs->policy;cpumask_or (&done, &done, policy->cpus);if (policy->governor != &cpufreq_gov_ondemand)
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
Viresh Kumar
maintainerRe: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
Quoted 3 lines · Lucas Stach
Sorry for being late to the party, as this has long been applied, but I stumbled upon this while looking at other stuff and wanted to express my finding.
No issues, we can still fix things if they have gone wrong.
Quoted 1 line
From a short look this change is totally bogus.
That's a bit shocking, after two people reviewed and tested it :)
Quoted 1 line
The
*_dbs_timerfunctions are called from a workqueue on each CPU.
Right.
Quoted 2 lines
Moving the
time_stampand thetimer_mutexto a shared structure now causes a thundering herd issue on multicore systems.
You faced an issue or you just think that something is wrong ?
Quoted 3 lines
All workqueues wake
up at the same time with all but one of the worker bouncing on the contended mutex.
This is exactly what used to be done earlier, but it was designed badly. For example, for ondemand governor, each CPU had a struct od_cpu_dbs_info_s and we only used the od_cpu_dbs_info_s for the 'policy->cpu'. So, even if all CPUs had this structure (i.e. the time_stamp and the lock), we only used structure for policy->cpu.
Quoted 2 lines
Also sharing the
time_stampcauses all but the first worker to acquire the mutex to not properly update their sampling information, I guess.
Yeah, that's what we want per-policy.
Quoted 1 line
I didn't look to deeply into this.
Perhaps, yes.
Signature · 1 line
viresh
Re: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
Am Montag, den 30.11.2015, 10:49 +0530 schrieb Viresh Kumar:
Quoted 11 lines · Lucas Stach
Quoted 3 lines
Sorry for being late to the party, as this has long been applied, but I stumbled upon this while looking at other stuff and wanted to express my finding.
No issues, we can still fix things if they have gone wrong.
Quoted 1 line
From a short look this change is totally bogus.
That's a bit shocking, after two people reviewed and tested it :)
Quoted 8 lines
Quoted 1 line
The
*_dbs_timerfunctions are called from a workqueue on each CPU.Right.
Quoted 2 lines
Moving the
time_stampand thetimer_mutexto a shared structure now causes a thundering herd issue on multicore systems.You faced an issue or you just think that something is wrong ?
The timer_mutex is one of the top contended locks already on a quad core system. That really doesn't look right, as the timer is quite low frequency. It's causing excessive wake ups, as all 4 CPUs wake up to handle the WQ, 3 of them directly go back to sleep waiting for the mutex to be released, then same thing happens for CPUs-1 until we rippled down to a single CPU.
Quoted 11 lines
Quoted 4 lines
All workqueues wake
up at the same time with all but one of the worker bouncing on the contended mutex.This is exactly what used to be done earlier, but it was designed badly. For example, for ondemand governor, each CPU had a struct
od_cpu_dbs_info_sand we only used theod_cpu_dbs_info_sfor the 'policy->cpu'. So, even if all CPUs had this structure (i.e. thetime_stampand the lock), we only used structure forpolicy->cpu.
Ok, so this patch isn't actually at fault. My apologies. It was just the first thing down the git history that catched my eye when looking into why the timer mutex is this badly contended.
I would say it's still worth fixing. Perhaps by not waking all the workqueues at the same time, but spreading the wake times out over a jiffy.
Quoted 5 lines
Quoted 2 lines
Also sharing the
time_stampcauses all but the first worker to acquire the mutex to not properly update their sampling information, I guess.Yeah, that's what we want per-policy.
That I still don't really understand. If we only sample a single CPU, why do we also wake all the others? Also if we serialize on the timer_mutex and only the first CPU to acquire it updates the sampling information, is it ok that essentially a random CPU will do the work?
Again, I didn't really take the time to understand the code in depth, hoping that you could provide the answers to those questions more easily. :)
Thanks,
Lucas
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
Viresh Kumar
maintainerRe: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
Quoted 2 lines · Lucas Stach
The
timer_mutexis one of the top contended locks already on a quad core system.
My new series (un-committed) should fix that to some level hopefully:
http://marc.info/?l=linux-pm&m=144612165211091&w=2
Quoted 5 lines
That really doesn't look right, as the timer is quite low frequency. It's causing excessive wake ups, as all 4 CPUs wake up to handle the WQ, 3 of them directly go back to sleep waiting for the mutex to be released, then same thing happens for CPUs-1 until we rippled down to a single CPU.
There is a reason why we need to do this on all CPUs today. The timers are deferrable by nature, as we shouldn't wake up a CPU to change it frequency. Now group of CPUs that change their DVFS state together, or that change their voltage/clock rails are part of the same cpufreq-policy. We need to take load of all the CPUs, that are part of the policy, while update the frequency of the group.
If we queue the timer on any one CPU, then that CPU can go into idle and the deferred timer will not fire. But the other CPUs of the policy can still be active and the frequency of the group wouldn't change with load.
Hope that answers the query related to timers on all CPUs.
Quoted 3 lines
I would say it's still worth fixing. Perhaps by not waking all the workqueues at the same time, but spreading the wake times out over a jiffy.
Maybe.
Signature · 1 line
viresh
Re: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
Quoted 26 lines · Viresh Kumar
Quoted 2 lines · Lucas Stach
The
timer_mutexis one of the top contended locks already on a quad core system.My new series (un-committed) should fix that to some level hopefully:
http://marc.info/?l=linux-pm&m=144612165211091&w=2
Quoted 5 lines
That really doesn't look right, as the timer is quite low frequency. It's causing excessive wake ups, as all 4 CPUs wake up to handle the WQ, 3 of them directly go back to sleep waiting for the mutex to be released, then same thing happens for CPUs-1 until we rippled down to a single CPU.
There is a reason why we need to do this on all CPUs today. The timers are deferrable by nature, as we shouldn't wake up a CPU to change it frequency. Now group of CPUs that change their DVFS state together, or that change their voltage/clock rails are part of the same cpufreq-policy. We need to take load of all the CPUs, that are part of the policy, while update the frequency of the group.
If we queue the timer on any one CPU, then that CPU can go into idle and the deferred timer will not fire. But the other CPUs of the policy can still be active and the frequency of the group wouldn't change with load.
Quoted 8 lines
Hope that answers the query related to timers on all CPUs.
Quoted 3 lines
I would say it's still worth fixing. Perhaps by not waking all the workqueues at the same time, but spreading the wake times out over a jiffy.
Maybe.
There's a separate thread where we proposed a fix to deferrable timers that are stored globally if they are not CPU bound. That way, even if one CPU is up, they get handled. But TGLX had some valid concerns with cache thrashing and impact on some network code. So, last I heard, he was going to rewrite and fixed the deferrable timer problem by having the "orphaned" (because CPU has gone idle) deferrable timers being adopted by other CPUs while the original CPU is idle.
Once that's fixed, we just need one timer per policy. Long story short, CPU freq is working around a poor API semantic of deferrable timers.
-Saravana
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
Quoted 9 lines · Saravana Kannan
There's a separate thread where we proposed a fix to deferrable timers that are stored globally if they are not CPU bound. That way, even if one CPU is up, they get handled. But TGLX had some valid concerns with cache thrashing and impact on some network code. So, last I heard, he was going to rewrite and fixed the deferrable timer problem by having the "orphaned" (because CPU has gone idle) deferrable timers being adopted by other CPUs while the original CPU is idle.
Quoted 3 lines
Once that's fixed, we just need one timer per policy. Long story short, CPU freq is working around a poor API semantic of deferrable timers.
There is another idea that I have.
Lets sacrifice idleness of CPU0 (which is already considered as housekeeping CPU in scheduler) to save us from all the complexity we have today.
Suppose we have 16 CPUs, with 4 CPUs per policy and hence 4 policies.
- Keep a single delayed-work (non-deferrable) per policy and queue them as:
queue_work_on(CPU0).
- This will work because any CPU can calculate the load of other CPUs, and there is no dependency on the local CPU.
- CPU0 will hence get interrupted, check if the policy->cpus are idle or not, and if not, update their frequency (perhaps with an IPI).
Not sure if this will be better performance wise though.
Signature · 1 line
viresh
Re: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
Quoted 17 lines
There is another idea that I have.
Lets sacrifice idleness of CPU0 (which is already considered as housekeeping CPU in scheduler) to save us from all the complexity we have today.
Suppose we have 16 CPUs, with 4 CPUs per policy and hence 4 policies.
- Keep a single delayed-work (non-deferrable) per policy and queue them as:
queue_work_on(CPU0).
- This will work because any CPU can calculate the load of other CPUs,
and there is no dependency on the local CPU.
- CPU0 will hence get interrupted, check if thepolicy->cpusare idle or not, and if not, update their frequency (perhaps with an IPI).
Quoted 2 lines
Not sure if this will be better performance wise though.
No it is not. In your example, everything could be okay with many ifs. I'm from the HPC field and would expect load imbalances because this approach does not scale with the number of frequency domains.
In 2011, AMD released Bulldozer servers with 4 sockets and each of these sockets had 2 dies with 4 modules. Each module has its own frequency domain, which sums up to 32 different domains. Putting all the load to CPU0 will be unfavorable for a balanced performance over all CPUS. This is worsened by the cache coherence protocol which might delay accesses to shared and foreign memory significantly.
And this had been in 2011. Today we have Haswell-EP quad socket servers with up to 72 frequency domains for all CPUs of all sockets put together. If you have an SGI Altix UV, you even have 256 sockets with 18 domains each, which sums up to 4608 domains.
I would expect architectures with more frequency domains in the future.
Robert
Signature · 10 lines
Dipl.-Inf. Robert Schoene Computer Scientist - R&D Energy Efficient Computing Technische Universitaet Dresden Center for Information Services and High Performance Computing Distributed and Data Intensive Computing 01062 Dresden Tel.: +49 (351) 463-42483 Fax : +49 (351) 463-37773 E-Mail: [email protected]
Re: [PATCH V2 06/10] cpufreq: governor: Keep single copy of information common to policy->cpus
Quoted 10 lines · Robert Schöne
Quoted 10 lines
There is another idea that I have.
Lets sacrifice idleness of CPU0 (which is already considered as housekeeping CPU in scheduler) to save us from all the complexity we have today.
Suppose we have 16 CPUs, with 4 CPUs per policy and hence 4 policies.
- Keep a single delayed-work (non-deferrable) per policy and queue them as:
This is a 100% no go. Non-deferrable is going to kill mobile idle power.
Quoted 7 lines
Quoted 7 lines
queue_work_on(CPU0).- This will work because any CPU can calculate the load of other CPUs,
and there is no dependency on the local CPU.
- CPU0 will hence get interrupted, check if thepolicy->cpusare idle or not, and if not, update their frequency (perhaps with an IPI).
Quoted 6 lines
Quoted 2 lines
Not sure if this will be better performance wise though.
No it is not. In your example, everything could be okay with many ifs. I'm from the HPC field and would expect load imbalances because this approach does not scale with the number of frequency domains.
Agree. I think that's why we had per policy timers (forced to be implemented with per CPU deferrable timers) in the first place.
-Saravana
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerPATCH v2 7/10 cpufreq: governor: split out common part of {cs|od}_dbs_timer()
In reply to Viresh KumarSome part of cs_dbs_timer() and od_dbs_timer() is exactly same and is unnecessarily duplicated.
Create the real work-handler in cpufreq_governor.c and put the common code in this routine (dbs_timer()).
Shouldn't make any functional change.
--- drivers/cpufreq/cpufreq_conservative.c | 27 +++++++----------------- drivers/cpufreq/cpufreq_governor.c | 38 ++++++++++++++++++++++++++++++---- drivers/cpufreq/cpufreq_governor.h | 10 ++++----- drivers/cpufreq/cpufreq_ondemand.c | 36 +++++++++++++------------------- 4 files changed, 60 insertions(+), 51 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +7 -20
@@ -102,28 +102,15 @@ static void cs_check_cpu(int cpu, unsigned int load) } } -static void cs_dbs_timer (struct work_struct *work) +static unsigned int cs_dbs_timer (struct cpu_dbs_info *cdbs, +struct dbs_data *dbs_data,bool modify_all) { -struct cs_cpu_dbs_info_s *dbs_info =container_of (work, - struct cs_cpu_dbs_info_s, cdbs.dwork.work); -struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy; -unsigned int cpu = policy->cpu; -struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu (cs_cpu_dbs_info, - cpu); -struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; -int delay =delay_for_sampling_rate (cs_tuners->sampling_rate); -bool modify_all = true; - -mutex_lock (&core_dbs_info->cdbs.ccdbs->timer_mutex); -if (!need_load_eval (core_dbs_info->cdbs.ccdbs, - cs_tuners->sampling_rate)) - modify_all = false; -else -dbs_check_cpu (dbs_data, cpu); - -gov_queue_work (dbs_data, policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.ccdbs->timer_mutex); + +if (modify_all) +dbs_check_cpu (dbs_data, cdbs->ccdbs->policy->cpu); + +return delay_for_sampling_rate (cs_tuners->sampling_rate); } static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
drivers/cpufreq/cpufreq_governor.c +34 -4
@@ -207,8 +207,8 @@ static inline void gov_cancel_work(struct dbs_data *dbs_data, }/* Will return if we need to evaluate cpu load again or not */ -bool need_load_eval (struct cpu_common_dbs_info *ccdbs, -unsigned int sampling_rate) +static bool need_load_eval (struct cpu_common_dbs_info *ccdbs, +unsigned int sampling_rate) {if (policy_is_shared (ccdbs->policy)) {ktime_t time_now =ktime_get ();
@@ -223,7 +223,37 @@ bool need_load_eval(struct cpu_common_dbs_info *ccdbs,return true; } -EXPORT_SYMBOL_GPL (need_load_eval); + +static void dbs_timer (struct work_struct *work) +{ + struct cpu_dbs_info *cdbs = container_of(work, struct cpu_dbs_info, + dwork.work); +struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs; +struct cpufreq_policy *policy = ccdbs->policy; +struct dbs_data *dbs_data = policy->governor_data; +unsigned int sampling_rate, delay; +bool modify_all = true; + +mutex_lock (&ccdbs->timer_mutex); + +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; + } + +if (!need_load_eval (cdbs->ccdbs, sampling_rate)) + modify_all = false; + + delay = dbs_data->cdata->gov_dbs_timer (cdbs, dbs_data, modify_all); +gov_queue_work (dbs_data, policy, delay, modify_all); + +mutex_unlock (&ccdbs->timer_mutex); +}static void set_sampling_rate (struct dbs_data *dbs_data,unsigned int sampling_rate)
@@ -411,7 +441,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,if (ignore_nice) j_cdbs->prev_cpu_nice =kcpustat_cpu (j).cpustat[CPUTIME_NICE ]; -INIT_DEFERRABLE_WORK (&j_cdbs->dwork, cdata->gov_dbs_timer); +INIT_DEFERRABLE_WORK (&j_cdbs->dwork, dbs_timer); }if (cdata->governor ==GOV_CONSERVATIVE ) {
drivers/cpufreq/cpufreq_governor.h +5 -5
@@ -132,8 +132,8 @@ static void *get_cpu_dbs_info_s(int cpu) \struct cpu_common_dbs_info {struct cpufreq_policy *policy;/* -* percpu mutex that serializes governor limit change with gov_dbs_timer -* invocation. We do not want gov_dbs_timer to run when user is changing +* percpu mutex that serializes governor limit change with dbs_timer +* invocation. We do not want dbs_timer to run when user is changing * the governor or limits. */ struct mutex timer_mutex;
@@ -210,7 +210,9 @@ struct common_dbs_data {struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);void *(*get_cpu_dbs_info_s)(int cpu); -void (*gov_dbs_timer)(struct work_struct *work); +unsigned int (*gov_dbs_timer)(struct cpu_dbs_info *cdbs, +struct dbs_data *dbs_data, +bool modify_all);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);
@@ -269,8 +271,6 @@ static ssize_t show_sampling_rate_min_gov_pol \extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct dbs_data *dbs_data,int cpu); -bool need_load_eval (struct cpu_common_dbs_info *ccdbs, -unsigned int sampling_rate);int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event); void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
drivers/cpufreq/cpufreq_ondemand.c +14 -22
@@ -191,48 +191,40 @@ static void od_check_cpu(int cpu, unsigned int load) } } -static void od_dbs_timer (struct work_struct *work) +static unsigned int od_dbs_timer (struct cpu_dbs_info *cdbs, +struct dbs_data *dbs_data,bool modify_all) { -struct od_cpu_dbs_info_s *dbs_info = - container_of(work, struct od_cpu_dbs_info_s, cdbs.dwork.work); -struct cpufreq_policy *policy = dbs_info->cdbs.ccdbs->policy; +struct cpufreq_policy *policy = cdbs->ccdbs->policy;unsigned int cpu = policy->cpu; -struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu (od_cpu_dbs_info, +struct od_cpu_dbs_info_s *dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners; -int delay =0 , sample_type = core_dbs_info->sample_type; -bool modify_all = true; +int delay =0 , sample_type = dbs_info->sample_type; -mutex_lock (&core_dbs_info->cdbs.ccdbs->timer_mutex); -if (!need_load_eval (core_dbs_info->cdbs.ccdbs, - od_tuners->sampling_rate)) { - modify_all = false; +if (!modify_all) goto max_delay; - }/* Common NORMAL_SAMPLE setup */ - core_dbs_info->sample_type =OD_NORMAL_SAMPLE ; + dbs_info->sample_type =OD_NORMAL_SAMPLE ;if (sample_type ==OD_SUB_SAMPLE ) { - delay = core_dbs_info->freq_lo_jiffies; -__cpufreq_driver_target (policy, core_dbs_info->freq_lo, + delay = dbs_info->freq_lo_jiffies; +__cpufreq_driver_target (policy, dbs_info->freq_lo,CPUFREQ_RELATION_H ); }else {dbs_check_cpu (dbs_data, cpu); -if (core_dbs_info->freq_lo) { +if (dbs_info->freq_lo) {/* Setup timer for SUB_SAMPLE */ - core_dbs_info->sample_type =OD_SUB_SAMPLE ; - delay = core_dbs_info->freq_hi_jiffies; + dbs_info->sample_type =OD_SUB_SAMPLE ; + delay = dbs_info->freq_hi_jiffies; } } max_delay:if (!delay) delay =delay_for_sampling_rate (od_tuners->sampling_rate - * core_dbs_info->rate_mult); + * dbs_info->rate_mult); -gov_queue_work (dbs_data, policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.ccdbs->timer_mutex); +return delay; }/************************** sysfs interface ************************/
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 8/10 cpufreq: governor: Avoid invalid states with additional checks
In reply to Viresh KumarThere can be races where the request has come to a wrong state. For example INIT followed by STOP (instead of START) or START followed by EXIT (instead of STOP).
Tackle these races by comparing making sure the state-machine never gets into any invalid state. And return error if an invalid state-transition is requested.
--- drivers/cpufreq/cpufreq_governor.c | 46 +++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 11 deletions(-)
drivers/cpufreq/cpufreq_governor.c +35 -11
@@ -305,6 +305,10 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,unsigned int latency;int ret; +/* State should be equivalent to EXIT */ +if (policy->governor_data) +return -EBUSY ; +if (dbs_data) {if (WARN_ON (have_governor_per_policy ()))return -EINVAL ;
@@ -375,10 +379,15 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,return ret; } -static void cpufreq_governor_exit (struct cpufreq_policy *policy, -struct dbs_data *dbs_data) +static int cpufreq_governor_exit (struct cpufreq_policy *policy, +struct dbs_data *dbs_data) {struct common_dbs_data *cdata = dbs_data->cdata; +struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (policy->cpu); + +/* State should be equivalent to INIT */ +if (!cdbs->ccdbs || cdbs->ccdbs->policy) +return -EBUSY ; policy->governor_data =NULL ;if (!--dbs_data->usage_count) {
@@ -395,6 +404,7 @@ static void cpufreq_governor_exit(struct cpufreq_policy *policy, }free_ccdbs (policy, cdata); +return 0 ; } static int cpufreq_governor_start(struct cpufreq_policy *policy,
@@ -409,6 +419,10 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,if (!policy->cur)return -EINVAL ; +/* State should be equivalent to INIT */ +if (!ccdbs || ccdbs->policy) +return -EBUSY ; +if (cdata->governor ==GOV_CONSERVATIVE ) {struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
@@ -465,14 +479,18 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,return 0 ; } -static void cpufreq_governor_stop (struct cpufreq_policy *policy, -struct dbs_data *dbs_data) +static int cpufreq_governor_stop (struct cpufreq_policy *policy, +struct dbs_data *dbs_data) {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu);struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs; +/* State should be equivalent to START */ +if (!ccdbs || !ccdbs->policy) +return -EBUSY ; +gov_cancel_work (dbs_data, policy);if (cdata->governor ==GOV_CONSERVATIVE ) {
@@ -484,17 +502,19 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy, ccdbs->policy =NULL ;mutex_destroy (&ccdbs->timer_mutex); +return 0 ; } -static void cpufreq_governor_limits (struct cpufreq_policy *policy, -struct dbs_data *dbs_data) +static int cpufreq_governor_limits (struct cpufreq_policy *policy, +struct dbs_data *dbs_data) {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); +/* State should be equivalent to START */ if (!cdbs->ccdbs || !cdbs->ccdbs->policy) -return ; +return -EBUSY ;mutex_lock (&cdbs->ccdbs->timer_mutex);if (policy->max < cdbs->ccdbs->policy->cur)
@@ -505,13 +525,15 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy, CPUFREQ_RELATION_L);dbs_check_cpu (dbs_data, cpu);mutex_unlock (&cdbs->ccdbs->timer_mutex); + +return 0 ; }int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event) {struct dbs_data *dbs_data; -int ret =0 ; +int ret;/* Lock governor to block concurrent initialization of governor */ mutex_lock (&cdata->mutex);
@@ -531,17 +553,19 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, ret =cpufreq_governor_init (policy, dbs_data, cdata);break ;case CPUFREQ_GOV_POLICY_EXIT : -cpufreq_governor_exit (policy, dbs_data); + ret =cpufreq_governor_exit (policy, dbs_data);break ;case CPUFREQ_GOV_START : ret =cpufreq_governor_start (policy, dbs_data);break ;case CPUFREQ_GOV_STOP : -cpufreq_governor_stop (policy, dbs_data); + ret =cpufreq_governor_stop (policy, dbs_data);break ;case CPUFREQ_GOV_LIMITS : -cpufreq_governor_limits (policy, dbs_data); + ret =cpufreq_governor_limits (policy, dbs_data);break ; +default : + ret = -EINVAL ; } unlock:
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Re: [PATCH V2 08/10] cpufreq: governor: Avoid invalid states with additional checks
Quoted 9 lines · Viresh Kumar
There can be races where the request has come to a wrong state. For example INIT followed by STOP (instead of START) or START followed by EXIT (instead of STOP).
Tackle these races by comparing making sure the state-machine never gets into any invalid state. And return error if an invalid state-transition is requested.
Signed-off-by Viresh Kumar <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 9/10 cpufreq: governor: Don't WARN on invalid states
In reply to Viresh KumarWith previous commit, governors have started to return errors on invalid state-transition requests. We already have a WARN for an invalid state-transition request in cpufreq_governor_dbs(). This does trigger today, as the sequence of events isn't guaranteed by cpufreq core.
Lets stop warning on that for now, and make sure we don't enter an invalid state.
--- drivers/cpufreq/cpufreq_governor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/cpufreq_governor.c +1 -1
@@ -543,7 +543,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,else dbs_data = cdata->gdbs_data; -if (WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT ))) { +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { ret = -EINVAL ; goto unlock; }
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerPATCH v2 10/10 cpufreq: propagate errors returned from __cpufreq_governor()
In reply to Viresh KumarReturn codes aren't honored properly in cpufreq_set_policy(). This can lead to two problems:
- wrong errors propagated to sysfs
- we try to do next state-change even if the previous one failed
cpufreq_governor_dbs() now returns proper errors on all invalid state-transition requests and this code should honor that.
--- drivers/cpufreq/cpufreq.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq.c +13 -9
@@ -2284,16 +2284,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor;/* end old governor */ if (old_gov) { -__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); -up_write (&policy->rwsem); -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); -down_write (&policy->rwsem); +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ))) { +up_write (&policy->rwsem); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); +down_write (&policy->rwsem); + } + +if (ret) +return ret; }/* start new governor */ policy->governor = new_policy->governor; -if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) { -if (!__cpufreq_governor (policy,CPUFREQ_GOV_START )) +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ))) { +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ))) goto out;up_write (&policy->rwsem);
@@ -2305,11 +2309,11 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,pr_debug ("starting governor %s failed\n" , policy->governor->name);if (old_gov) { policy->governor = old_gov; -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ); -__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) +__cpufreq_governor (policy,CPUFREQ_GOV_START ); } -return -EINVAL ; +return ret; out:pr_debug ("governor: change or update limits\n" );
Signature · 4 lines
2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Re: [PATCH V2 10/10] cpufreq: propagate errors returned from __cpufreq_governor()
Quoted 9 lines · Viresh Kumar
Return codes aren't honored properly in
cpufreq_set_policy(). This can lead to two problems:
- wrong errors propagated to sysfs
- we try to do next state-change even if the previous one failed
cpufreq_governor_dbs()now returns proper errors on all invalid state-transition requests and this code should honor that.Signed-off-by Viresh Kumar <[email protected]>
Minor points below.
Quoted 14 lines
--- drivers/cpufreq/cpufreq.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)drivers/cpufreq/cpufreq.c +0
-2@@ -2284,16 +2284,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor;/* end old governor */ if (old_gov) { -__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); -up_write (&policy->rwsem);
drivers/cpufreq/cpufreq.c static int cpufreq_set_policy(struct cpufreq_policy *policy, · line 2294
Quoted 10 lines
drivers/cpufreq/cpufreq.c +8
-2-__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); -down_write (&policy->rwsem); +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ))) { +up_write (&policy->rwsem); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); +down_write (&policy->rwsem); + } + +if (ret) +return ret;
Some debug prints help here? Like "Failed to stop govenor".
Quoted 12 lines
drivers/cpufreq/cpufreq.c +2
-2}/* start new governor */ policy->governor = new_policy->governor; -if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) { -if (!__cpufreq_governor (policy,CPUFREQ_GOV_START )) +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ))) { +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ))) goto out;up_write (&policy->rwsem);
drivers/cpufreq/cpufreq.c static int cpufreq_set_policy(struct cpufreq_policy *policy, · line 2313
Quoted 7 lines
drivers/cpufreq/cpufreq.c +2
-2@@ -2305,11 +2309,11 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,pr_debug ("starting governor %s failed\n" , policy->governor->name);if (old_gov) { policy->governor = old_gov; -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ); -__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) +__cpufreq_governor (policy,CPUFREQ_GOV_START );
If INIT itself fails, we need to set policy->governor to NULL. I included this in the testing of the patchset.
Regards
Preeti U Murthy
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerRe: [PATCH V2 10/10] cpufreq: propagate errors returned from __cpufreq_governor()
Quoted 2 lines · Preeti U Murthy
If INIT itself fails, we need to set
policy->governorto NULL. I included this in the testing of the patchset.
Diff from earlier patch:
drivers/cpufreq/cpufreq.c +16 -5
@@ -2284,14 +2284,23 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor;/* end old governor */ if (old_gov) { -if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ))) { + ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); +if (ret) { +/* This can happen due to race with other operations */ +pr_debug ("%s: Failed to Stop Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + }else {up_write (&policy->rwsem); ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT );down_write (&policy->rwsem); - } -if (ret) -return ret; +if (ret) { +pr_err ("%s: Failed to Exit Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + } + } }/* start new governor */
@@ -2309,7 +2318,9 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,pr_debug ("starting governor %s failed\n" , policy->governor->name);if (old_gov) { policy->governor = old_gov; -if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) +if (__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) + policy->governor =NULL ; +else __cpufreq_governor (policy,CPUFREQ_GOV_START ); }
New-patch:
--------------8<-------------------
From: Viresh Kumar <[email protected]>
Date: Thu, 11 Sep 2014 10:50:48 +0530
Subject: [PATCH] cpufreq: propagate errors returned from __cpufreq_governor()
Return codes aren't honored properly in cpufreq_set_policy(). This can lead to two problems:
- wrong errors propagated to sysfs
- we try to do next state-change even if the previous one failed
cpufreq_governor_dbs() now returns proper errors on all invalid state-transition requests and this code should honor that.
--- drivers/cpufreq/cpufreq.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq.c +24 -9
@@ -2284,16 +2284,29 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor;/* end old governor */ if (old_gov) { -__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); -up_write (&policy->rwsem); -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); -down_write (&policy->rwsem); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); +if (ret) { +/* This can happen due to race with other operations */ +pr_debug ("%s: Failed to Stop Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + }else { +up_write (&policy->rwsem); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); +down_write (&policy->rwsem); + +if (ret) { +pr_err ("%s: Failed to Exit Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + } + } }/* start new governor */ policy->governor = new_policy->governor; -if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) { -if (!__cpufreq_governor (policy,CPUFREQ_GOV_START )) +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ))) { +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ))) goto out;up_write (&policy->rwsem);
@@ -2305,11 +2318,13 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,pr_debug ("starting governor %s failed\n" , policy->governor->name);if (old_gov) { policy->governor = old_gov; -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ); -__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) + policy->governor =NULL ; +else +__cpufreq_governor (policy,CPUFREQ_GOV_START ); } -return -EINVAL ; +return ret; out:pr_debug ("governor: change or update limits\n" );
Signature · 3 lines
viresh -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Re: [PATCH V2 10/10] cpufreq: propagate errors returned from __cpufreq_governor()
Quoted 120 lines · Viresh Kumar
Quoted 2 lines · Preeti U Murthy
If INIT itself fails, we need to set
policy->governorto NULL. I included this in the testing of the patchset.Diff from earlier patch:
drivers/cpufreq/cpufreq.c +16
-5@@ -2284,14 +2284,23 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor;/* end old governor */ if (old_gov) { -if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ))) { + ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); +if (ret) { +/* This can happen due to race with other operations */ +pr_debug ("%s: Failed to Stop Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + }else {up_write (&policy->rwsem); ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT );down_write (&policy->rwsem); - } -if (ret) -return ret; +if (ret) { +pr_err ("%s: Failed to Exit Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + } + } }/* start new governor */ @@ -2309,7 +2318,9 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,pr_debug ("starting governor %s failed\n" , policy->governor->name);if (old_gov) { policy->governor = old_gov; -if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) +if (__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) + policy->governor =NULL ; +else __cpufreq_governor (policy,CPUFREQ_GOV_START ); }New-patch:
--------------8<-------------------
From: Viresh Kumar <[email protected]>
Date: Thu, 11 Sep 2014 10:50:48 +0530
Subject: [PATCH] cpufreq: propagate errors returned from__cpufreq_governor()Return codes aren't honored properly in
cpufreq_set_policy(). This can lead to two problems:
- wrong errors propagated to sysfs
- we try to do next state-change even if the previous one failed
cpufreq_governor_dbs()now returns proper errors on all invalid state-transition requests and this code should honor that.Reviewed-and-Tested-by Preeti U Murthy <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)drivers/cpufreq/cpufreq.c +20
-8@@ -2284,16 +2284,29 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor;/* end old governor */ if (old_gov) { -__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); -up_write (&policy->rwsem); -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); -down_write (&policy->rwsem); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); +if (ret) { +/* This can happen due to race with other operations */ +pr_debug ("%s: Failed to Stop Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + }else { +up_write (&policy->rwsem); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); +down_write (&policy->rwsem); + +if (ret) { +pr_err ("%s: Failed to Exit Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + } + } }/* start new governor */ policy->governor = new_policy->governor; -if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) { -if (!__cpufreq_governor (policy,CPUFREQ_GOV_START )) +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ))) { +if (!(ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ))) goto out;up_write (&policy->rwsem);@@ -2305,11 +2318,13 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,pr_debug ("starting governor %s failed\n" , policy->governor->name);if (old_gov) { policy->governor = old_gov; -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ); -__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ))
drivers/cpufreq/cpufreq.c static int cpufreq_set_policy(struct cpufreq_policy *policy, · line 2330
Quoted 10 lines
drivers/cpufreq/cpufreq.c +4
-1+ policy->governor =NULL ; + else +__cpufreq_governor (policy ,CPUFREQ_GOV_START ); } -return -EINVAL ; +return ret; out:pr_debug ("governor: change or update limits\n" );
There are some checkpatch errors on this patch.
Regards
Preeti U Murthy
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerRe: [PATCH V2 10/10] cpufreq: propagate errors returned from __cpufreq_governor()
Quoted 1 line · Preeti U Murthy
There are some checkpatch errors on this patch.
--------------8<------------------
From: Viresh Kumar <[email protected]>
Date: Thu, 11 Sep 2014 10:50:48 +0530
Subject: [PATCH] cpufreq: propagate errors returned from __cpufreq_governor()
Return codes aren't honored properly in cpufreq_set_policy(). This can lead to two problems:
- wrong errors propagated to sysfs
- we try to do next state-change even if the previous one failed
cpufreq_governor_dbs() now returns proper errors on all invalid state-transition requests and this code should honor that.
--- drivers/cpufreq/cpufreq.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
drivers/cpufreq/cpufreq.c +24 -7
@@ -2284,16 +2284,31 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor;/* end old governor */ if (old_gov) { -__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); +if (ret) { +/* This can happen due to race with other operations */ +pr_debug ("%s: Failed to Stop Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + } +up_write (&policy->rwsem); -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT );down_write (&policy->rwsem); + +if (ret) { +pr_err ("%s: Failed to Exit Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + } }/* start new governor */ policy->governor = new_policy->governor; -if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) { -if (!__cpufreq_governor (policy,CPUFREQ_GOV_START )) + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ); +if (!ret) { + ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (!ret) goto out;up_write (&policy->rwsem);
@@ -2305,11 +2320,13 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,pr_debug ("starting governor %s failed\n" , policy->governor->name);if (old_gov) { policy->governor = old_gov; -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ); -__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) + policy->governor =NULL ; +else +__cpufreq_governor (policy,CPUFREQ_GOV_START ); } -return -EINVAL ; +return ret; out:pr_debug ("governor: change or update limits\n" );
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Re: [PATCH V2 10/10] cpufreq: propagate errors returned from __cpufreq_governor()
In reply to Viresh KumarQuoted 9 lines · Viresh Kumar
Return codes aren't honored properly in
cpufreq_set_policy(). This can lead to two problems:
- wrong errors propagated to sysfs
- we try to do next state-change even if the previous one failed
cpufreq_governor_dbs()now returns proper errors on all invalid state-transition requests and this code should honor that.Signed-off-by Viresh Kumar <[email protected]>
Incorporating the nit in my previous reply to this patch would mean,
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Quoted 6 lines · Viresh Kumar
Hi Rafael/Preeti,
This is another attempt to fix the crashes reported by Preeti. They work quite well for me now, and I hope they would work well for Preeti as well :)
Quoted 7 lines
So, patches [1-7,9] are already Reviewed by Preeti.
The first 5 patches are minor cleanups, 6th & 7th try to optimize few things to make code less complex.
Patches 8-10 actually solve (or try to solve :)) the synchronization problems, or the crashes I was getting.
This series has been tested on powerpc, by running governor switching in parallel and another test where hotplug, governor switching were run in parallel.So,
Quoted 27 lines
V1->V2:
- 7/11 is dropped and only 8/11 is updated, which is 8/10 now.
- Avoid taking the same mutex in bothcpufreq_governor_dbs()and work-handler as that has given us some locdeps, classic ABBA stuff.
- And sotimer_mutex, responsible for synchronizing governor work-handlers is kept as is.
- Later patches are almost same with minor updates.
- Invalid state-transitions are sensed better now with improved checks.
- I have run enough tests on my exynos dual core board and failed to crash at all. Not that I wanted to crash :)Rebased over pm/bleeeding-edge.
Viresh Kumar (10):
cpufreq: governor: Name delayed-work as dwork
cpufreq: governor: Drop unused field 'cpu'
cpufreq: governor: Rename 'cpu_dbs_common_info' to 'cpu_dbs_info'
cpufreq: governor: name pointer tocpu_dbs_infoas 'cdbs'
cpufreq: governor: renamecur_policyas policy
cpufreq: governor: Keep single copy of information common topolicy->cpuscpufreq: governor: split out common part of {cs|od}_dbs_timer()
cpufreq: governor: Avoid invalid states with additional checks
cpufreq: governor: Don't WARN on invalid states
cpufreq: propagate errors returned from__cpufreq_governor()
Quoted 7 lines
drivers/cpufreq/cpufreq.c | 22 ++-- drivers/cpufreq/cpufreq_conservative.c | 25 ++--- drivers/cpufreq/cpufreq_governor.c | 196 ++++++++++++++++++++++++--------- drivers/cpufreq/cpufreq_governor.h | 40 ++++--- drivers/cpufreq/cpufreq_ondemand.c | 67 ++++++----- 5 files changed, 220 insertions(+), 130 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Quoted 40 lines · Viresh Kumar
Hi Rafael/Preeti,
This is another attempt to fix the crashes reported by Preeti. They work quite well for me now, and I hope they would work well for Preeti as well :)
So, patches [1-7,9] are already Reviewed by Preeti.
The first 5 patches are minor cleanups, 6th & 7th try to optimize few things to make code less complex.
Patches 8-10 actually solve (or try to solve :)) the synchronization problems, or the crashes I was getting.
V1->V2:
- 7/11 is dropped and only 8/11 is updated, which is 8/10 now.
- Avoid taking the same mutex in bothcpufreq_governor_dbs()and work-handler as that has given us some locdeps, classic ABBA stuff.
- And sotimer_mutex, responsible for synchronizing governor work-handlers is kept as is.
- Later patches are almost same with minor updates.
- Invalid state-transitions are sensed better now with improved checks.
- I have run enough tests on my exynos dual core board and failed to crash at all. Not that I wanted to crash :)Rebased over pm/bleeeding-edge.
Viresh Kumar (10):
cpufreq: governor: Name delayed-work as dwork
cpufreq: governor: Drop unused field 'cpu'
cpufreq: governor: Rename 'cpu_dbs_common_info' to 'cpu_dbs_info'
cpufreq: governor: name pointer tocpu_dbs_infoas 'cdbs'
cpufreq: governor: renamecur_policyas policy
cpufreq: governor: Keep single copy of information common topolicy->cpuscpufreq: governor: split out common part of {cs|od}_dbs_timer()
cpufreq: governor: Avoid invalid states with additional checks
cpufreq: governor: Don't WARN on invalid states
cpufreq: propagate errors returned from__cpufreq_governor()
Quoted 6 lines
drivers/cpufreq/cpufreq.c | 22 ++-- drivers/cpufreq/cpufreq_conservative.c | 25 ++--- drivers/cpufreq/cpufreq_governor.c | 196 ++++++++++++++++++++++++--------- drivers/cpufreq/cpufreq_governor.h | 40 ++++--- drivers/cpufreq/cpufreq_ondemand.c | 67 ++++++----- 5 files changed, 220 insertions(+), 130 deletions(-)
So as I said before, I'm not regarding this as 4.2 material.
The fact that you manage to send your stuff before the merge window opens doesn't automatically mean that it has to go in during that merge window.
Signature · 4 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Viresh Kumar
maintainerQuoted 1 line · Rafael J. Wysocki
So as I said before, I'm not regarding this as 4.2 material.
Okay.
Quoted 2 lines
The fact that you manage to send your stuff before the merge window opens doesn't automatically mean that it has to go in during that merge window.
Yeah, that wasn't the intention. There were few reasons for me to hurry up:
- It was related to crashes that we are aware of since few kernel releases now. And these aren't new features, but bug fixes, ofcourse not for bugs that are introduced in 4.2 material :)
- I wanted Preeti to test this stuff, as she wouldn't be working on this stuff anymore after this month.
Anyway, its fine to me even if this goes in the next cycle.
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
Quoted 25 lines · Viresh Kumar
Hi Rafael/Preeti,
This is another attempt to fix the crashes reported by Preeti. They work quite well for me now, and I hope they would work well for Preeti as well :)
So, patches [1-7,9] are already Reviewed by Preeti.
The first 5 patches are minor cleanups, 6th & 7th try to optimize few things to make code less complex.
Patches 8-10 actually solve (or try to solve :)) the synchronization problems, or the crashes I was getting.
V1->V2:
- 7/11 is dropped and only 8/11 is updated, which is 8/10 now.
- Avoid taking the same mutex in bothcpufreq_governor_dbs()and work-handler as that has given us some locdeps, classic ABBA stuff.
- And sotimer_mutex, responsible for synchronizing governor work-handlers is kept as is.
- Later patches are almost same with minor updates.
- Invalid state-transitions are sensed better now with improved checks.
- I have run enough tests on my exynos dual core board and failed to crash at all. Not that I wanted to crash :)
Quoted 8 lines
Rebased over pm/bleeeding-edge.
Viresh Kumar (10):
cpufreq: governor: Name delayed-work as dwork
cpufreq: governor: Drop unused field 'cpu'
cpufreq: governor: Rename 'cpu_dbs_common_info' to 'cpu_dbs_info'
cpufreq: governor: name pointer tocpu_dbs_infoas 'cdbs'
cpufreq: governor: renamecur_policyas policy
The above are all fine by me, so I'm queuing them up for 4.3.
The one below needs a small update in my view which will propagate to the next ones.
Quoted 6 lines
cpufreq: governor: Keep single copy of information common to
policy->cpuscpufreq: governor: split out common part of {cs|od}_dbs_timer()
cpufreq: governor: Avoid invalid states with additional checks
cpufreq: governor: Don't WARN on invalid states
cpufreq: propagate errors returned from__cpufreq_governor()
Thanks,
Rafael
Viresh Kumar
maintainerv3 cover cpufreq: governor: Avoid invalid state-transitions
Hi Rafael,
These are rest of the patches after replacing 'ccdbs' with 'shared' as you suggested.
Viresh Kumar (5):
cpufreq: governor: Keep single copy of information common to
policy->cpus
cpufreq: governor: split out common part of {cs|od}_dbs_timer()
cpufreq: governor: Avoid invalid states with additional checks
cpufreq: governor: Don't WARN on invalid states
cpufreq: propagate errors returned from __cpufreq_governor()
drivers/cpufreq/cpufreq.c | 31 ++++-- drivers/cpufreq/cpufreq_conservative.c | 25 ++--- drivers/cpufreq/cpufreq_governor.c | 174 ++++++++++++++++++++++++++------- drivers/cpufreq/cpufreq_governor.h | 26 +++-- drivers/cpufreq/cpufreq_ondemand.c | 58 +++++------ 5 files changed, 210 insertions(+), 104 deletions(-)
Signature · 1 line
2.4.0
Viresh Kumar
maintainerPATCH v3 1/5 cpufreq: governor: Keep single copy of information common to policy->cpus
Some information is common to all CPUs belonging to a policy, but are kept on per-cpu basis. Lets keep that in another structure common to all policy->cpus. That will make updates/reads to that less complex and less error prone.
The memory for cpu_common_dbs_info is allocated/freed at INIT/EXIT, so that it we don't reallocate it for STOP/START sequence. It will be also be used (in next patch) while the governor is stopped and so must not be freed that early.
--- drivers/cpufreq/cpufreq_conservative.c | 18 ++++--- drivers/cpufreq/cpufreq_governor.c | 92 +++++++++++++++++++++++++--------- drivers/cpufreq/cpufreq_governor.h | 24 +++++---- drivers/cpufreq/cpufreq_ondemand.c | 38 +++++++------- 4 files changed, 114 insertions(+), 58 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +10 -8
@@ -47,7 +47,7 @@ static inline unsigned int get_freq_target(struct cs_dbs_tuners *cs_tuners,static void cs_check_cpu (int cpu,unsigned int load) {struct cs_cpu_dbs_info_s *dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.policy; +struct cpufreq_policy *policy = dbs_info->cdbs.shared->policy;struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
@@ -106,22 +106,24 @@ static void cs_dbs_timer(struct work_struct *work) {struct cs_cpu_dbs_info_s *dbs_info =container_of (work, struct cs_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.policy->cpu; +struct cpufreq_policy *policy = dbs_info->cdbs.shared->policy; +unsigned int cpu = policy->cpu;struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu (cs_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data; +struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;int delay =delay_for_sampling_rate (cs_tuners->sampling_rate);bool modify_all = true; -mutex_lock (&core_dbs_info->cdbs.timer_mutex); -if (!need_load_eval (&core_dbs_info->cdbs, cs_tuners->sampling_rate)) +mutex_lock (&core_dbs_info->cdbs.shared->timer_mutex); +if (!need_load_eval (core_dbs_info->cdbs.shared, + cs_tuners->sampling_rate)) modify_all = false;else dbs_check_cpu (dbs_data, cpu); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.timer_mutex); +gov_queue_work (dbs_data, policy, delay, modify_all); +mutex_unlock (&core_dbs_info->cdbs.shared->timer_mutex); } static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
@@ -135,7 +137,7 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,if (!dbs_info->enable)return 0 ; - policy = dbs_info->cdbs.policy; + policy = dbs_info->cdbs.shared->policy;/* * we only care if our internally tracked freq moves outside the 'valid'
drivers/cpufreq/cpufreq_governor.c +68 -24
203 lines not on this page · raw message
drivers/cpufreq/cpufreq_governor.h +15 -9
@@ -128,6 +128,18 @@ static void *get_cpu_dbs_info_s(int cpu) \* cs_*: Conservative governor */ +/* Common to all CPUs of a policy */ +struct cpu_common_dbs_info { +struct cpufreq_policy *policy; +/* +* percpu mutex that serializes governor limit change with gov_dbs_timer +* invocation. We do not want gov_dbs_timer to run when user is changing +* the governor or limits. +*/ +struct mutex timer_mutex; +ktime_t time_stamp; +}; +/* Per cpu structures */ struct cpu_dbs_info {u64 prev_cpu_idle;
@@ -140,15 +152,8 @@ struct cpu_dbs_info {* wake-up from idle. */ unsigned int prev_load; -struct cpufreq_policy *policy;struct delayed_work dwork; -/* -* percpu mutex that serializes governor limit change with gov_dbs_timer -* invocation. We do not want gov_dbs_timer to run when user is changing -* the governor or limits. -*/ -struct mutex timer_mutex; -ktime_t time_stamp; +struct cpu_common_dbs_info *shared; };struct od_cpu_dbs_info_s {
@@ -264,7 +269,8 @@ static ssize_t show_sampling_rate_min_gov_pol \extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct dbs_data *dbs_data,int cpu); -bool need_load_eval (struct cpu_dbs_info *cdbs,unsigned int sampling_rate); +bool need_load_eval (struct cpu_common_dbs_info *shared, +unsigned int sampling_rate);int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event); void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
drivers/cpufreq/cpufreq_ondemand.c +21 -17
@@ -155,7 +155,7 @@ static void dbs_freq_increase(struct cpufreq_policy *policy, unsigned int freq)static void od_check_cpu (int cpu,unsigned int load) {struct od_cpu_dbs_info_s *dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct cpufreq_policy *policy = dbs_info->cdbs.policy; +struct cpufreq_policy *policy = dbs_info->cdbs.shared->policy;struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners;
@@ -195,16 +195,18 @@ static void od_dbs_timer(struct work_struct *work) {struct od_cpu_dbs_info_s *dbs_info = container_of(work, struct od_cpu_dbs_info_s, cdbs.dwork.work); -unsigned int cpu = dbs_info->cdbs.policy->cpu; +struct cpufreq_policy *policy = dbs_info->cdbs.shared->policy; +unsigned int cpu = policy->cpu;struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = dbs_info->cdbs.policy->governor_data; +struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners;int delay =0 , sample_type = core_dbs_info->sample_type;bool modify_all = true; -mutex_lock (&core_dbs_info->cdbs.timer_mutex); -if (!need_load_eval (&core_dbs_info->cdbs, od_tuners->sampling_rate)) { +mutex_lock (&core_dbs_info->cdbs.shared->timer_mutex); +if (!need_load_eval (core_dbs_info->cdbs.shared, + od_tuners->sampling_rate)) { modify_all = false; goto max_delay; }
@@ -213,8 +215,7 @@ static void od_dbs_timer(struct work_struct *work) core_dbs_info->sample_type =OD_NORMAL_SAMPLE ;if (sample_type ==OD_SUB_SAMPLE ) { delay = core_dbs_info->freq_lo_jiffies; -__cpufreq_driver_target (core_dbs_info->cdbs.policy, - core_dbs_info->freq_lo, +__cpufreq_driver_target (policy, core_dbs_info->freq_lo,CPUFREQ_RELATION_H ); }else {dbs_check_cpu (dbs_data, cpu);
@@ -230,8 +231,8 @@ static void od_dbs_timer(struct work_struct *work) delay =delay_for_sampling_rate (od_tuners->sampling_rate * core_dbs_info->rate_mult); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.timer_mutex); +gov_queue_work (dbs_data, policy, delay, modify_all); +mutex_unlock (&core_dbs_info->cdbs.shared->timer_mutex); }/************************** sysfs interface ************************/
@@ -274,10 +275,10 @@ static void update_sampling_rate(struct dbs_data *dbs_data, dbs_info = &per_cpu (od_cpu_dbs_info, cpu);cpufreq_cpu_put (policy); -mutex_lock (&dbs_info->cdbs.timer_mutex); +mutex_lock (&dbs_info->cdbs.shared->timer_mutex);if (!delayed_work_pending (&dbs_info->cdbs.dwork)) { -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.shared->timer_mutex);continue ; }
@@ -286,15 +287,15 @@ static void update_sampling_rate(struct dbs_data *dbs_data,if (time_before (next_sampling, appointed_at)) { -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.shared->timer_mutex);cancel_delayed_work_sync (&dbs_info->cdbs.dwork); -mutex_lock (&dbs_info->cdbs.timer_mutex); +mutex_lock (&dbs_info->cdbs.shared->timer_mutex); -gov_queue_work (dbs_data, dbs_info->cdbs.policy, +gov_queue_work (dbs_data, policy,usecs_to_jiffies (new_rate), true); } -mutex_unlock (&dbs_info->cdbs.timer_mutex); +mutex_unlock (&dbs_info->cdbs.shared->timer_mutex); } }
@@ -557,13 +558,16 @@ static void od_set_powersave_bias(unsigned int powersave_bias)get_online_cpus ();for_each_online_cpu (cpu) { +struct cpu_common_dbs_info *shared; +if (cpumask_test_cpu (cpu, &done))continue ; - policy =per_cpu (od_cpu_dbs_info, cpu).cdbs.policy; -if (!policy) + shared =per_cpu (od_cpu_dbs_info, cpu).cdbs.shared; +if (!shared)continue ; + policy = shared->policy;cpumask_or (&done, &done, policy->cpus);if (policy->governor != &cpufreq_gov_ondemand)
Signature · 1 line
2.4.0
Viresh Kumar
maintainerPATCH v3 2/5 cpufreq: governor: split out common part of {cs|od}_dbs_timer()
In reply to Viresh KumarSome part of cs_dbs_timer() and od_dbs_timer() is exactly same and is unnecessarily duplicated.
Create the real work-handler in cpufreq_governor.c and put the common code in this routine (dbs_timer()).
Shouldn't make any functional change.
--- drivers/cpufreq/cpufreq_conservative.c | 27 +++++++----------------- drivers/cpufreq/cpufreq_governor.c | 38 ++++++++++++++++++++++++++++++---- drivers/cpufreq/cpufreq_governor.h | 10 ++++----- drivers/cpufreq/cpufreq_ondemand.c | 36 +++++++++++++------------------- 4 files changed, 60 insertions(+), 51 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +7 -20
@@ -102,28 +102,15 @@ static void cs_check_cpu(int cpu, unsigned int load) } } -static void cs_dbs_timer (struct work_struct *work) +static unsigned int cs_dbs_timer (struct cpu_dbs_info *cdbs, +struct dbs_data *dbs_data,bool modify_all) { -struct cs_cpu_dbs_info_s *dbs_info =container_of (work, - struct cs_cpu_dbs_info_s, cdbs.dwork.work); -struct cpufreq_policy *policy = dbs_info->cdbs.shared->policy; -unsigned int cpu = policy->cpu; -struct cs_cpu_dbs_info_s *core_dbs_info = &per_cpu (cs_cpu_dbs_info, - cpu); -struct dbs_data *dbs_data = policy->governor_data;struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; -int delay =delay_for_sampling_rate (cs_tuners->sampling_rate); -bool modify_all = true; - -mutex_lock (&core_dbs_info->cdbs.shared->timer_mutex); -if (!need_load_eval (core_dbs_info->cdbs.shared, - cs_tuners->sampling_rate)) - modify_all = false; -else -dbs_check_cpu (dbs_data, cpu); - -gov_queue_work (dbs_data, policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.shared->timer_mutex); + +if (modify_all) +dbs_check_cpu (dbs_data, cdbs->shared->policy->cpu); + +return delay_for_sampling_rate (cs_tuners->sampling_rate); } static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
drivers/cpufreq/cpufreq_governor.c +34 -4
@@ -207,8 +207,8 @@ static inline void gov_cancel_work(struct dbs_data *dbs_data, }/* Will return if we need to evaluate cpu load again or not */ -bool need_load_eval (struct cpu_common_dbs_info *shared, -unsigned int sampling_rate) +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 ();
@@ -223,7 +223,37 @@ bool need_load_eval(struct cpu_common_dbs_info *shared,return true; } -EXPORT_SYMBOL_GPL (need_load_eval); + +static void dbs_timer (struct work_struct *work) +{ + struct cpu_dbs_info *cdbs = container_of(work, struct cpu_dbs_info, + dwork.work); +struct cpu_common_dbs_info *shared = cdbs->shared; +struct cpufreq_policy *policy = shared->policy; +struct dbs_data *dbs_data = policy->governor_data; +unsigned int sampling_rate, delay; +bool modify_all = true; + +mutex_lock (&shared->timer_mutex); + +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; + } + +if (!need_load_eval (cdbs->shared, sampling_rate)) + modify_all = false; + + delay = dbs_data->cdata->gov_dbs_timer (cdbs, dbs_data, modify_all); +gov_queue_work (dbs_data, policy, delay, modify_all); + +mutex_unlock (&shared->timer_mutex); +}static void set_sampling_rate (struct dbs_data *dbs_data,unsigned int sampling_rate)
@@ -411,7 +441,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,if (ignore_nice) j_cdbs->prev_cpu_nice =kcpustat_cpu (j).cpustat[CPUTIME_NICE ]; -INIT_DEFERRABLE_WORK (&j_cdbs->dwork, cdata->gov_dbs_timer); +INIT_DEFERRABLE_WORK (&j_cdbs->dwork, dbs_timer); }if (cdata->governor ==GOV_CONSERVATIVE ) {
drivers/cpufreq/cpufreq_governor.h +5 -5
@@ -132,8 +132,8 @@ static void *get_cpu_dbs_info_s(int cpu) \struct cpu_common_dbs_info {struct cpufreq_policy *policy;/* -* percpu mutex that serializes governor limit change with gov_dbs_timer -* invocation. We do not want gov_dbs_timer to run when user is changing +* percpu mutex that serializes governor limit change with dbs_timer +* invocation. We do not want dbs_timer to run when user is changing * the governor or limits. */ struct mutex timer_mutex;
@@ -210,7 +210,9 @@ struct common_dbs_data {struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);void *(*get_cpu_dbs_info_s)(int cpu); -void (*gov_dbs_timer)(struct work_struct *work); +unsigned int (*gov_dbs_timer)(struct cpu_dbs_info *cdbs, +struct dbs_data *dbs_data, +bool modify_all);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);
@@ -269,8 +271,6 @@ static ssize_t show_sampling_rate_min_gov_pol \extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct dbs_data *dbs_data,int cpu); -bool need_load_eval (struct cpu_common_dbs_info *shared, -unsigned int sampling_rate);int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event); void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
drivers/cpufreq/cpufreq_ondemand.c +14 -22
@@ -191,48 +191,40 @@ static void od_check_cpu(int cpu, unsigned int load) } } -static void od_dbs_timer (struct work_struct *work) +static unsigned int od_dbs_timer (struct cpu_dbs_info *cdbs, +struct dbs_data *dbs_data,bool modify_all) { -struct od_cpu_dbs_info_s *dbs_info = - container_of(work, struct od_cpu_dbs_info_s, cdbs.dwork.work); -struct cpufreq_policy *policy = dbs_info->cdbs.shared->policy; +struct cpufreq_policy *policy = cdbs->shared->policy;unsigned int cpu = policy->cpu; -struct od_cpu_dbs_info_s *core_dbs_info = &per_cpu (od_cpu_dbs_info, +struct od_cpu_dbs_info_s *dbs_info = &per_cpu (od_cpu_dbs_info, cpu); -struct dbs_data *dbs_data = policy->governor_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners; -int delay =0 , sample_type = core_dbs_info->sample_type; -bool modify_all = true; +int delay =0 , sample_type = dbs_info->sample_type; -mutex_lock (&core_dbs_info->cdbs.shared->timer_mutex); -if (!need_load_eval (core_dbs_info->cdbs.shared, - od_tuners->sampling_rate)) { - modify_all = false; +if (!modify_all) goto max_delay; - }/* Common NORMAL_SAMPLE setup */ - core_dbs_info->sample_type =OD_NORMAL_SAMPLE ; + dbs_info->sample_type =OD_NORMAL_SAMPLE ;if (sample_type ==OD_SUB_SAMPLE ) { - delay = core_dbs_info->freq_lo_jiffies; -__cpufreq_driver_target (policy, core_dbs_info->freq_lo, + delay = dbs_info->freq_lo_jiffies; +__cpufreq_driver_target (policy, dbs_info->freq_lo,CPUFREQ_RELATION_H ); }else {dbs_check_cpu (dbs_data, cpu); -if (core_dbs_info->freq_lo) { +if (dbs_info->freq_lo) {/* Setup timer for SUB_SAMPLE */ - core_dbs_info->sample_type =OD_SUB_SAMPLE ; - delay = core_dbs_info->freq_hi_jiffies; + dbs_info->sample_type =OD_SUB_SAMPLE ; + delay = dbs_info->freq_hi_jiffies; } } max_delay:if (!delay) delay =delay_for_sampling_rate (od_tuners->sampling_rate - * core_dbs_info->rate_mult); + * dbs_info->rate_mult); -gov_queue_work (dbs_data, policy, delay, modify_all); -mutex_unlock (&core_dbs_info->cdbs.shared->timer_mutex); +return delay; }/************************** sysfs interface ************************/
Signature · 1 line
2.4.0
Viresh Kumar
maintainerPATCH v3 3/5 cpufreq: governor: Avoid invalid states with additional checks
In reply to Viresh KumarThere can be races where the request has come to a wrong state. For example INIT followed by STOP (instead of START) or START followed by EXIT (instead of STOP).
Tackle these races by comparing making sure the state-machine never gets into any invalid state. And return error if an invalid state-transition is requested.
--- drivers/cpufreq/cpufreq_governor.c | 46 +++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 11 deletions(-)
drivers/cpufreq/cpufreq_governor.c +35 -11
@@ -305,6 +305,10 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,unsigned int latency;int ret; +/* State should be equivalent to EXIT */ +if (policy->governor_data) +return -EBUSY ; +if (dbs_data) {if (WARN_ON (have_governor_per_policy ()))return -EINVAL ;
@@ -375,10 +379,15 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,return ret; } -static void cpufreq_governor_exit (struct cpufreq_policy *policy, -struct dbs_data *dbs_data) +static int cpufreq_governor_exit (struct cpufreq_policy *policy, +struct dbs_data *dbs_data) {struct common_dbs_data *cdata = dbs_data->cdata; +struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (policy->cpu); + +/* State should be equivalent to INIT */ +if (!cdbs->shared || cdbs->shared->policy) +return -EBUSY ; policy->governor_data =NULL ;if (!--dbs_data->usage_count) {
@@ -395,6 +404,7 @@ static void cpufreq_governor_exit(struct cpufreq_policy *policy, }free_common_dbs_info (policy, cdata); +return 0 ; } static int cpufreq_governor_start(struct cpufreq_policy *policy,
@@ -409,6 +419,10 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,if (!policy->cur)return -EINVAL ; +/* State should be equivalent to INIT */ +if (!shared || shared->policy) +return -EBUSY ; +if (cdata->governor ==GOV_CONSERVATIVE ) {struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
@@ -465,14 +479,18 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,return 0 ; } -static void cpufreq_governor_stop (struct cpufreq_policy *policy, -struct dbs_data *dbs_data) +static int cpufreq_governor_stop (struct cpufreq_policy *policy, +struct dbs_data *dbs_data) {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu);struct cpu_common_dbs_info *shared = cdbs->shared; +/* State should be equivalent to START */ +if (!shared || !shared->policy) +return -EBUSY ; +gov_cancel_work (dbs_data, policy);if (cdata->governor ==GOV_CONSERVATIVE ) {
@@ -484,17 +502,19 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy, shared->policy =NULL ;mutex_destroy (&shared->timer_mutex); +return 0 ; } -static void cpufreq_governor_limits (struct cpufreq_policy *policy, -struct dbs_data *dbs_data) +static int cpufreq_governor_limits (struct cpufreq_policy *policy, +struct dbs_data *dbs_data) {struct common_dbs_data *cdata = dbs_data->cdata;unsigned int cpu = policy->cpu;struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs (cpu); +/* State should be equivalent to START */ if (!cdbs->shared || !cdbs->shared->policy) -return ; +return -EBUSY ;mutex_lock (&cdbs->shared->timer_mutex);if (policy->max < cdbs->shared->policy->cur)
@@ -505,13 +525,15 @@ static void cpufreq_governor_limits(struct cpufreq_policy *policy, CPUFREQ_RELATION_L);dbs_check_cpu (dbs_data, cpu);mutex_unlock (&cdbs->shared->timer_mutex); + +return 0 ; }int cpufreq_governor_dbs (struct cpufreq_policy *policy,struct common_dbs_data *cdata,unsigned int event) {struct dbs_data *dbs_data; -int ret =0 ; +int ret;/* Lock governor to block concurrent initialization of governor */ mutex_lock (&cdata->mutex);
@@ -531,17 +553,19 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy, ret =cpufreq_governor_init (policy, dbs_data, cdata);break ;case CPUFREQ_GOV_POLICY_EXIT : -cpufreq_governor_exit (policy, dbs_data); + ret =cpufreq_governor_exit (policy, dbs_data);break ;case CPUFREQ_GOV_START : ret =cpufreq_governor_start (policy, dbs_data);break ;case CPUFREQ_GOV_STOP : -cpufreq_governor_stop (policy, dbs_data); + ret =cpufreq_governor_stop (policy, dbs_data);break ;case CPUFREQ_GOV_LIMITS : -cpufreq_governor_limits (policy, dbs_data); + ret =cpufreq_governor_limits (policy, dbs_data);break ; +default : + ret = -EINVAL ; } unlock:
Signature · 1 line
2.4.0
Viresh Kumar
maintainerPATCH v3 4/5 cpufreq: governor: Don't WARN on invalid states
In reply to Viresh KumarWith previous commit, governors have started to return errors on invalid state-transition requests. We already have a WARN for an invalid state-transition request in cpufreq_governor_dbs(). This does trigger today, as the sequence of events isn't guaranteed by cpufreq core.
Lets stop warning on that for now, and make sure we don't enter an invalid state.
--- drivers/cpufreq/cpufreq_governor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/cpufreq_governor.c +1 -1
@@ -543,7 +543,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,else dbs_data = cdata->gdbs_data; -if (WARN_ON (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT ))) { +if (!dbs_data && (event !=CPUFREQ_GOV_POLICY_INIT )) { ret = -EINVAL ; goto unlock; }
Signature · 1 line
2.4.0
Viresh Kumar
maintainerPATCH v3 5/5 cpufreq: propagate errors returned from __cpufreq_governor()
In reply to Viresh KumarReturn codes aren't honored properly in cpufreq_set_policy(). This can lead to two problems:
- wrong errors propagated to sysfs
- we try to do next state-change even if the previous one failed
cpufreq_governor_dbs() now returns proper errors on all invalid state-transition requests and this code should honor that.
--- drivers/cpufreq/cpufreq.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
drivers/cpufreq/cpufreq.c +24 -7
@@ -2295,16 +2295,31 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, old_gov = policy->governor;/* end old governor */ if (old_gov) { -__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); +if (ret) { +/* This can happen due to race with other operations */ +pr_debug ("%s: Failed to Stop Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + } +up_write (&policy->rwsem); -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT );down_write (&policy->rwsem); + +if (ret) { +pr_err ("%s: Failed to Exit Governor: %s (%d)\n" , + __func__, old_gov->name, ret); +return ret; + } }/* start new governor */ policy->governor = new_policy->governor; -if (!__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) { -if (!__cpufreq_governor (policy,CPUFREQ_GOV_START )) + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ); +if (!ret) { + ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (!ret) goto out;up_write (&policy->rwsem);
@@ -2316,11 +2331,13 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,pr_debug ("starting governor %s failed\n" , policy->governor->name);if (old_gov) { policy->governor = old_gov; -__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT ); -__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_INIT )) + policy->governor =NULL ; +else +__cpufreq_governor (policy,CPUFREQ_GOV_START ); } -return -EINVAL ; +return ret; out:pr_debug ("governor: change or update limits\n" );
Signature · 1 line
2.4.0
Quoted 13 lines · Viresh Kumar
Hi Rafael,
These are rest of the patches after replacing 'ccdbs' with 'shared' as you suggested.
Viresh Kumar (5):
cpufreq: governor: Keep single copy of information common topolicy->cpuscpufreq: governor: split out common part of {cs|od}_dbs_timer()
cpufreq: governor: Avoid invalid states with additional checks
cpufreq: governor: Don't WARN on invalid states
cpufreq: propagate errors returned from__cpufreq_governor()
Quoted 6 lines
drivers/cpufreq/cpufreq.c | 31 ++++-- drivers/cpufreq/cpufreq_conservative.c | 25 ++--- drivers/cpufreq/cpufreq_governor.c | 174 ++++++++++++++++++++++++++------- drivers/cpufreq/cpufreq_governor.h | 26 +++-- drivers/cpufreq/cpufreq_ondemand.c | 58 +++++------ 5 files changed, 210 insertions(+), 104 deletions(-)
All queued up for 4.3, thanks!
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.