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

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@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.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_stats.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_stats.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_stats.c
+++ linux-pm/drivers/cpufreq/cpufreq_stats.c
@@ -322,6 +322,8 @@ static int __init cpufreq_stats_init(voi
 	if (ret)
 		return ret;
 
+	get_online_cpus();
+
 	for_each_online_cpu(cpu)
 		cpufreq_stats_create_table(cpu);
 
@@ -332,21 +334,27 @@ static int __init cpufreq_stats_init(voi
 				CPUFREQ_POLICY_NOTIFIER);
 		for_each_online_cpu(cpu)
 			cpufreq_stats_free_table(cpu);
-		return ret;
 	}
 
-	return 0;
+	put_online_cpus();
+
+	return ret;
 }
 static void __exit cpufreq_stats_exit(void)
 {
 	unsigned int cpu;
 
+	get_online_cpus();
+
 	cpufreq_unregister_notifier(&notifier_policy_block,
 			CPUFREQ_POLICY_NOTIFIER);
 	cpufreq_unregister_notifier(&notifier_trans_block,
 			CPUFREQ_TRANSITION_NOTIFIER);
+
 	for_each_online_cpu(cpu)
 		cpufreq_stats_free_table(cpu);
+
+	put_online_cpus();
 }
 
 MODULE_AUTHOR("Zou Nan hai <nanhai.zou@intel.com>");

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@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.wysocki@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);
 	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);
-		return ret;
 	}
 
-	return 0;
+ out:
+	put_online_cpus();
+
+	return ret;
 }
 static void __exit cpufreq_stats_exit(void)
 {
 	unsigned int cpu;
 
+	get_online_cpus();
+
 	cpufreq_unregister_notifier(&notifier_policy_block,
 			CPUFREQ_POLICY_NOTIFIER);
 	cpufreq_unregister_notifier(&notifier_trans_block,
 			CPUFREQ_TRANSITION_NOTIFIER);
+
 	for_each_online_cpu(cpu)
 		cpufreq_stats_free_table(cpu);
+
+	put_online_cpus();
 }
 
 MODULE_AUTHOR("Zou Nan hai <nanhai.zou@intel.com>");

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

[Viresh Kumar]

On 20-05-16, 03:41, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@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.wysocki@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 ?

>  	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 ?

-- 
viresh

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

---
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 there to prevent those routines
from racing with the policy notifier.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 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);
 }
 

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.wysocki@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.wysocki@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.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);
 
 	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);
 }
 

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

[Viresh Kumar]

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

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

[Rafael J. Wysocki]

On Monday, May 23, 2016 09:27:03 AM Viresh Kumar wrote:
> 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.

I'm not sure what you mean here.

> > ---
> > 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 ?

Say cpufreq_stats_init() runs in parallel with a CPU online (say someone
loads the cpufreq_stats module and a CPU goes online at the same time,
not likely to happen, but still possible).

