lkml.org 
[lkml]   [2016]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked
On 20-05-16, 23:33, Rafael J. Wysocki wrote:
> The policy rwsem is really only needed in cpufreq_stats_create_table(), because
> the policy notifier is gone when _free_table() runs, so another version of the
> patch goes below.

Right. I saw that while reading your previous version but didn't reply
because I wanted to do a more careful review.

The first issue I have here is that the _init and _exit routines in
cpufreq-stats aren't opposite of each other. Which shouldn't be the
case.

> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] cpufreq: stats: Fix race conditions on init and cleanup
>
> Loops over online CPUs in cpufreq_stats_init() and cpufreq_stats_exit()
> are not carried out with CPU offline/online locked, so races are
> possible with respect to policy initialization and cleanup.
>
> To prevent that from happening, change the loops to walk all possible
> CPUs, as cpufreq_stats_create_table() and cpufreq_stats_free_table()
> handle the case when there's no policy for the given CPU cleanly, but
> also use policy->rwsem in cpufreq_stats_create_table() to prevent it
> from racing with the policy notifier.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpufreq/cpufreq_stats.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq_stats.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_stats.c
> +++ linux-pm/drivers/cpufreq/cpufreq_stats.c
> @@ -238,7 +238,13 @@ static void cpufreq_stats_create_table(u
> if (likely(!policy))
> return;
>
> + /*
> + * The policy notifier may run in parallel with this code, so use the
> + * policy rwsem to avoid racing with it.
> + */
> + down_write(&policy->rwsem);
> __cpufreq_stats_create_table(policy);
> + up_write(&policy->rwsem);

I am still trying to understand why we will ever have a race here. We
might have it, but I just want to know how.

This is what we do in on addition of a policy:
- send the CREATE notifier
- Add policy to the list

So, the notifiers are guaranteed to complete before the policy is
present in the list.

CPU 0 CPU 1
notifier cpufreq_stats_init()
CREATE-POLICY X cpufreq_stats_create_table()
__cpufreq_stats_create_table() cpufreq_cpu_get()

AFAICT, whatever may happen, __cpufreq_stats_create_table() will *not*
get called in parallel for the same policy.

If __cpufreq_stats_create_table() is in progress on CPU0, CPU 1 will
not find the policy with cpufreq_cpu_get(). And if cpufreq_cpu_get()
finds a policy, the notifier would already have completed.

What do you say ?

--
viresh

\
 
 \ /
  Last update: 2016-05-23 06:41    [W:0.178 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site