lkml.org 
[lkml]   [2016]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] cpufreq: governor: Fix race in dbs_update_util_handler()
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

\
 
 \ /
  Last update: 2016-02-22 14:21    [W:0.071 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site