Then, the notifier may get invoked when the loop is in progress and because the
CPU is added to policy->cpus (and the CPU's per-CPU pointer is set to it) before
invoking the notifier, cpufreq_stats_init() may get the policy pointer for a
policy that hasn't been initialized completely yet and then run in parallel with
the notifier for that policy.

At least I don't see anything to prevent that from happening, maybe I'm overlooking
something.

Thanks,
Rafael

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

[Viresh Kumar]

On 23-05-16, 15:40, Rafael J. Wysocki wrote:
> On Monday, May 23, 2016 09:27:03 AM Viresh Kumar wrote:
> > 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.
> 
> I'm not sure what you mean here.

Sorry about that. I meant that exit() should look opposite of init() ideally,
whereas if you look at current code, both are (un)registering the
POLICY_NOTIFIER at the top.

> > 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 ?

Until now I thought you are trying to prevent the race where
__cpufreq_stats_create_table() gets called in parallel for the same policy. So,
above explains that it can't happen for sure.

> Say cpufreq_stats_init() runs in parallel with a CPU online (say someone
> loads the cpufreq_stats module and a CPU goes online at the same time,
> not likely to happen, but still possible).

Of course, that will be a design problem if it ever happens. I agree.

> Then, the notifier may get invoked when the loop is in progress and because the
> CPU is added to policy->cpus (and the CPU's per-CPU pointer is set to it) before
> invoking the notifier, cpufreq_stats_init() may get the policy pointer for a
> policy that hasn't been initialized completely yet and then run in parallel with
> the notifier for that policy.

If the policy isn't initialized fully before its added to the list, then that's
a problem in cpufreq.c I would say.

But, I don't see a problem here. The policy's kobject, etc gets initialized
fully before its added to the list or the notifier is sent for CREATE_POLICY.
Just that the governor isn't set properly, nothing else.

And if you think about it the other way round, we are sending the CREATE_POLICY
notifier right at that point where we add it to the list, and the cpufreq-stats
layer is expected to work on the policy right from that call. So, it is fully
initialized from the perspective of the stats layer. Nothing should go wrong.

-- 
viresh

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

[Rafael J. Wysocki]

On Mon, May 23, 2016 at 5:19 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 23-05-16, 15:40, Rafael J. Wysocki wrote:
>> On Monday, May 23, 2016 09:27:03 AM Viresh Kumar wrote:
>> > 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.
>>
>> I'm not sure what you mean here.
>
> Sorry about that. I meant that exit() should look opposite of init() ideally,
> whereas if you look at current code, both are (un)registering the
> POLICY_NOTIFIER at the top.

That actually sort of makes sense, though, except that the code is racy. :-)

>> > 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 ?
>
> Until now I thought you are trying to prevent the race where
> __cpufreq_stats_create_table() gets called in parallel for the same policy. So,
> above explains that it can't happen for sure.

Assuming that the loops are over online CPUs and not over possible
CPUs I suppose?

Anyway, if you are talking about the code without the patch (which I
guess is the case), the reason why it is racy is because, if
cpufreq_stats_init() runs in parallel with CPU online, the CPU going
online may be missed by it.  To my eyes that happens if
cpufreq_online() has already advanced beyond the point where the
notifier would have been invoked, but hasn't returned yet when the
for_each_online_cpu() loop in cpufreq_stats_init() is executed.

Worse yet, if a CPU goes offline when cpufreq_stats_exit() is running
and that happens exactly between the notifier unregistration and the
for_each_online_cpu() loop, the stats table will never be freed for
that CPU (say the policy isn't shared).

Switching over to loops over possible CPUs doesn't address those races
(at least not the second one), and I'm not really sure why I thought
it would address them, but adding CPU online/offline locking to
cpufreq_stats_init/exit() can address them, so it looks like the very
first version of my patch (ie.
https://patchwork.kernel.org/patch/9128509/) was actually correct,
because it didn't put too much code under the CPU offline/online
locking. :-)

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

[Viresh Kumar]

On 23-05-16, 22:47, Rafael J. Wysocki wrote:
> Assuming that the loops are over online CPUs and not over possible
> CPUs I suppose?

I wasn't focussing on that loop lately but the policy->rwsem :)

> Anyway, if you are talking about the code without the patch (which I
> guess is the case), the reason why it is racy is because, if
> cpufreq_stats_init() runs in parallel with CPU online, the CPU going
> online may be missed by it.  To my eyes that happens if
> cpufreq_online() has already advanced beyond the point where the
> notifier would have been invoked, but hasn't returned yet when the
> for_each_online_cpu() loop in cpufreq_stats_init() is executed.

Yes. That's a race we need to fix. I agree.

> Worse yet, if a CPU goes offline when cpufreq_stats_exit() is running
> and that happens exactly between the notifier unregistration and the
> for_each_online_cpu() loop, the stats table will never be freed for
> that CPU (say the policy isn't shared).

Same here.

> Switching over to loops over possible CPUs doesn't address those races
> (at least not the second one), and I'm not really sure why I thought
> it would address them, but adding CPU online/offline locking to
> cpufreq_stats_init/exit() can address them, so it looks like the very
> first version of my patch (ie.
> https://patchwork.kernel.org/patch/9128509/) was actually correct,
> because it didn't put too much code under the CPU offline/online
> locking. :-)

Well, I think there is one more way of getting all this fixed, which may
eventually look much more cleaner.

What if we update cpufreq core instead of stats with something like this:

-------------------------8<-------------------------

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 24 May 2016 10:16:25 +0530
Subject: [PATCH] cpufreq: Initiate notifiers for existing policy

Races are possible in the init/exit paths of the cpufreq-stats layer,
which may lead to 'stats' sysfs directory not getting created or removed
for some of the policies. This can happen while the policy is getting
created while cpufreq_stats_init/exit() are getting called.

To avoid adding unnecessary locks in the init/exit paths of the
cpufreq-stats layer, update the policy notifier register/unregister
routines to send notifications for any existing cpufreq policies.

Also make sure (with help of cpufreq_driver_lock) that
CPUFREQ_CREATE/REMOVE notifiers aren't getting issued in parallel from
the policy creation/removal paths.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c       | 35 +++++++++++++++++++++++++-
 drivers/cpufreq/cpufreq_stats.c | 55 ++++++-----------------------------------
 2 files changed, 41 insertions(+), 49 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c3f950f0e5f0..90f4bf03701d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1269,10 +1269,10 @@ static int cpufreq_online(unsigned int cpu)
 		ret = cpufreq_add_dev_interface(policy);
 		if (ret)
 			goto out_exit_policy;
+		write_lock_irqsave(&cpufreq_driver_lock, flags);
 		blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
 				CPUFREQ_CREATE_POLICY, policy);
 
-		write_lock_irqsave(&cpufreq_driver_lock, flags);
 		list_add(&policy->policy_list, &cpufreq_policy_list);
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
@@ -1728,6 +1728,8 @@ EXPORT_SYMBOL_GPL(cpufreq_get_driver_data);
  */
 int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list)
 {
+	struct cpufreq_policy *policy;
+	unsigned long flags;
 	int ret;
 
 	if (cpufreq_disabled())
@@ -1751,8 +1753,28 @@ int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list)
 		mutex_unlock(&cpufreq_fast_switch_lock);
 		break;
 	case CPUFREQ_POLICY_NOTIFIER:
+		write_lock_irqsave(&cpufreq_driver_lock, flags);
+
+		/* Notify about all existing policies */
+		for_each_policy(policy) {
+			nb->notifier_call(nb, CPUFREQ_CREATE_POLICY,
+					policy);
+			if (policy_is_inactive(policy))
+				continue;
+
+			nb->notifier_call(nb, CPUFREQ_START, policy);
+		}
+
 		ret = blocking_notifier_chain_register(
 				&cpufreq_policy_notifier_list, nb);
+		if (ret) {
+			/* Notify about all existing policies */
+			for_each_policy(policy) {
+				nb->notifier_call(nb, CPUFREQ_REMOVE_POLICY,
+						  policy);
+			}
+		}
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 		break;
 	default:
 		ret = -EINVAL;
@@ -1774,6 +1796,8 @@ EXPORT_SYMBOL(cpufreq_register_notifier);
  */
 int cpufreq_unregister_notifier(struct notifier_block *nb, unsigned int list)
 {
+	struct cpufreq_policy *policy;
+	unsigned long flags;
 	int ret;
 
 	if (cpufreq_disabled())
@@ -1793,6 +1817,15 @@ int cpufreq_unregister_notifier(struct notifier_block *nb, unsigned int list)
 	case CPUFREQ_POLICY_NOTIFIER:
 		ret = blocking_notifier_chain_unregister(
 				&cpufreq_policy_notifier_list, nb);
+		if (!ret) {
+			write_lock_irqsave(&cpufreq_driver_lock, flags);
+			/* Notify about all existing policies */
+			for_each_policy(policy) {
+				nb->notifier_call(nb, CPUFREQ_REMOVE_POLICY,
+						  policy);
+			}
+			write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		}
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 5e370a30a964..d4618144b4c0 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -130,7 +130,7 @@ static int freq_table_get_index(struct cpufreq_stats *stats, unsigned int freq)
 	return -1;
 }
 
-static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
+static void cpufreq_stats_free_table(struct cpufreq_policy *policy)
 {
 	struct cpufreq_stats *stats = policy->stats;
 
@@ -146,20 +146,7 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 	policy->stats = NULL;
 }
 
-static void cpufreq_stats_free_table(unsigned int cpu)
-{
-	struct cpufreq_policy *policy;
-
-	policy = cpufreq_cpu_get(cpu);
-	if (!policy)
-		return;
-
-	__cpufreq_stats_free_table(policy);
-
-	cpufreq_cpu_put(policy);
-}
-
-static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
+static int cpufreq_stats_create_table(struct cpufreq_policy *policy)
 {
 	unsigned int i = 0, count = 0, ret = -ENOMEM;
 	struct cpufreq_stats *stats;
@@ -226,23 +213,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	return ret;
 }
 
-static void cpufreq_stats_create_table(unsigned int cpu)
-{
-	struct cpufreq_policy *policy;
-
-	/*
-	 * "likely(!policy)" because normally cpufreq_stats will be registered
-	 * before cpufreq driver
-	 */
-	policy = cpufreq_cpu_get(cpu);
-	if (likely(!policy))
-		return;
-
-	__cpufreq_stats_create_table(policy);
-
-	cpufreq_cpu_put(policy);
-}
-
 static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
 		unsigned long val, void *data)
 {
@@ -250,9 +220,9 @@ static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
 	struct cpufreq_policy *policy = data;
 
 	if (val == CPUFREQ_CREATE_POLICY)
-		ret = __cpufreq_stats_create_table(policy);
+		ret = cpufreq_stats_create_table(policy);
 	else if (val == CPUFREQ_REMOVE_POLICY)
-		__cpufreq_stats_free_table(policy);
+		cpufreq_stats_free_table(policy);
 
 	return ret;
 }
