| Message ID | 532AA7A8.3040508@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On 20 March 2014 14:02, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote: > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 199b52b..5283f10 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -349,6 +349,39 @@ void cpufreq_notify_post_transition(struct cpufreq_policy *policy, > EXPORT_SYMBOL_GPL(cpufreq_notify_post_transition); > > > +void cpufreq_freq_transition_begin(struct cpufreq_policy *policy, > + struct cpufreq_freqs *freqs, unsigned int state) > +{ > +wait: > + wait_event(&policy->transition_wait, !policy->transition_ongoing); > + > + mutex_lock(&policy->transition_lock); > + > + if (policy->transition_ongoing) { > + mutex_unlock(&policy->transition_lock); > + goto wait; > + } > + > + policy->transition_ongoing = true; > + > + mutex_unlock(&policy->transition_lock); > + > + cpufreq_notify_transition(policy, freqs, CPUFREQ_PRECHANGE); > +} > + > +void cpufreq_freq_transition_end(struct cpufreq_policy *policy, > + struct cpufreq_freqs *freqs, unsigned int state) > +{ > + cpufreq_notify_transition(policy, freqs, CPUFREQ_POSTCHANGE); > + > + mutex_lock(&policy->transition_lock); Why do we need locking here? You explained that earlier :) Also, I would like to add this here: WARN_ON(policy->transition_ongoing); > + policy->transition_ongoing = false; > + mutex_unlock(&policy->transition_lock); > + > + wake_up(&policy->transition_wait); > +} -- 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 03/20/2014 02:07 PM, Viresh Kumar wrote: > On 20 March 2014 14:02, Srivatsa S. Bhat > <srivatsa.bhat@linux.vnet.ibm.com> wrote: >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 199b52b..5283f10 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -349,6 +349,39 @@ void cpufreq_notify_post_transition(struct cpufreq_policy *policy, >> EXPORT_SYMBOL_GPL(cpufreq_notify_post_transition); >> >> >> +void cpufreq_freq_transition_begin(struct cpufreq_policy *policy, >> + struct cpufreq_freqs *freqs, unsigned int state) >> +{ >> +wait: >> + wait_event(&policy->transition_wait, !policy->transition_ongoing); >> + >> + mutex_lock(&policy->transition_lock); >> + >> + if (policy->transition_ongoing) { >> + mutex_unlock(&policy->transition_lock); >> + goto wait; >> + } >> + >> + policy->transition_ongoing = true; >> + >> + mutex_unlock(&policy->transition_lock); >> + >> + cpufreq_notify_transition(policy, freqs, CPUFREQ_PRECHANGE); >> +} >> + >> +void cpufreq_freq_transition_end(struct cpufreq_policy *policy, >> + struct cpufreq_freqs *freqs, unsigned int state) >> +{ >> + cpufreq_notify_transition(policy, freqs, CPUFREQ_POSTCHANGE); >> + >> + mutex_lock(&policy->transition_lock); > > Why do we need locking here? You explained that earlier :) > Hmm.. I had thought of some complex race condition which would make tasks miss the wake-up event and sleep forever, and hence added the locking there to prevent that. But now that I think more closely, I'm not able to recall that race... I will give some more thought to it and if I can't find any loopholes in doing the second update to the ongoing flag without locks, then I'll post the patchset with that lockless version itself. > Also, I would like to add this here: > > WARN_ON(policy->transition_ongoing); > Hmm? Won't it always be true? We are the ones who set that flag to true earlier, right? I guess you meant WARN_ON(!policy->transition_ongoing) perhaps? I'm not sure whether its really worth it, because it kinda looks obvious. Not sure what kind of bugs it would catch. I can't think of any such scenario :-( >> + policy->transition_ongoing = false; >> + mutex_unlock(&policy->transition_lock); >> + >> + wake_up(&policy->transition_wait); >> +} > Regards, Srivatsa S. Bhat -- 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 20 March 2014 14:54, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote: > On 03/20/2014 02:07 PM, Viresh Kumar wrote: >> WARN_ON(policy->transition_ongoing); >> > > I guess you meant WARN_ON(!policy->transition_ongoing) > perhaps? Ooops!! > I'm not sure whether its really worth it, because it kinda looks > obvious. Not sure what kind of bugs it would catch. I can't think of any > such scenario :-( Just to catch if somebody is sending a POSTCHANGE one without first sending a PRECHANGE one.. Just another check to make sure things are in order. -- 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 03/20/2014 03:03 PM, Viresh Kumar wrote: > On 20 March 2014 14:54, Srivatsa S. Bhat > <srivatsa.bhat@linux.vnet.ibm.com> wrote: >> On 03/20/2014 02:07 PM, Viresh Kumar wrote: >>> WARN_ON(policy->transition_ongoing); >>> >> >> I guess you meant WARN_ON(!policy->transition_ongoing) >> perhaps? > > Ooops!! > >> I'm not sure whether its really worth it, because it kinda looks >> obvious. Not sure what kind of bugs it would catch. I can't think of any >> such scenario :-( > > Just to catch if somebody is sending a POSTCHANGE one without first > sending a PRECHANGE one.. Just another check to make sure things are > in order. > Well, that's unlikely, since they will have to call _end() before _begin() :-) That's the power of having great function names - they make it impossible to use them incorrectly ;-) But anyway, I can add the check, just in case somebody misses even such an obvious cue! :-) By the way, I'm also thinking of using a spinlock instead of a mutex. The critical section is tiny and we don't sleep inside the critical section - sounds like the perfect case for a spinlock. Regards, Srivatsa S. Bhat -- 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 20 March 2014 15:15, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote: > By the way, I'm also thinking of using a spinlock instead of a mutex. > The critical section is tiny and we don't sleep inside the critical > section - sounds like the perfect case for a spinlock. Probably yes. -- 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.c b/drivers/cpufreq/cpufreq.c index 199b52b..5283f10 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -349,6 +349,39 @@ void cpufreq_notify_post_transition(struct cpufreq_policy *policy, EXPORT_SYMBOL_GPL(cpufreq_notify_post_transition); +void cpufreq_freq_transition_begin(struct cpufreq_policy *policy, + struct cpufreq_freqs *freqs, unsigned int state) +{ +wait: + wait_event(&policy->transition_wait, !policy->transition_ongoing); + + mutex_lock(&policy->transition_lock); + + if (policy->transition_ongoing) { + mutex_unlock(&policy->transition_lock); + goto wait; + } + + policy->transition_ongoing = true; + + mutex_unlock(&policy->transition_lock); + + cpufreq_notify_transition(policy, freqs, CPUFREQ_PRECHANGE); +} + +void cpufreq_freq_transition_end(struct cpufreq_policy *policy, + struct cpufreq_freqs *freqs, unsigned int state) +{ + cpufreq_notify_transition(policy, freqs, CPUFREQ_POSTCHANGE); + + mutex_lock(&policy->transition_lock); + policy->transition_ongoing = false; + mutex_unlock(&policy->transition_lock); + + wake_up(&policy->transition_wait); +} + + /********************************************************************* * SYSFS INTERFACE * *********************************************************************/ @@ -968,6 +1001,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void) INIT_LIST_HEAD(&policy->policy_list); init_rwsem(&policy->rwsem); + mutex_init(&policy->transition_lock); + init_waitqueue_head(&policy->transition_wait); return policy; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 4d89e0e..8bded24 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -101,6 +101,11 @@ struct cpufreq_policy { * __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); */ struct rw_semaphore rwsem; + + /* Synchronization for frequency transitions */ + bool transition_ongoing; /* Tracks transition status */ + struct mutex transition_lock; + wait_queue_head_t transition_wait; }; /* Only for ACPI */