| Message ID | 0447fedc2d07cd35eed66d7de97ad4e0940b6110.1420177186.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 4 January 2015 at 04:13, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > This is pants, sorry. I am sorry :( > Whenever you change locking, you need to know exactly (a) what is wrong with > the way it is currently done and (b) how you are going to improve it. All of > that needs to be described here. Thanks for kicking me here.. > For example, you seem to be thinking that locking is missing from > __cpufreq_stats_free_table(), so you are adding it in there. Thus you need to > describe (a) why it is missing (eg. what races are possible because of that) > and (b) why adding it the way you do is going to improve the situation. Yeah, that's what I thought. That race between create/free of stats will be eliminated by this.. But now as you forced me to write it properly, I am failing to understand why do we need to have any locking in place for stats ? These are the reasons why I think we can remove all locking stuff from stats: 1.) create/free calls to stats can't run in parallel. They are all sequential. It happens with: - cpu hotplug: which is sequential with proper locking in place. - driver unregister: again sequential - stats unregister: again sequential So, actually we will never try to allocate stats for a single policy in parallel threads. Same goes for freeing as well.. 2.) Any race possible with sysfs reads ? These routines are called from show() routine from cpufreq.c and it has policy locks on the way. So, policy can't go away and so will stats. Also, if stats unregister we will unregister the notifiers first. And that will again make things fall in line. What else are we protecting? Maybe the calls to cpufreq_stats_update() can happen in parallel and so we may need locking only within that routine. I might be missing the obvious logic, but then what exactly ? -- viresh -- 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 6 January 2015 at 05:05, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > Hard to say from the top of my head and I'm not sure if I have the time to > have a deeper look into that during the next few days. Ok, let me resend the patches then with better improved logs. And lets see if something is still missing. -- 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
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index de55ca86b6f1..95867985ea02 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -150,10 +150,12 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy) pr_debug("%s: Free stat table\n", __func__); + mutex_lock(&cpufreq_stats_lock); sysfs_remove_group(&policy->kobj, &stats_attr_group); kfree(stat->time_in_state); kfree(stat); policy->stats_data = NULL; + mutex_unlock(&cpufreq_stats_lock); } static void cpufreq_stats_free_table(unsigned int cpu) @@ -182,13 +184,17 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) if (unlikely(!table)) return 0; + mutex_lock(&cpufreq_stats_lock); + /* stats already initialized */ - if (policy->stats_data) - return -EEXIST; + if (policy->stats_data) { + ret = -EEXIST; + goto unlock; + } stat = kzalloc(sizeof(*stat), GFP_KERNEL); if (!stat) - return -ENOMEM; + goto unlock; /* Find total allocation size */ cpufreq_for_each_valid_entry(pos, table) @@ -219,21 +225,21 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) stat->freq_table[i++] = pos->frequency; stat->state_num = i; - mutex_lock(&cpufreq_stats_lock); stat->last_time = get_jiffies_64(); stat->last_index = freq_table_get_index(stat, policy->cur); - mutex_unlock(&cpufreq_stats_lock); policy->stats_data = stat; ret = sysfs_create_group(&policy->kobj, &stats_attr_group); if (!ret) - return 0; + goto unlock; /* We failed, release resources */ policy->stats_data = NULL; kfree(stat->time_in_state); free_stat: kfree(stat); +unlock: + mutex_unlock(&cpufreq_stats_lock); return ret; }