| Message ID | 2172360.cldhrkXzeh@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 08-02-16, 03:08, Rafael J. Wysocki wrote: > Moreover, update_sampling_rate() doesn't need to walk the cpu_dbs_infos, > it may walk policies instead. Like after the (untested) appended patch. > > Then, if we have a governor_data_lock in struct policy, we can use that > to protect policy_dbs while it is being access there and we're done. > > I'll try to prototype something along these lines tomorrow. I have solved that in a different way, and dropped the lock from update_sampling_rate(). Please see if that looks good.
On Mon, Feb 8, 2016 at 12:52 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 08-02-16, 03:08, Rafael J. Wysocki wrote: >> Moreover, update_sampling_rate() doesn't need to walk the cpu_dbs_infos, >> it may walk policies instead. Like after the (untested) appended patch. >> >> Then, if we have a governor_data_lock in struct policy, we can use that >> to protect policy_dbs while it is being access there and we're done. >> >> I'll try to prototype something along these lines tomorrow. > > I have solved that in a different way, and dropped the lock from > update_sampling_rate(). Please see if that looks good. Well, almost. I like the list approach, but you need to be careful about it. Let me comment more on the patches in the series. I have a gut feeling that my idea of walking policies will end up being simpler in the end, but let's see. :-) Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 8, 2016 at 1:52 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Mon, Feb 8, 2016 at 12:52 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> On 08-02-16, 03:08, Rafael J. Wysocki wrote: >>> Moreover, update_sampling_rate() doesn't need to walk the cpu_dbs_infos, >>> it may walk policies instead. Like after the (untested) appended patch. >>> >>> Then, if we have a governor_data_lock in struct policy, we can use that >>> to protect policy_dbs while it is being access there and we're done. >>> >>> I'll try to prototype something along these lines tomorrow. >> >> I have solved that in a different way, and dropped the lock from >> update_sampling_rate(). Please see if that looks good. > > Well, almost. > > I like the list approach, but you need to be careful about it. Let me > comment more on the patches in the series. > > I have a gut feeling that my idea of walking policies will end up > being simpler in the end, but let's see. :-) Well, my gut feeling seems to have been incorrect, as often happens. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c =================================================================== --- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c +++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c @@ -254,34 +254,23 @@ static void update_sampling_rate(struct cpumask_copy(&cpumask, cpu_online_mask); for_each_cpu(cpu, &cpumask) { - struct cpufreq_policy *policy; - struct od_cpu_dbs_info_s *dbs_info; - struct cpu_dbs_info *cdbs; + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); struct policy_dbs_info *policy_dbs; - dbs_info = &per_cpu(od_cpu_dbs_info, cpu); - cdbs = &dbs_info->cdbs; - policy_dbs = cdbs->policy_dbs; - - /* - * A valid policy_dbs and policy_dbs->policy means governor - * hasn't stopped or exited yet. - */ - if (!policy_dbs || !policy_dbs->policy) + if (!policy) continue; - policy = policy_dbs->policy; - /* clear all CPUs of this policy */ cpumask_andnot(&cpumask, &cpumask, policy->cpus); + policy_dbs = policy->governor_data; /* * Update sampling rate for CPUs whose policy is governed by * dbs_data. In case of governor_per_policy, only a single * policy will be governed by dbs_data, otherwise there can be * multiple policies that are governed by the same dbs_data. */ - if (dbs_data == policy_dbs->dbs_data) { + if (policy_dbs && policy_dbs->dbs_data == dbs_data) { mutex_lock(&policy_dbs->timer_mutex); /* * On 32-bit architectures this may race with the @@ -304,6 +293,8 @@ static void update_sampling_rate(struct gov_update_sample_delay(policy_dbs, new_rate); mutex_unlock(&policy_dbs->timer_mutex); } + + cpufreq_cpu_put(policy); } mutex_unlock(&dbs_data_mutex);