cpufreq: governors: Fix ABBA lockups
Viresh Kumar
maintainerv3 cover cpufreq: governors: Fix ABBA lockups
Hi Rafael,
Things look much much better now. I have rebased this series over pm/bleeding-edge, that has all your patches.
I have moved ahead and done few more changes in this series, that should get rid of all the lockdeps we were getting earlier, that includes fixing lockdep issue in update_sampling_rate() that we were chasing.
These are pushed here again:git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/governor-kobject
@Juri/Shilpa: Can you please confirm if all the issues got resolved now ?
V2->V3:
- The first patch from V2, that was moving min_sampling_rate to per governor structure was dropped, as you suggested.
- Also, I have moved common tunables to struct dbs_data now, which you also suggested sometime back.
- Last 5 patches are all new and fix rest of the issues we were facing.
--
viresh
Viresh Kumar (13):
cpufreq: governor: Create generic macro for global tuners
cpufreq: governor: Move common tunables to 'struct dbs_data'
cpufreq: governor: New sysfs show/store callbacks for governor
tunables
cpufreq: governor: Drop unused macros for creating governor tunable
attributes
Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
cpufreq: Merge cpufreq_offline_prepare/finish routines
cpufreq: Call __cpufreq_governor() with policy->rwsem held
cpufreq: Remove cpufreq_governor_lock
cpufreq: governor: Move common sysfs tunables to cpufreq_governor.c
cpufreq: governor: No need to manage state machine now
cpufreq: governor: Keep list of policy_dbs within dbs_data
cpufreq: ondemand: Traverse list of policy_dbs in
update_sampling_rate()
cpufreq: conservative: Update sample_delay_ns immediately
drivers/cpufreq/cpufreq.c | 98 ++++++------ drivers/cpufreq/cpufreq_conservative.c | 143 ++++-------------- drivers/cpufreq/cpufreq_governor.c | 266 +++++++++++++++++++++++++-------- drivers/cpufreq/cpufreq_governor.h | 156 ++++++++----------- drivers/cpufreq/cpufreq_ondemand.c | 241 +++++------------------------ include/linux/cpufreq.h | 4 - 6 files changed, 381 insertions(+), 527 deletions(-)
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerPATCH v3 1/13 cpufreq: governor: Create generic macro for global tuners
Some tunables are present in governor specific structures, whereas one (min_sampling_rate) is present in global 'struct dbs_data'.
The macro for that is pretty much specific to sampling_rate_min for now. Next patch would be moving few more tunables to the 'struct dbs_data', then it would be useful to create a generic macro for such cases.
--- drivers/cpufreq/cpufreq_conservative.c | 8 ++++---- drivers/cpufreq/cpufreq_governor.h | 36 +++++++++++++++++++--------------- drivers/cpufreq/cpufreq_ondemand.c | 8 ++++---- 3 files changed, 28 insertions(+), 24 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +4 -4
@@ -245,7 +245,7 @@ show_store_one(cs, up_threshold);show_store_one (cs, down_threshold);show_store_one (cs, ignore_nice_load);show_store_one (cs, freq_step); -declare_show_sampling_rate_min (cs); +show_one_global (cs, min_sampling_rate);gov_sys_pol_attr_rw (sampling_rate);gov_sys_pol_attr_rw (sampling_down_factor);
@@ -253,10 +253,10 @@ gov_sys_pol_attr_rw(up_threshold);gov_sys_pol_attr_rw (down_threshold);gov_sys_pol_attr_rw (ignore_nice_load);gov_sys_pol_attr_rw (freq_step); -gov_sys_pol_attr_ro (sampling_rate_min); +gov_sys_pol_attr_ro (min_sampling_rate);static struct attribute *dbs_attributes_gov_sys[] = { - &sampling_rate_min_gov_sys.attr, + &min_sampling_rate_gov_sys.attr, &sampling_rate_gov_sys.attr, &sampling_down_factor_gov_sys.attr, &up_threshold_gov_sys.attr,
@@ -272,7 +272,7 @@ static struct attribute_group cs_attr_group_gov_sys = { };static struct attribute *dbs_attributes_gov_pol[] = { - &sampling_rate_min_gov_pol.attr, + &min_sampling_rate_gov_pol.attr, &sampling_rate_gov_pol.attr, &sampling_down_factor_gov_pol.attr, &up_threshold_gov_pol.attr,
drivers/cpufreq/cpufreq_governor.h +20 -16
@@ -110,6 +110,26 @@ static ssize_t store_##file_name##_gov_pol \show_one (_gov, file_name); \ store_one(_gov, file_name) +#define show_one_global (_gov, file_name) \ +static ssize_t show_##file_name##_gov_sys \ +(struct kobject *kobj, struct attribute *attr, char *buf) \ +{ \ + struct dbs_data *dbs_data = _gov##_dbs_gov.gdbs_data; \ + return sprintf(buf, "%u\n", dbs_data->file_name); \ +} \ + \ +static ssize_t show_##file_name##_gov_pol \ +(struct cpufreq_policy *policy, char *buf) \ +{ \ + struct policy_dbs_info *policy_dbs = policy->governor_data; \ + struct dbs_data *dbs_data = policy_dbs->dbs_data; \ + return sprintf(buf, "%u\n", dbs_data->file_name); \ +} + +#define show_store_one_global (_gov, file_name) \ +show_one_global(_gov, file_name); \ +store_one(_gov, file_name) +/* create helper routines */ #define define_get_cpu_dbs_routines (_dbs_info) \ static struct cpu_dbs_info *get_cpu_cdbs(int cpu) \
@@ -264,22 +284,6 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate)return delay; } -#define declare_show_sampling_rate_min (_gov) \ -static ssize_t show_sampling_rate_min_gov_sys \ -(struct kobject *kobj, struct attribute *attr, char *buf) \ -{ \ - struct dbs_data *dbs_data = _gov##_dbs_gov.gdbs_data; \ - return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \ -} \ - \ -static ssize_t show_sampling_rate_min_gov_pol \ -(struct cpufreq_policy *policy, char *buf) \ -{ \ - struct policy_dbs_info *policy_dbs = policy->governor_data; \ - struct dbs_data *dbs_data = policy_dbs->dbs_data; \ - return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \ -} -extern struct mutex dbs_data_mutex;extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct cpufreq_policy *policy);
drivers/cpufreq/cpufreq_ondemand.c +4 -4
@@ -443,7 +443,7 @@ show_store_one(od, up_threshold);show_store_one (od, sampling_down_factor);show_store_one (od, ignore_nice_load);show_store_one (od, powersave_bias); -declare_show_sampling_rate_min (od); +show_one_global (od, min_sampling_rate);gov_sys_pol_attr_rw (sampling_rate);gov_sys_pol_attr_rw (io_is_busy);
@@ -451,10 +451,10 @@ gov_sys_pol_attr_rw(up_threshold);gov_sys_pol_attr_rw (sampling_down_factor);gov_sys_pol_attr_rw (ignore_nice_load);gov_sys_pol_attr_rw (powersave_bias); -gov_sys_pol_attr_ro (sampling_rate_min); +gov_sys_pol_attr_ro (min_sampling_rate);static struct attribute *dbs_attributes_gov_sys[] = { - &sampling_rate_min_gov_sys.attr, + &min_sampling_rate_gov_sys.attr, &sampling_rate_gov_sys.attr, &up_threshold_gov_sys.attr, &sampling_down_factor_gov_sys.attr,
@@ -470,7 +470,7 @@ static struct attribute_group od_attr_group_gov_sys = { };static struct attribute *dbs_attributes_gov_pol[] = { - &sampling_rate_min_gov_pol.attr, + &min_sampling_rate_gov_pol.attr, &sampling_rate_gov_pol.attr, &up_threshold_gov_pol.attr, &sampling_down_factor_gov_pol.attr,
Signature · 1 line
2.7.1.370.gb2aa7f8
Rafael J. Wysocki
maintainerRe: [PATCH V3 01/13] cpufreq: governor: Create generic macro for global tuners
Quoted 2 lines · Viresh Kumar
Some tunables are present in governor specific structures, whereas one (
min_sampling_rate) is present in global 'structdbs_data'.
To me, the word "global" is not the best one to express that. "Common" would probably be better. Or "generic" or similar.
It's just that these tunables apply to both governors (or "all" governors if you will) as opposed to some other ones specific to a particular governor type.
So I'd suggest replacing "global" with "common" in the patch in the names of the macros too.
Thanks,
Rafael
Viresh Kumar
maintainerPATCH v3 2/13 cpufreq: governor: Move common tunables to 'struct dbs_data'
In reply to Viresh KumarThere are few more common tunables shared across ondemand and conservative governors. Move them to 'struct dbs_data' to simplify code.
--- drivers/cpufreq/cpufreq_conservative.c | 38 ++++++++++++++----------------- drivers/cpufreq/cpufreq_governor.c | 37 ++++++------------------------ drivers/cpufreq/cpufreq_governor.h | 14 +++++------- drivers/cpufreq/cpufreq_ondemand.c | 41 +++++++++++++++------------------- 4 files changed, 47 insertions(+), 83 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +17 -21
@@ -60,7 +60,7 @@ static void cs_check_cpu(int cpu, unsigned int load)return ;/* Check for frequency increase */ -if (load > cs_tuners->up_threshold) { +if (load > dbs_data->up_threshold) { dbs_info->down_skip =0 ;/* if we are already at full speed then break out early */
@@ -78,7 +78,7 @@ static void cs_check_cpu(int cpu, unsigned int load) }/* if sampling_down_factor is active break out early */ -if (++dbs_info->down_skip < cs_tuners->sampling_down_factor) +if (++dbs_info->down_skip < dbs_data->sampling_down_factor)return ; dbs_info->down_skip =0 ;
@@ -107,10 +107,9 @@ static unsigned int cs_dbs_timer(struct cpufreq_policy *policy) {struct policy_dbs_info *policy_dbs = policy->governor_data;struct dbs_data *dbs_data = policy_dbs->dbs_data; -struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;dbs_check_cpu (policy); -return delay_for_sampling_rate (cs_tuners->sampling_rate); +return delay_for_sampling_rate (dbs_data->sampling_rate); } static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
@@ -126,7 +125,6 @@ static struct dbs_governor cs_dbs_gov;static ssize_t store_sampling_down_factor (struct dbs_data *dbs_data,const char *buf,size_t count) { -struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;unsigned int input;int ret; ret =sscanf (buf,"%u" , &input);
@@ -134,14 +132,13 @@ static ssize_t store_sampling_down_factor(struct dbs_data *dbs_data,if (ret !=1 || input >MAX_SAMPLING_DOWN_FACTOR || input <1 )return -EINVAL ; - cs_tuners->sampling_down_factor = input; + dbs_data->sampling_down_factor = input;return count; }static ssize_t store_sampling_rate (struct dbs_data *dbs_data,const char *buf,size_t count) { -struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;unsigned int input;int ret; ret =sscanf (buf,"%u" , &input);
@@ -149,7 +146,7 @@ static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const char *buf,if (ret !=1 )return -EINVAL ; - cs_tuners->sampling_rate =max (input, dbs_data->min_sampling_rate); + dbs_data->sampling_rate =max (input, dbs_data->min_sampling_rate);return count; }
@@ -164,7 +161,7 @@ static ssize_t store_up_threshold(struct dbs_data *dbs_data, const char *buf,if (ret !=1 || input >100 || input <= cs_tuners->down_threshold)return -EINVAL ; - cs_tuners->up_threshold = input; + dbs_data->up_threshold = input;return count; }
@@ -178,7 +175,7 @@ static ssize_t store_down_threshold(struct dbs_data *dbs_data, const char *buf,/* cannot be lower than 11 otherwise freq will not fall */ if (ret !=1 || input <11 || input >100 || - input >= cs_tuners->up_threshold) + input >= dbs_data->up_threshold)return -EINVAL ; cs_tuners->down_threshold = input;
@@ -188,7 +185,6 @@ static ssize_t store_down_threshold(struct dbs_data *dbs_data, const char *buf,static ssize_t store_ignore_nice_load (struct dbs_data *dbs_data,const char *buf,size_t count) { -struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;unsigned int input, j;int ret;
@@ -199,10 +195,10 @@ static ssize_t store_ignore_nice_load(struct dbs_data *dbs_data,if (input >1 ) input =1 ; -if (input == cs_tuners->ignore_nice_load)/* nothing to do */ +if (input == dbs_data->ignore_nice_load)/* nothing to do */ return count; - cs_tuners->ignore_nice_load = input; + dbs_data->ignore_nice_load = input;/* we need to re-evaluate prev_cpu_idle */ for_each_online_cpu (j) {
@@ -210,7 +206,7 @@ static ssize_t store_ignore_nice_load(struct dbs_data *dbs_data, dbs_info = &per_cpu (cs_cpu_dbs_info, j); dbs_info->cdbs.prev_cpu_idle =get_cpu_idle_time (j, &dbs_info->cdbs.prev_cpu_wall,0 ); -if (cs_tuners->ignore_nice_load) +if (dbs_data->ignore_nice_load) dbs_info->cdbs.prev_cpu_nice =kcpustat_cpu (j).cpustat[CPUTIME_NICE ]; }
@@ -239,12 +235,12 @@ static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf,return count; } -show_store_one (cs, sampling_rate); -show_store_one (cs, sampling_down_factor); -show_store_one (cs, up_threshold);show_store_one (cs, down_threshold); -show_store_one (cs, ignore_nice_load);show_store_one (cs, freq_step); +show_store_one_global (cs, sampling_rate); +show_store_one_global (cs, sampling_down_factor); +show_store_one_global (cs, up_threshold); +show_store_one_global (cs, ignore_nice_load);show_one_global (cs, min_sampling_rate);gov_sys_pol_attr_rw (sampling_rate);
@@ -299,11 +295,11 @@ static int cs_init(struct dbs_data *dbs_data, bool notify)return -ENOMEM ; } - tuners->up_threshold =DEF_FREQUENCY_UP_THRESHOLD ; tuners->down_threshold =DEF_FREQUENCY_DOWN_THRESHOLD ; - tuners->sampling_down_factor =DEF_SAMPLING_DOWN_FACTOR ; - tuners->ignore_nice_load =0 ; tuners->freq_step =DEF_FREQUENCY_STEP ; + dbs_data->up_threshold =DEF_FREQUENCY_UP_THRESHOLD ; + dbs_data->sampling_down_factor =DEF_SAMPLING_DOWN_FACTOR ; + dbs_data->ignore_nice_load =0 ; dbs_data->tuners = tuners; dbs_data->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
drivers/cpufreq/cpufreq_governor.c +7 -30
@@ -38,10 +38,9 @@ void dbs_check_cpu(struct cpufreq_policy *policy)struct policy_dbs_info *policy_dbs = policy->governor_data;struct dbs_data *dbs_data = policy_dbs->dbs_data;struct od_dbs_tuners *od_tuners = dbs_data->tuners; -struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; -unsigned int sampling_rate; +unsigned int sampling_rate = dbs_data->sampling_rate; +unsigned int ignore_nice = dbs_data->ignore_nice_load;unsigned int max_load =0 ; -unsigned int ignore_nice;unsigned int j;if (gov->governor ==GOV_ONDEMAND ) {
@@ -54,13 +53,8 @@ void dbs_check_cpu(struct cpufreq_policy *policy)* the 'sampling_rate', so as to keep the wake-up-from-idle * detection logic a bit conservative. */ - sampling_rate = od_tuners->sampling_rate; sampling_rate *= od_dbs_info->rate_mult; - ignore_nice = od_tuners->ignore_nice_load; - }else { - sampling_rate = cs_tuners->sampling_rate; - ignore_nice = cs_tuners->ignore_nice_load; }/* Get Absolute Load */
@@ -279,19 +273,6 @@ static void dbs_update_util_handler(struct update_util_data *data, u64 time,atomic_dec (&policy_dbs->skip_work); } -static void set_sampling_rate (struct dbs_data *dbs_data, -struct dbs_governor *gov, -unsigned int sampling_rate) -{ -if (gov->governor ==GOV_CONSERVATIVE ) { -struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; - cs_tuners->sampling_rate = sampling_rate; - }else { -struct od_dbs_tuners *od_tuners = dbs_data->tuners; - od_tuners->sampling_rate = sampling_rate; - } -} -static struct policy_dbs_info *alloc_policy_dbs_info (struct cpufreq_policy *policy,struct dbs_governor *gov) {
@@ -383,8 +364,8 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy)/* Bring kernel and HW constraints together */ dbs_data->min_sampling_rate =max (dbs_data->min_sampling_rate,MIN_LATENCY_MULTIPLIER * latency); -set_sampling_rate (dbs_data, gov,max (dbs_data->min_sampling_rate, - latency *LATENCY_MULTIPLIER )); + dbs_data->sampling_rate =max (dbs_data->min_sampling_rate, +LATENCY_MULTIPLIER * latency);if (!have_governor_per_policy ()) gov->gdbs_data = dbs_data;
@@ -456,16 +437,12 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy)if (policy_dbs->policy)return -EBUSY ; -if (gov->governor ==GOV_CONSERVATIVE ) { -struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; + sampling_rate = dbs_data->sampling_rate; + ignore_nice = dbs_data->ignore_nice_load; - sampling_rate = cs_tuners->sampling_rate; - ignore_nice = cs_tuners->ignore_nice_load; - }else { +if (gov->governor ==GOV_ONDEMAND ) {struct od_dbs_tuners *od_tuners = dbs_data->tuners; - sampling_rate = od_tuners->sampling_rate; - ignore_nice = od_tuners->ignore_nice_load; io_busy = od_tuners->io_is_busy; }
drivers/cpufreq/cpufreq_governor.h +5 -9
@@ -153,9 +153,13 @@ static void *get_cpu_dbs_info_s(int cpu) \/* Governor demand based switching data (per-policy or global). */ struct dbs_data { -unsigned int min_sampling_rate;int usage_count;void *tuners; +unsigned int min_sampling_rate; +unsigned int ignore_nice_load; +unsigned int sampling_rate; +unsigned int sampling_down_factor; +unsigned int up_threshold; };/* Common to all CPUs of a policy */
@@ -216,19 +220,11 @@ struct cs_cpu_dbs_info_s {/* Per policy Governors sysfs tunables */ struct od_dbs_tuners { -unsigned int ignore_nice_load; -unsigned int sampling_rate; -unsigned int sampling_down_factor; -unsigned int up_threshold;unsigned int powersave_bias;unsigned int io_is_busy; };struct cs_dbs_tuners { -unsigned int ignore_nice_load; -unsigned int sampling_rate; -unsigned int sampling_down_factor; -unsigned int up_threshold;unsigned int down_threshold;unsigned int freq_step; };
drivers/cpufreq/cpufreq_ondemand.c +18 -23
140 lines not on this page · raw message
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerPATCH v3 3/13 cpufreq: governor: New sysfs show/store callbacks for governor tunables
In reply to Viresh KumarThe ondemand and conservative governors use the global-attr or freq-attr structures to represent sysfs attributes corresponding to their tunables (which of them is actually used depends on whether or not different policy objects can use the same governor with different tunables at the same time and, consequently, on where those attributes are located in sysfs).
Unfortunately, in the freq-attr case, the standard cpufreq show/store sysfs attribute callbacks are applied to the governor tunable attributes and they always acquire the policy->rwsem lock before carrying out the operation. That may lead to an ABBA deadlock if governor tunable attributes are removed under policy->rwsem while one of them is being accessed concurrently (if sysfs attributes removal wins the race, it will wait for the access to complete with policy->rwsem held while the attribute callback will block on policy->rwsem indefinitely).
We attempted to address this issue by dropping policy->rwsem around governor tunable attributes removal (that is, around invocations of the ->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT) in cpufreq_set_policy(), but that opened up race conditions that had not been possible with policy->rwsem held all the time. Therefore policy->rwsem cannot be dropped in cpufreq_set_policy() at any point, but the deadlock situation described above must be avoided too.
To that end, use the observation that in principle governor tunables may be represented by the same data type regardless of whether the governor is system-wide or per-policy and introduce a new structure, struct governor_attr, for representing them and new corresponding macros for creating show/store sysfs callbacks for them. Also make their parent kobject use a new kobject type whose default show/store callbacks are not related to the standard core cpufreq ones in any way (and they don't acquire policy->rwsem in particular).
--- drivers/cpufreq/cpufreq_conservative.c | 73 ++++++++++++---------------------- drivers/cpufreq/cpufreq_governor.c | 68 +++++++++++++++++++++++++++---- drivers/cpufreq/cpufreq_governor.h | 40 ++++++++++++++++++- drivers/cpufreq/cpufreq_ondemand.c | 73 ++++++++++++---------------------- 4 files changed, 150 insertions(+), 104 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +26 -47
@@ -235,54 +235,33 @@ static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf,return count; } -show_store_one (cs, down_threshold); -show_store_one (cs, freq_step); -show_store_one_global (cs, sampling_rate); -show_store_one_global (cs, sampling_down_factor); -show_store_one_global (cs, up_threshold); -show_store_one_global (cs, ignore_nice_load); -show_one_global (cs, min_sampling_rate); - -gov_sys_pol_attr_rw (sampling_rate); -gov_sys_pol_attr_rw (sampling_down_factor); -gov_sys_pol_attr_rw (up_threshold); -gov_sys_pol_attr_rw (down_threshold); -gov_sys_pol_attr_rw (ignore_nice_load); -gov_sys_pol_attr_rw (freq_step); -gov_sys_pol_attr_ro (min_sampling_rate); - -static struct attribute *dbs_attributes_gov_sys[] = { - &min_sampling_rate_gov_sys.attr, - &sampling_rate_gov_sys.attr, - &sampling_down_factor_gov_sys.attr, - &up_threshold_gov_sys.attr, - &down_threshold_gov_sys.attr, - &ignore_nice_load_gov_sys.attr, - &freq_step_gov_sys.attr, +gov_show_one (sampling_rate); +gov_show_one (sampling_down_factor); +gov_show_one (up_threshold); +gov_show_one (ignore_nice_load); +gov_show_one (min_sampling_rate); +gov_show_one_tunable (cs, down_threshold); +gov_show_one_tunable (cs, freq_step); + +gov_attr_rw (sampling_rate); +gov_attr_rw (sampling_down_factor); +gov_attr_rw (up_threshold); +gov_attr_rw (ignore_nice_load); +gov_attr_ro (min_sampling_rate); +gov_attr_rw (down_threshold); +gov_attr_rw (freq_step); + +static struct attribute *cs_attributes[] = { + &min_sampling_rate.attr, + &sampling_rate.attr, + &sampling_down_factor.attr, + &up_threshold.attr, + &down_threshold.attr, + &ignore_nice_load.attr, + &freq_step.attr,NULL }; -static struct attribute_group cs_attr_group_gov_sys = { - .attrs = dbs_attributes_gov_sys, - .name ="conservative" , -}; - -static struct attribute *dbs_attributes_gov_pol[] = { - &min_sampling_rate_gov_pol.attr, - &sampling_rate_gov_pol.attr, - &sampling_down_factor_gov_pol.attr, - &up_threshold_gov_pol.attr, - &down_threshold_gov_pol.attr, - &ignore_nice_load_gov_pol.attr, - &freq_step_gov_pol.attr, -NULL -}; - -static struct attribute_group cs_attr_group_gov_pol = { - .attrs = dbs_attributes_gov_pol, - .name ="conservative" , -}; -/************************** sysfs end ************************/ static int cs_init (struct dbs_data *dbs_data,bool notify)
@@ -331,8 +310,8 @@ static struct dbs_governor cs_dbs_gov = { .owner = THIS_MODULE, }, .governor = GOV_CONSERVATIVE, - .attr_group_gov_sys = &cs_attr_group_gov_sys, - .attr_group_gov_pol = &cs_attr_group_gov_pol, + .kobj_name = "conservative", + .kobj_type = { .default_attrs = cs_attributes }, .get_cpu_cdbs = get_cpu_cdbs, .get_cpu_dbs_info_s = get_cpu_dbs_info_s, .gov_dbs_timer = cs_dbs_timer,
drivers/cpufreq/cpufreq_governor.c +60 -8
102 lines not on this page · raw message
drivers/cpufreq/cpufreq_governor.h +38 -2
@@ -160,8 +160,44 @@ struct dbs_data {unsigned int sampling_rate;unsigned int sampling_down_factor;unsigned int up_threshold; + +struct kobject kobj; +/* Protect concurrent updates to governor tunables from sysfs */ +struct mutex mutex; +}; + +/* Governor's specific attributes */ +struct dbs_data ; +struct governor_attr { +struct attribute attr; +ssize_t (*show)(struct dbs_data *dbs_data,char *buf); +ssize_t (*store)(struct dbs_data *dbs_data,const char *buf, +size_t count); }; +#define gov_show_one_tunable (_gov, file_name) \ +static ssize_t show_##file_name \ +(struct dbs_data *dbs_data, char *buf) \ +{ \ + struct _gov##_dbs_tuners *tuners = dbs_data->tuners; \ + return sprintf(buf, "%u\n", tuners->file_name); \ +} + +#define gov_show_one (file_name) \ +static ssize_t show_##file_name \ +(struct dbs_data *dbs_data, char *buf) \ +{ \ + return sprintf(buf, "%u\n", dbs_data->file_name); \ +} + +#define gov_attr_ro (_name) \ +static struct governor_attr _name = \ +__ATTR(_name, 0444, show_##_name, NULL) + +#define gov_attr_rw (_name) \ +static struct governor_attr _name = \ +__ATTR(_name, 0644, show_##_name, store_##_name) +/* Common to all CPUs of a policy */ struct policy_dbs_info {struct cpufreq_policy *policy;
@@ -236,8 +272,8 @@ struct dbs_governor {#define GOV_ONDEMAND 0#define GOV_CONSERVATIVE 1int governor; -struct attribute_group *attr_group_gov_sys;/* one governor - system */ -struct attribute_group *attr_group_gov_pol;/* one governor - policy */ +const char *kobj_name; +struct kobj_type kobj_type;/* * Common data for platforms that don't set
drivers/cpufreq/cpufreq_ondemand.c +26 -47
88 lines not on this page · raw message
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerRe: [PATCH V3 03/13] cpufreq: governor: New sysfs show/store callbacks for governor tunables
drivers/cpufreq/cpufreq_conservative.c static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf, · line 244
Quoted 7 lines · Viresh Kumar
drivers/cpufreq/cpufreq_conservative.c +7
-0+gov_show_one (sampling_rate); +gov_show_one (sampling_down_factor); +gov_show_one (up_threshold); +gov_show_one (ignore_nice_load); +gov_show_one (min_sampling_rate); +gov_show_one_tunable (cs, down_threshold); +gov_show_one_tunable (cs, freq_step);
Based on the review comments on 1/13, I will do:
- s/gov_show_one/gov_show_one_common
- s/gov_show_one_tunable/gov_show_one
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerRe: [PATCH V3 03/13] cpufreq: governor: New sysfs show/store callbacks for governor tunables
Quoted 12 lines · Viresh Kumar
drivers/cpufreq/cpufreq_conservative.c static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf, · line 244
Quoted 7 lines · Viresh Kumar
drivers/cpufreq/cpufreq_conservative.c +7
-0+gov_show_one (sampling_rate); +gov_show_one (sampling_down_factor); +gov_show_one (up_threshold); +gov_show_one (ignore_nice_load); +gov_show_one (min_sampling_rate); +gov_show_one_tunable (cs, down_threshold); +gov_show_one_tunable (cs, freq_step);Based on the review comments on 1/13, I will do:
- s/gov_show_one/gov_show_one_common
- s/gov_show_one_tunable/gov_show_one
OK
Rafael J. Wysocki
maintainerRe: [PATCH V3 03/13] cpufreq: governor: New sysfs show/store callbacks for governor tunables
In reply to Viresh KumarOn Mon, Feb 8, 2016 at 12:39 PM, Viresh Kumar <[email protected]> wrote:
[cut]
drivers/cpufreq/cpufreq_conservative.c static struct dbs_governor cs_dbs_gov = { · line 313
Quoted 7 lines
drivers/cpufreq/cpufreq_conservative.c +1
-2@@ -331,8 +310,8 @@ static struct dbs_governor cs_dbs_gov = { .owner = THIS_MODULE, }, .governor = GOV_CONSERVATIVE, - .attr_group_gov_sys = &cs_attr_group_gov_sys, - .attr_group_gov_pol = &cs_attr_group_gov_pol, + .kobj_name = "conservative",
I don't think you need this.
Quoted 4 lines
drivers/cpufreq/cpufreq_conservative.c +1
-0+ .kobj_type = { .default_attrs = cs_attributes }, .get_cpu_cdbs = get_cpu_cdbs, .get_cpu_dbs_info_s = get_cpu_dbs_info_s, .gov_dbs_timer = cs_dbs_timer,
[cut]
drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 426
Quoted 11 lines
drivers/cpufreq/cpufreq_governor.c +4
-3@@ -373,10 +420,15 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) policy_dbs->dbs_data = dbs_data; policy->governor_data = policy_dbs; - ret =sysfs_create_group (get_governor_parent_kobj (policy), -get_sysfs_attr (gov)); -if (ret) + gov->kobj_type.sysfs_ops = &governor_sysfs_ops; + ret =kobject_init_and_add (&dbs_data->kobj, &gov->kobj_type, +get_governor_parent_kobj (policy), + gov->kobj_name);
gov->gov.name can be used here instead of the new kobj_name thing.
Besides, you forgot about the format argument for kobject_init_and_add().
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.c +4
-0+if (ret) { +pr_err ("cpufreq: Governor initialization failed (dbs_data kobject initialization error %d)\n" , + ret); goto reset_gdbs_data; + }return 0 ;
[cut]
Quoted 50 lines
drivers/cpufreq/cpufreq_governor.h +36
-0@@ -160,8 +160,44 @@ struct dbs_data {unsigned int sampling_rate;unsigned int sampling_down_factor;unsigned int up_threshold; + +struct kobject kobj; +/* Protect concurrent updates to governor tunables from sysfs */ +struct mutex mutex; +}; + +/* Governor's specific attributes */ +struct dbs_data ; +struct governor_attr { +struct attribute attr; +ssize_t (*show)(struct dbs_data *dbs_data,char *buf); +ssize_t (*store)(struct dbs_data *dbs_data,const char *buf, +size_t count); }; +#define gov_show_one_tunable (_gov, file_name) \ +static ssize_t show_##file_name \ +(struct dbs_data *dbs_data, char *buf) \ +{ \ + struct _gov##_dbs_tuners *tuners = dbs_data->tuners; \ + return sprintf(buf, "%u\n", tuners->file_name); \ +} + +#define gov_show_one (file_name) \ +static ssize_t show_##file_name \ +(struct dbs_data *dbs_data, char *buf) \ +{ \ + return sprintf(buf, "%u\n", dbs_data->file_name); \ +} + +#define gov_attr_ro (_name) \ +static struct governor_attr _name = \ +__ATTR(_name, 0444, show_##_name, NULL) + +#define gov_attr_rw (_name) \ +static struct governor_attr _name = \ +__ATTR(_name, 0644, show_##_name, store_##_name) +/* Common to all CPUs of a policy */ struct policy_dbs_info {struct cpufreq_policy *policy;
drivers/cpufreq/cpufreq_governor.h struct dbs_governor { · line 275
Quoted 6 lines
drivers/cpufreq/cpufreq_governor.h +1
-2@@ -236,8 +272,8 @@ struct dbs_governor {#define GOV_ONDEMAND 0#define GOV_CONSERVATIVE 1int governor; -struct attribute_group *attr_group_gov_sys;/* one governor - system */ -struct attribute_group *attr_group_gov_pol;/* one governor - policy */ +const char *kobj_name;
So this isn't really necessary.
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.h +1
-0+struct kobj_type kobj_type;/* * Common data for platforms that don't set drivers/cpufreq/cpufreq_ondemand.c +0
-0
[cut]
drivers/cpufreq/cpufreq_ondemand.c static struct dbs_governor od_dbs_gov = { · line 526
Quoted 7 lines
drivers/cpufreq/cpufreq_ondemand.c +1
-2@@ -544,8 +523,8 @@ static struct dbs_governor od_dbs_gov = { .owner = THIS_MODULE, }, .governor = GOV_ONDEMAND, - .attr_group_gov_sys = &od_attr_group_gov_sys, - .attr_group_gov_pol = &od_attr_group_gov_pol, + .kobj_name = "ondemand",
And this isn't necessary too.
drivers/cpufreq/cpufreq_ondemand.c static struct dbs_governor od_dbs_gov = {
Quoted 5 lines
drivers/cpufreq/cpufreq_ondemand.c +1
-1+ .kobj_type = { .default_attrs = od_attributes }, .get_cpu_cdbs = get_cpu_cdbs, .get_cpu_dbs_info_s = get_cpu_dbs_info_s, .gov_dbs_timer = od_dbs_timer, --
Thanks,
Rafael
Viresh Kumar
maintainerRe: [PATCH V3 03/13] cpufreq: governor: New sysfs show/store callbacks for governor tunables
Quoted 6 lines · Rafael J. Wysocki
drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 426
Quoted 3 lines · Viresh Kumar
drivers/cpufreq/cpufreq_governor.c +3
-0+ ret =kobject_init_and_add (&dbs_data->kobj, &gov->kobj_type, +get_governor_parent_kobj (policy), + gov->kobj_name);Besides, you forgot about the format argument for
kobject_init_and_add().
What about that? Why is it required here ? We don't have to modify the gov->gov.name string at all, and that string can be used here without adding any more format arguments.
Signature · 1 line
viresh
Re: [PATCH V3 03/13] cpufreq: governor: New sysfs show/store callbacks for governor tunables
Quoted 11 lines · Viresh Kumar
Quoted 6 lines · Rafael J. Wysocki
drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 426
Quoted 3 lines · Viresh Kumar
drivers/cpufreq/cpufreq_governor.c +3
-0+ ret =kobject_init_and_add (&dbs_data->kobj, &gov->kobj_type, +get_governor_parent_kobj (policy), + gov->kobj_name);Besides, you forgot about the format argument for
kobject_init_and_add().What about that? Why is it required here ? We don't have to modify the
gov->gov.namestring at all, and that string can be used here without adding any more format arguments.
But that's because the governor name is a static string and we can safely pass it as a format (because we know that it doesn't contain any output field specifiers in particular).
So either there should be a comment to that effect in the code, or the format argument should be present.
Thanks,
Rafael
Viresh Kumar
maintainerPATCH v3 4/13 cpufreq: governor: Drop unused macros for creating governor tunable attributes
In reply to Viresh Kumar"The previous commit introduced a new set of macros for creating sysfs attributes that represent governor tunables and the old macros used for this purpose are not needed any more, so drop them."
--- drivers/cpufreq/cpufreq_governor.h | 89 -------------------------------------- 1 file changed, 89 deletions(-)
drivers/cpufreq/cpufreq_governor.h +0 -89
@@ -41,95 +41,6 @@/* Ondemand Sampling types */ enum {OD_NORMAL_SAMPLE ,OD_SUB_SAMPLE }; -/* -* Macro for creating governors sysfs routines -* -* - gov_sys: One governor instance per whole system -* - gov_pol: One governor instance per policy -*/ - -/* Create attributes */ -#define gov_sys_attr_ro (_name) \ -static struct global_attr _name##_gov_sys = \ -__ATTR(_name, 0444, show_##_name##_gov_sys, NULL) - -#define gov_sys_attr_rw (_name) \ -static struct global_attr _name##_gov_sys = \ -__ATTR(_name, 0644, show_##_name##_gov_sys, store_##_name##_gov_sys) - -#define gov_pol_attr_ro (_name) \ -static struct freq_attr _name##_gov_pol = \ -__ATTR(_name, 0444, show_##_name##_gov_pol, NULL) - -#define gov_pol_attr_rw (_name) \ -static struct freq_attr _name##_gov_pol = \ -__ATTR(_name, 0644, show_##_name##_gov_pol, store_##_name##_gov_pol) - -#define gov_sys_pol_attr_rw (_name) \ - gov_sys_attr_rw(_name); \ - gov_pol_attr_rw(_name) - -#define gov_sys_pol_attr_ro (_name) \ - gov_sys_attr_ro(_name); \ - gov_pol_attr_ro(_name) - -/* Create show/store routines */ -#define show_one (_gov, file_name) \ -static ssize_t show_##file_name##_gov_sys \ -(struct kobject *kobj, struct attribute *attr, char *buf) \ -{ \ - struct _gov##_dbs_tuners *tuners = _gov##_dbs_gov.gdbs_data->tuners; \ - return sprintf(buf, "%u\n", tuners->file_name); \ -} \ - \ -static ssize_t show_##file_name##_gov_pol \ -(struct cpufreq_policy *policy, char *buf) \ -{ \ - struct policy_dbs_info *policy_dbs = policy->governor_data; \ - struct dbs_data *dbs_data = policy_dbs->dbs_data; \ - struct _gov##_dbs_tuners *tuners = dbs_data->tuners; \ - return sprintf(buf, "%u\n", tuners->file_name); \ -} - -#define store_one (_gov, file_name) \ -static ssize_t store_##file_name##_gov_sys \ -(struct kobject *kobj, struct attribute *attr, const char *buf, size_t count) \ -{ \ - struct dbs_data *dbs_data = _gov##_dbs_gov.gdbs_data; \ - return store_##file_name(dbs_data, buf, count); \ -} \ - \ -static ssize_t store_##file_name##_gov_pol \ -(struct cpufreq_policy *policy, const char *buf, size_t count) \ -{ \ - struct policy_dbs_info *policy_dbs = policy->governor_data; \ - return store_##file_name(policy_dbs->dbs_data, buf, count); \ -} - -#define show_store_one (_gov, file_name) \ -show_one(_gov, file_name); \ -store_one(_gov, file_name) - -#define show_one_global (_gov, file_name) \ -static ssize_t show_##file_name##_gov_sys \ -(struct kobject *kobj, struct attribute *attr, char *buf) \ -{ \ - struct dbs_data *dbs_data = _gov##_dbs_gov.gdbs_data; \ - return sprintf(buf, "%u\n", dbs_data->file_name); \ -} \ - \ -static ssize_t show_##file_name##_gov_pol \ -(struct cpufreq_policy *policy, char *buf) \ -{ \ - struct policy_dbs_info *policy_dbs = policy->governor_data; \ - struct dbs_data *dbs_data = policy_dbs->dbs_data; \ - return sprintf(buf, "%u\n", dbs_data->file_name); \ -} - -#define show_store_one_global (_gov, file_name) \ -show_one_global(_gov, file_name); \ -store_one(_gov, file_name) -/* create helper routines */ #define define_get_cpu_dbs_routines (_dbs_info) \ static struct cpu_dbs_info *get_cpu_cdbs(int cpu) \
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerPATCH v3 5/13 Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
In reply to Viresh KumarEarlier, when the struct freq-attr was used to represent governor attributes, the standard cpufreq show/store sysfs attribute callbacks were applied to the governor tunable attributes and they always acquire the policy->rwsem lock before carrying out the operation. That could have resulted in an ABBA deadlock if governor tunable attributes are removed under policy->rwsem while one of them is being accessed concurrently (if sysfs attributes removal wins the race, it will wait for the access to complete with policy->rwsem held while the attribute callback will block on policy->rwsem indefinitely).
We attempted to address this issue by dropping policy->rwsem around governor tunable attributes removal (that is, around invocations of the ->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT) in cpufreq_set_policy(), but that opened up race conditions that had not been possible with policy->rwsem held all the time.
The previous commit, "cpufreq: governor: New sysfs show/store callbacks for governor tunables", fixed the original ABBA deadlock by adding new governor specific show/store callbacks.
We don't have to drop rwsem around invocations of governor event CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
--- drivers/cpufreq/cpufreq.c | 5 ----- include/linux/cpufreq.h | 4 ---- 2 files changed, 9 deletions(-)
drivers/cpufreq/cpufreq.c +0 -5
@@ -2204,10 +2204,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,return ret; } -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);
@@ -2223,9 +2220,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,if (!ret) goto out; -up_write (&policy->rwsem);__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); -down_write (&policy->rwsem); }/* new governor failed, so re-start old one */
include/linux/cpufreq.h +0 -4
@@ -100,10 +100,6 @@ struct cpufreq_policy {* - Any routine that will write to the policy structure and/or may take away * the policy altogether (eg. CPU hotplug), will hold this lock in write * mode before doing so. -* -* Additional rules: -* - Lock should not be held across -* __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); */ struct rw_semaphore rwsem;
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerPATCH v3 6/13 cpufreq: Merge cpufreq_offline_prepare/finish routines
In reply to Viresh KumarThe offline routine was separated into two halves earlier by 'commit 1aee40ac9c86 ("cpufreq: Invoke __cpufreq_remove_dev_finish() after releasing cpu_hotplug.lock");.
And the reasons cited were, race issues between accessing policy's sysfs files and policy kobject's cleanup.
That race isn't valid anymore, as we don't remove the policy & its kobject completely on hotplugs, but do that from ->remove() callback of subsys framework.
These two routines can be merged back now.
This is a preparatory step for the next patch, that will enforce policy->rwsem lock around __cpufreq_governor() routines STOP/EXIT sequence.
--- drivers/cpufreq/cpufreq.c | 36 ++++++++++-------------------------- 1 file changed, 10 insertions(+), 26 deletions(-)
drivers/cpufreq/cpufreq.c +10 -26
@@ -1361,9 +1361,10 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)return ret; } -static void cpufreq_offline_prepare (unsigned int cpu) +static void cpufreq_offline (unsigned int cpu) {struct cpufreq_policy *policy; +int ret;pr_debug ("%s: unregistering CPU %u\n" , __func__, cpu);
@@ -1374,7 +1375,7 @@ static void cpufreq_offline_prepare(unsigned int cpu) }if (has_target ()) { -int ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP );if (ret)pr_err ("%s: Failed to stop governor\n" , __func__); }
@@ -1397,34 +1398,23 @@ static void cpufreq_offline_prepare(unsigned int cpu)/* Start governor again for active policy */ if (!policy_is_inactive (policy)) {if (has_target ()) { -int ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_START );if (!ret) ret =__cpufreq_governor (policy,CPUFREQ_GOV_LIMITS );if (ret)pr_err ("%s: Failed to start governor\n" , __func__); } - }else if (cpufreq_driver->stop_cpu) { - cpufreq_driver->stop_cpu (policy); - } -} -static void cpufreq_offline_finish (unsigned int cpu) -{ -struct cpufreq_policy *policy =per_cpu (cpufreq_cpu_data, cpu); - -if (!policy) { -pr_debug ("%s: No cpu_data found\n" , __func__);return ; } -/* Only proceed for inactive policies */ -if (!policy_is_inactive (policy)) -return ; +if (cpufreq_driver->stop_cpu) + cpufreq_driver->stop_cpu (policy);/* If cpu is last user of policy, free policy */ if (has_target ()) { -int ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT );if (ret)pr_err ("%s: Failed to exit governor\n" , __func__); }
@@ -1453,10 +1443,8 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)if (!policy)return ; -if (cpu_online (cpu)) { -cpufreq_offline_prepare (cpu); -cpufreq_offline_finish (cpu); - } +if (cpu_online (cpu)) +cpufreq_offline (cpu);cpumask_clear_cpu (cpu, policy->real_cpus);remove_cpu_dev_symlink (policy, cpu);
@@ -2304,11 +2292,7 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,break ;case CPU_DOWN_PREPARE : -cpufreq_offline_prepare (cpu); -break ; - -case CPU_POST_DEAD : -cpufreq_offline_finish (cpu); +cpufreq_offline (cpu);break ;case CPU_DOWN_FAILED :
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerPATCH v3 7/13 cpufreq: Call __cpufreq_governor() with policy->rwsem held
In reply to Viresh KumarThis isn't followed properly by all parts of the core code, some follow it, whereas others don't.
Enforcing it will also enable us to remove cpufreq_governor_lock, that is used today because we can't guarantee that __cpufreq_governor() isn't executed in parallel.
We should also ensure that the lock is held across state changes to the governors.
For example, while adding a CPU to the policy on cpu-online path, we need to stop the governor, change policy->cpus, start the governor and then refresh its limits. The complete sequence must be guaranteed to execute without any concurrent races. And that can be achieved using policy->rwsem around these use cases.
Also note that cpufreq_driver->stop_cpu() and ->exit() can get called while policy->rwsem is held. That shouldn't have any side effects though.
--- drivers/cpufreq/cpufreq.c | 49 +++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 16 deletions(-)
drivers/cpufreq/cpufreq.c +33 -16
@@ -1048,30 +1048,29 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpif (cpumask_test_cpu (cpu, policy->cpus))return 0 ; +down_write (&policy->rwsem);if (has_target ()) { ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP );if (ret) {pr_err ("%s: Failed to stop governor\n" , __func__); -return ret; + goto unlock; } } -down_write (&policy->rwsem);cpumask_set_cpu (cpu, policy->cpus); -up_write (&policy->rwsem);if (has_target ()) { ret =__cpufreq_governor (policy,CPUFREQ_GOV_START );if (!ret) ret =__cpufreq_governor (policy,CPUFREQ_GOV_LIMITS ); -if (ret) { +if (ret)pr_err ("%s: Failed to start governor\n" , __func__); -return ret; - } } -return 0 ; +unlock: +up_write (&policy->rwsem); +return ret; }static struct cpufreq_policy *cpufreq_policy_alloc (unsigned int cpu)
@@ -1374,13 +1373,13 @@ static void cpufreq_offline(unsigned int cpu)return ; } +down_write (&policy->rwsem);if (has_target ()) { ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP );if (ret)pr_err ("%s: Failed to stop governor\n" , __func__); } -down_write (&policy->rwsem);cpumask_clear_cpu (cpu, policy->cpus);if (policy_is_inactive (policy)) {
@@ -1393,7 +1392,6 @@ static void cpufreq_offline(unsigned int cpu)/* Nominate new CPU */ policy->cpu =cpumask_any (policy->cpus); } -up_write (&policy->rwsem);/* Start governor again for active policy */ if (!policy_is_inactive (policy)) {
@@ -1406,7 +1404,7 @@ static void cpufreq_offline(unsigned int cpu)pr_err ("%s: Failed to start governor\n" , __func__); } -return ; + goto unlock; }if (cpufreq_driver->stop_cpu)
@@ -1428,6 +1426,9 @@ static void cpufreq_offline(unsigned int cpu) cpufreq_driver->exit (policy); policy->freq_table =NULL ; } + +unlock: +up_write (&policy->rwsem); }/**
@@ -1624,6 +1625,7 @@ EXPORT_SYMBOL(cpufreq_generic_suspend);void cpufreq_suspend (void ) {struct cpufreq_policy *policy; +int ret;if (!cpufreq_driver)return ;
@@ -1634,7 +1636,11 @@ void cpufreq_suspend(void)pr_debug ("%s: Suspending Governors\n" , __func__);for_each_active_policy (policy) { -if (__cpufreq_governor (policy,CPUFREQ_GOV_STOP )) +down_write (&policy->rwsem); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP ); +up_write (&policy->rwsem); + +if (ret)pr_err ("%s: Failed to stop governor for policy: %p\n" , __func__, policy); else if (cpufreq_driver->suspend
@@ -1656,6 +1662,7 @@ void cpufreq_suspend(void)void cpufreq_resume (void ) {struct cpufreq_policy *policy; +int ret;if (!cpufreq_driver)return ;
@@ -1668,13 +1675,20 @@ void cpufreq_resume(void)pr_debug ("%s: Resuming Governors\n" , __func__);for_each_active_policy (policy) { -if (cpufreq_driver->resume && cpufreq_driver->resume (policy)) +if (cpufreq_driver->resume && cpufreq_driver->resume (policy)) {pr_err ("%s: Failed to resume driver: %p\n" , __func__, policy); -else if (__cpufreq_governor (policy,CPUFREQ_GOV_START ) - ||__cpufreq_governor (policy,CPUFREQ_GOV_LIMITS )) -pr_err ("%s: Failed to start governor for policy: %p\n" , - __func__, policy); + }else { +down_write (&policy->rwsem); + ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (!ret) +__cpufreq_governor (policy,CPUFREQ_GOV_LIMITS ); +up_write (&policy->rwsem); + +if (ret) +pr_err ("%s: Failed to start governor for policy: %p\n" , + __func__, policy); + } }/*
@@ -2325,8 +2339,11 @@ static int cpufreq_boost_set_sw(int state) __func__);break ; } + +down_write (&policy->rwsem); policy->user_policy.max = policy->max;__cpufreq_governor (policy,CPUFREQ_GOV_LIMITS ); +up_write (&policy->rwsem); } }
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerPATCH v3 8/13 cpufreq: Remove cpufreq_governor_lock
In reply to Viresh KumarWe used to drop policy->rwsem just before calling __cpufreq_governor() in some cases earlier and so it was possible that __cpufreq_governor() runs concurrently via separate threads.
In order to guarantee valid state transitions for governors, 'governor_enabled' was required to be protected using some locking and we created cpufreq_governor_lock for that.
But now, __cpufreq_governor() is always called from within policy->rwsem held and so 'governor_enabled' is protected against races even without cpufreq_governor_lock.
Get rid of the extra lock now.
--- drivers/cpufreq/cpufreq.c | 8 -------- drivers/cpufreq/cpufreq_governor.h | 1 - 2 files changed, 9 deletions(-)
drivers/cpufreq/cpufreq.c +0 -8
@@ -146,8 +146,6 @@ void cpufreq_update_util(u64 time, unsigned long util, unsigned long max)rcu_read_unlock (); } -DEFINE_MUTEX (cpufreq_governor_lock); -/* Flag to suspend/resume CPUFreq governors */ static bool cpufreq_suspended;
@@ -2014,11 +2012,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,pr_debug ("%s: for CPU %u, event %u\n" , __func__, policy->cpu, event); -mutex_lock (&cpufreq_governor_lock);if ((policy->governor_enabled && event ==CPUFREQ_GOV_START ) || (!policy->governor_enabled && (event ==CPUFREQ_GOV_LIMITS || event ==CPUFREQ_GOV_STOP ))) { -mutex_unlock (&cpufreq_governor_lock);return -EBUSY ; }
@@ -2027,8 +2023,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,else if (event ==CPUFREQ_GOV_START ) policy->governor_enabled = true; -mutex_unlock (&cpufreq_governor_lock); - ret = policy->governor->governor (policy, event);if (!ret) {
@@ -2038,12 +2032,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->governor->initialized--; }else {/* Restore original values */ -mutex_lock (&cpufreq_governor_lock);if (event ==CPUFREQ_GOV_STOP ) policy->governor_enabled = true;else if (event ==CPUFREQ_GOV_START ) policy->governor_enabled = false; -mutex_unlock (&cpufreq_governor_lock); } if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||
drivers/cpufreq/cpufreq_governor.h +0 -1
@@ -228,7 +228,6 @@ static inline int delay_for_sampling_rate(unsigned int sampling_rate) }extern struct mutex dbs_data_mutex; -extern struct mutex cpufreq_governor_lock;void dbs_check_cpu (struct cpufreq_policy *policy);int cpufreq_governor_dbs (struct cpufreq_policy *policy,unsigned int event); void od_register_powersave_bias_handler(unsigned int (*f)
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerPATCH v3 9/13 cpufreq: governor: Move common sysfs tunables to cpufreq_governor.c
In reply to Viresh KumarWe have got five common sysfs tunables between ondemand and conservative governors, move their callbacks to cpufreq_governor.c to get rid of redundant code.
Because of minor differences in the implementation of the callbacks, some more per-governor callbacks are introduced in order to not introduce any more "governor == ONDEMAND/CONSERVATIVE" like checks.
--- drivers/cpufreq/cpufreq_conservative.c | 80 +++++--------------------- drivers/cpufreq/cpufreq_governor.c | 100 +++++++++++++++++++++++++++++++++ drivers/cpufreq/cpufreq_governor.h | 16 +++++- drivers/cpufreq/cpufreq_ondemand.c | 100 ++++++++------------------------- 4 files changed, 151 insertions(+), 145 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +14 -66
@@ -122,47 +122,17 @@ static struct notifier_block cs_cpufreq_notifier_block = {/************************** sysfs interface ************************/ static struct dbs_governor cs_dbs_gov; -static ssize_t store_sampling_down_factor (struct dbs_data *dbs_data, -const char *buf,size_t count) +static bool invalid_up_threshold (struct dbs_data *dbs_data, +unsigned int threshold) { -unsigned int input; -int ret; - ret =sscanf (buf,"%u" , &input); - -if (ret !=1 || input >MAX_SAMPLING_DOWN_FACTOR || input <1 ) -return -EINVAL ; +struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; - dbs_data->sampling_down_factor = input; -return count; +return threshold >100 || threshold <= cs_tuners->down_threshold; } -static ssize_t store_sampling_rate (struct dbs_data *dbs_data,const char *buf, -size_t count) +static bool invalid_sampling_down_factor (unsigned int factor) { -unsigned int input; -int ret; - ret =sscanf (buf,"%u" , &input); - -if (ret !=1 ) -return -EINVAL ; - - dbs_data->sampling_rate =max (input, dbs_data->min_sampling_rate); -return count; -} - -static ssize_t store_up_threshold (struct dbs_data *dbs_data,const char *buf, -size_t count) -{ -struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; -unsigned int input; -int ret; - ret =sscanf (buf,"%u" , &input); - -if (ret !=1 || input >100 || input <= cs_tuners->down_threshold) -return -EINVAL ; - - dbs_data->up_threshold = input; -return count; +return factor >MAX_SAMPLING_DOWN_FACTOR ; } static ssize_t store_down_threshold(struct dbs_data *dbs_data, const char *buf,
@@ -182,27 +152,13 @@ static ssize_t store_down_threshold(struct dbs_data *dbs_data, const char *buf,return count; } -static ssize_t store_ignore_nice_load (struct dbs_data *dbs_data, -const char *buf,size_t count) +static void update_ignore_nice_load (struct dbs_data *dbs_data) { -unsigned int input, j; -int ret; - - ret =sscanf (buf,"%u" , &input); -if (ret !=1 ) -return -EINVAL ; - -if (input >1 ) - input =1 ; - -if (input == dbs_data->ignore_nice_load)/* nothing to do */ -return count; - - dbs_data->ignore_nice_load = input; +struct cs_cpu_dbs_info_s *dbs_info; +unsigned int j;/* we need to re-evaluate prev_cpu_idle */ for_each_online_cpu (j) { -struct cs_cpu_dbs_info_s *dbs_info; dbs_info = &per_cpu (cs_cpu_dbs_info, j); dbs_info->cdbs.prev_cpu_idle =get_cpu_idle_time (j, &dbs_info->cdbs.prev_cpu_wall,0 );
@@ -210,7 +166,6 @@ static ssize_t store_ignore_nice_load(struct dbs_data *dbs_data, dbs_info->cdbs.prev_cpu_nice =kcpustat_cpu (j).cpustat[CPUTIME_NICE ]; } -return count; } static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf,
@@ -235,21 +190,11 @@ static ssize_t store_freq_step(struct dbs_data *dbs_data, const char *buf,return count; } -gov_show_one (sampling_rate); -gov_show_one (sampling_down_factor); -gov_show_one (up_threshold); -gov_show_one (ignore_nice_load); -gov_show_one (min_sampling_rate);gov_show_one_tunable (cs, down_threshold);gov_show_one_tunable (cs, freq_step); -gov_attr_rw (sampling_rate); -gov_attr_rw (sampling_down_factor); -gov_attr_rw (up_threshold); -gov_attr_rw (ignore_nice_load); -gov_attr_ro (min_sampling_rate); -gov_attr_rw (down_threshold); -gov_attr_rw (freq_step); +static gov_attr_rw (down_threshold); +static gov_attr_rw (freq_step);static struct attribute *cs_attributes[] = { &min_sampling_rate.attr,
@@ -316,6 +261,9 @@ static struct dbs_governor cs_dbs_gov = { .get_cpu_dbs_info_s = get_cpu_dbs_info_s, .gov_dbs_timer = cs_dbs_timer, .gov_check_cpu = cs_check_cpu, + .invalid_up_threshold = invalid_up_threshold, + .invalid_sampling_down_factor = invalid_sampling_down_factor, + .update_ignore_nice_load = update_ignore_nice_load, .init = cs_init, .exit = cs_exit, };
drivers/cpufreq/cpufreq_governor.c +100 -0
112 lines not on this page · raw message
drivers/cpufreq/cpufreq_governor.h +14 -2
@@ -64,6 +64,7 @@ static void *get_cpu_dbs_info_s(int cpu) \/* Governor demand based switching data (per-policy or global). */ struct dbs_data { +struct dbs_governor *gov;int usage_count;void *tuners;unsigned int min_sampling_rate;
@@ -102,13 +103,19 @@ static ssize_t show_##file_name \ }#define gov_attr_ro (_name) \ -static struct governor_attr _name = \ +struct governor_attr _name = \ __ATTR(_name, 0444, show_##_name, NULL)#define gov_attr_rw (_name) \ -static struct governor_attr _name = \ +struct governor_attr _name = \ __ATTR(_name, 0644, show_##_name, store_##_name) +extern struct governor_attr sampling_rate; +extern struct governor_attr up_threshold; +extern struct governor_attr sampling_down_factor; +extern struct governor_attr ignore_nice_load; +extern struct governor_attr min_sampling_rate; +/* Common to all CPUs of a policy */ struct policy_dbs_info {struct cpufreq_policy *policy;
@@ -198,6 +205,11 @@ struct dbs_governor {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); +bool (*invalid_up_threshold)(struct dbs_data *dbs_data,unsigned int threshold); +bool (*invalid_sampling_down_factor)(unsigned int factor); +void (*update_sampling_rate)(struct dbs_data *dbs_data); +void (*update_sampling_down_factor)(struct dbs_data *dbs_data); +void (*update_ignore_nice_load)(struct dbs_data *dbs_data);/* Governor specific ops, see below */ void *gov_ops;
drivers/cpufreq/cpufreq_ondemand.c +23 -77
155 lines not on this page · raw message
Signature · 1 line
2.7.1.370.gb2aa7f8
Rafael J. Wysocki
maintainerRe: [PATCH V3 09/13] cpufreq: governor: Move common sysfs tunables to cpufreq_governor.c
Quoted 7 lines · Viresh Kumar
We have got five common sysfs tunables between ondemand and conservative governors, move their callbacks to
cpufreq_governor.cto get rid of redundant code.Because of minor differences in the implementation of the callbacks, some more per-governor callbacks are introduced in order to not introduce any more "governor == ONDEMAND/CONSERVATIVE" like checks.
My most fundamental concern here is that attributes that don't apply to a particular governor should not appear in sysfs at all when that governor is in use (instead of appearing and always returning -EINVAL which is sort of silly).
That doesn't mean the common code cannot access them, though. They still can be present in the data structure, but it may be a good idea to set them to special values clearly meaning "invalid" then.
Thanks,
Rafael
Viresh Kumar
maintainerRe: [PATCH V3 09/13] cpufreq: governor: Move common sysfs tunables to cpufreq_governor.c
Quoted 3 lines · Rafael J. Wysocki
My most fundamental concern here is that attributes that don't apply to a particular governor should not appear in sysfs at all when that governor is in use (instead of appearing and always returning
-EINVAL
s/is in use/is not in use/ ??
Quoted 1 line
which is sort of silly).
But who said that I have made them available always ? Sorry, I didn't understood your input.
I have just moved the show/store callbacks and the struct governor_attr definition to cpufreq_governor.c. And sysfs files are created only for the ones that are valid for a governor.
Quoted 3 lines
That doesn't mean the common code cannot access them, though. They still can be present in the data structure, but it may be a good idea to set them to special values clearly meaning "invalid" then.
Or are you saying that we should move all the tunables to dbs_data ?
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerRe: [PATCH V3 09/13] cpufreq: governor: Move common sysfs tunables to cpufreq_governor.c
Quoted 7 lines · Viresh Kumar
Quoted 3 lines · Rafael J. Wysocki
My most fundamental concern here is that attributes that don't apply to a particular governor should not appear in sysfs at all when that governor is in use (instead of appearing and always returning
-EINVALs/is in use/is not in use/ ??
Quoted 8 lines
Quoted 1 line
which is sort of silly).
But who said that I have made them available always ? Sorry, I didn't understood your input.
I have just moved the show/store callbacks and the struct
governor_attrdefinition tocpufreq_governor.c. And sysfs files are created only for the ones that are valid for a governor.
OK, I need to look at it more carefully then.
Quoted 5 lines
Quoted 3 lines
That doesn't mean the common code cannot access them, though. They still can be present in the data structure, but it may be a good idea to set them to special values clearly meaning "invalid" then.
Or are you saying that we should move all the tunables to
dbs_data?
Well, maybe. I'm not sure, but that may be done later in any case.
Thanks,
Rafael
Viresh Kumar
maintainerPATCH v3 10/13 cpufreq: governor: No need to manage state machine now
In reply to Viresh Kumarcpufreq core now guarantees that policy->rwsem wouldn't get dropped while calling CPUFREQ_GOV_POLICY_EXIT governor event and will be kept acquired until the complete sequence of governor state changes has finished.
And so we can remove the state machine checks that were put in place earlier.
This also means that policy_dbs->policy can be initialized while policy_dbs is allocated, to move all initialization together.
--- drivers/cpufreq/cpufreq_governor.c | 27 +++++---------------------- drivers/cpufreq/cpufreq_ondemand.c | 6 +++--- 2 files changed, 8 insertions(+), 25 deletions(-)
drivers/cpufreq/cpufreq_governor.c +5 -22
@@ -332,8 +332,10 @@ static inline void gov_clear_update_util(struct cpufreq_policy *policy)synchronize_rcu (); } -static void gov_cancel_work (struct policy_dbs_info *policy_dbs) +static void gov_cancel_work (struct cpufreq_policy *policy) { +struct policy_dbs_info *policy_dbs = policy->governor_data; +/* Tell dbs_update_util_handler() to skip queuing up work items. */ atomic_inc (&policy_dbs->skip_work);/*
@@ -429,6 +431,7 @@ static struct policy_dbs_info *alloc_policy_dbs_info(struct cpufreq_policy *poliif (!policy_dbs)return NULL ; + policy_dbs->policy = policy;mutex_init (&policy_dbs->timer_mutex);atomic_set (&policy_dbs->skip_work,0 );init_irq_work (&policy_dbs->irq_work, dbs_irq_work);
@@ -551,10 +554,6 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy)struct policy_dbs_info *policy_dbs = policy->governor_data;struct dbs_data *dbs_data = policy_dbs->dbs_data; -/* State should be equivalent to INIT */ -if (policy_dbs->policy) -return -EBUSY ; -if (!--dbs_data->usage_count) {kobject_put (&dbs_data->kobj);
@@ -585,10 +584,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy)if (!policy->cur)return -EINVAL ; -/* State should be equivalent to INIT */ -if (policy_dbs->policy) -return -EBUSY ; - sampling_rate = dbs_data->sampling_rate; ignore_nice = dbs_data->ignore_nice_load;
@@ -613,7 +608,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy)if (ignore_nice) j_cdbs->prev_cpu_nice =kcpustat_cpu (j).cpustat[CPUTIME_NICE ]; } - policy_dbs->policy = policy;if (gov->governor ==GOV_CONSERVATIVE ) { struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -636,14 +630,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy)static int cpufreq_governor_stop (struct cpufreq_policy *policy) { -struct policy_dbs_info *policy_dbs = policy->governor_data; - -/* State should be equivalent to START */ -if (!policy_dbs->policy) -return -EBUSY ; - -gov_cancel_work (policy_dbs); - policy_dbs->policy =NULL ; +gov_cancel_work (policy);return 0 ; }
@@ -652,10 +639,6 @@ static int cpufreq_governor_limits(struct cpufreq_policy *policy) {struct policy_dbs_info *policy_dbs = policy->governor_data; -/* State should be equivalent to START */ -if (!policy_dbs->policy) -return -EBUSY ; -mutex_lock (&policy_dbs->timer_mutex);if (policy->max < policy->cur)__cpufreq_driver_target (policy, policy->max,CPUFREQ_RELATION_H );
drivers/cpufreq/cpufreq_ondemand.c +3 -3
@@ -258,10 +258,10 @@ static void update_sampling_rate(struct dbs_data *dbs_data) policy_dbs = cdbs->policy_dbs;/* -* A valid policy_dbs and policy_dbs->policy means governor -* hasn't stopped or exited yet. +* A valid policy_dbs means governor hasn't stopped or exited +* yet. */ -if (!policy_dbs || !policy_dbs->policy) +if (!policy_dbs)continue ; policy = policy_dbs->policy;
Signature · 1 line
2.7.1.370.gb2aa7f8
Viresh Kumar
maintainerPATCH v3 11/13 cpufreq: governor: Keep list of policy_dbs within dbs_data
In reply to Viresh KumarAn instance of 'struct dbs_data' can support multiple 'struct policy_dbs_info' instances. To traverse all policy_dbs supported by a dbs_data, create a list of policy_dbs within dbs_data.
--- drivers/cpufreq/cpufreq_governor.c | 12 ++++++++++++ drivers/cpufreq/cpufreq_governor.h | 7 ++++++- 2 files changed, 18 insertions(+), 1 deletion(-)
drivers/cpufreq/cpufreq_governor.c +12 -0
@@ -489,6 +489,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count++; policy_dbs->dbs_data = dbs_data; policy->governor_data = policy_dbs; + +mutex_lock (&dbs_data->mutex); +list_add (&policy_dbs->list, &dbs_data->policy_dbs_list); +mutex_unlock (&dbs_data->mutex); +return 0 ; }
@@ -500,8 +505,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count =1 ; dbs_data->gov = gov; +INIT_LIST_HEAD (&dbs_data->policy_dbs_list);mutex_init (&dbs_data->mutex); +list_add (&policy_dbs->list, &dbs_data->policy_dbs_list); + ret = gov->init (dbs_data, !policy->governor->initialized);if (ret) goto free_policy_dbs_info;
@@ -554,6 +562,10 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy)struct policy_dbs_info *policy_dbs = policy->governor_data;struct dbs_data *dbs_data = policy_dbs->dbs_data; +mutex_lock (&dbs_data->mutex); +list_del (&policy_dbs->list); +mutex_unlock (&dbs_data->mutex); +if (!--dbs_data->usage_count) {kobject_put (&dbs_data->kobj);
drivers/cpufreq/cpufreq_governor.h +6 -1
@@ -74,7 +74,11 @@ struct dbs_data {unsigned int up_threshold;struct kobject kobj; -/* Protect concurrent updates to governor tunables from sysfs */ +struct list_head policy_dbs_list; +/* +* Protect concurrent updates to governor tunables from sysfs and +* policy_dbs_list. +*/ struct mutex mutex; };
@@ -132,6 +136,7 @@ struct policy_dbs_info {struct work_struct work;/* dbs_data may be shared between multiple policy objects */ struct dbs_data *dbs_data; +struct list_head list; }; static inline void gov_update_sample_delay(struct policy_dbs_info *policy_dbs,
Signature · 1 line
2.7.1.370.gb2aa7f8
Rafael J. Wysocki
maintainerRe: [PATCH V3 11/13] cpufreq: governor: Keep list of policy_dbs within dbs_data
Quoted 5 lines · Viresh Kumar
An instance of 'struct
dbs_data' can support multiple 'structpolicy_dbs_info' instances. To traverse allpolicy_dbssupported by adbs_data, create a list ofpolicy_dbswithindbs_data.Signed-off-by Viresh Kumar <[email protected]>
Good idea overall, I like this.
Quoted 10 lines
--- drivers/cpufreq/cpufreq_governor.c | 12 ++++++++++++ drivers/cpufreq/cpufreq_governor.h | 7 ++++++- 2 files changed, 18 insertions(+), 1 deletion(-)drivers/cpufreq/cpufreq_governor.c +0
-0
drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 495
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +4
-0@@ -489,6 +489,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count++; policy_dbs->dbs_data = dbs_data; policy->governor_data = policy_dbs; + +mutex_lock (&dbs_data->mutex); +list_add (&policy_dbs->list, &dbs_data->policy_dbs_list); +mutex_unlock (&dbs_data->mutex);
The previous statements should be under the mutex too IMO, at least the usage count incrementation in case two instances of this happen at the same time.
That can't happen now, but if we want to get rid of dbs_data_mutex going forward, having it under the mutex will be actually useful.
drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 511
Quoted 12 lines
drivers/cpufreq/cpufreq_governor.c +3
-0+return 0 ; }@@ -500,8 +505,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count =1 ; dbs_data->gov = gov; +INIT_LIST_HEAD (&dbs_data->policy_dbs_list);mutex_init (&dbs_data->mutex); +list_add (&policy_dbs->list, &dbs_data->policy_dbs_list);
That line should go to where policy_dbs->dbs_data is set so it is clear that they go together. And I'd set the usage count to 1 in there too for consistency.
drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_exit(struct cpufreq_policy *policy) · line 568
Quoted 12 lines
drivers/cpufreq/cpufreq_governor.c +5
-0+ ret = gov->init (dbs_data, !policy->governor->initialized);if (ret) goto free_policy_dbs_info;@@ -554,6 +562,10 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy)struct policy_dbs_info *policy_dbs = policy->governor_data;struct dbs_data *dbs_data = policy_dbs->dbs_data; +mutex_lock (&dbs_data->mutex); +list_del (&policy_dbs->list); +mutex_unlock (&dbs_data->mutex); +
Same here. I'd do the decrementation of the counter under the mutex along with the removal from the list. They are related in any case ("one user goes away") and will be useful for dbs_data_mutex elimination.
Quoted 8 lines
drivers/cpufreq/cpufreq_governor.c +0
-0if (!--dbs_data->usage_count) {kobject_put (&dbs_data->kobj);drivers/cpufreq/cpufreq_governor.h +0
-0
drivers/cpufreq/cpufreq_governor.h struct dbs_data { · line 81
Quoted 9 lines
drivers/cpufreq/cpufreq_governor.h +5
-1@@ -74,7 +74,11 @@ struct dbs_data {unsigned int up_threshold;struct kobject kobj; -/* Protect concurrent updates to governor tunables from sysfs */ +struct list_head policy_dbs_list; +/* +* Protect concurrent updates to governor tunables from sysfs and +* policy_dbs_list. +*/
And if the counter is modified under the mutex, it should be mentioned in this comment.
Maybe keep the counter close to the list and the mutex in the structure definition too?
drivers/cpufreq/cpufreq_governor.h struct policy_dbs_info {
Quoted 12 lines
drivers/cpufreq/cpufreq_governor.h +1
-1struct mutex mutex; };@@ -132,6 +136,7 @@ struct policy_dbs_info {struct work_struct work;/* dbs_data may be shared between multiple policy objects */ struct dbs_data *dbs_data; +struct list_head list; }; static inline void gov_update_sample_delay(struct policy_dbs_info *policy_dbs, --
Thanks,
Rafael
Viresh Kumar
maintainerRe: [PATCH V3 11/13] cpufreq: governor: Keep list of policy_dbs within dbs_data
Quoted 8 lines · Rafael J. Wysocki
Quoted 5 lines · Viresh Kumar
An instance of 'struct
dbs_data' can support multiple 'structpolicy_dbs_info' instances. To traverse allpolicy_dbssupported by adbs_data, create a list ofpolicy_dbswithindbs_data.Signed-off-by Viresh Kumar <[email protected]>Good idea overall, I like this.
Thanks.
Quoted 22 lines
Quoted 10 lines
--- drivers/cpufreq/cpufreq_governor.c | 12 ++++++++++++ drivers/cpufreq/cpufreq_governor.h | 7 ++++++- 2 files changed, 18 insertions(+), 1 deletion(-)drivers/cpufreq/cpufreq_governor.c +0
-0drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 495
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +4
-0@@ -489,6 +489,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count++; policy_dbs->dbs_data = dbs_data; policy->governor_data = policy_dbs; + +mutex_lock (&dbs_data->mutex); +list_add (&policy_dbs->list, &dbs_data->policy_dbs_list); +mutex_unlock (&dbs_data->mutex);The previous statements should be under the mutex too IMO, at least the usage count incrementation in case two instances of this happen at the same time.
Quoted 2 lines
That can't happen now, but if we want to get rid of
dbs_data_mutexgoing forward, having it under the mutex will be actually useful.
I think we should keep it precise for now. Right now, we are only concerned about the list modification, so just lock around that.
Once we are going to remove dbs_data_mutex, then we can cover more things under it.
Is there anything that is broken right now ?
Quoted 11 lines
Quoted 11 lines
drivers/cpufreq/cpufreq_governor.c +2
-0+return 0 ; }@@ -500,8 +505,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count =1 ; dbs_data->gov = gov; +INIT_LIST_HEAD (&dbs_data->policy_dbs_list);mutex_init (&dbs_data->mutex);
Quoted 4 lines
drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 511
Quoted 1 line
drivers/cpufreq/cpufreq_governor.c +1
-0+list_add (&policy_dbs->list, &dbs_data->policy_dbs_list);That line should go to where
policy_dbs->dbs_datais set so it is clear that they go together.
Okay.
Quoted 2 lines
And I'd set the usage count to 1 in
there too for consistency.
I am not sure about including any updates within the lock, which don't need protection in current state of code.
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerRe: [PATCH V3 11/13] cpufreq: governor: Keep list of policy_dbs within dbs_data
Quoted 37 lines · Viresh Kumar
Quoted 8 lines · Rafael J. Wysocki
Quoted 5 lines · Viresh Kumar
An instance of 'struct
dbs_data' can support multiple 'structpolicy_dbs_info' instances. To traverse allpolicy_dbssupported by adbs_data, create a list ofpolicy_dbswithindbs_data.Signed-off-by Viresh Kumar <[email protected]>Good idea overall, I like this.
Thanks.
Quoted 22 lines
Quoted 10 lines
--- drivers/cpufreq/cpufreq_governor.c | 12 ++++++++++++ drivers/cpufreq/cpufreq_governor.h | 7 ++++++- 2 files changed, 18 insertions(+), 1 deletion(-)drivers/cpufreq/cpufreq_governor.c +0
-0drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 495
Quoted 7 lines
drivers/cpufreq/cpufreq_governor.c +4
-0@@ -489,6 +489,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count++; policy_dbs->dbs_data = dbs_data; policy->governor_data = policy_dbs; + +mutex_lock (&dbs_data->mutex); +list_add (&policy_dbs->list, &dbs_data->policy_dbs_list); +mutex_unlock (&dbs_data->mutex);The previous statements should be under the mutex too IMO, at least the usage count incrementation in case two instances of this happen at the same time.
Quoted 2 lines
That can't happen now, but if we want to get rid of
dbs_data_mutexgoing forward, having it under the mutex will be actually useful.
Quoted 7 lines
I think we should keep it precise for now. Right now, we are only concerned about the list modification, so just lock around that.
Once we are going to remove
dbs_data_mutex, then we can cover more things under it.Is there anything that is broken right now ?
Yes, the logic.
The counter technically is the number of items in policy_dbs_list. Updating the list alone under the lock is simply illogical.
Quoted 16 lines
Quoted 11 lines
Quoted 11 lines
drivers/cpufreq/cpufreq_governor.c +2
-0+return 0 ; }@@ -500,8 +505,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count =1 ; dbs_data->gov = gov; +INIT_LIST_HEAD (&dbs_data->policy_dbs_list);mutex_init (&dbs_data->mutex);Quoted 4 lines
drivers/cpufreq/cpufreq_governor.c static int cpufreq_governor_init(struct cpufreq_policy *policy) · line 511
Quoted 1 line
drivers/cpufreq/cpufreq_governor.c +1
-0+list_add (&policy_dbs->list, &dbs_data->policy_dbs_list);That line should go to where
policy_dbs->dbs_datais set so it is clear that they go together.
Quoted 7 lines
Okay.
Quoted 2 lines
And I'd set the usage count to 1 in
there too for consistency.I am not sure about including any updates within the lock, which don't need protection in current state of code.
Well, if you're not sure, then please simply follow my request. :-)
Thanks,
Rafael
Viresh Kumar
maintainerPATCH v3 12/13 cpufreq: ondemand: Traverse list of policy_dbs in update_sampling_rate()
In reply to Viresh KumarNow that we maintain a list of all 'struct policy_dbs_info' for an instance of 'struct dbs_data', we can traverse that instead of traversing the loop for all online CPUs.
This also solves the circular dependency lockdep reported by Juri (and verified by Shilpa) earlier:
======================================================
[ INFO: possible circular locking dependency detected ]
4.4.0+ #445 Not tainted
-------------------------------------------------------
trace.sh/1723 is trying to acquire lock:
(s_active#48){++++.+}, at: [<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94
but task is already holding lock:
(od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (od_dbs_cdata.mutex){+.+.+.}:
[<c075b040>] mutex_lock_nested+0x7c/0x434
[<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
[<c0017c10>] return_to_handler+0x0/0x18-> #1 (&policy->rwsem){+++++.}:
[<c075ca8c>] down_read+0x58/0x94
[<c057c244>] show+0x30/0x60
[<c01f934c>] sysfs_kf_seq_show+0x90/0xfc
[<c01f7ad8>] kernfs_seq_show+0x34/0x38
[<c01a22ec>] seq_read+0x1e4/0x4e4
[<c01f8694>] kernfs_fop_read+0x120/0x1a0
[<c01794b4>] __vfs_read+0x3c/0xe0
[<c017a378>] vfs_read+0x98/0x104
[<c017a434>] SyS_read+0x50/0x90
[<c000fd40>] ret_fast_syscall+0x0/0x1c-> #0 (s_active#48){++++.+}:
[<c008238c>] lock_acquire+0xd4/0x20c
[<c01f6ae4>] __kernfs_remove+0x288/0x328
[<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94
[<c01fa024>] remove_files+0x44/0x88
[<c01fa5a4>] sysfs_remove_group+0x50/0xa4
[<c058285c>] cpufreq_governor_dbs+0x3f0/0x5d4
[<c0017c10>] return_to_handler+0x0/0x18other info that might help us debug this:
Chain exists of: s_active#48 --> &policy->rwsem --> od_dbs_cdata.mutex Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(od_dbs_cdata.mutex);
lock(&policy->rwsem);
lock(od_dbs_cdata.mutex); lock(s_active#48);
*** DEADLOCK ***
5 locks held by trace.sh/1723:
#0: (sb_writers#6){.+.+.+}, at: [<c017beb8>] __sb_start_write+0xb4/0xc0
#1: (&of->mutex){+.+.+.}, at: [<c01f8418>] kernfs_fop_write+0x6c/0x1c8
#2: (s_active#35){.+.+.+}, at: [<c01f8420>] kernfs_fop_write+0x74/0x1c8
#3: (cpu_hotplug.lock){++++++}, at: [<c0029e6c>] get_online_cpus+0x48/0xb8
#4: (od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4stack backtrace: CPU: 2 PID: 1723 Comm: trace.sh Not tainted 4.4.0+ #445 Hardware name: ARM-Versatile Express [<c001883c>] (unwind_backtrace) from [<c0013f50>] (show_stack+0x20/0x24) [<c0013f50>] (show_stack) from [<c044ad90>] (dump_stack+0x80/0xb4) [<c044ad90>] (dump_stack) from [<c0128edc>] (print_circular_bug+0x29c/0x2f0) [<c0128edc>] (print_circular_bug) from [<c0081708>] (__lock_acquire+0x163c/0x1d74) [<c0081708>] (__lock_acquire) from [<c008238c>] (lock_acquire+0xd4/0x20c) [<c008238c>] (lock_acquire) from [<c01f6ae4>] (__kernfs_remove+0x288/0x328) [<c01f6ae4>] (__kernfs_remove) from [<c01f78c8>] (kernfs_remove_by_name_ns+0x4c/0x94) [<c01f78c8>] (kernfs_remove_by_name_ns) from [<c01fa024>] (remove_files+0x44/0x88) [<c01fa024>] (remove_files) from [<c01fa5a4>] (sysfs_remove_group+0x50/0xa4) [<c01fa5a4>] (sysfs_remove_group) from [<c058285c>] (cpufreq_governor_dbs+0x3f0/0x5d4) [<c058285c>] (cpufreq_governor_dbs) from [<c0017c10>] (return_to_handler+0x0/0x18)
--- drivers/cpufreq/cpufreq_ondemand.c | 89 ++++++++++---------------------------- 1 file changed, 22 insertions(+), 67 deletions(-)
drivers/cpufreq/cpufreq_ondemand.c +22 -67
@@ -224,83 +224,38 @@ static struct dbs_governor od_dbs_gov;/** * update_sampling_rate - update sampling rate effective immediately if needed. * @new_rate: new sampling rate -* -* If new rate is smaller than the old, simply updating -* dbs_tuners_int.sampling_rate might not be appropriate. For example, if the -* original sampling_rate was 1 second and the requested new sampling rate is 10 -* ms because the user needs immediate reaction from ondemand governor, but not -* sure if higher frequency will be required or not, then, the governor may -* change the sampling rate too late; up to 1 second later. Thus, if we are -* reducing the sampling rate, we need to make the new value effective -* immediately. */ static void update_sampling_rate (struct dbs_data *dbs_data) { -struct cpumask cpumask; +struct policy_dbs_info *policy_dbs;unsigned int new_rate = dbs_data->sampling_rate; -int cpu;/* -* Lock governor so that governor start/stop can't execute in parallel. +* We are operating under dbs_data->mutex and so the list and its +* entries can't be freed concurrently. */ -mutex_lock (&dbs_data_mutex); - -cpumask_copy (&cpumask, cpu_online_mask); - -for_each_cpu (cpu, &cpumask) { -struct cpufreq_policy *policy; -struct od_cpu_dbs_info_s *dbs_info; -struct cpu_dbs_info *cdbs; -struct policy_dbs_info *policy_dbs; - - dbs_info = &per_cpu (od_cpu_dbs_info, cpu); - cdbs = &dbs_info->cdbs; - policy_dbs = cdbs->policy_dbs; - +list_for_each_entry (policy_dbs, &dbs_data->policy_dbs_list, list) { +mutex_lock (&policy_dbs->timer_mutex);/* -* A valid policy_dbs means governor hasn't stopped or exited -* yet. +* On 32-bit architectures this may race with the +* sample_delay_ns read in dbs_update_util_handler(), but that +* really doesn't matter. If the read returns a value that's +* too big, the sample will be skipped, but the next invocation +* of dbs_update_util_handler() (when the update has been +* completed) will take a sample. If the returned value is too +* small, the sample will be taken immediately, but that isn't a +* problem, as we want the new rate to take effect immediately +* anyway. +* +* If this runs in parallel with dbs_work_handler(), we may end +* up overwriting the sample_delay_ns value that it has just +* written, but the difference should not be too big and it will +* be corrected next time a sample is taken, so it shouldn't be +* significant. */ -if (!policy_dbs) -continue ; - - policy = policy_dbs->policy; - -/* clear all CPUs of this policy */ -cpumask_andnot (&cpumask, &cpumask, policy->cpus); - -/* -* Update sampling rate for CPUs whose policy is governed by -* dbs_data. In case of governor_per_policy, only a single -* policy will be governed by dbs_data, otherwise there can be -* multiple policies that are governed by the same dbs_data. -*/ -if (dbs_data == policy_dbs->dbs_data) { -mutex_lock (&policy_dbs->timer_mutex); -/* -* On 32-bit architectures this may race with the -* sample_delay_ns read in dbs_update_util_handler(), -* but that really doesn't matter. If the read returns -* a value that's too big, the sample will be skipped, -* but the next invocation of dbs_update_util_handler() -* (when the update has been completed) will take a -* sample. If the returned value is too small, the -* sample will be taken immediately, but that isn't a -* problem, as we want the new rate to take effect -* immediately anyway. -* -* If this runs in parallel with dbs_work_handler(), we -* may end up overwriting the sample_delay_ns value that -* it has just written, but the difference should not be -* too big and it will be corrected next time a sample -* is taken, so it shouldn't be significant. -*/ -gov_update_sample_delay (policy_dbs, new_rate); -mutex_unlock (&policy_dbs->timer_mutex); - } +gov_update_sample_delay (policy_dbs, new_rate); +mutex_unlock (&policy_dbs->timer_mutex); } - -mutex_unlock (&dbs_data_mutex); } static bool invalid_up_threshold(struct dbs_data *dbs_data,
Signature · 1 line
2.7.1.370.gb2aa7f8
Rafael J. Wysocki
maintainerRe: [PATCH V3 12/13] cpufreq: ondemand: Traverse list of policy_dbs in update_sampling_rate()
Quoted 98 lines · Viresh Kumar
Now that we maintain a list of all 'struct
policy_dbs_info' for an instance of 'structdbs_data', we can traverse that instead of traversing the loop for all online CPUs.This also solves the circular dependency lockdep reported by Juri (and verified by Shilpa) earlier:
======================================================
[ INFO: possible circular locking dependency detected ]
4.4.0+ #445 Not tainted
-------------------------------------------------------
trace.sh/1723 is trying to acquire lock:
(s_active#48){++++.+}, at: [<c01f78c8>]kernfs_remove_by_name_ns+0x4c/0x94but task is already holding lock:
(od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>]cpufreq_governor_dbs+0x34/0x5d4which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (od_dbs_cdata.mutex){+.+.+.}:
[<c075b040>] mutex_lock_nested+0x7c/0x434 [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4 [<c0017c10>] return_to_handler+0x0/0x18-> #1 (&policy->rwsem){+++++.}:
[<c075ca8c>] down_read+0x58/0x94 [<c057c244>] show+0x30/0x60 [<c01f934c>] sysfs_kf_seq_show+0x90/0xfc [<c01f7ad8>] kernfs_seq_show+0x34/0x38 [<c01a22ec>] seq_read+0x1e4/0x4e4 [<c01f8694>] kernfs_fop_read+0x120/0x1a0 [<c01794b4>] __vfs_read+0x3c/0xe0 [<c017a378>] vfs_read+0x98/0x104 [<c017a434>] SyS_read+0x50/0x90 [<c000fd40>] ret_fast_syscall+0x0/0x1c-> #0 (s_active#48){++++.+}:
[<c008238c>] lock_acquire+0xd4/0x20c [<c01f6ae4>] __kernfs_remove+0x288/0x328 [<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94 [<c01fa024>] remove_files+0x44/0x88 [<c01fa5a4>] sysfs_remove_group+0x50/0xa4 [<c058285c>] cpufreq_governor_dbs+0x3f0/0x5d4 [<c0017c10>] return_to_handler+0x0/0x18other info that might help us debug this:
Chain exists of: s_active#48 --> &policy->rwsem --> od_dbs_cdata.mutex Possible unsafe locking scenario:CPU0 CPU1 ---- ---- lock(od_dbs_cdata.mutex); lock(&policy->rwsem); lock(od_dbs_cdata.mutex);
lock(s_active#48);*** DEADLOCK ***
5 locks held by trace.sh/1723:
#0: (sb_writers#6){.+.+.+}, at: [<c017beb8>] __sb_start_write+0xb4/0xc0 #1: (&of->mutex){+.+.+.}, at: [<c01f8418>] kernfs_fop_write+0x6c/0x1c8 #2: (s_active#35){.+.+.+}, at: [<c01f8420>] kernfs_fop_write+0x74/0x1c8 #3: (cpu_hotplug.lock){++++++}, at: [<c0029e6c>] get_online_cpus+0x48/0xb8 #4: (od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4stack backtrace: CPU: 2 PID: 1723 Comm: trace.sh Not tainted 4.4.0+ #445 Hardware name: ARM-Versatile Express [<c001883c>] (unwind_backtrace) from [<c0013f50>] (show_stack+0x20/0x24) [<c0013f50>] (show_stack) from [<c044ad90>] (dump_stack+0x80/0xb4) [<c044ad90>] (dump_stack) from [<c0128edc>] (print_circular_bug+0x29c/0x2f0) [<c0128edc>] (print_circular_bug) from [<c0081708>] (__lock_acquire+0x163c/0x1d74) [<c0081708>] (__lock_acquire) from [<c008238c>] (lock_acquire+0xd4/0x20c) [<c008238c>] (lock_acquire) from [<c01f6ae4>] (__kernfs_remove+0x288/0x328) [<c01f6ae4>] (__kernfs_remove) from [<c01f78c8>] (kernfs_remove_by_name_ns+0x4c/0x94) [<c01f78c8>] (kernfs_remove_by_name_ns) from [<c01fa024>] (remove_files+0x44/0x88) [<c01fa024>] (remove_files) from [<c01fa5a4>] (sysfs_remove_group+0x50/0xa4) [<c01fa5a4>] (sysfs_remove_group) from [<c058285c>] (cpufreq_governor_dbs+0x3f0/0x5d4) [<c058285c>] (cpufreq_governor_dbs) from [<c0017c10>] (return_to_handler+0x0/0x18)Reported-by Juri Lelli <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_ondemand.c | 89 ++++++++++---------------------------- 1 file changed, 22 insertions(+), 67 deletions(-)drivers/cpufreq/cpufreq_ondemand.c +0
-0@@ -224,83 +224,38 @@ static struct dbs_governor od_dbs_gov;/** * update_sampling_rate - update sampling rate effective immediately if needed.
drivers/cpufreq/cpufreq_ondemand.c static struct dbs_governor od_dbs_gov; · line 235
Quoted 10 lines
drivers/cpufreq/cpufreq_ondemand.c +0
-9* @new_rate: new sampling rate -* -* If new rate is smaller than the old, simply updating -* dbs_tuners_int.sampling_rate might not be appropriate. For example, if the -* original sampling_rate was 1 second and the requested new sampling rate is 10 -* ms because the user needs immediate reaction from ondemand governor, but not -* sure if higher frequency will be required or not, then, the governor may -* change the sampling rate too late; up to 1 second later. Thus, if we are -* reducing the sampling rate, we need to make the new value effective -* immediately.
The comment still applies.
Moreover, please extend it to say that this must be called with dbs_data->mutex held (or it looks racy otherwise).
Thanks,
Rafael
Quoted 208 lines · Viresh Kumar
Now that we maintain a list of all 'struct
policy_dbs_info' for an instance of 'structdbs_data', we can traverse that instead of traversing the loop for all online CPUs.This also solves the circular dependency lockdep reported by Juri (and verified by Shilpa) earlier:
======================================================
[ INFO: possible circular locking dependency detected ]
4.4.0+ #445 Not tainted
-------------------------------------------------------
trace.sh/1723 is trying to acquire lock:
(s_active#48){++++.+}, at: [<c01f78c8>]kernfs_remove_by_name_ns+0x4c/0x94but task is already holding lock:
(od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>]cpufreq_governor_dbs+0x34/0x5d4which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (od_dbs_cdata.mutex){+.+.+.}:
[<c075b040>] mutex_lock_nested+0x7c/0x434 [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4 [<c0017c10>] return_to_handler+0x0/0x18-> #1 (&policy->rwsem){+++++.}:
[<c075ca8c>] down_read+0x58/0x94 [<c057c244>] show+0x30/0x60 [<c01f934c>] sysfs_kf_seq_show+0x90/0xfc [<c01f7ad8>] kernfs_seq_show+0x34/0x38 [<c01a22ec>] seq_read+0x1e4/0x4e4 [<c01f8694>] kernfs_fop_read+0x120/0x1a0 [<c01794b4>] __vfs_read+0x3c/0xe0 [<c017a378>] vfs_read+0x98/0x104 [<c017a434>] SyS_read+0x50/0x90 [<c000fd40>] ret_fast_syscall+0x0/0x1c-> #0 (s_active#48){++++.+}:
[<c008238c>] lock_acquire+0xd4/0x20c [<c01f6ae4>] __kernfs_remove+0x288/0x328 [<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94 [<c01fa024>] remove_files+0x44/0x88 [<c01fa5a4>] sysfs_remove_group+0x50/0xa4 [<c058285c>] cpufreq_governor_dbs+0x3f0/0x5d4 [<c0017c10>] return_to_handler+0x0/0x18other info that might help us debug this:
Chain exists of: s_active#48 --> &policy->rwsem --> od_dbs_cdata.mutex Possible unsafe locking scenario:CPU0 CPU1 ---- ---- lock(od_dbs_cdata.mutex); lock(&policy->rwsem); lock(od_dbs_cdata.mutex);
lock(s_active#48);*** DEADLOCK ***
5 locks held by trace.sh/1723:
#0: (sb_writers#6){.+.+.+}, at: [<c017beb8>] __sb_start_write+0xb4/0xc0 #1: (&of->mutex){+.+.+.}, at: [<c01f8418>] kernfs_fop_write+0x6c/0x1c8 #2: (s_active#35){.+.+.+}, at: [<c01f8420>] kernfs_fop_write+0x74/0x1c8 #3: (cpu_hotplug.lock){++++++}, at: [<c0029e6c>] get_online_cpus+0x48/0xb8 #4: (od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4stack backtrace: CPU: 2 PID: 1723 Comm: trace.sh Not tainted 4.4.0+ #445 Hardware name: ARM-Versatile Express [<c001883c>] (unwind_backtrace) from [<c0013f50>] (show_stack+0x20/0x24) [<c0013f50>] (show_stack) from [<c044ad90>] (dump_stack+0x80/0xb4) [<c044ad90>] (dump_stack) from [<c0128edc>] (print_circular_bug+0x29c/0x2f0) [<c0128edc>] (print_circular_bug) from [<c0081708>] (__lock_acquire+0x163c/0x1d74) [<c0081708>] (__lock_acquire) from [<c008238c>] (lock_acquire+0xd4/0x20c) [<c008238c>] (lock_acquire) from [<c01f6ae4>] (__kernfs_remove+0x288/0x328) [<c01f6ae4>] (__kernfs_remove) from [<c01f78c8>] (kernfs_remove_by_name_ns+0x4c/0x94) [<c01f78c8>] (kernfs_remove_by_name_ns) from [<c01fa024>] (remove_files+0x44/0x88) [<c01fa024>] (remove_files) from [<c01fa5a4>] (sysfs_remove_group+0x50/0xa4) [<c01fa5a4>] (sysfs_remove_group) from [<c058285c>] (cpufreq_governor_dbs+0x3f0/0x5d4) [<c058285c>] (cpufreq_governor_dbs) from [<c0017c10>] (return_to_handler+0x0/0x18)Reported-by Juri Lelli <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_ondemand.c | 89 ++++++++++---------------------------- 1 file changed, 22 insertions(+), 67 deletions(-)drivers/cpufreq/cpufreq_ondemand.c +22
-68@@ -224,83 +224,38 @@ static struct dbs_governor od_dbs_gov;/** * update_sampling_rate - update sampling rate effective immediately if needed. * @new_rate: new sampling rate -* -* If new rate is smaller than the old, simply updating -* dbs_tuners_int.sampling_rate might not be appropriate. For example, if the -* original sampling_rate was 1 second and the requested new sampling rate is 10 -* ms because the user needs immediate reaction from ondemand governor, but not -* sure if higher frequency will be required or not, then, the governor may -* change the sampling rate too late; up to 1 second later. Thus, if we are -* reducing the sampling rate, we need to make the new value effective -* immediately. */ static void update_sampling_rate (struct dbs_data *dbs_data) { -struct cpumask cpumask; +struct policy_dbs_info *policy_dbs;unsigned int new_rate = dbs_data->sampling_rate; -int cpu;/* -* Lock governor so that governor start/stop can't execute in parallel. +* We are operating under dbs_data->mutex and so the list and its +* entries can't be freed concurrently. */ -mutex_lock (&dbs_data_mutex); - -cpumask_copy (&cpumask, cpu_online_mask); - -for_each_cpu (cpu, &cpumask) { -struct cpufreq_policy *policy; -struct od_cpu_dbs_info_s *dbs_info; -struct cpu_dbs_info *cdbs; -struct policy_dbs_info *policy_dbs; - - dbs_info = &per_cpu (od_cpu_dbs_info, cpu); - cdbs = &dbs_info->cdbs; - policy_dbs = cdbs->policy_dbs; - +list_for_each_entry (policy_dbs, &dbs_data->policy_dbs_list, list) { +mutex_lock (&policy_dbs->timer_mutex);/* -* A valid policy_dbs means governor hasn't stopped or exited -* yet. +* On 32-bit architectures this may race with the +* sample_delay_ns read in dbs_update_util_handler(), but that +* really doesn't matter. If the read returns a value that's +* too big, the sample will be skipped, but the next invocation +* of dbs_update_util_handler() (when the update has been +* completed) will take a sample. If the returned value is too +* small, the sample will be taken immediately, but that isn't a +* problem, as we want the new rate to take effect immediately +* anyway. +* +* If this runs in parallel with dbs_work_handler(), we may end +* up overwriting the sample_delay_ns value that it has just +* written, but the difference should not be too big and it will +* be corrected next time a sample is taken, so it shouldn't be +* significant. */ -if (!policy_dbs) -continue ; - - policy = policy_dbs->policy; - -/* clear all CPUs of this policy */ -cpumask_andnot (&cpumask, &cpumask, policy->cpus); - -/* -* Update sampling rate for CPUs whose policy is governed by -* dbs_data. In case of governor_per_policy, only a single -* policy will be governed by dbs_data, otherwise there can be -* multiple policies that are governed by the same dbs_data. -*/ -if (dbs_data == policy_dbs->dbs_data) { -mutex_lock (&policy_dbs->timer_mutex); -/* -* On 32-bit architectures this may race with the -* sample_delay_ns read in dbs_update_util_handler(), -* but that really doesn't matter. If the read returns -* a value that's too big, the sample will be skipped, -* but the next invocation of dbs_update_util_handler() -* (when the update has been completed) will take a -* sample. If the returned value is too small, the -* sample will be taken immediately, but that isn't a -* problem, as we want the new rate to take effect -* immediately anyway. -* -* If this runs in parallel with dbs_work_handler(), we -* may end up overwriting the sample_delay_ns value that -* it has just written, but the difference should not be -* too big and it will be corrected next time a sample -* is taken, so it shouldn't be significant. -*/ -gov_update_sample_delay (policy_dbs, new_rate); -mutex_unlock (&policy_dbs->timer_mutex); - } +gov_update_sample_delay (policy_dbs, new_rate); +mutex_unlock (&policy_dbs->timer_mutex); } - -mutex_unlock (&dbs_data_mutex); } static bool invalid_up_threshold(struct dbs_data *dbs_data, --2.7.1.370.gb2aa7f8
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Viresh Kumar
maintainerRe: [PATCH V3 12/13] cpufreq: ondemand: Traverse list of policy_dbs in update_sampling_rate()
Quoted 10 lines · Rafael J. Wysocki
Quoted 8 lines
- * If new rate is smaller than the old, simply updating
- * dbs_tuners_int.sampling_rate might not be appropriate. For example, if the
- * originalsampling_ratewas 1 second and the requested new sampling rate is 10
- * ms because the user needs immediate reaction from ondemand governor, but not
- * sure if higher frequency will be required or not, then, the governor may
- * change the sampling rate too late; up to 1 second later. Thus, if we are
- * reducing the sampling rate, we need to make the new value effective
- * immediately.The comment still applies.
Why? It talks about the case where we have reduced sampling rate, but that's not the case anymore. We *always* update sample_delay_ns now.
Quoted 2 lines
Moreover, please extend it to say that this must be called with
dbs_data->mutexheld (or it looks racy otherwise).
Yeah, that can be done.
Rafael J. Wysocki
maintainerRe: [PATCH V3 12/13] cpufreq: ondemand: Traverse list of policy_dbs in update_sampling_rate()
Quoted 10 lines · Viresh Kumar
Quoted 9 lines · Rafael J. Wysocki
Quoted 8 lines
- * If new rate is smaller than the old, simply updating
- * dbs_tuners_int.sampling_rate might not be appropriate. For example, if the
- * originalsampling_ratewas 1 second and the requested new sampling rate is 10
- * ms because the user needs immediate reaction from ondemand governor, but not
- * sure if higher frequency will be required or not, then, the governor may
- * change the sampling rate too late; up to 1 second later. Thus, if we are
- * reducing the sampling rate, we need to make the new value effective
- * immediately.
Quoted 4 lines
Quoted 1 line
The comment still applies.
Why? It talks about the case where we have reduced sampling rate, but that's not the case anymore. We *always* update
sample_delay_nsnow.
But the comment explains *why* we do that, doesn't it?
If it doesn't apply, then why do we need this function at all?
Quoted 4 lines
Quoted 2 lines
Moreover, please extend it to say that this must be called with
dbs_data->mutexheld (or it looks racy otherwise).Yeah, that can be done.
OK
Thanks,
Rafael
Viresh Kumar
maintainerRe: [PATCH V3 12/13] cpufreq: ondemand: Traverse list of policy_dbs in update_sampling_rate()
In reply to Rafael J. WysockiQuoted 4 lines · Rafael J. Wysocki
The comment still applies.
Moreover, please extend it to say that this must be called with
dbs_data->mutexheld (or it looks racy otherwise).
Modified it as:
+ *
+ * Simply updating dbs_tuners_int.sampling_rate might not be appropriate here.
+ * For example, if the original sampling_rate was 1 second and the requested new
+ * sampling rate is 10 ms because the user needs immediate reaction from
+ * ondemand governor, otherwise the governor may change the sampling rate too
+ * late; up to 1 second later.
+ *
+ * Similar logic applies while increasing the sampling rate. And so we need to
+ * update it with immediate effect.
+ *
+ * This must be called with dbs_data->mutex held, otherwise traversing
+ * policy_dbs_list isn't safe.
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerRe: [PATCH V3 12/13] cpufreq: ondemand: Traverse list of policy_dbs in update_sampling_rate()
Quoted 6 lines · Viresh Kumar
Quoted 4 lines · Rafael J. Wysocki
The comment still applies.
Moreover, please extend it to say that this must be called with
dbs_data->mutexheld (or it looks racy otherwise).
Quoted 8 lines
Modified it as:
+ *
+ * Simply updating dbs_tuners_int.sampling_rate might not be appropriate here.
+ * For example, if the originalsampling_ratewas 1 second and the requested new
+ * sampling rate is 10 ms because the user needs immediate reaction from
+ * ondemand governor, otherwise the governor may change the sampling rate too
+ * late; up to 1 second later.
The "otherwise" doesn't seem to be necessary here.
Quoted 3 lines
+ *
+ * Similar logic applies while increasing the sampling rate. And so we need to
+ * update it with immediate effect.
Actually, no, it doesn't apply. If you increase the sampling rate, the governor will never be late. It may be early, but that's fine in this case.
It just doesn't hurt to update immediately in this case too.
Quoted 3 lines
+ *
+ * This must be called withdbs_data->mutexheld, otherwise traversing
+ *policy_dbs_listisn't safe.
Thanks,
Rafael
Rafael J. Wysocki
maintainerRe: [PATCH V3 12/13] cpufreq: ondemand: Traverse list of policy_dbs in update_sampling_rate()
Quoted 22 lines · Rafael J. Wysocki
Quoted 6 lines · Viresh Kumar
Quoted 4 lines · Rafael J. Wysocki
The comment still applies.
Moreover, please extend it to say that this must be called with
dbs_data->mutexheld (or it looks racy otherwise).Quoted 8 lines
Modified it as:
+ *
+ * Simply updating dbs_tuners_int.sampling_rate might not be appropriate here.
+ * For example, if the originalsampling_ratewas 1 second and the requested new
+ * sampling rate is 10 ms because the user needs immediate reaction from
+ * ondemand governor, otherwise the governor may change the sampling rate too
+ * late; up to 1 second later.The "otherwise" doesn't seem to be necessary here.
Quoted 3 lines
+ *
+ * Similar logic applies while increasing the sampling rate. And so we need to
+ * update it with immediate effect.
Quoted 9 lines
Actually, no, it doesn't apply. If you increase the sampling rate, the governor will never be late. It may be early, but that's fine in this case.
It just doesn't hurt to update immediately in this case too.
Quoted 3 lines
+ *
+ * This must be called withdbs_data->mutexheld, otherwise traversing
+ *policy_dbs_listisn't safe.
I really don't know what's wrong with retaining the original paragraph and adding the above sentence after it.
Thanks,
Rafael
Viresh Kumar
maintainerPATCH v3 13/13 cpufreq: conservative: Update sample_delay_ns immediately
In reply to Viresh KumarOndemand governor already updates sample_delay_ns immediately on updates to sampling rate, but conservative isn't doing that.
It was left out earlier as the code has been really complex to get that done easily. But now things are sorted out very well, and we can follow the same for conservative governor as well.
--- drivers/cpufreq/cpufreq_governor.c | 30 +++++++++++++++++++++++++++--- drivers/cpufreq/cpufreq_governor.h | 1 - drivers/cpufreq/cpufreq_ondemand.c | 38 -------------------------------------- 3 files changed, 27 insertions(+), 42 deletions(-)
drivers/cpufreq/cpufreq_governor.c +27 -3
@@ -29,7 +29,7 @@ EXPORT_SYMBOL_GPL(dbs_data_mutex);static ssize_t store_sampling_rate (struct dbs_data *dbs_data,const char *buf,size_t count) { -struct dbs_governor *gov = dbs_data->gov; +struct policy_dbs_info *policy_dbs;unsigned int rate;int ret; ret =sscanf (buf,"%u" , &rate);
@@ -38,8 +38,32 @@ static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const char *buf, dbs_data->sampling_rate =max (rate, dbs_data->min_sampling_rate); -if (gov->update_sampling_rate) - gov->update_sampling_rate (dbs_data); +/* +* We are operating under dbs_data->mutex and so the list and its +* entries can't be freed concurrently. +*/ +list_for_each_entry (policy_dbs, &dbs_data->policy_dbs_list, list) { +mutex_lock (&policy_dbs->timer_mutex); +/* +* On 32-bit architectures this may race with the +* sample_delay_ns read in dbs_update_util_handler(), but that +* really doesn't matter. If the read returns a value that's +* too big, the sample will be skipped, but the next invocation +* of dbs_update_util_handler() (when the update has been +* completed) will take a sample. If the returned value is too +* small, the sample will be taken immediately, but that isn't a +* problem, as we want the new rate to take effect immediately +* anyway. +* +* If this runs in parallel with dbs_work_handler(), we may end +* up overwriting the sample_delay_ns value that it has just +* written, but the difference should not be too big and it will +* be corrected next time a sample is taken, so it shouldn't be +* significant. +*/ +gov_update_sample_delay (policy_dbs, dbs_data->sampling_rate); +mutex_unlock (&policy_dbs->timer_mutex); + }return count; }
drivers/cpufreq/cpufreq_governor.h +0 -1
@@ -212,7 +212,6 @@ struct dbs_governor {void (*exit)(struct dbs_data *dbs_data,bool notify);bool (*invalid_up_threshold)(struct dbs_data *dbs_data,unsigned int threshold);bool (*invalid_sampling_down_factor)(unsigned int factor); -void (*update_sampling_rate)(struct dbs_data *dbs_data);void (*update_sampling_down_factor)(struct dbs_data *dbs_data);void (*update_ignore_nice_load)(struct dbs_data *dbs_data);
drivers/cpufreq/cpufreq_ondemand.c +0 -38
@@ -221,43 +221,6 @@ static unsigned int od_dbs_timer(struct cpufreq_policy *policy)/************************** sysfs interface ************************/ static struct dbs_governor od_dbs_gov; -/** -* update_sampling_rate - update sampling rate effective immediately if needed. -* @new_rate: new sampling rate -*/ -static void update_sampling_rate (struct dbs_data *dbs_data) -{ -struct policy_dbs_info *policy_dbs; -unsigned int new_rate = dbs_data->sampling_rate; - -/* -* We are operating under dbs_data->mutex and so the list and its -* entries can't be freed concurrently. -*/ -list_for_each_entry (policy_dbs, &dbs_data->policy_dbs_list, list) { -mutex_lock (&policy_dbs->timer_mutex); -/* -* On 32-bit architectures this may race with the -* sample_delay_ns read in dbs_update_util_handler(), but that -* really doesn't matter. If the read returns a value that's -* too big, the sample will be skipped, but the next invocation -* of dbs_update_util_handler() (when the update has been -* completed) will take a sample. If the returned value is too -* small, the sample will be taken immediately, but that isn't a -* problem, as we want the new rate to take effect immediately -* anyway. -* -* If this runs in parallel with dbs_work_handler(), we may end -* up overwriting the sample_delay_ns value that it has just -* written, but the difference should not be too big and it will -* be corrected next time a sample is taken, so it shouldn't be -* significant. -*/ -gov_update_sample_delay (policy_dbs, new_rate); -mutex_unlock (&policy_dbs->timer_mutex); - } -} -static bool invalid_up_threshold (struct dbs_data *dbs_data,unsigned int threshold) {
@@ -425,7 +388,6 @@ static struct dbs_governor od_dbs_gov = { .get_cpu_dbs_info_s = get_cpu_dbs_info_s, .gov_dbs_timer = od_dbs_timer, .gov_check_cpu = od_check_cpu, - .update_sampling_rate = update_sampling_rate, .invalid_up_threshold = invalid_up_threshold, .invalid_sampling_down_factor = invalid_sampling_down_factor, .update_sampling_down_factor = update_sampling_down_factor,
Signature · 1 line
2.7.1.370.gb2aa7f8
Hi,
Quoted 9 lines · Viresh Kumar
Hi Rafael,
Things look much much better now. I have rebased this series over pm/bleeding-edge, that has all your patches.
I have moved ahead and done few more changes in this series, that should get rid of all the lockdeps we were getting earlier, that includes fixing lockdep issue in
update_sampling_rate()that we were chasing.
Quoted 5 lines
These are pushed here again:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.gitcpufreq/governor-kobject@Juri/Shilpa: Can you please confirm if all the issues got resolved now ?
I have tested this patchset on Power8 box and did not find any lockdep warnings.
Thanks and Regards,
Shilpa
Viresh Kumar
maintainerQuoted 12 lines · Shilpasri G Bhat
Hi,
Quoted 9 lines · Viresh Kumar
Hi Rafael,
Things look much much better now. I have rebased this series over pm/bleeding-edge, that has all your patches.
I have moved ahead and done few more changes in this series, that should get rid of all the lockdeps we were getting earlier, that includes fixing lockdep issue in
update_sampling_rate()that we were chasing.
Quoted 7 lines
Quoted 5 lines
These are pushed here again:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.gitcpufreq/governor-kobject@Juri/Shilpa: Can you please confirm if all the issues got resolved now ?
I have tested this patchset on Power8 box and did not find any lockdep warnings.
Awesome news Shilpa. Thanks a lot for helping us out :)
Signature · 1 line
viresh
Hi,
Quoted 13 lines · Viresh Kumar
Quoted 12 lines · Shilpasri G Bhat
Hi,
Quoted 9 lines · Viresh Kumar
Hi Rafael,
Things look much much better now. I have rebased this series over pm/bleeding-edge, that has all your patches.
I have moved ahead and done few more changes in this series, that should get rid of all the lockdeps we were getting earlier, that includes fixing lockdep issue in
update_sampling_rate()that we were chasing.
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
These are pushed here again:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.gitcpufreq/governor-kobject@Juri/Shilpa: Can you please confirm if all the issues got resolved now ?
I have tested this patchset on Power8 box and did not find any lockdep warnings.
Everything looks good on TC2/Juno as well :).
Best,
- Juri
Viresh Kumar
maintainerQuoted 19 lines · Juri Lelli
Hi,
Quoted 16 lines · Viresh Kumar
Quoted 15 lines · Shilpasri G Bhat
Hi,
Quoted 12 lines · Viresh Kumar
Hi Rafael,
Things look much much better now. I have rebased this series over pm/bleeding-edge, that has all your patches.
I have moved ahead and done few more changes in this series, that should get rid of all the lockdeps we were getting earlier, that includes fixing lockdep issue in
update_sampling_rate()that we were chasing.These are pushed here again:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.gitcpufreq/governor-kobject
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
@Juri/Shilpa: Can you please confirm if all the issues got resolved now ?
I have tested this patchset on Power8 box and did not find any lockdep warnings.
Everything looks good on TC2/Juno as well :).
Yeah, we finally solved the puzzle :)
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerHi,
Quoted 34 lines · Viresh Kumar
Hi Rafael,
Things look much much better now. I have rebased this series over pm/bleeding-edge, that has all your patches.
I have moved ahead and done few more changes in this series, that should get rid of all the lockdeps we were getting earlier, that includes fixing lockdep issue in
update_sampling_rate()that we were chasing.These are pushed here again:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.gitcpufreq/governor-kobject@Juri/Shilpa: Can you please confirm if all the issues got resolved now ?
V2->V3:
- The first patch from V2, that was movingmin_sampling_rateto per governor structure was dropped, as you suggested.
- Also, I have moved common tunables to structdbs_datanow, which you also suggested sometime back.
- Last 5 patches are all new and fix rest of the issues we were facing.--
vireshViresh Kumar (13):
cpufreq: governor: Create generic macro for global tuners
cpufreq: governor: Move common tunables to 'structdbs_data'
cpufreq: governor: New sysfs show/store callbacks for governor
tunables
cpufreq: governor: Drop unused macros for creating governor tunable
attributes
Revert "cpufreq: Drop rwsem lock aroundCPUFREQ_GOV_POLICY_EXIT"
cpufreq: Merge cpufreq_offline_prepare/finish routines
Quoted 8 lines
cpufreq: Call
__cpufreq_governor()withpolicy->rwsemheld
cpufreq: Removecpufreq_governor_lock
cpufreq: governor: Move common sysfs tunables tocpufreq_governor.c
cpufreq: governor: No need to manage state machine now
cpufreq: governor: Keep list ofpolicy_dbswithindbs_data
cpufreq: ondemand: Traverse list ofpolicy_dbsin
update_sampling_rate()
cpufreq: conservative: Updatesample_delay_nsimmediately
This is OK overall, but we'll need to reorder it somewhat.
I have reviewed patches [1-5,12/13]. Where I didn't send comments, I had none.
In my opinion, those 6 patches should go in first. At least I don't see why [12/13] cannot be reworked on top of [1-5/13]. If there is any fundamental reason, please let me know what it is. Otherwise, please first send new versions of those 6 patches, preferably as a new series.
The rest of the patchset will require more review time, so please send them again later, when you're done with the first item.
Thanks,
Rafael