Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1338842 > unrolled thread

[PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()

Started by"Rafael J. Wysocki" <rjw@rjwysocki.net>
First post2016-02-21 03:20 +0100
Last post2016-02-22 14:50 +0100
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-21 03:20 +0100
    Re: [PATCH 1/2] cpufreq: governor: Fix race in  dbs_update_util_handler() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-22 06:30 +0100
      Re: [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler() "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-22 13:30 +0100
        Re: [PATCH 1/2] cpufreq: governor: Fix race in  dbs_update_util_handler() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-22 14:10 +0100
          Re: [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler() "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-22 14:40 +0100
    [PATCH v2 1/2] cpufreq: governor: Fix race in dbs_update_util_handler() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-22 14:20 +0100
      Re: [PATCH v2 1/2] cpufreq: governor: Fix race in  dbs_update_util_handler() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-22 14:50 +0100

#1338842 — [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2016-02-21 03:20 +0100
Subject[PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()
Message-ID<r4rMR-799-1@gated-at.bofh.it>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is a scenarion that may lead to undesired results in
dbs_update_util_handler().  Namely, if two CPUs sharing a policy
enter the funtion at the same time, pass the sample delay check
and then one of them is stalled until dbs_work_handler() (queued
up by the other CPU) clears the work counter, it may update the
work counter and queue up another work item prematurely.

To prevent that from happening, use the observation that the CPU
queuing up a work item in dbs_update_util_handler() updates the
last sample time.  This means that if another CPU was stalling after
passing the sample delay check and now successfully updated the work
counter as a result of the race described above, it will see the new
value of the last sample time which is different from what it used in
the sample delay check before.  If that happens, the sample delay
check passed previously is not valid any more, so the CPU should not
continue, but leaving the funtion at that point might miss an
opportunity to take the next sample, so simply clear the work
counter and jump to the beginning of the function in that case.

Fixes: f17cbb53783c (cpufreq: governor: Avoid atomic operations in hot paths)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_governor.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -341,8 +341,9 @@ static void dbs_update_util_handler(stru
 {
 	struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
 	struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
-	u64 delta_ns;
+	u64 delta_ns, lst;
 
+ start:
 	/*
 	 * The work may not be allowed to be queued up right now.
 	 * Possible reasons:
@@ -357,7 +358,8 @@ static void dbs_update_util_handler(stru
 	 * of sample_delay_ns used in the computation may be stale.
 	 */
 	smp_rmb();
-	delta_ns = time - policy_dbs->last_sample_time;
+	lst = ACCESS_ONCE(policy_dbs->last_sample_time);
+	delta_ns = time - lst;
 	if ((s64)delta_ns < policy_dbs->sample_delay_ns)
 		return;
 
@@ -366,9 +368,19 @@ static void dbs_update_util_handler(stru
 	 * at this point.  Otherwise, we need to ensure that only one of the
 	 * CPUs sharing the policy will do that.
 	 */
-	if (policy_dbs->is_shared &&
-	    !atomic_add_unless(&policy_dbs->work_count, 1, 1))
-		return;
+	if (policy_dbs->is_shared) {
+		if (!atomic_add_unless(&policy_dbs->work_count, 1, 1))
+			return;
+
+		/*
+		 * If another CPU updated last_sample_time in the meantime, we
+		 * shouldn't be here, so clear the work counter and try again.
+		 */
+		if (unlikely(lst != ACCESS_ONCE(policy_dbs->last_sample_time))) {
+			atomic_set(&policy_dbs->work_count, 0);
+			goto start;
+		}
+	}
 
 	policy_dbs->last_sample_time = time;
 	policy_dbs->work_in_progress = true;

[toc] | [next] | [standalone]


#1339136 — Re: [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-02-22 06:30 +0100
SubjectRe: [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()
Message-ID<r4Reh-Ny-5@gated-at.bofh.it>
In reply to#1338842
On 21-02-16, 03:14, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There is a scenarion that may lead to undesired results in

             scenario

> dbs_update_util_handler().  Namely, if two CPUs sharing a policy
> enter the funtion at the same time, pass the sample delay check
> and then one of them is stalled until dbs_work_handler() (queued
> up by the other CPU) clears the work counter, it may update the
> work counter and queue up another work item prematurely.
> 
> To prevent that from happening, use the observation that the CPU
> queuing up a work item in dbs_update_util_handler() updates the
> last sample time.  This means that if another CPU was stalling after
> passing the sample delay check and now successfully updated the work
> counter as a result of the race described above, it will see the new
> value of the last sample time which is different from what it used in
> the sample delay check before.  If that happens, the sample delay
> check passed previously is not valid any more, so the CPU should not
> continue, but leaving the funtion at that point might miss an
> opportunity to take the next sample, so simply clear the work
> counter and jump to the beginning of the function in that case.
> 
> Fixes: f17cbb53783c (cpufreq: governor: Avoid atomic operations in hot paths)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_governor.c |   22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -341,8 +341,9 @@ static void dbs_update_util_handler(stru
>  {
>  	struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
>  	struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
> -	u64 delta_ns;
> +	u64 delta_ns, lst;
>  
> + start:
>  	/*
>  	 * The work may not be allowed to be queued up right now.
>  	 * Possible reasons:
> @@ -357,7 +358,8 @@ static void dbs_update_util_handler(stru
>  	 * of sample_delay_ns used in the computation may be stale.
>  	 */
>  	smp_rmb();
> -	delta_ns = time - policy_dbs->last_sample_time;
> +	lst = ACCESS_ONCE(policy_dbs->last_sample_time);

The comment on the top of ACCESS_ONCE() asks us to use READ_ONCE() if possible.

> +	delta_ns = time - lst;
>  	if ((s64)delta_ns < policy_dbs->sample_delay_ns)
>  		return;
>  
> @@ -366,9 +368,19 @@ static void dbs_update_util_handler(stru
>  	 * at this point.  Otherwise, we need to ensure that only one of the
>  	 * CPUs sharing the policy will do that.
>  	 */
> -	if (policy_dbs->is_shared &&
> -	    !atomic_add_unless(&policy_dbs->work_count, 1, 1))
> -		return;
> +	if (policy_dbs->is_shared) {
> +		if (!atomic_add_unless(&policy_dbs->work_count, 1, 1))
> +			return;
> +
> +		/*
> +		 * If another CPU updated last_sample_time in the meantime, we
> +		 * shouldn't be here, so clear the work counter and try again.
> +		 */
> +		if (unlikely(lst != ACCESS_ONCE(policy_dbs->last_sample_time))) {
> +			atomic_set(&policy_dbs->work_count, 0);
> +			goto start;
> +		}

I think we should be doing this here:

	delta_ns = time - ACCESS_ONCE(policy_dbs->last_sample_time);
 	if ((s64)delta_ns < policy_dbs->sample_delay_ns) {
                atomic_set(&policy_dbs->work_count, 0);
 		return;
        }

There is no point running the routine again, we already have ->work_count
incremented for us, lets do the check right now.

Over that, in theory, with your code, it is possible that one of the CPU can get
stuck in the goto->start loop indefinitely :)

> +	}
>  
>  	policy_dbs->last_sample_time = time;
>  	policy_dbs->work_in_progress = true;

-- 
viresh

[toc] | [prev] | [next] | [standalone]


#1339444

From"Rafael J. Wysocki" <rafael@kernel.org>
Date2016-02-22 13:30 +0100
Message-ID<r4XMK-5Nd-19@gated-at.bofh.it>
In reply to#1339136
On Mon, Feb 22, 2016 at 6:23 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 21-02-16, 03:14, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> There is a scenarion that may lead to undesired results in
>
>              scenario
>
>> dbs_update_util_handler().  Namely, if two CPUs sharing a policy
>> enter the funtion at the same time, pass the sample delay check
>> and then one of them is stalled until dbs_work_handler() (queued
>> up by the other CPU) clears the work counter, it may update the
>> work counter and queue up another work item prematurely.
>>
>> To prevent that from happening, use the observation that the CPU
>> queuing up a work item in dbs_update_util_handler() updates the
>> last sample time.  This means that if another CPU was stalling after
>> passing the sample delay check and now successfully updated the work
>> counter as a result of the race described above, it will see the new
>> value of the last sample time which is different from what it used in
>> the sample delay check before.  If that happens, the sample delay
>> check passed previously is not valid any more, so the CPU should not
>> continue, but leaving the funtion at that point might miss an
>> opportunity to take the next sample, so simply clear the work
>> counter and jump to the beginning of the function in that case.
>>
>> Fixes: f17cbb53783c (cpufreq: governor: Avoid atomic operations in hot paths)
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  drivers/cpufreq/cpufreq_governor.c |   22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
>> @@ -341,8 +341,9 @@ static void dbs_update_util_handler(stru
>>  {
>>       struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
>>       struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
>> -     u64 delta_ns;
>> +     u64 delta_ns, lst;
>>
>> + start:
>>       /*
>>        * The work may not be allowed to be queued up right now.
>>        * Possible reasons:
>> @@ -357,7 +358,8 @@ static void dbs_update_util_handler(stru
>>        * of sample_delay_ns used in the computation may be stale.
>>        */
>>       smp_rmb();
>> -     delta_ns = time - policy_dbs->last_sample_time;
>> +     lst = ACCESS_ONCE(policy_dbs->last_sample_time);
>
> The comment on the top of ACCESS_ONCE() asks us to use READ_ONCE() if possible.

I forgot about this one, thanks!

>> +     delta_ns = time - lst;
>>       if ((s64)delta_ns < policy_dbs->sample_delay_ns)
>>               return;
>>
>> @@ -366,9 +368,19 @@ static void dbs_update_util_handler(stru
>>        * at this point.  Otherwise, we need to ensure that only one of the
>>        * CPUs sharing the policy will do that.
>>        */
>> -     if (policy_dbs->is_shared &&
>> -         !atomic_add_unless(&policy_dbs->work_count, 1, 1))
>> -             return;
>> +     if (policy_dbs->is_shared) {
>> +             if (!atomic_add_unless(&policy_dbs->work_count, 1, 1))
>> +                     return;
>> +
>> +             /*
>> +              * If another CPU updated last_sample_time in the meantime, we
>> +              * shouldn't be here, so clear the work counter and try again.
>> +              */
>> +             if (unlikely(lst != ACCESS_ONCE(policy_dbs->last_sample_time))) {
>> +                     atomic_set(&policy_dbs->work_count, 0);
>> +                     goto start;
>> +             }
>
> I think we should be doing this here:
>
>         delta_ns = time - ACCESS_ONCE(policy_dbs->last_sample_time);
>         if ((s64)delta_ns < policy_dbs->sample_delay_ns) {
>                 atomic_set(&policy_dbs->work_count, 0);
>                 return;
>         }
>
> There is no point running the routine again, we already have ->work_count
> incremented for us, lets do the check right now.

No, we need to check work_in_progress too.

> Over that, in theory, with your code, it is possible that one of the CPU can get
> stuck in the goto->start loop indefinitely :)

Good point (although that's not very likely to happen in practice).

There's one more problem, however.  The value of time is already stale
at this point, so going to start and keeping the time value unmodified
is a mistake.

Let me respin the patch.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]


#1339463 — Re: [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-02-22 14:10 +0100
SubjectRe: [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()
Message-ID<r4Ypt-6jn-45@gated-at.bofh.it>
In reply to#1339444
On 22-02-16, 13:26, Rafael J. Wysocki wrote:
> On Mon, Feb 22, 2016 at 6:23 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 21-02-16, 03:14, Rafael J. Wysocki wrote:
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> There is a scenarion that may lead to undesired results in
> >
> >              scenario
> >
> >> dbs_update_util_handler().  Namely, if two CPUs sharing a policy
> >> enter the funtion at the same time, pass the sample delay check
> >> and then one of them is stalled until dbs_work_handler() (queued
> >> up by the other CPU) clears the work counter, it may update the
> >> work counter and queue up another work item prematurely.
> >>
> >> To prevent that from happening, use the observation that the CPU
> >> queuing up a work item in dbs_update_util_handler() updates the
> >> last sample time.  This means that if another CPU was stalling after
> >> passing the sample delay check and now successfully updated the work
> >> counter as a result of the race described above, it will see the new
> >> value of the last sample time which is different from what it used in
> >> the sample delay check before.  If that happens, the sample delay
> >> check passed previously is not valid any more, so the CPU should not
> >> continue, but leaving the funtion at that point might miss an
> >> opportunity to take the next sample, so simply clear the work
> >> counter and jump to the beginning of the function in that case.
> >>
> >> Fixes: f17cbb53783c (cpufreq: governor: Avoid atomic operations in hot paths)
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>  drivers/cpufreq/cpufreq_governor.c |   22 +++++++++++++++++-----
> >>  1 file changed, 17 insertions(+), 5 deletions(-)
> >>
> >> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> >> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> >> @@ -341,8 +341,9 @@ static void dbs_update_util_handler(stru
> >>  {
> >>       struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
> >>       struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
> >> -     u64 delta_ns;
> >> +     u64 delta_ns, lst;
> >>
> >> + start:
> >>       /*
> >>        * The work may not be allowed to be queued up right now.
> >>        * Possible reasons:
> >> @@ -357,7 +358,8 @@ static void dbs_update_util_handler(stru
> >>        * of sample_delay_ns used in the computation may be stale.
> >>        */
> >>       smp_rmb();
> >> -     delta_ns = time - policy_dbs->last_sample_time;
> >> +     lst = ACCESS_ONCE(policy_dbs->last_sample_time);
> >
> > The comment on the top of ACCESS_ONCE() asks us to use READ_ONCE() if possible.
> 
> I forgot about this one, thanks!
> 
> >> +     delta_ns = time - lst;
> >>       if ((s64)delta_ns < policy_dbs->sample_delay_ns)
> >>               return;
> >>
> >> @@ -366,9 +368,19 @@ static void dbs_update_util_handler(stru
> >>        * at this point.  Otherwise, we need to ensure that only one of the
> >>        * CPUs sharing the policy will do that.
> >>        */
> >> -     if (policy_dbs->is_shared &&
> >> -         !atomic_add_unless(&policy_dbs->work_count, 1, 1))
> >> -             return;
> >> +     if (policy_dbs->is_shared) {
> >> +             if (!atomic_add_unless(&policy_dbs->work_count, 1, 1))
> >> +                     return;
> >> +
> >> +             /*
> >> +              * If another CPU updated last_sample_time in the meantime, we
> >> +              * shouldn't be here, so clear the work counter and try again.
> >> +              */
> >> +             if (unlikely(lst != ACCESS_ONCE(policy_dbs->last_sample_time))) {
> >> +                     atomic_set(&policy_dbs->work_count, 0);
> >> +                     goto start;
> >> +             }
> >
> > I think we should be doing this here:
> >
> >         delta_ns = time - ACCESS_ONCE(policy_dbs->last_sample_time);
> >         if ((s64)delta_ns < policy_dbs->sample_delay_ns) {
> >                 atomic_set(&policy_dbs->work_count, 0);
> >                 return;
> >         }
> >
> > There is no point running the routine again, we already have ->work_count
> > incremented for us, lets do the check right now.
> 
> No, we need to check work_in_progress too.

Then maybe move first half of this routine into a separate function
and call it from the beginning and here ?

-- 
viresh

[toc] | [prev] | [next] | [standalone]


#1339475

From"Rafael J. Wysocki" <rafael@kernel.org>
Date2016-02-22 14:40 +0100
Message-ID<r4YSu-6vs-9@gated-at.bofh.it>
In reply to#1339463
On Mon, Feb 22, 2016 at 2:04 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 22-02-16, 13:26, Rafael J. Wysocki wrote:
>> On Mon, Feb 22, 2016 at 6:23 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 21-02-16, 03:14, Rafael J. Wysocki wrote:
>> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

[cut]

>> > I think we should be doing this here:
>> >
>> >         delta_ns = time - ACCESS_ONCE(policy_dbs->last_sample_time);
>> >         if ((s64)delta_ns < policy_dbs->sample_delay_ns) {
>> >                 atomic_set(&policy_dbs->work_count, 0);
>> >                 return;
>> >         }
>> >
>> > There is no point running the routine again, we already have ->work_count
>> > incremented for us, lets do the check right now.
>>
>> No, we need to check work_in_progress too.
>
> Then maybe move first half of this routine into a separate function
> and call it from the beginning and here ?

That won't help.   The time value is still going to be stale.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]


#1339466 — [PATCH v2 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2016-02-22 14:20 +0100
Subject[PATCH v2 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()
Message-ID<r4Yz8-6mO-13@gated-at.bofh.it>
In reply to#1338842
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is a scenario that may lead to undesired results in
dbs_update_util_handler().  Namely, if two CPUs sharing a policy
enter the funtion at the same time, pass the sample delay check
and then one of them is stalled until dbs_work_handler() (queued
up by the other CPU) clears the work counter, it may update the
work counter and queue up another work item prematurely.

To prevent that from happening, use the observation that the CPU
queuing up a work item in dbs_update_util_handler() updates the
last sample time.  This means that if another CPU was stalling after
passing the sample delay check and now successfully updated the work
counter as a result of the race described above, it will see the new
value of the last sample time which is different from what it used in
the sample delay check before.  If that happens, the sample delay
check passed previously is not valid any more, so the CPU should not
continue.

Fixes: f17cbb53783c (cpufreq: governor: Avoid atomic operations in hot paths)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Changes from v1:
- Typo in the changelog fixed.
- READ_ONCE() used instead of ACCESS_ONCE().
- If the race is detected, return instead of looping.

---
 drivers/cpufreq/cpufreq_governor.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -341,7 +341,7 @@ static void dbs_update_util_handler(stru
 {
 	struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
 	struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
-	u64 delta_ns;
+	u64 delta_ns, lst;
 
 	/*
 	 * The work may not be allowed to be queued up right now.
@@ -357,7 +357,8 @@ static void dbs_update_util_handler(stru
 	 * of sample_delay_ns used in the computation may be stale.
 	 */
 	smp_rmb();
-	delta_ns = time - policy_dbs->last_sample_time;
+	lst = READ_ONCE(policy_dbs->last_sample_time);
+	delta_ns = time - lst;
 	if ((s64)delta_ns < policy_dbs->sample_delay_ns)
 		return;
 
@@ -366,9 +367,19 @@ static void dbs_update_util_handler(stru
 	 * at this point.  Otherwise, we need to ensure that only one of the
 	 * CPUs sharing the policy will do that.
 	 */
-	if (policy_dbs->is_shared &&
-	    !atomic_add_unless(&policy_dbs->work_count, 1, 1))
-		return;
+	if (policy_dbs->is_shared) {
+		if (!atomic_add_unless(&policy_dbs->work_count, 1, 1))
+			return;
+
+		/*
+		 * If another CPU updated last_sample_time in the meantime, we
+		 * shouldn't be here, so clear the work counter and bail out.
+		 */
+		if (unlikely(lst != READ_ONCE(policy_dbs->last_sample_time))) {
+			atomic_set(&policy_dbs->work_count, 0);
+			return;
+		}
+	}
 
 	policy_dbs->last_sample_time = time;
 	policy_dbs->work_in_progress = true;

[toc] | [prev] | [next] | [standalone]


#1339484 — Re: [PATCH v2 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-02-22 14:50 +0100
SubjectRe: [PATCH v2 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()
Message-ID<r4Z2b-6yP-27@gated-at.bofh.it>
In reply to#1339466
On 22-02-16, 14:14, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There is a scenario that may lead to undesired results in
> dbs_update_util_handler().  Namely, if two CPUs sharing a policy
> enter the funtion at the same time, pass the sample delay check
> and then one of them is stalled until dbs_work_handler() (queued
> up by the other CPU) clears the work counter, it may update the
> work counter and queue up another work item prematurely.
> 
> To prevent that from happening, use the observation that the CPU
> queuing up a work item in dbs_update_util_handler() updates the
> last sample time.  This means that if another CPU was stalling after
> passing the sample delay check and now successfully updated the work
> counter as a result of the race described above, it will see the new
> value of the last sample time which is different from what it used in
> the sample delay check before.  If that happens, the sample delay
> check passed previously is not valid any more, so the CPU should not
> continue.
> 
> Fixes: f17cbb53783c (cpufreq: governor: Avoid atomic operations in hot paths)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Changes from v1:
> - Typo in the changelog fixed.
> - READ_ONCE() used instead of ACCESS_ONCE().
> - If the race is detected, return instead of looping.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web