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

[Rafael J. Wysocki]

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.wysocki@xxxxxxxxx>
> > 
> > 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.wysocki@xxxxxxxxx>
> > ---
> > 
> > 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.

---
From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
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 there to prevent those routines
from racing with the policy notifier.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
---
 drivers/cpufreq/cpufreq_stats.c |   14 +++++++++-----
 1 file changed, 9 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
@@ -154,7 +154,9 @@ static void cpufreq_stats_free_table(uns
 	if (!policy)
 		return;
 
+	down_write(&policy->rwsem);
 	__cpufreq_stats_free_table(policy);
+	up_write(&policy->rwsem);
 
 	cpufreq_cpu_put(policy);
 }
@@ -238,7 +240,9 @@ static void cpufreq_stats_create_table(u
 	if (likely(!policy))
 		return;
 
+	down_write(&policy->rwsem);
 	__cpufreq_stats_create_table(policy);
+	up_write(&policy->rwsem);
 
 	cpufreq_cpu_put(policy);
 }
@@ -322,7 +326,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 +334,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 +348,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);
 }