@@ -314,7 +284,6 @@ static struct notifier_block notifier_trans_block = {
 static int __init cpufreq_stats_init(void)
 {
 	int ret;
-	unsigned int cpu;
 
 	spin_lock_init(&cpufreq_stats_lock);
 	ret = cpufreq_register_notifier(&notifier_policy_block,
@@ -322,31 +291,21 @@ static int __init cpufreq_stats_init(void)
 	if (ret)
 		return ret;
 
-	for_each_online_cpu(cpu)
-		cpufreq_stats_create_table(cpu);
-
 	ret = cpufreq_register_notifier(&notifier_trans_block,
 				CPUFREQ_TRANSITION_NOTIFIER);
 	if (ret) {
 		cpufreq_unregister_notifier(&notifier_policy_block,
 				CPUFREQ_POLICY_NOTIFIER);
-		for_each_online_cpu(cpu)
-			cpufreq_stats_free_table(cpu);
-		return ret;
 	}
 
-	return 0;
+	return ret;
 }
 static void __exit cpufreq_stats_exit(void)
 {
-	unsigned int cpu;
-
-	cpufreq_unregister_notifier(&notifier_policy_block,
-			CPUFREQ_POLICY_NOTIFIER);
 	cpufreq_unregister_notifier(&notifier_trans_block,
 			CPUFREQ_TRANSITION_NOTIFIER);
-	for_each_online_cpu(cpu)
-		cpufreq_stats_free_table(cpu);
+	cpufreq_unregister_notifier(&notifier_policy_block,
+			CPUFREQ_POLICY_NOTIFIER);
 }
 
 MODULE_AUTHOR("Zou Nan hai <nanhai.zou@intel.com>");

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

[Rafael J. Wysocki]

On Tuesday, May 24, 2016 10:26:30 AM Viresh Kumar wrote:
> On 23-05-16, 22:47, Rafael J. Wysocki wrote:
> > Assuming that the loops are over online CPUs and not over possible
> > CPUs I suppose?
> 
> I wasn't focussing on that loop lately but the policy->rwsem :)
> 
> > Anyway, if you are talking about the code without the patch (which I
> > guess is the case), the reason why it is racy is because, if
> > cpufreq_stats_init() runs in parallel with CPU online, the CPU going
> > online may be missed by it.  To my eyes that happens if
> > cpufreq_online() has already advanced beyond the point where the
> > notifier would have been invoked, but hasn't returned yet when the
> > for_each_online_cpu() loop in cpufreq_stats_init() is executed.
> 
> Yes. That's a race we need to fix. I agree.
> 
> > Worse yet, if a CPU goes offline when cpufreq_stats_exit() is running
> > and that happens exactly between the notifier unregistration and the
> > for_each_online_cpu() loop, the stats table will never be freed for
> > that CPU (say the policy isn't shared).
> 
> Same here.
> 
> > Switching over to loops over possible CPUs doesn't address those races
> > (at least not the second one), and I'm not really sure why I thought
> > it would address them, but adding CPU online/offline locking to
> > cpufreq_stats_init/exit() can address them, so it looks like the very
> > first version of my patch (ie.
> > https://patchwork.kernel.org/patch/9128509/) was actually correct,
> > because it didn't put too much code under the CPU offline/online
> > locking. :-)
> 
> Well, I think there is one more way of getting all this fixed, which may
> eventually look much more cleaner.
> 
> What if we update cpufreq core instead of stats with something like this:

The problem is in the stats module, so it needs to be addressed in there.

We can modify the core too if there are other problems with notifiers that
need to be addressed.

For now, I'm going to apply https://patchwork.kernel.org/patch/9128509/
unless you point out a technical problem with it.

> -------------------------8<-------------------------
> 
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Tue, 24 May 2016 10:16:25 +0530
> Subject: [PATCH] cpufreq: Initiate notifiers for existing policy
> 
> Races are possible in the init/exit paths of the cpufreq-stats layer,
> which may lead to 'stats' sysfs directory not getting created or removed
> for some of the policies. This can happen while the policy is getting
> created while cpufreq_stats_init/exit() are getting called.
> 
> To avoid adding unnecessary locks in the init/exit paths of the

I don't really get it why you don't like get/put_online_cpus() so much.

Quite arguably, they aren't unnecessary in the stats code, because
looping over online CPUs without ensuring that the mask won't change
when the loop is in progress is sort of buggy anyway.

> cpufreq-stats layer, update the policy notifier register/unregister
> routines to send notifications for any existing cpufreq policies.
> 
> Also make sure (with help of cpufreq_driver_lock) that
> CPUFREQ_CREATE/REMOVE notifiers aren't getting issued in parallel from
> the policy creation/removal paths.

So you add a lot of complexity to the core just in order to avoid the
extra get/put_online_cpus() invocations?  And what about people trying
to understand why the hell the code is not racy in the future?

No, sorry.

Thanks,
Rafael

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

[Viresh Kumar]

On 24-05-16, 14:13, Rafael J. Wysocki wrote:
> I don't really get it why you don't like get/put_online_cpus() so much.

Not that I don't like them, I just wanted to see if its possible to
work without any additional locking.

Anyway, so the first version of your patch did the get_online_cpus()
around a bigger section of the code instead of just the
for_each_online_cpu() loop. Should we change that? 

-- 
viresh

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

[Rafael J. Wysocki]

On Tuesday, May 24, 2016 05:47:17 PM Viresh Kumar wrote:
> On 24-05-16, 14:13, Rafael J. Wysocki wrote:
> > I don't really get it why you don't like get/put_online_cpus() so much.
> 
> Not that I don't like them, I just wanted to see if its possible to
> work without any additional locking.
> 
> Anyway, so the first version of your patch did the get_online_cpus()
> around a bigger section of the code instead of just the
> for_each_online_cpu() loop. Should we change that?

It did that, because locking around the loop alone wouldn't close the
races I'm concerned about.

That said, those same races are possible when the cpufreq driver is
loaded along with the cpufreq_stats module or is unloaded along with
that module.  Again, if ether a CPU goes online or the cpufreq driver
is loaded during cpufreq_stats_init(), the new CPU *or* a new policy
may be missed by it.  Similarly, if either a CPU goes offline or
the cpufreq driver is unloaded during cpufreq_stats_exit(), that
CPU or a policy that has just gone away may be missed by it.

The CPU "hotplug" locking is not sufficient to close the races related
to the cpufreq driver load/unload, so an alternative approach is needed.
IMO, however, changing the way the notifiers work is not the way to go
here, because the problem is specific to the stats module.  Unless there
are other users of the notifiers with the same problem, it should be
addressed in the stats code.

The stats code looks all pants to me now, TBH, and the way it uses notifiers
(both policy notifiers and transition notifiers) is questionable at the very
least.  It looks like it would just be better to redesign it from scratch.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Wed, May 25, 2016 at 2:00 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, May 24, 2016 05:47:17 PM Viresh Kumar wrote:
>> On 24-05-16, 14:13, Rafael J. Wysocki wrote:
>> > I don't really get it why you don't like get/put_online_cpus() so much.
>>
>> Not that I don't like them, I just wanted to see if its possible to
>> work without any additional locking.
>>
>> Anyway, so the first version of your patch did the get_online_cpus()
>> around a bigger section of the code instead of just the
>> for_each_online_cpu() loop. Should we change that?
>
> It did that, because locking around the loop alone wouldn't close the
> races I'm concerned about.
>
> That said, those same races are possible when the cpufreq driver is
> loaded along with the cpufreq_stats module or is unloaded along with
> that module.  Again, if ether a CPU goes online or the cpufreq driver
> is loaded during cpufreq_stats_init(), the new CPU *or* a new policy
> may be missed by it.  Similarly, if either a CPU goes offline or
> the cpufreq driver is unloaded during cpufreq_stats_exit(), that
> CPU or a policy that has just gone away may be missed by it.
>
> The CPU "hotplug" locking is not sufficient to close the races related
> to the cpufreq driver load/unload, so an alternative approach is needed.
> IMO, however, changing the way the notifiers work is not the way to go
> here, because the problem is specific to the stats module.  Unless there
> are other users of the notifiers with the same problem, it should be
> addressed in the stats code.
>
> The stats code looks all pants to me now, TBH, and the way it uses notifiers
> (both policy notifiers and transition notifiers) is questionable at the very
> least.  It looks like it would just be better to redesign it from scratch.

I'm actually considering making the stats code non-modular.

It won't have a reason to use notifiers then and may be simplified
quite a bit this way.  As an added benefit, if it doesn't use the
transition notifier, it won't interfere with fast frequency switching
in the schedutil governor any more.

Are there any drawbacks?

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

[Viresh Kumar]

On 25-05-16, 02:48, Rafael J. Wysocki wrote:
> I'm actually considering making the stats code non-modular.
> 
> It won't have a reason to use notifiers then and may be simplified
> quite a bit this way.  As an added benefit, if it doesn't use the
> transition notifier, it won't interfere with fast frequency switching
> in the schedutil governor any more.
> 
> Are there any drawbacks?

I don't think so. Please do that :)

-- 
viresh