* [PATCH] cpufreq: stats: Walk online CPUs with CPU offline/online locked @ 2016-05-20 1:34 Rafael J. Wysocki 2016-05-20 1:41 ` [PATCH v2] " Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-20 1:34 UTC (permalink / raw) To: Linux PM list Cc: Linux Kernel Mailing List, Viresh Kumar, Srinivas Pandruvada 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(¬ifier_policy_block, CPUFREQ_POLICY_NOTIFIER); cpufreq_unregister_notifier(¬ifier_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>"); ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-20 1:34 [PATCH] cpufreq: stats: Walk online CPUs with CPU offline/online locked Rafael J. Wysocki @ 2016-05-20 1:41 ` Rafael J. Wysocki 2016-05-20 2:22 ` Viresh Kumar 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-20 1:41 UTC (permalink / raw) To: Linux PM list Cc: Linux Kernel Mailing List, Viresh Kumar, Srinivas Pandruvada 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(¬ifier_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(¬ifier_policy_block, CPUFREQ_POLICY_NOTIFIER); cpufreq_unregister_notifier(¬ifier_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>"); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-20 1:41 ` [PATCH v2] " Rafael J. Wysocki @ 2016-05-20 2:22 ` Viresh Kumar 2016-05-20 12:13 ` Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Viresh Kumar @ 2016-05-20 2:22 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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(¬ifier_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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-20 2:22 ` Viresh Kumar @ 2016-05-20 12:13 ` Rafael J. Wysocki 2016-05-20 21:33 ` Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-20 12:13 UTC (permalink / raw) To: Viresh Kumar Cc: Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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(¬ifier_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(¬ifier_trans_block, @@ -330,12 +334,11 @@ static int __init cpufreq_stats_init(voi if (ret) { cpufreq_unregister_notifier(¬ifier_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(¬ifier_trans_block, CPUFREQ_TRANSITION_NOTIFIER); - for_each_online_cpu(cpu) + + for_each_possible_cpu(cpu) cpufreq_stats_free_table(cpu); } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-20 12:13 ` Rafael J. Wysocki @ 2016-05-20 21:33 ` Rafael J. Wysocki 2016-05-23 3:57 ` Viresh Kumar 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-20 21:33 UTC (permalink / raw) To: Viresh Kumar, Linux PM list Cc: Linux Kernel Mailing List, Srinivas Pandruvada 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(¬ifier_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(¬ifier_trans_block, @@ -330,12 +336,11 @@ static int __init cpufreq_stats_init(voi if (ret) { cpufreq_unregister_notifier(¬ifier_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(¬ifier_trans_block, CPUFREQ_TRANSITION_NOTIFIER); - for_each_online_cpu(cpu) + + for_each_possible_cpu(cpu) cpufreq_stats_free_table(cpu); } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-20 21:33 ` Rafael J. Wysocki @ 2016-05-23 3:57 ` Viresh Kumar 2016-05-23 13:40 ` Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Viresh Kumar @ 2016-05-23 3:57 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-23 3:57 ` Viresh Kumar @ 2016-05-23 13:40 ` Rafael J. Wysocki 2016-05-23 15:19 ` Viresh Kumar 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-23 13:40 UTC (permalink / raw) To: Viresh Kumar Cc: Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-23 13:40 ` Rafael J. Wysocki @ 2016-05-23 15:19 ` Viresh Kumar 2016-05-23 20:47 ` Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Viresh Kumar @ 2016-05-23 15:19 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-23 15:19 ` Viresh Kumar @ 2016-05-23 20:47 ` Rafael J. Wysocki 2016-05-24 4:56 ` Viresh Kumar 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-23 20:47 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael J. Wysocki, Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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. :-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-23 20:47 ` Rafael J. Wysocki @ 2016-05-24 4:56 ` Viresh Kumar 2016-05-24 12:13 ` Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Viresh Kumar @ 2016-05-24 4:56 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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(¬ifier_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(¬ifier_trans_block, CPUFREQ_TRANSITION_NOTIFIER); if (ret) { cpufreq_unregister_notifier(¬ifier_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(¬ifier_policy_block, - CPUFREQ_POLICY_NOTIFIER); cpufreq_unregister_notifier(¬ifier_trans_block, CPUFREQ_TRANSITION_NOTIFIER); - for_each_online_cpu(cpu) - cpufreq_stats_free_table(cpu); + cpufreq_unregister_notifier(¬ifier_policy_block, + CPUFREQ_POLICY_NOTIFIER); } MODULE_AUTHOR("Zou Nan hai <nanhai.zou@intel.com>"); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-24 4:56 ` Viresh Kumar @ 2016-05-24 12:13 ` Rafael J. Wysocki 2016-05-24 12:17 ` Viresh Kumar 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-24 12:13 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael J. Wysocki, Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-24 12:13 ` Rafael J. Wysocki @ 2016-05-24 12:17 ` Viresh Kumar 2016-05-25 0:00 ` Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Viresh Kumar @ 2016-05-24 12:17 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-24 12:17 ` Viresh Kumar @ 2016-05-25 0:00 ` Rafael J. Wysocki 2016-05-25 0:48 ` Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-25 0:00 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael J. Wysocki, Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-25 0:00 ` Rafael J. Wysocki @ 2016-05-25 0:48 ` Rafael J. Wysocki 2016-05-25 2:07 ` Viresh Kumar 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2016-05-25 0:48 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Viresh Kumar, Rafael J. Wysocki, Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked 2016-05-25 0:48 ` Rafael J. Wysocki @ 2016-05-25 2:07 ` Viresh Kumar 0 siblings, 0 replies; 15+ messages in thread From: Viresh Kumar @ 2016-05-25 2:07 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Linux PM list, Linux Kernel Mailing List, Srinivas Pandruvada 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-05-25 2:08 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-20 1:34 [PATCH] cpufreq: stats: Walk online CPUs with CPU offline/online locked Rafael J. Wysocki 2016-05-20 1:41 ` [PATCH v2] " Rafael J. Wysocki 2016-05-20 2:22 ` Viresh Kumar 2016-05-20 12:13 ` Rafael J. Wysocki 2016-05-20 21:33 ` Rafael J. Wysocki 2016-05-23 3:57 ` Viresh Kumar 2016-05-23 13:40 ` Rafael J. Wysocki 2016-05-23 15:19 ` Viresh Kumar 2016-05-23 20:47 ` Rafael J. Wysocki 2016-05-24 4:56 ` Viresh Kumar 2016-05-24 12:13 ` Rafael J. Wysocki 2016-05-24 12:17 ` Viresh Kumar 2016-05-25 0:00 ` Rafael J. Wysocki 2016-05-25 0:48 ` Rafael J. Wysocki 2016-05-25 2:07 ` Viresh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).