Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked

[Rafael J. Wysocki]

On Friday, May 20, 2016 02:13:26 PM Rafael J. Wysocki wrote:
> On Friday, May 20, 2016 07:52:47 AM Viresh Kumar wrote:
> > On 20-05-16, 03:41, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> > > 
> > > Loops over online CPUs in cpufreq_stats_init() and cpufreq_stats_exit()
> > > should be carried out with CPU offline/online locked or races are
> > > possible otherwise, so make that happen.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> > > ---
> > > 
> > > v1 -> v2: On a second thought, add the policy notifier in 
> > > cpufreq_stats_init()
> > >   with CPU offline/online locked too.
> > > 
> > > ---
> > >  drivers/cpufreq/cpufreq_stats.c |   16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > Index: linux-pm/drivers/cpufreq/cpufreq_stats.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/cpufreq/cpufreq_stats.c
> > > +++ linux-pm/drivers/cpufreq/cpufreq_stats.c
> > > @@ -317,10 +317,13 @@ static int __init cpufreq_stats_init(voi
> > >   unsigned int cpu;
> > >  
> > >   spin_lock_init(&cpufreq_stats_lock);
> > > +
> > > + get_online_cpus();
> > > +
> > >   ret = cpufreq_register_notifier(&notifier_policy_block,
> > >                           CPUFREQ_POLICY_NOTIFIER);
> > 
> > Why is this required to be protected ?
> 
> Last night I thought I saw a scenario in which that notifier could run
> in parallel with the loop below even with get_online_cpus() between them,
> but I don't see it right now.
> 
> Maybe I should not look at stuff late in the night ...
> 
> > >   if (ret)
> > > -         return ret;
> > > +         goto out;
> > >  
> > >   for_each_online_cpu(cpu)
> > >           cpufreq_stats_create_table(cpu);
> > > @@ -332,21 +335,28 @@ static int __init cpufreq_stats_init(voi
> > >                           CPUFREQ_POLICY_NOTIFIER);
> > >           for_each_online_cpu(cpu)
> > >                   cpufreq_stats_free_table(cpu);
> > 
> > Maybe we can make this for_each_possible_cpu() then, and so getting a
> > policy will fail for CPUs which aren't online.
> > 
> > And we wouldn't need to use get_online_cpus() then ?
> 
> That could be done, but then there would be nothing to prevent the
> policy notifier from running in parallel with the loop.
> 
> Something like the patch below should do the trick, though.

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.

---
From: Rafael J. Wysocki <rafael.j.wyso...@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.wyso...@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);
 
        cpufreq_cpu_put(policy);
 }
@@ -322,7 +328,7 @@ static int __init cpufreq_stats_init(voi
        if (ret)
                return ret;
 
-       for_each_online_cpu(cpu)
+       for_each_possible_cpu(cpu)
                cpufreq_stats_create_table(cpu);
 
        ret = cpufreq_register_notifier(&notifier_trans_block,
@@ -330,12 +336,11 @@ static int __init cpufreq_stats_init(voi
        if (ret) {
                cpufreq_unregister_notifier(&notifier_policy_block,
                                CPUFREQ_POLICY_NOTIFIER);
-               for_each_online_cpu(cpu)
+               for_each_possible_cpu(cpu)
                        cpufreq_stats_free_table(cpu);
-               return ret;
        }
 
-       return 0;
+       return ret;
 }
 static void __exit cpufreq_stats_exit(void)
 {
@@ -345,7 +350,8 @@ static void __exit cpufreq_stats_exit(vo
                        CPUFREQ_POLICY_NOTIFIER);
        cpufreq_unregister_notifier(&notifier_trans_block,
                        CPUFREQ_TRANSITION_NOTIFIER);
-       for_each_online_cpu(cpu)
+
+       for_each_possible_cpu(cpu)
                cpufreq_stats_free_table(cpu);
 }