The field 'transition_task' of policy structure is used to track the
task which is performing the frequency transition. Using this field to
print a warning once detect a case where the same task is calling
_begin() again before completing the preivous frequency transition via
the _end().
However, there is a potential race condition in _end() and _begin() APIs
while updating the field 'transition_task' of policy, the scenario is
depicted below:
Task A Task B
/* 1st freq transition */
Invoke _begin() {
...
...
}
/* 2nd freq transition */
Invoke _begin() {
... //waiting for A to
... //clear
... //transition_ongoing
... //in _end() for
... //the 1st transition
|
Change the frequency |
|
Invoke _end() { |
... |
... |
transition_ongoing = false; V
transition_ongoing = true;
transition_task = current;
transition_task = NULL;
... //A overwrites the task
... //performing the transition
... //result in error warning.
}
To fix this race condition, the order of the updates to the
'transition_task' and 'transition_ongoing' fields has been changed, the
'transition_task' field is now cleared before the 'transition_ongoing'
field, which ensure that only one task can update the 'transition_task'
field at a time.
Fixes: ca654dc3a93d ("cpufreq: Catch double invocations of cpufreq_freq_transition_begin/end")
Signed-off-by: Liao Chang <[email protected]>
---
drivers/cpufreq/cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a757f90aa9d6..f8eb6dde57f2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -455,8 +455,8 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
policy->cur,
policy->cpuinfo.max_freq);
- policy->transition_ongoing = false;
policy->transition_task = NULL;
+ policy->transition_ongoing = false;
wake_up(&policy->transition_wait);
}
--
2.34.1
On Mon, Aug 28, 2023 at 10:52 AM Viresh Kumar <[email protected]> wrote:
>
> On 28-08-23, 16:29, Liao, Chang wrote:
> > Task B does not necessarily go to sleep when it calls wait_event(), it depends on
> > the condition to wait for evaluate false or not. So there is a small race window
> > where Task A already set 'transition_ongoing' to false and Task B can cross wait_event()
> > immediately.
> >
> > wait_event:
> > do {
> > might_sleep();
> > if (condition) // !transition_ongoing
> > break;
> > __wait_event();
> > };
> >
> > I hope I do not miss something important in the code above.
>
> > Yes, if the CPU uses weak memroy model, it is possible for the instructions to be reordered.
> > therefore, it is a good idea to insert an smb() between these two lines if there is race here.
>
> Maybe it would be better to do this instead ?
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6b52ebe5a890..f11b01b25e8d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -455,8 +455,10 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
> policy->cur,
> policy->cpuinfo.max_freq);
>
> + spin_lock(&policy->transition_lock);
> policy->transition_ongoing = false;
> policy->transition_task = NULL;
> + spin_unlock(&policy->transition_lock);
>
> wake_up(&policy->transition_wait);
> }
>
> --
I was about to suggest the same thing.
wake_up() is a full memory barrier only if it actually wakes up a task
and if it doesn't do that, without the locking the other task may see
a state in which transition_ongoing is false already and
transition_task is still NULL regardless of the relative ordering of
the statements before the wake_up() call.
在 2023/8/28 16:52, Viresh Kumar 写道:
> On 28-08-23, 16:29, Liao, Chang wrote:
>> Task B does not necessarily go to sleep when it calls wait_event(), it depends on
>> the condition to wait for evaluate false or not. So there is a small race window
>> where Task A already set 'transition_ongoing' to false and Task B can cross wait_event()
>> immediately.
>>
>> wait_event:
>> do {
>> might_sleep();
>> if (condition) // !transition_ongoing
>> break;
>> __wait_event();
>> };
>>
>> I hope I do not miss something important in the code above.
>
>> Yes, if the CPU uses weak memroy model, it is possible for the instructions to be reordered.
>> therefore, it is a good idea to insert an smb() between these two lines if there is race here.
>
> Maybe it would be better to do this instead ?
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6b52ebe5a890..f11b01b25e8d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -455,8 +455,10 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
> policy->cur,
> policy->cpuinfo.max_freq);
>
> + spin_lock(&policy->transition_lock);
> policy->transition_ongoing = false;
> policy->transition_task = NULL;
> + spin_unlock(&policy->transition_lock);
I think it is more straightforward, I will use it in next revision.
Thanks.
>
> wake_up(&policy->transition_wait);
> }
>
--
BR
Liao, Chang