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 */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
