On 03/14/2014 01:13 PM, Viresh Kumar wrote:
Whenever we are changing frequency of a cpu, we are calling PRECHANGE and POSTCHANGE notifiers. They must be serialized. i.e. PRECHANGE or POSTCHANGE shouldn't be called twice continuously. Following examples show why this is important:
[...]
This was discussed earlier here: https://lkml.org/lkml/2013/9/25/402
Where Rafael asked for better fix, as he called the V1 fixes: "quick and dirty". This is another approach, much simpler than previous one. Please see if this looks fine. There is a TODO note in there as I wanted some suggestions on how exactly should we wait for a transition to get over.
drivers/cpufreq/cpufreq.c | 39 +++++++++++++++++++++++++++++++++++++-- include/linux/cpufreq.h | 2 ++ 2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2677ff1..66bdfff 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -324,6 +324,13 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy, } }
+static void notify_transition_for_each_cpu(struct cpufreq_policy *policy,
struct cpufreq_freqs *freqs, unsigned int state)+{
- for_each_cpu(freqs->cpu, policy->cpus)
__cpufreq_notify_transition(policy, freqs, state);+}
/**
- cpufreq_notify_transition - call notifier chain and adjust_jiffies
- on frequency transition.
@@ -335,8 +342,35 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy, void cpufreq_notify_transition(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, unsigned int state) {
- for_each_cpu(freqs->cpu, policy->cpus)
__cpufreq_notify_transition(policy, freqs, state);
- if ((state != CPUFREQ_PRECHANGE) && (state != CPUFREQ_POSTCHANGE))
Wait a min, when is this condition ever true? I mean, what else can 'state' ever be, apart from CPUFREQ_PRECHANGE and POSTCHANGE?
return notify_transition_for_each_cpu(policy, freqs, state);- /* Serialize pre-post notifications */
- mutex_lock(&policy->transition_lock);
Nope, this is definitely not the way to go, IMHO. We should enforce that the *callers* serialize the transitions, something like this:
cpufreq_transition_lock();
cpufreq_notify_transition(CPUFREQ_PRECHANGE);
//Perform the frequency change
cpufreq_notify_transition(CPUFREQ_POSTCHANGE);
cpufreq_transition_unlock();
That's it!
[ We can either introduce a new "transition" lock or perhaps even reuse the cpufreq_driver_lock if it fits... but the point is, the _caller_ has to perform the locking; trying to be smart inside cpufreq_notify_transition() is a recipe for headache :-( ]
Is there any problem with this approach due to which you didn't take this route?
- if (unlikely(WARN_ON(!policy->transition_ongoing &&
(state == CPUFREQ_POSTCHANGE)))) {mutex_unlock(&policy->transition_lock);return;- }
- if (state == CPUFREQ_PRECHANGE) {
while (policy->transition_ongoing) {mutex_unlock(&policy->transition_lock);/* TODO: Can we do something better here? */cpu_relax();mutex_lock(&policy->transition_lock);
If the caller takes care of the synchronization, we can avoid these sorts of acrobatics ;-)
Regards, Srivatsa S. Bhat
}policy->transition_ongoing = true;mutex_unlock(&policy->transition_lock);- }
- notify_transition_for_each_cpu(policy, freqs, state);
- if (state == CPUFREQ_POSTCHANGE) {
policy->transition_ongoing = false;mutex_unlock(&policy->transition_lock);- }
} EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
@@ -983,6 +1017,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
INIT_LIST_HEAD(&policy->policy_list); init_rwsem(&policy->rwsem);
mutex_init(&policy->transition_lock);
return policy;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 31c431e..e5cebce 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -104,6 +104,8 @@ struct cpufreq_policy { * __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); */ struct rw_semaphore rwsem;
- bool transition_ongoing; /* Tracks transition status */
- struct mutex transition_lock;
};
/* Only for ACPI */