cpufreq: Don't loose cpufreq history on CPU hotplug
Viresh Kumar
maintainerv2 cover cpufreq: Don't loose cpufreq history on CPU hotplug
Hi Rafael,
The aim of this series is to stop managing cpufreq sysfs directories on CPU hotplugs.
Currently on removal of a 'cpu != policy->cpu', we remove its sysfs directories by removing the soft-link. And on removal of policy->cpu, we migrate the sysfs directories to the next cpu. But if policy->cpu was the last CPU, we remove the policy completely and allocate it again as soon as the CPUs come back. This has shortcomings:
- Code Complexity
- Slower hotplug
- sysfs file permissions are reset after all policy->cpus are offlined
- CPUFreq stats history lost after all policy->cpus are offlined
- Special management of sysfs stuff during suspend/resume
To make things simple we stop playing with sysfs files unless the driver is getting removed. Also the policy is kept intact to be used later.
First few patches provide a clean base for others *more important* patches.
Rebased-over: your bleeding edge branch as there were dependencies on my earlier patches.
Pushed here:
git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/core/sysfs
v1->V2:
- Dropped the idea of using policy-lists for getting policy for any cpu
- Also dropped fallback list and its per-cpu variable
- Stopped cleaning cpufreq_cpu_data and doing list_del(policy) on logical hotplug.
- Added support for physical hotplug of CPUs (Untested).
@Srivatsa: Can you please have a look at the above change? I have cc'd you only on this one.
Saravana Kannan (1):
cpufreq: Track cpu managing sysfs kobjects separately
Viresh Kumar (19):
cpufreq: Add doc style comment about cpufreq_cpu_{get|put}()
cpufreq: Merge __cpufreq_add_dev() and cpufreq_add_dev()
cpufreq: Throw warning when we try to get policy for an invalid CPU
cpufreq: Keep a single path for adding managed CPUs
cpufreq: Clear policy->cpus even for the last CPU
cpufreq: Create for_each_{in}active_policy()
cpufreq: Call schedule_work() for the last active policy
cpufreq: Don't clear cpufreq_cpu_data and policy list for inactive
policies
cpufreq: Get rid of cpufreq_cpu_data_fallback
cpufreq: Don't traverse list of all policies for adding policy for a
cpu
cpufreq: Manage governor usage history with 'policy->last_governor'
cpufreq: Mark policy->governor = NULL for inactive policies
cpufreq: Don't allow updating inactive-policies from sysfs
cpufreq: Stop migrating sysfs files on hotplug
cpufreq: Remove cpufreq_update_policy()
cpufreq: Initialize policy->kobj while allocating policy
cpufreq: Call cpufreq_policy_put_kobj() from cpufreq_policy_free()
cpufreq: Restart governor as soon as possible
cpufreq: Add support for physical hoplug of CPUs
drivers/cpufreq/cpufreq.c | 593 ++++++++++++++++++++++++++-------------------- include/linux/cpufreq.h | 5 +- 2 files changed, 340 insertions(+), 258 deletions(-)
Signature · 1 line
2.3.0.rc0.44.ga94655d
Viresh Kumar
maintainerPATCH v2 1/20 cpufreq: Add doc style comment about cpufreq_cpu_{get|put}()
This clearly states what the code inside these routines is doing and how these must be used.
--- drivers/cpufreq/cpufreq.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
drivers/cpufreq/cpufreq.c +27 -0
@@ -207,6 +207,23 @@ struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)return per_cpu (cpufreq_cpu_data, cpu); } +/** +* cpufreq_cpu_get: returns policy for a cpu and marks it busy. +* +* @cpu: cpu to find policy for. +* +* This returns policy for 'cpu', returns NULL if it doesn't exist. +* It also increments the kobject reference count to mark it busy and so would +* require a corresponding call to cpufreq_cpu_put() to decrement it back. +* If corresponding call cpufreq_cpu_put() isn't made, the policy wouldn't be +* freed as that depends on the kobj count. +* +* It also takes a read-lock of 'cpufreq_rwsem' and doesn't put it back if a +* valid policy is found. This is done to make sure the driver doesn't get +* unregistered while the policy is being used. +* +* Return: A valid policy on success, otherwise NULL on failure. +*/ struct cpufreq_policy *cpufreq_cpu_get (unsigned int cpu) {struct cpufreq_policy *policy =NULL ;
@@ -237,6 +254,16 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) }EXPORT_SYMBOL_GPL (cpufreq_cpu_get); +/** +* cpufreq_cpu_put: Decrements the usage count of a policy +* +* @policy: policy earlier returned by cpufreq_cpu_get(). +* +* This decrements the kobject reference count incremented earlier by calling +* cpufreq_cpu_get(). +* +* It also drops the read-lock of 'cpufreq_rwsem' taken at cpufreq_cpu_get(). +*/ void cpufreq_cpu_put (struct cpufreq_policy *policy) {kobject_put (&policy->kobj);
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 01/20] cpufreq: Add doc style comment about cpufreq_cpu_{get|put}()
Quoted 44 lines · Viresh Kumar
This clearly states what the code inside these routines is doing and how these must be used.
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)drivers/cpufreq/cpufreq.c +20
-0@@ -207,6 +207,23 @@ struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)return per_cpu (cpufreq_cpu_data, cpu); } +/** +* cpufreq_cpu_get: returns policy for a cpu and marks it busy. +* +* @cpu: cpu to find policy for. +* +* This returns policy for 'cpu', returns NULL if it doesn't exist. +* It also increments the kobject reference count to mark it busy and so would +* require a corresponding call to cpufreq_cpu_put() to decrement it back. +* If corresponding call cpufreq_cpu_put() isn't made, the policy wouldn't be +* freed as that depends on the kobj count. +* +* It also takes a read-lock of 'cpufreq_rwsem' and doesn't put it back if a +* valid policy is found. This is done to make sure the driver doesn't get +* unregistered while the policy is being used. +* +* Return: A valid policy on success, otherwise NULL on failure. +*/ struct cpufreq_policy *cpufreq_cpu_get (unsigned int cpu) {struct cpufreq_policy *policy =NULL ;@@ -237,6 +254,16 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) }EXPORT_SYMBOL_GPL (cpufreq_cpu_get); +/** +* cpufreq_cpu_put: Decrements the usage count of a policy +*
drivers/cpufreq/cpufreq.c struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) · line 269
Quoted 10 lines
drivers/cpufreq/cpufreq.c +7
-0+* @policy: policy earlier returned by cpufreq_cpu_get(). +* +* This decrements the kobject reference count incremented earlier by calling +* cpufreq_cpu_get(). +* +* It also drops the read-lock of 'cpufreq_rwsem' taken at cpufreq_cpu_get(). +*/ void cpufreq_cpu_put (struct cpufreq_policy *policy) {kobject_put (&policy->kobj);
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerPATCH v2 2/20 cpufreq: Merge __cpufreq_add_dev() and cpufreq_add_dev()
In reply to Viresh Kumarcpufreq_add_dev() is an unnecessary wrapper over __cpufreq_add_dev(). Merge them.
--- drivers/cpufreq/cpufreq.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)
drivers/cpufreq/cpufreq.c +12 -17
@@ -1125,7 +1125,16 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,return 0 ; } -static int __cpufreq_add_dev (struct device *dev,struct subsys_interface *sif) +/** +* cpufreq_add_dev - add a CPU device +* +* Adds the cpufreq interface for a CPU device. +* +* The Oracle says: try running cpufreq registration/unregistration concurrently +* with with cpu hotplugging and all hell will break loose. Tried to clean this +* mess up, but more thorough testing is needed. - Mathieu +*/ +static int cpufreq_add_dev (struct device *dev,struct subsys_interface *sif) {unsigned int j, cpu = dev->id;int ret = -ENOMEM ;
@@ -1336,20 +1345,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)return ret; } -/** -* cpufreq_add_dev - add a CPU device -* -* Adds the cpufreq interface for a CPU device. -* -* The Oracle says: try running cpufreq registration/unregistration concurrently -* with with cpu hotplugging and all hell will break loose. Tried to clean this -* mess up, but more thorough testing is needed. - Mathieu -*/ -static int cpufreq_add_dev (struct device *dev,struct subsys_interface *sif) -{ -return __cpufreq_add_dev (dev, sif); -} -static int __cpufreq_remove_dev_prepare (struct device *dev,struct subsys_interface *sif) {
@@ -2328,7 +2323,7 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,if (dev) {switch (action & ~CPU_TASKS_FROZEN ) {case CPU_ONLINE : -__cpufreq_add_dev (dev,NULL ); +cpufreq_add_dev (dev,NULL );break ;case CPU_DOWN_PREPARE :
@@ -2340,7 +2335,7 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,break ;case CPU_DOWN_FAILED : -__cpufreq_add_dev (dev,NULL ); +cpufreq_add_dev (dev,NULL );break ; } }
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 02/20] cpufreq: Merge __cpufreq_add_dev() and cpufreq_add_dev()
Quoted 62 lines · Viresh Kumar
cpufreq_add_dev()is an unnecessary wrapper over__cpufreq_add_dev(). Merge them.Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)drivers/cpufreq/cpufreq.c +11
-16@@ -1125,7 +1125,16 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,return 0 ; } -static int __cpufreq_add_dev (struct device *dev,struct subsys_interface *sif) +/** +* cpufreq_add_dev - add a CPU device +* +* Adds the cpufreq interface for a CPU device. +* +* The Oracle says: try running cpufreq registration/unregistration concurrently +* with with cpu hotplugging and all hell will break loose. Tried to clean this +* mess up, but more thorough testing is needed. - Mathieu +*/ +static int cpufreq_add_dev (struct device *dev,struct subsys_interface *sif) {unsigned int j, cpu = dev->id;int ret = -ENOMEM ;@@ -1336,20 +1345,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)return ret; } -/** -* cpufreq_add_dev - add a CPU device -* -* Adds the cpufreq interface for a CPU device. -* -* The Oracle says: try running cpufreq registration/unregistration concurrently -* with with cpu hotplugging and all hell will break loose. Tried to clean this -* mess up, but more thorough testing is needed. - Mathieu -*/ -static int cpufreq_add_dev (struct device *dev,struct subsys_interface *sif) -{ -return __cpufreq_add_dev (dev, sif); -} -static int __cpufreq_remove_dev_prepare (struct device *dev,struct subsys_interface *sif) {@@ -2328,7 +2323,7 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,if (dev) {switch (action & ~CPU_TASKS_FROZEN ) {case CPU_ONLINE : -__cpufreq_add_dev (dev,NULL ); +cpufreq_add_dev (dev,NULL );break ;case CPU_DOWN_PREPARE :
drivers/cpufreq/cpufreq.c static int cpufreq_cpu_callback(struct notifier_block *nfb, · line 2341
Quoted 8 lines
drivers/cpufreq/cpufreq.c +1
-1@@ -2340,7 +2335,7 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,break ;case CPU_DOWN_FAILED : -__cpufreq_add_dev (dev,NULL ); +cpufreq_add_dev (dev,NULL );break ; } }
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerPATCH v2 3/20 cpufreq: Throw warning when we try to get policy for an invalid CPU
In reply to Viresh KumarSimply returning here with an error is not enough. It shouldn't be allowed at all to try calling cpufreq_cpu_get() for an invalid CPU.
Add a WARN here to make it clear that it wouldn't be acceptable at all.
--- drivers/cpufreq/cpufreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/cpufreq.c +1 -1
@@ -229,7 +229,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)struct cpufreq_policy *policy =NULL ;unsigned long flags; -if (cpu >= nr_cpu_ids) +if (WARN_ON (cpu >= nr_cpu_ids))return NULL ;if (!down_read_trylock (&cpufreq_rwsem))
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 03/20] cpufreq: Throw warning when we try to get policy for an invalid CPU
Quoted 16 lines · Viresh Kumar
Simply returning here with an error is not enough. It shouldn't be allowed at all to try calling
cpufreq_cpu_get()for an invalid CPU.Add a WARN here to make it clear that it wouldn't be acceptable at all.
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/cpufreq/cpufreq.c +0
-0
drivers/cpufreq/cpufreq.c struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) · line 235
Quoted 8 lines
drivers/cpufreq/cpufreq.c +1
-1@@ -229,7 +229,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)struct cpufreq_policy *policy =NULL ;unsigned long flags; -if (cpu >= nr_cpu_ids) +if (WARN_ON (cpu >= nr_cpu_ids))return NULL ;if (!down_read_trylock (&cpufreq_rwsem))
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerPATCH v2 4/20 cpufreq: Keep a single path for adding managed CPUs
In reply to Viresh KumarThere are two cases when we may try to add already managed CPUs:
- On boot, the first cpu has marked all policy->cpus managed and so we will find policy for all other policy->cpus later on.
- When a managed cpu is hotplugged out and later brought back in.
Currently we manage these two with separate paths and checks. While the first one is detected by testing cpu against 'policy->cpus', the other one is detected by testing cpu against 'policy->related_cpus'.
We can manage both these via a single path and there is no need to do special checking for the first one.
--- drivers/cpufreq/cpufreq.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/cpufreq/cpufreq.c +5 -7
@@ -992,6 +992,10 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,int ret =0 ;unsigned long flags; +/* Is cpu already managed ? */ +if (cpumask_test_cpu (cpu, policy->cpus)) +return 0 ; +if (has_target ()) { ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP );if (ret) {
@@ -1147,16 +1151,10 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)pr_debug ("adding CPU %u\n" , cpu); -/* check whether a different CPU already registered this -* CPU because it is in the same boat. */ - policy =cpufreq_cpu_get_raw (cpu); -if (unlikely (policy)) -return 0 ; -if (!down_read_trylock (&cpufreq_rwsem))return 0 ; -/* Check if this cpu was hot-unplugged earlier and has siblings */ +/* Check if this cpu already has a policy to manage it */ read_lock_irqsave (&cpufreq_driver_lock, flags);for_each_policy (policy) {if (cpumask_test_cpu (cpu, policy->related_cpus)) {
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 04/20] cpufreq: Keep a single path for adding managed CPUs
Quoted 1 line · Viresh Kumar
There are two cases when we may try to add already managed CPUs:
The term manages CPUs is a bit unclear/confusing. When I hear managed CPUs, I think CPUs that are already marked in policy->cpus and the governor has already been started on them.
Can you clarify this commit text please?
Quoted 40 lines
- On boot, the first cpu has marked all
policy->cpusmanaged and so we will find policy for all otherpolicy->cpuslater on.
- When a managed cpu is hotplugged out and later brought back in.Currently we manage these two with separate paths and checks. While the first one is detected by testing cpu against '
policy->cpus', the other one is detected by testing cpu against 'policy->related_cpus'.We can manage both these via a single path and there is no need to do special checking for the first one.
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)drivers/cpufreq/cpufreq.c +4
-4@@ -992,6 +992,10 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,int ret =0 ;unsigned long flags; +/* Is cpu already managed ? */ +if (cpumask_test_cpu (cpu, policy->cpus)) +return 0 ; +if (has_target ()) { ret =__cpufreq_governor (policy,CPUFREQ_GOV_STOP );if (ret) {@@ -1147,16 +1151,10 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)pr_debug ("adding CPU %u\n" , cpu); -/* check whether a different CPU already registered this -* CPU because it is in the same boat. */ - policy =cpufreq_cpu_get_raw (cpu); -if (unlikely (policy))
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) · line 1160
Quoted 10 lines
drivers/cpufreq/cpufreq.c +1
-3-return 0 ; -if (!down_read_trylock (&cpufreq_rwsem))return 0 ; -/* Check if this cpu was hot-unplugged earlier and has siblings */ +/* Check if this cpu already has a policy to manage it */ read_lock_irqsave (&cpufreq_driver_lock, flags);for_each_policy (policy) {if (cpumask_test_cpu (cpu, policy->related_cpus)) {
If comment text is fixed,
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 04/20] cpufreq: Keep a single path for adding managed CPUs
Quoted 5 lines · Saravana Kannan
The term manages CPUs is a bit unclear/confusing. When I hear managed CPUs, I think CPUs that are already marked in
policy->cpusand the governor has already been started on them.Can you clarify this commit text please?
Sure.
Viresh Kumar
maintainerPATCH v2 5/20 cpufreq: Clear policy->cpus even for the last CPU
In reply to Viresh KumarWe clear policy->cpus mask while CPUs are hotplugged out. We do it for all CPUs except the last CPU of the policy. I don't remember what the rationale behind that was, but I couldn't think of anything that will break if we remove this conditional clearing and always clear policy->cpus.
The benefit we get out of it is, we can know if a policy is active or not by checking if this field is empty or not. That will be used by later commits.
--- drivers/cpufreq/cpufreq.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/cpufreq/cpufreq.c +1 -3
@@ -1430,9 +1430,7 @@ static int __cpufreq_remove_dev_finish(struct device *dev,down_write (&policy->rwsem); cpus =cpumask_weight (policy->cpus); - -if (cpus >1 ) -cpumask_clear_cpu (cpu, policy->cpus); +cpumask_clear_cpu (cpu, policy->cpus);up_write (&policy->rwsem);/* If cpu is last user of policy, free policy */
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 05/20] cpufreq: Clear policy->cpus even for the last CPU
Quoted 19 lines · Viresh Kumar
We clear
policy->cpusmask while CPUs are hotplugged out. We do it for all CPUs except the last CPU of the policy. I don't remember what the rationale behind that was, but I couldn't think of anything that will break if we remove this conditional clearing and always clearpolicy->cpus.The benefit we get out of it is, we can know if a policy is active or not by checking if this field is empty or not. That will be used by later commits.
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/cpufreq/cpufreq.c +0
-0
drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev_finish(struct device *dev, · line 1436
Quoted 10 lines
drivers/cpufreq/cpufreq.c +1
-3@@ -1430,9 +1430,7 @@ static int __cpufreq_remove_dev_finish(struct device *dev,down_write (&policy->rwsem); cpus =cpumask_weight (policy->cpus); - -if (cpus >1 ) -cpumask_clear_cpu (cpu, policy->cpus); +cpumask_clear_cpu (cpu, policy->cpus);up_write (&policy->rwsem);/* If cpu is last user of policy, free policy */
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerPATCH v2 6/20 cpufreq: Create for_each_{in}active_policy()
In reply to Viresh Kumarpolicy->cpus is cleared unconditionally now on hotplug-out of a CPU and it can be checked to know if a policy is active or inactive. Create helper routines to iterate over all active/inactive policies, based on policy->cpus field.
Replace all instances of for_each_policy() with for_each_active_policy() to make them iterate only for active policies. (We haven't made changes yet to keep inactive policies in the same list, but that will be followed in a later patch).
--- drivers/cpufreq/cpufreq.c | 81 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 72 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq.c +72 -9
@@ -32,11 +32,74 @@#include <trace/events/power.h> /* Macros to iterate over lists */ -/* Iterate over online CPUs policies */ static LIST_HEAD (cpufreq_policy_list); +static inline bool policy_is_inactive (struct cpufreq_policy *policy) +{ +return cpumask_empty (policy->cpus); +} + +/* +* Finds Next Acive/Inactive policy +* +* policy: Previous policy. +* active: Looking for active policy (true) or Inactive policy (false). +*/ +static struct cpufreq_policy *next_policy (struct cpufreq_policy *policy, +bool active) +{ +while (1 ) { +if (likely (policy)) + policy =list_next_entry (policy, policy_list); +else + policy =list_first_entry (&cpufreq_policy_list, +typeof (*policy), policy_list); + +/* No more policies */ +if (&policy->policy_list == &cpufreq_policy_list) +return policy; + +/* +* Table to explains logic behind following expression: +* +* Active policy_is_inactive Result +* (policy or next) +* +* 0 0 next +* 0 1 policy +* 1 0 policy +* 1 1 next +*/ +if (active ^policy_is_inactive (policy)) +return policy; + }; +} + +/* +* Iterate over online CPUs policies +* +* Explanation: +* - expr1: marks __temp NULL and gets the first __active policy. +* - expr2: checks if list is finished, if yes then it sets __policy to the last +* __active policy and returns 0 to end the loop. +* - expr3: preserves last __active policy and gets the next one. +*/ +#define __for_each_active_policy (__policy, __temp, __active) \ + for (__temp = NULL, __policy = next_policy(NULL, __active); \ + &__policy->policy_list != &cpufreq_policy_list || \ + ((__policy = __temp) && 0); \ + __temp = __policy, __policy = next_policy(__policy, __active)) +#define for_each_policy (__policy) \ list_for_each_entry(__policy, &cpufreq_policy_list, policy_list) +/* +* Routines to iterate over active or inactive policies +* __policy: next active/inactive policy will be returned in this. +* __temp: for internal purpose, not to be used by caller. +*/ +#define for_each_active_policy (__policy, __temp) __for_each_active_policy(__policy, __temp, true) +#define for_each_inactive_policy (__policy, __temp) __for_each_active_policy(__policy, __temp, false) +/* Iterate over governors */ static LIST_HEAD (cpufreq_governor_list);#define for_each_governor (__governor) \
@@ -1142,7 +1205,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) {unsigned int j, cpu = dev->id;int ret = -ENOMEM ; -struct cpufreq_policy *policy; +struct cpufreq_policy *policy, *tpolicy;unsigned long flags;bool recover_policy = cpufreq_suspended;
@@ -1156,7 +1219,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)/* Check if this cpu already has a policy to manage it */ read_lock_irqsave (&cpufreq_driver_lock, flags); -for_each_policy (policy) { +for_each_active_policy (policy, tpolicy) {if (cpumask_test_cpu (cpu, policy->related_cpus)) {read_unlock_irqrestore (&cpufreq_driver_lock, flags); ret =cpufreq_add_policy_cpu (policy, cpu, dev);
@@ -1664,7 +1727,7 @@ EXPORT_SYMBOL(cpufreq_generic_suspend);*/ void cpufreq_suspend (void ) { -struct cpufreq_policy *policy; +struct cpufreq_policy *policy, *tpolicy;if (!cpufreq_driver)return ;
@@ -1674,7 +1737,7 @@ void cpufreq_suspend(void)pr_debug ("%s: Suspending Governors\n" , __func__); -for_each_policy (policy) { +for_each_active_policy (policy, tpolicy) {if (__cpufreq_governor (policy,CPUFREQ_GOV_STOP ))pr_err ("%s: Failed to stop governor for policy: %p\n" , __func__, policy);
@@ -1696,7 +1759,7 @@ void cpufreq_suspend(void)*/ void cpufreq_resume (void ) { -struct cpufreq_policy *policy; +struct cpufreq_policy *policy, *tpolicy;if (!cpufreq_driver)return ;
@@ -1708,7 +1771,7 @@ void cpufreq_resume(void)pr_debug ("%s: Resuming Governors\n" , __func__); -for_each_policy (policy) { +for_each_active_policy (policy, tpolicy) {if (cpufreq_driver->resume && cpufreq_driver->resume (policy))pr_err ("%s: Failed to resume driver: %p\n" , __func__, policy);
@@ -2348,10 +2411,10 @@ static struct notifier_block __refdata cpufreq_cpu_notifier = {static int cpufreq_boost_set_sw (int state) {struct cpufreq_frequency_table *freq_table; -struct cpufreq_policy *policy; +struct cpufreq_policy *policy, *tpolicy;int ret = -EINVAL ; -for_each_policy (policy) { +for_each_active_policy (policy, tpolicy) { freq_table =cpufreq_frequency_get_table (policy->cpu);if (freq_table) { ret = cpufreq_frequency_table_cpuinfo(policy,
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 06/20] cpufreq: Create for_each_{in}active_policy()
Quoted 28 lines · Viresh Kumar
policy->cpusis cleared unconditionally now on hotplug-out of a CPU and it can be checked to know if a policy is active or inactive. Create helper routines to iterate over all active/inactive policies, based onpolicy->cpusfield.Replace all instances of
for_each_policy()withfor_each_active_policy()to make them iterate only for active policies. (We haven't made changes yet to keep inactive policies in the same list, but that will be followed in a later patch).Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 81 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 72 insertions(+), 9 deletions(-)drivers/cpufreq/cpufreq.c +5
-1@@ -32,11 +32,74 @@#include <trace/events/power.h> /* Macros to iterate over lists */ -/* Iterate over online CPUs policies */ static LIST_HEAD (cpufreq_policy_list); +static inline bool policy_is_inactive (struct cpufreq_policy *policy) +{ +return cpumask_empty (policy->cpus); +} +
drivers/cpufreq/cpufreq.c line 50
Quoted 10 lines
drivers/cpufreq/cpufreq.c +10
-0+/* +* Finds Next Acive/Inactive policy +* +* policy: Previous policy. +* active: Looking for active policy (true) or Inactive policy (false). +*/ +static struct cpufreq_policy *next_policy (struct cpufreq_policy *policy, +bool active) +{ +while (1 ) {
I don't like while(1) unless it's really meant to be an infinite loop. I think a do while would work here and also be more compact and readable.
drivers/cpufreq/cpufreq.c line 55
Quoted 5 lines
drivers/cpufreq/cpufreq.c +5
-0+if (likely (policy)) + policy =list_next_entry (policy, policy_list); +else + policy =list_first_entry (&cpufreq_policy_list, +typeof (*policy), policy_list);
Can't you just move this part into expr1? That would make it much clear/easier to understand
drivers/cpufreq/cpufreq.c line 59
Quoted 4 lines
drivers/cpufreq/cpufreq.c +4
-0+ +/* No more policies */ +if (&policy->policy_list == &cpufreq_policy_list) +return policy;
I'm kinda confused by the fact that you return policy here unconditionally. I think it's a bug. No? Eg: Is there's only one policy in the system and you are looking for an inactive policy. Wouldn't this return the only policy -- the one that's active.
Quoted 36 lines
drivers/cpufreq/cpufreq.c +33
-0+ +/* +* Table to explains logic behind following expression: +* +* Active policy_is_inactive Result +* (policy or next) +* +* 0 0 next +* 0 1 policy +* 1 0 policy +* 1 1 next +*/ +if (active ^policy_is_inactive (policy)) +return policy; + }; +} + +/* +* Iterate over online CPUs policies +* +* Explanation: +* - expr1: marks __temp NULL and gets the first __active policy. +* - expr2: checks if list is finished, if yes then it sets __policy to the last +* __active policy and returns 0 to end the loop. +* - expr3: preserves last __active policy and gets the next one. +*/ +#define __for_each_active_policy (__policy, __temp, __active) \ + for (__temp = NULL, __policy = next_policy(NULL, __active); \ + &__policy->policy_list != &cpufreq_policy_list || \ + ((__policy = __temp) && 0); \ + __temp = __policy, __policy = next_policy(__policy, __active)) +#define for_each_policy (__policy) \ list_for_each_entry(__policy, &cpufreq_policy_list, policy_list) +/*
drivers/cpufreq/cpufreq.c line 105
Quoted 10 lines
drivers/cpufreq/cpufreq.c +7
-0+* Routines to iterate over active or inactive policies +* __policy: next active/inactive policy will be returned in this. +* __temp: for internal purpose, not to be used by caller. +*/ +#define for_each_active_policy (__policy, __temp) __for_each_active_policy(__policy, __temp, true) +#define for_each_inactive_policy (__policy, __temp) __for_each_active_policy(__policy, __temp, false) +/* Iterate over governors */ static LIST_HEAD (cpufreq_governor_list);#define for_each_governor (__governor) \
Stuff below this looks fine.
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) · line 1211
Quoted 9 lines
drivers/cpufreq/cpufreq.c +1
-1@@ -1142,7 +1205,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) {unsigned int j, cpu = dev->id;int ret = -ENOMEM ; -struct cpufreq_policy *policy; +struct cpufreq_policy *policy, *tpolicy;unsigned long flags;bool recover_policy = cpufreq_suspended;
<snip>
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 06/20] cpufreq: Create for_each_{in}active_policy()
Quoted 9 lines · Saravana Kannan
Review
Quoted 4 lines · Viresh Kumar
(quoted diff) +4
-0+static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy, + bool active) +{ + while (1) {I don't like while(1) unless it's really meant to be an infinite loop. I
I don't hate it that much, and neither does other parts of kernel :)
Quoted 1 line
think a do while would work here and also be more compact and readable.
So you want this:
drivers/cpufreq/cpufreq.c +0 -0
@@ -47,7 +47,7 @@ static inline bool policy_is_inactive(struct
cpufreq_policy *policy)
static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
bool active)
{
- while (1) {
+ do {
if (likely(policy))
policy = list_next_entry(policy, policy_list);
else
@@ -69,9 +69,9 @@ static struct cpufreq_policy *next_policy(struct
cpufreq_policy *policy,
* 1 0 policy
* 1 1 next
*/- if (active ^ policy_is_inactive(policy))
- return policy;
- };
+ } while (!(active ^ policy_is_inactive(policy)));
+
+ return policy;
}
Not sure which one looked better :)
Quoted 10 lines
Quoted 6 lines
(quoted diff) +5
-0+ if (likely(policy)) + policy = list_next_entry(policy, policy_list); + else + policy = list_first_entry(&cpufreq_policy_list, + typeof(*policy),
policy_list);Can't you just move this part into expr1? That would make it much clear/easier to understand
No, because we want that for-loop to iterate over active/inactive policies only, and we need to run this routine to find it..
For ex:
- We want to iterate over active policies only
- The first policy of the list is inactive
- The change you are suggesting will break things here..
Quoted 10 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ + /* No more policies */ + if (&policy->policy_list == &cpufreq_policy_list) + return policy;I'm kinda confused by the fact that you return policy here unconditionally. I think it's a bug. No? Eg: Is there's only one policy in the system and you are looking for an inactive policy. Wouldn't this return the only policy -- the one that's active.
No. What we are returning here isn't a real policy actually but an container-of of the HEAD of the list, so it only has a valid ->policy_list value, others might
give us a crash. See how list_next_entry() works :)
Re: [PATCH V2 06/20] cpufreq: Create for_each_{in}active_policy()
Quoted 33 lines · Viresh Kumar
Quoted 9 lines · Saravana Kannan
Review
Quoted 4 lines · Viresh Kumar
(quoted diff) +4
-0+static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy, + bool active) +{ + while (1) {I don't like while(1) unless it's really meant to be an infinite loop. I
I don't hate it that much, and neither does other parts of kernel :)
Quoted 1 line
think a do while would work here and also be more compact and readable.
So you want this:
drivers/cpufreq/cpufreq.c +1
-1@@ -47,7 +47,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy)static struct cpufreq_policy *next_policy (struct cpufreq_policy *policy,bool active) { -while (1 ) { + do { if (likely(policy)) policy = list_next_entry(policy, policy_list);else
drivers/cpufreq/cpufreq.c +0
-0@@ -69,9 +69,9 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
drivers/cpufreq/cpufreq.c static struct cpufreq_policy *next_policy(struct · line 76
Quoted 10 lines
drivers/cpufreq/cpufreq.c +3
-3* 1 0 policy * 1 1 next */ -if (active ^policy_is_inactive (policy)) -return policy; - }; + }while (!(active ^policy_is_inactive (policy))); + +return policy; }
Yes please!! The other uses like inside a thread seem more reasonable to me.
Quoted 14 lines
drivers/cpufreq/cpufreq.c +0
-0Not sure which one looked better :)Quoted 11 lines
Quoted 7 lines
(quoted diff) +5
-0+ if (likely(policy)) + policy = list_next_entry(policy, policy_list); + else + policy = list_first_entry(&cpufreq_policy_list, + typeof(*policy),
policy_list);Can't you just move this part into expr1? That would make it much clear/easier to understand
Quoted 7 lines
No, because we want that for-loop to iterate over active/inactive policies only, and we need to run this routine to find it..
For ex:
- We want to iterate over active policies only
- The first policy of the list is inactive
- The change you are suggesting will break things here..
Ah, right. Makes sense.
Quoted 12 lines
Quoted 11 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+ + /* No more policies */ + if (&policy->policy_list == &cpufreq_policy_list) + return policy;I'm kinda confused by the fact that you return policy here unconditionally. I think it's a bug. No? Eg: Is there's only one policy in the system and you are looking for an inactive policy. Wouldn't this return the only policy -- the one that's active.
Quoted 4 lines
No. What we are returning here isn't a real policy actually but an container-of of the HEAD of the list, so it only has a valid ->policy_list value, others might
give us a crash. See howlist_next_entry()works :)
I thought the last valid entry is what would point to the list head. Not the actual list head. I'll check again.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH V2 06/20] cpufreq: Create for_each_{in}active_policy()
Quoted 38 lines · Saravana Kannan
Quoted 37 lines · Viresh Kumar
Quoted 9 lines · Saravana Kannan
Review
Quoted 4 lines · Viresh Kumar
(quoted diff) +4
-0+static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy, + bool active) +{ + while (1) {I don't like while(1) unless it's really meant to be an infinite loop. I
I don't hate it that much, and neither does other parts of kernel :)
Quoted 1 line
think a do while would work here and also be more compact and readable.
So you want this:
drivers/cpufreq/cpufreq.c +1
-1@@ -47,7 +47,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy)static struct cpufreq_policy *next_policy (struct cpufreq_policy *policy,bool active) { -while (1 ) { + do { if (likely(policy)) policy = list_next_entry(policy, policy_list);else
drivers/cpufreq/cpufreq.c +0
-1@@ -69,9 +69,9 @@ static struct cpufreq_policy *next_policy(structcpufreq_policy *policy, * 1 0 policy * 1 1 next */ -if (active ^policy_is_inactive (policy))
Quoted 8 lines
drivers/cpufreq/cpufreq.c static struct cpufreq_policy *next_policy(struct · line 76
Quoted 6 lines
drivers/cpufreq/cpufreq.c +3
-2-return policy; - }; + }while (!(active ^policy_is_inactive (policy))); + +return policy; }Yes please!! The other uses like inside a thread seem more reasonable to me.
Viresh, have you ever sent an update of this one?
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 06/20] cpufreq: Create for_each_{in}active_policy()
Quoted 1 line · Rafael J. Wysocki
Viresh, have you ever sent an update of this one?
I haven't sent updates for any patches, was waiting for reviews to finish. But will send the new copies now.
Viresh Kumar
maintainerPATCH v2 7/20 cpufreq: Call schedule_work() for the last active policy
In reply to Viresh KumarWe need to call schedule_work() only for the policy belonging to boot CPU, i.e. CPU0. Checking for list_is_last() wouldn't work anymore as there can be inactive policies present in the list after the last active one. 'policy' still points to the last active policy at the termination of the for_each_active_policy() loop, use that to call schedule_work().
--- drivers/cpufreq/cpufreq.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/cpufreq/cpufreq.c +7 -8
@@ -1779,15 +1779,14 @@ void cpufreq_resume(void) || __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS))pr_err ("%s: Failed to start governor for policy: %p\n" , __func__, policy); - -/* -* schedule call cpufreq_update_policy() for boot CPU, i.e. last -* policy in list. It will verify that the current freq is in -* sync with what we believe it to be. -*/ -if (list_is_last (&policy->policy_list, &cpufreq_policy_list)) -schedule_work (&policy->update); } + +/* +* schedule call cpufreq_update_policy() for boot CPU, i.e. last +* policy in list. It will verify that the current freq is in +* sync with what we believe it to be. +*/ +schedule_work (&policy->update); }/**
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 07/20] cpufreq: Call schedule_work() for the last active policy
Quoted 28 lines · Viresh Kumar
We need to call
schedule_work()only for the policy belonging to boot CPU, i.e. CPU0. Checking forlist_is_last()wouldn't work anymore as there can be inactive policies present in the list after the last active one. 'policy' still points to the last active policy at the termination of thefor_each_active_policy()loop, use that to callschedule_work().Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)drivers/cpufreq/cpufreq.c +0
-8@@ -1779,15 +1779,14 @@ void cpufreq_resume(void) || __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS))pr_err ("%s: Failed to start governor for policy: %p\n" , __func__, policy); - -/* -* schedule call cpufreq_update_policy() for boot CPU, i.e. last -* policy in list. It will verify that the current freq is in -* sync with what we believe it to be. -*/ -if (list_is_last (&policy->policy_list, &cpufreq_policy_list)) -schedule_work (&policy->update); }
drivers/cpufreq/cpufreq.c void cpufreq_resume(void) · line 1792
Quoted 10 lines
drivers/cpufreq/cpufreq.c +7
-0+ +/* +* schedule call cpufreq_update_policy() for boot CPU, i.e. last +* policy in list. It will verify that the current freq is in +* sync with what we believe it to be. +*/ +schedule_work (&policy->update); }/**
Ok, I don't think this will work after the next patch/end of the series.
Some ground rules/clarification: When the suspend/resume code and this cpufreq_suspend/resume functions talk about boot CPU, they don't really mean the boot CPU. It really means the CPU with the smallest CPU ID when suspend is initiated. For example in a 4 core system, if CPU0 is hotplugged off, then "boot CPU" in the context of suspend/resume is actually CPU1. That's the reason we can't simply do "get policy of CPU0" and the schedule work.
So what this patch really is assuming is that the last active policy in the policy list corresponds to the CPU with the smallest CPU ID when suspend is initiated.
That was true when we added/deleted policies when CPUs are hotplugged on/off (even during lightweight tear-down, we did add/delete). However, at the end of the series, we won't be adding/deleting these policies from the list for each hotplug add/remove. So, the last active policy in the list doesn't always correspond to the online CPU with the smallest CPU ID during suspend/resume.
Here's an example case:
* Assume we never physically hot plug the CPUs.
* Assume it's a system with 4 independent CPUs.
* We boot with just CPU0 active.
* So, the policy list will now have: CPU0 policy.
* CPU2 is then brought online.
* So, the policy list will now have: CPU2 policy, CPU0 policy.
* CPU1 is then brought online.
* So, the policy list will now have: CPU1 policy CPU2 policy, CPU0 policy.
* CPU0 is taken offline.
* So, the policy list will now have: CPU1 policy CPU2 policy, CPU0 inactive policy.
* Now suspend happens and then we resume.
* We really need to be scheduling the work for CPU1 since that's the "boot cpu" for suspend/resume.
* But the last active policy is for CPU2 and we schedule work for that.
Nack for the patch, unless I missed some list maintenance code.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 07/20] cpufreq: Call schedule_work() for the last active policy
Quoted 1 line · Saravana Kannan
Ok, I don't think this will work after the next patch/end of the series.
Sigh, Its broken today as well. See my another patch:
[PATCH] cpufreq: Schedule work for the first-online CPU on resume
This patch is dropped now.
Re: [PATCH V2 07/20] cpufreq: Call schedule_work() for the last active policy
Quoted 8 lines · Viresh Kumar
Quoted 1 line · Saravana Kannan
Ok, I don't think this will work after the next patch/end of the series.
Sigh, Its broken today as well. See my another patch:
[PATCH] cpufreq: Schedule work for the first-online CPU on resume
This patch is dropped now.
What exactly does this mean?
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 07/20] cpufreq: Call schedule_work() for the last active policy
Quoted 1 line · Rafael J. Wysocki
What exactly does this mean?
That the $Subject patch is replaced by:
c75de0ac0756 ("cpufreq: Schedule work for the first-online CPU on resume")
as its not that my series has broken what Saravana has found out. But it was broken even without my series.
Viresh Kumar
maintainerPATCH v2 8/20 cpufreq: Don't clear cpufreq_cpu_data and policy list for inactive policies
In reply to Viresh KumarNow that we can check policy->cpus to find if policy is active or not, we don't need to clean cpufreq_cpu_data and delete policy from the list on light weight tear down of policies (like in suspend).
To make it consistent and clean, set cpufreq_cpu_data for all related CPUs when the policy is first created and clean it only while it is freed.
Also update cpufreq_cpu_get_raw() to check if cpu is part of policy->cpus mask, so that we don't end up getting policies for unmanaged CPUs.
In order to make sure that no users of 'policy' are using an inactive policy, use cpufreq_cpu_get_raw() instead of directly accessing cpufreq_cpu_data.
--- drivers/cpufreq/cpufreq.c | 79 ++++++++++++++++++++--------------------------- 1 file changed, 34 insertions(+), 45 deletions(-)
drivers/cpufreq/cpufreq.c +34 -45
@@ -250,10 +250,18 @@ int cpufreq_generic_init(struct cpufreq_policy *policy, }EXPORT_SYMBOL_GPL (cpufreq_generic_init); -unsigned int cpufreq_generic_get (unsigned int cpu) +/* Only for cpufreq core internal use */ +struct cpufreq_policy *cpufreq_cpu_get_raw (unsigned int cpu) {struct cpufreq_policy *policy =per_cpu (cpufreq_cpu_data, cpu); +return policy &&cpumask_test_cpu (cpu, policy->cpus) ? policy :NULL ; +} + +unsigned int cpufreq_generic_get (unsigned int cpu) +{ +struct cpufreq_policy *policy =cpufreq_cpu_get_raw (cpu); +if (!policy ||IS_ERR (policy->clk)) {pr_err ("%s: No %s associated to cpu: %d\n" , __func__, policy ?"clk" :"policy" , cpu);
@@ -264,12 +272,6 @@ unsigned int cpufreq_generic_get(unsigned int cpu) }EXPORT_SYMBOL_GPL (cpufreq_generic_get); -/* Only for cpufreq core internal use */ -struct cpufreq_policy *cpufreq_cpu_get_raw (unsigned int cpu) -{ -return per_cpu (cpufreq_cpu_data, cpu); -} -/** * cpufreq_cpu_get: returns policy for a cpu and marks it busy. *
@@ -303,7 +305,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)if (cpufreq_driver) {/* get the CPU */ - policy =per_cpu (cpufreq_cpu_data, cpu); + policy =cpufreq_cpu_get_raw (cpu);if (policy)kobject_get (&policy->kobj); }
@@ -1053,7 +1055,6 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, struct device *dev) { int ret = 0; -unsigned long flags;/* Is cpu already managed ? */ if (cpumask_test_cpu (cpu, policy->cpus))
@@ -1068,13 +1069,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, }down_write (&policy->rwsem); - -write_lock_irqsave (&cpufreq_driver_lock, flags); -cpumask_set_cpu (cpu, policy->cpus); -per_cpu (cpufreq_cpu_data, cpu) = policy; -write_unlock_irqrestore (&cpufreq_driver_lock, flags); -up_write (&policy->rwsem);if (has_target ()) {
@@ -1165,6 +1160,17 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)static void cpufreq_policy_free (struct cpufreq_policy *policy) { +unsigned long flags; +int cpu; + +/* Remove policy from list */ +write_lock_irqsave (&cpufreq_driver_lock, flags); +list_del (&policy->policy_list); + +for_each_cpu (cpu, policy->related_cpus) +per_cpu (cpufreq_cpu_data, cpu) =NULL ; +write_unlock_irqrestore (&cpufreq_driver_lock, flags); +free_cpumask_var (policy->related_cpus);free_cpumask_var (policy->cpus);kfree (policy);
@@ -1286,12 +1292,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) __func__, ret); goto err_init_policy_kobj; } - } -write_lock_irqsave (&cpufreq_driver_lock, flags); -for_each_cpu (j, policy->cpus) -per_cpu (cpufreq_cpu_data, j) = policy; -write_unlock_irqrestore (&cpufreq_driver_lock, flags); +write_lock_irqsave (&cpufreq_driver_lock, flags); +for_each_cpu (j, policy->related_cpus) +per_cpu (cpufreq_cpu_data, j) = policy; +write_unlock_irqrestore (&cpufreq_driver_lock, flags); + }if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { policy->cur = cpufreq_driver->get (policy->cpu);
@@ -1350,11 +1356,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) goto err_out_unregister;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); +write_lock_irqsave (&cpufreq_driver_lock, flags); +list_add (&policy->policy_list, &cpufreq_policy_list); +write_unlock_irqrestore (&cpufreq_driver_lock, flags); + }cpufreq_init_policy (policy);
@@ -1378,11 +1384,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) err_out_unregister: err_get_freq: -write_lock_irqsave (&cpufreq_driver_lock, flags); -for_each_cpu (j, policy->cpus) -per_cpu (cpufreq_cpu_data, j) =NULL ; -write_unlock_irqrestore (&cpufreq_driver_lock, flags); -if (!recover_policy) {kobject_put (&policy->kobj);wait_for_completion (&policy->kobj_unregister);
@@ -1418,7 +1419,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,write_lock_irqsave (&cpufreq_driver_lock, flags); - policy =per_cpu (cpufreq_cpu_data, cpu); + policy =cpufreq_cpu_get_raw (cpu);/* Save the policy somewhere when doing a light-weight tear-down */ if (cpufreq_suspended)
@@ -1476,15 +1477,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,static int __cpufreq_remove_dev_finish (struct device *dev,struct subsys_interface *sif) { -unsigned int cpu = dev->id, cpus; +unsigned int cpu = dev->id;int ret; -unsigned long flags; -struct cpufreq_policy *policy; - -write_lock_irqsave (&cpufreq_driver_lock, flags); - policy =per_cpu (cpufreq_cpu_data, cpu); -per_cpu (cpufreq_cpu_data, cpu) =NULL ; -write_unlock_irqrestore (&cpufreq_driver_lock, flags); +struct cpufreq_policy *policy =cpufreq_cpu_get_raw (cpu);if (!policy) {pr_debug ("%s: No cpu_data found\n" , __func__);
@@ -1492,12 +1487,11 @@ static int __cpufreq_remove_dev_finish(struct device *dev, }down_write (&policy->rwsem); - cpus =cpumask_weight (policy->cpus);cpumask_clear_cpu (cpu, policy->cpus);up_write (&policy->rwsem);/* If cpu is last user of policy, free policy */ -if (cpus ==1 ) { +if (policy_is_inactive (policy)) {if (has_target ()) { ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT );
@@ -1519,11 +1513,6 @@ static int __cpufreq_remove_dev_finish(struct device *dev,if (cpufreq_driver->exit) cpufreq_driver->exit (policy); -/* Remove policy from list of active policies */ -write_lock_irqsave (&cpufreq_driver_lock, flags); -list_del (&policy->policy_list); -write_unlock_irqrestore (&cpufreq_driver_lock, flags); -if (!cpufreq_suspended)cpufreq_policy_free (policy); }else if (has_target ()) {
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 08/20] cpufreq: Don't clear cpufreq_cpu_data and policy list for inactive policies
Quoted 9 lines · Viresh Kumar
Now that we can check
policy->cpusto find if policy is active or not, we don't need to cleancpufreq_cpu_dataand delete policy from the list on light weight tear down of policies (like in suspend).To make it consistent and clean, set
cpufreq_cpu_datafor all related CPUs when the policy is first created and clean it only while it is freed.Also update
cpufreq_cpu_get_raw()to check if cpu is part ofpolicy->cpusmask, so that we don't end up getting policies for unmanaged CPUs.
When you say unmanaged CPUs, do you mean offline CPUs? If so, could you use that term instead? The term unmanaged is kinda ambiguous. This comment applies to the entire series.
Quoted 16 lines
In order to make sure that no users of 'policy' are using an inactive policy, use
cpufreq_cpu_get_raw()instead of directly accessingcpufreq_cpu_data.Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 79 ++++++++++++++++++++--------------------------- 1 file changed, 34 insertions(+), 45 deletions(-)drivers/cpufreq/cpufreq.c +0
-0@@ -250,10 +250,18 @@ int cpufreq_generic_init(struct cpufreq_policy *policy, }EXPORT_SYMBOL_GPL (cpufreq_generic_init);
<SNIP>
Quoted 45 lines
drivers/cpufreq/cpufreq.c +11
-8@@ -1053,7 +1055,6 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, struct device *dev) { int ret = 0; -unsigned long flags;/* Is cpu already managed ? */ if (cpumask_test_cpu (cpu, policy->cpus))@@ -1068,13 +1069,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, }down_write (&policy->rwsem); - -write_lock_irqsave (&cpufreq_driver_lock, flags); -cpumask_set_cpu (cpu, policy->cpus); -per_cpu (cpufreq_cpu_data, cpu) = policy; -write_unlock_irqrestore (&cpufreq_driver_lock, flags); -up_write (&policy->rwsem);if (has_target ()) {@@ -1165,6 +1160,17 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)static void cpufreq_policy_free (struct cpufreq_policy *policy) { +unsigned long flags; +int cpu; + +/* Remove policy from list */ +write_lock_irqsave (&cpufreq_driver_lock, flags); +list_del (&policy->policy_list); + +for_each_cpu (cpu, policy->related_cpus) +per_cpu (cpufreq_cpu_data, cpu) =NULL ; +write_unlock_irqrestore (&cpufreq_driver_lock, flags); +free_cpumask_var (policy->related_cpus);free_cpumask_var (policy->cpus);kfree (policy);@@ -1286,12 +1292,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) __func__, ret); goto err_init_policy_kobj; } - }
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) · line 1300
Quoted 10 lines
drivers/cpufreq/cpufreq.c +5
-4-write_lock_irqsave (&cpufreq_driver_lock, flags); -for_each_cpu (j, policy->cpus) -per_cpu (cpufreq_cpu_data, j) = policy; -write_unlock_irqrestore (&cpufreq_driver_lock, flags); +write_lock_irqsave (&cpufreq_driver_lock, flags); +for_each_cpu (j, policy->related_cpus) +per_cpu (cpufreq_cpu_data, j) = policy; +write_unlock_irqrestore (&cpufreq_driver_lock, flags); + }
Why not do this is cpufreq_policy_alloc() to be consistent/symmetric with cpufreq_policy_free()? You had to set cpufreq_cpu_data/add to the list this late in cpufreq_add_dev before because you shouldn't be adding a unfinished policy to the cpufreq_cpu_data/list. But now that you check for policy->cpus before returning it, you may be do all of this in cpufreq_policy_alloc()?
Quoted 5 lines
drivers/cpufreq/cpufreq.c +0
-0if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { policy->cur = cpufreq_driver->get (policy->cpu);@@ -1350,11 +1356,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) goto err_out_unregister; blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) · line 1363
Quoted 10 lines
drivers/cpufreq/cpufreq.c +4
-4CPUFREQ_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); +write_lock_irqsave (&cpufreq_driver_lock, flags); +list_add (&policy->policy_list, &cpufreq_policy_list); +write_unlock_irqrestore (&cpufreq_driver_lock, flags); + }
You could probably do this as part of policy alloc too?
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) · line 1384
Quoted 5 lines
drivers/cpufreq/cpufreq.c +0
-0cpufreq_init_policy (policy);@@ -1378,11 +1384,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
Rest of the patch looks good.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 08/20] cpufreq: Don't clear cpufreq_cpu_data and policy list for inactive policies
Quoted 3 lines · Saravana Kannan
When you say unmanaged CPUs, do you mean offline CPUs? If so, could you use that term instead? The term unmanaged is kinda ambiguous. This comment applies to the entire series.
Hmm, will take a note of that.
Quoted 11 lines
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) · line 1300
Quoted 5 lines
drivers/cpufreq/cpufreq.c +5
-0+write_lock_irqsave (&cpufreq_driver_lock, flags); +for_each_cpu (j, policy->related_cpus) +per_cpu (cpufreq_cpu_data, j) = policy; +write_unlock_irqrestore (&cpufreq_driver_lock, flags); + }Why not do this is
cpufreq_policy_alloc()to be consistent/symmetric withcpufreq_policy_free()? You had to set cpufreq_cpu_data/add to the list this late incpufreq_add_devbefore because you shouldn't be adding a unfinished policy to the cpufreq_cpu_data/list. But now that you check forpolicy->cpusbefore returning it, you may be do all of this incpufreq_policy_alloc()?
We don't know related_cpus until the time ->init() is called and that happens after alloc()..
Quoted 6 lines
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) · line 1363
Quoted 4 lines
drivers/cpufreq/cpufreq.c +4
-0+write_lock_irqsave (&cpufreq_driver_lock, flags); +list_add (&policy->policy_list, &cpufreq_policy_list); +write_unlock_irqrestore (&cpufreq_driver_lock, flags); + }You could probably do this as part of policy alloc too?
I wouldn't like to add a unfinished policy (which may or maynot be simultaneously referred by other parts of code) to the list.
Viresh Kumar
maintainerPATCH v2 9/20 cpufreq: Get rid of cpufreq_cpu_data_fallback
In reply to Viresh KumarWe can extract the same information from cpufreq_cpu_data as it is also available for inactive policies now.
--- drivers/cpufreq/cpufreq.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
drivers/cpufreq/cpufreq.c +6 -19
@@ -112,7 +112,6 @@ static LIST_HEAD(cpufreq_governor_list);*/ static struct cpufreq_driver *cpufreq_driver; static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); -static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);static DEFINE_RWLOCK (cpufreq_driver_lock);DEFINE_MUTEX (cpufreq_governor_lock);
@@ -1092,13 +1091,14 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)unsigned long flags;read_lock_irqsave (&cpufreq_driver_lock, flags); - - policy =per_cpu (cpufreq_cpu_data_fallback, cpu); - + policy =per_cpu (cpufreq_cpu_data, cpu);read_unlock_irqrestore (&cpufreq_driver_lock, flags); -if (policy) +if (likely (policy)) { +/* Policy should be inactive here */ +WARN_ON (!policy_is_inactive (policy)); policy->governor =NULL ; + }return policy; }
@@ -1394,11 +1394,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)if (cpufreq_driver->exit) cpufreq_driver->exit (policy); err_set_policy_cpu: -if (recover_policy) { -/* Do not leave stale fallback data behind. */ -per_cpu (cpufreq_cpu_data_fallback, cpu) =NULL ; +if (recover_policy)cpufreq_policy_put_kobj (policy); - }cpufreq_policy_free (policy); nomem_out:
@@ -1412,21 +1409,11 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, {unsigned int cpu = dev->id, cpus;int ret; -unsigned long flags;struct cpufreq_policy *policy;pr_debug ("%s: unregistering CPU %u\n" , __func__, cpu); -write_lock_irqsave (&cpufreq_driver_lock, flags); - policy =cpufreq_cpu_get_raw (cpu); - -/* Save the policy somewhere when doing a light-weight tear-down */ -if (cpufreq_suspended) -per_cpu (cpufreq_cpu_data_fallback, cpu) = policy; - -write_unlock_irqrestore (&cpufreq_driver_lock, flags); -if (!policy) {pr_debug ("%s: No cpu_data found\n" , __func__);return -EINVAL ;
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 09/20] cpufreq: Get rid of cpufreq_cpu_data_fallback
Quoted 65 lines · Viresh Kumar
We can extract the same information from
cpufreq_cpu_dataas it is also available for inactive policies now.Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)drivers/cpufreq/cpufreq.c +6
-12@@ -112,7 +112,6 @@ static LIST_HEAD(cpufreq_governor_list);*/ static struct cpufreq_driver *cpufreq_driver; static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); -static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);static DEFINE_RWLOCK (cpufreq_driver_lock);DEFINE_MUTEX (cpufreq_governor_lock);@@ -1092,13 +1091,14 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)unsigned long flags;read_lock_irqsave (&cpufreq_driver_lock, flags); - - policy =per_cpu (cpufreq_cpu_data_fallback, cpu); - + policy =per_cpu (cpufreq_cpu_data, cpu);read_unlock_irqrestore (&cpufreq_driver_lock, flags); -if (policy) +if (likely (policy)) { +/* Policy should be inactive here */ +WARN_ON (!policy_is_inactive (policy)); policy->governor =NULL ; + }return policy; }@@ -1394,11 +1394,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)if (cpufreq_driver->exit) cpufreq_driver->exit (policy); err_set_policy_cpu: -if (recover_policy) { -/* Do not leave stale fallback data behind. */ -per_cpu (cpufreq_cpu_data_fallback, cpu) =NULL ; +if (recover_policy)cpufreq_policy_put_kobj (policy); - }cpufreq_policy_free (policy); nomem_out:@@ -1412,21 +1409,11 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, {unsigned int cpu = dev->id, cpus;int ret; -unsigned long flags;struct cpufreq_policy *policy;pr_debug ("%s: unregistering CPU %u\n" , __func__, cpu); -write_lock_irqsave (&cpufreq_driver_lock, flags); - policy =cpufreq_cpu_get_raw (cpu);
drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev_prepare(struct device *dev, · line 1419
Quoted 10 lines
drivers/cpufreq/cpufreq.c +0
-7- -/* Save the policy somewhere when doing a light-weight tear-down */ -if (cpufreq_suspended) -per_cpu (cpufreq_cpu_data_fallback, cpu) = policy; - -write_unlock_irqrestore (&cpufreq_driver_lock, flags); -if (!policy) {pr_debug ("%s: No cpu_data found\n" , __func__);return -EINVAL ;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerPATCH v2 10/20 cpufreq: Don't traverse list of all policies for adding policy for a cpu
In reply to Viresh KumarWe reach here while adding policy for a CPU and enter into the 'if' block only if a policy already exists for the CPU in question. As cpufreq_cpu_data is filled once and for all now, we can reuse it to find the relevant policy instead of traversing the list of all policies.
--- drivers/cpufreq/cpufreq.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
drivers/cpufreq/cpufreq.c +7 -10
@@ -1211,7 +1211,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) {unsigned int j, cpu = dev->id;int ret = -ENOMEM ; -struct cpufreq_policy *policy, *tpolicy; +struct cpufreq_policy *policy;unsigned long flags;bool recover_policy = cpufreq_suspended;
@@ -1224,16 +1224,13 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)return 0 ;/* Check if this cpu already has a policy to manage it */ -read_lock_irqsave (&cpufreq_driver_lock, flags); -for_each_active_policy (policy, tpolicy) { -if (cpumask_test_cpu (cpu, policy->related_cpus)) { -read_unlock_irqrestore (&cpufreq_driver_lock, flags); - ret =cpufreq_add_policy_cpu (policy, cpu, dev); -up_read (&cpufreq_rwsem); -return ret; - } + policy =per_cpu (cpufreq_cpu_data, cpu); +if (policy && !policy_is_inactive (policy)) { +WARN_ON (!cpumask_test_cpu (cpu, policy->related_cpus)); + ret =cpufreq_add_policy_cpu (policy, cpu, dev); +up_read (&cpufreq_rwsem); +return ret; } -read_unlock_irqrestore (&cpufreq_driver_lock, flags);/* * Restore the saved policy when doing light-weight init and fall back
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 10/20] cpufreq: Don't traverse list of all policies for adding policy for a cpu
Quoted 37 lines · Viresh Kumar
We reach here while adding policy for a CPU and enter into the 'if' block only if a policy already exists for the CPU in question. As
cpufreq_cpu_datais filled once and for all now, we can reuse it to find the relevant policy instead of traversing the list of all policies.Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)drivers/cpufreq/cpufreq.c +2
-9@@ -1211,7 +1211,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) {unsigned int j, cpu = dev->id;int ret = -ENOMEM ; -struct cpufreq_policy *policy, *tpolicy; +struct cpufreq_policy *policy;unsigned long flags;bool recover_policy = cpufreq_suspended;@@ -1224,16 +1224,13 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)return 0 ;/* Check if this cpu already has a policy to manage it */ -read_lock_irqsave (&cpufreq_driver_lock, flags); -for_each_active_policy (policy, tpolicy) { -if (cpumask_test_cpu (cpu, policy->related_cpus)) { -read_unlock_irqrestore (&cpufreq_driver_lock, flags); - ret =cpufreq_add_policy_cpu (policy, cpu, dev); -up_read (&cpufreq_rwsem); -return ret; - } + policy =per_cpu (cpufreq_cpu_data, cpu);
drivers/cpufreq/cpufreq.c static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) · line 1236
Quoted 10 lines
drivers/cpufreq/cpufreq.c +5
-1+if (policy && !policy_is_inactive (policy)) { +WARN_ON (!cpumask_test_cpu (cpu, policy->related_cpus)); + ret =cpufreq_add_policy_cpu (policy, cpu, dev); +up_read (&cpufreq_rwsem); +return ret; } -read_unlock_irqrestore (&cpufreq_driver_lock, flags);/* * Restore the saved policy when doing light-weight init and fall back
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerPATCH v2 11/20 cpufreq: Manage governor usage history with 'policy->last_governor'
In reply to Viresh KumarHistory of which governor was used last is common to all CPUs within a policy and maintaining it per-cpu isn't the best approach for sure.
Apart from wasting memory, this also increases the complexity of managing this data structure as it has to be updated for all CPUs.
To make that somewhat simpler, lets store this information in a new field 'last_governor' in struct cpufreq_policy and update it on removal of last cpu of a policy.
--- drivers/cpufreq/cpufreq.c | 30 +++++++++++++++--------------- include/linux/cpufreq.h | 1 + 2 files changed, 16 insertions(+), 15 deletions(-)
drivers/cpufreq/cpufreq.c +15 -15
@@ -115,9 +115,6 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);static DEFINE_RWLOCK (cpufreq_driver_lock);DEFINE_MUTEX (cpufreq_governor_lock); -/* This one keeps track of the previously set governor of a removed CPU */ -static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); -/* Flag to suspend/resume CPUFreq governors */ static bool cpufreq_suspended;
@@ -1028,7 +1025,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)memcpy (&new_policy, policy,sizeof (*policy));/* Update governor of new_policy to the governor used before hotplug */ - gov =find_governor (per_cpu (cpufreq_cpu_governor, policy->cpu)); + gov =find_governor (policy->last_governor);if (gov)pr_debug ("Restoring governor %s for cpu %d\n" , policy->governor->name, policy->cpu);
@@ -1422,14 +1419,15 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,pr_err ("%s: Failed to stop governor\n" , __func__);return ret; } - -strncpy (per_cpu (cpufreq_cpu_governor, cpu), - policy->governor->name,CPUFREQ_NAME_LEN ); } -down_read (&policy->rwsem); +down_write (&policy->rwsem); cpus =cpumask_weight (policy->cpus); -up_read (&policy->rwsem); + +if (has_target () && cpus ==1 ) +strncpy (policy->last_governor, policy->governor->name, +CPUFREQ_NAME_LEN ); +up_write (&policy->rwsem);if (cpu != policy->cpu) {sysfs_remove_link (&dev->kobj,"cpufreq" );
@@ -2142,7 +2140,8 @@ EXPORT_SYMBOL_GPL(cpufreq_register_governor);void cpufreq_unregister_governor (struct cpufreq_governor *governor) { -int cpu; +struct cpufreq_policy *policy, *tpolicy; +unsigned long flags;if (!governor)return ;
@@ -2150,12 +2149,13 @@ void cpufreq_unregister_governor(struct cpufreq_governor *governor)if (cpufreq_disabled ())return ; -for_each_present_cpu (cpu) { -if (cpu_online (cpu)) -continue ; -if (!strcmp (per_cpu (cpufreq_cpu_governor, cpu), governor->name)) -strcpy (per_cpu (cpufreq_cpu_governor, cpu),"\0" ); +/* clear last_governor for all inactive policies */ +read_lock_irqsave (&cpufreq_driver_lock, flags); +for_each_inactive_policy (policy, tpolicy) { +if (!strcmp (policy->last_governor, governor->name)) +strcpy (policy->last_governor,"\0" ); } +read_unlock_irqrestore (&cpufreq_driver_lock, flags);mutex_lock (&cpufreq_governor_mutex);list_del (&governor->governor_list);
include/linux/cpufreq.h +1 -0
@@ -80,6 +80,7 @@ struct cpufreq_policy {struct cpufreq_governor *governor;/* see below */ void *governor_data;bool governor_enabled;/* governor start/stop flag */ +char last_governor[CPUFREQ_NAME_LEN ];/* last governor used */ struct work_struct update;/* if update_policy() needs to be * called, but you're in IRQ context */
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 11/20] cpufreq: Manage governor usage history with 'policy->last_governor'
Quoted 9 lines · Viresh Kumar
History of which governor was used last is common to all CPUs within a policy and maintaining it per-cpu isn't the best approach for sure.
Apart from wasting memory, this also increases the complexity of managing this data structure as it has to be updated for all CPUs.
To make that somewhat simpler, lets store this information in a new field '
last_governor' in structcpufreq_policyand update it on removal of last cpu of a policy.
This governor restore code actually has bug -- at least in 3.12 (I think) but probably has a bug in this kernel too. So, this patch actually also fixes a bug.
The bug has to do with restoring the wrong governor (as in, not the governor that was last picked by userspace) for a particular hot plug sequence.
Assume one cluster has CPUs 2, 3.
Assume default governor is performance.
CPU2 is first brought online.
Governor is changed to ondemand.
CPU2 is taken offline.
CPU3 is brought online.
Governor gets set to performance.
So, governor for the cluster is now not what was last picked by userspace.
Quoted 62 lines
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 30 +++++++++++++++--------------- include/linux/cpufreq.h | 1 + 2 files changed, 16 insertions(+), 15 deletions(-)drivers/cpufreq/cpufreq.c +9
-10@@ -115,9 +115,6 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);static DEFINE_RWLOCK (cpufreq_driver_lock);DEFINE_MUTEX (cpufreq_governor_lock); -/* This one keeps track of the previously set governor of a removed CPU */ -static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); -/* Flag to suspend/resume CPUFreq governors */ static bool cpufreq_suspended;@@ -1028,7 +1025,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)memcpy (&new_policy, policy,sizeof (*policy));/* Update governor of new_policy to the governor used before hotplug */ - gov =find_governor (per_cpu (cpufreq_cpu_governor, policy->cpu)); + gov =find_governor (policy->last_governor);if (gov)pr_debug ("Restoring governor %s for cpu %d\n" , policy->governor->name, policy->cpu);@@ -1422,14 +1419,15 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,pr_err ("%s: Failed to stop governor\n" , __func__);return ret; } - -strncpy (per_cpu (cpufreq_cpu_governor, cpu), - policy->governor->name,CPUFREQ_NAME_LEN ); } -down_read (&policy->rwsem); +down_write (&policy->rwsem); cpus =cpumask_weight (policy->cpus); -up_read (&policy->rwsem); + +if (has_target () && cpus ==1 ) +strncpy (policy->last_governor, policy->governor->name, +CPUFREQ_NAME_LEN ); +up_write (&policy->rwsem);if (cpu != policy->cpu) {sysfs_remove_link (&dev->kobj,"cpufreq" );@@ -2142,7 +2140,8 @@ EXPORT_SYMBOL_GPL(cpufreq_register_governor);void cpufreq_unregister_governor (struct cpufreq_governor *governor) { -int cpu; +struct cpufreq_policy *policy, *tpolicy; +unsigned long flags;if (!governor)return ;
drivers/cpufreq/cpufreq.c void cpufreq_unregister_governor(struct cpufreq_governor *governor) · line 2153
Quoted 10 lines
drivers/cpufreq/cpufreq.c +2
-5@@ -2150,12 +2149,13 @@ void cpufreq_unregister_governor(struct cpufreq_governor *governor)if (cpufreq_disabled ())return ; -for_each_present_cpu (cpu) { -if (cpu_online (cpu)) -continue ; -if (!strcmp (per_cpu (cpufreq_cpu_governor, cpu), governor->name)) -strcpy (per_cpu (cpufreq_cpu_governor, cpu),"\0" ); +/* clear last_governor for all inactive policies */ +read_lock_irqsave (&cpufreq_driver_lock, flags);
You are writing to a variable that's protected. This should be a write lock.
Quoted 14 lines
drivers/cpufreq/cpufreq.c +4
-0+for_each_inactive_policy (policy, tpolicy) { +if (!strcmp (policy->last_governor, governor->name)) +strcpy (policy->last_governor,"\0" ); } +read_unlock_irqrestore (&cpufreq_driver_lock, flags);mutex_lock (&cpufreq_governor_mutex);list_del (&governor->governor_list);include/linux/cpufreq.h +0
-0
include/linux/cpufreq.h struct cpufreq_policy { · line 86
Quoted 7 lines
include/linux/cpufreq.h +1
-0@@ -80,6 +80,7 @@ struct cpufreq_policy {struct cpufreq_governor *governor;/* see below */ void *governor_data;bool governor_enabled;/* governor start/stop flag */ +char last_governor[CPUFREQ_NAME_LEN ];/* last governor used */ struct work_struct update;/* if update_policy() needs to be * called, but you're in IRQ context */
Nack due to lock.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 11/20] cpufreq: Manage governor usage history with 'policy->last_governor'
Quoted 7 lines · Saravana Kannan
This governor restore code actually has bug -- at least in 3.12 (I think) but probably has a bug in this kernel too. So, this patch actually also fixes a bug.
The bug has to do with restoring the wrong governor (as in, not the governor that was last picked by userspace) for a particular hot plug sequence.
Quoted 8 lines
Assume one cluster has CPUs 2, 3.
Assume default governor is performance.
CPU2 is first brought online.
Governor is changed to ondemand.
CPU2 is taken offline.
CPU3 is brought online.
Governor gets set to performance.
So, governor for the cluster is now not what was last picked by userspace.
Wasn't aware of that, you are right. Updated my logs for this.
Quoted 11 lines
drivers/cpufreq/cpufreq.c void cpufreq_unregister_governor(struct cpufreq_governor *governor) · line 2153
Quoted 8 lines
drivers/cpufreq/cpufreq.c +0
-4- for_each_present_cpu(cpu) { - if (cpu_online(cpu)) - continue; - if (!strcmp(per_cpu(cpufreq_cpu_governor, cpu),
governor->name))drivers/cpufreq/cpufreq.c +2
-1-strcpy (per_cpu (cpufreq_cpu_governor, cpu),"\0" ); +/* clear last_governor for all inactive policies */ +read_lock_irqsave (&cpufreq_driver_lock, flags);You are writing to a variable that's protected. This should be a write lock.
cpufreq_driver_lock isn't responsible for protecting policy fields, but the list of policies. And so was a read lock here.
Quoted 5 lines
drivers/cpufreq/cpufreq.c void cpufreq_unregister_governor(struct cpufreq_governor *governor) · line 2158
Quoted 5 lines
drivers/cpufreq/cpufreq.c +4
-0+for_each_inactive_policy (policy, tpolicy) { +if (!strcmp (policy->last_governor, governor->name)) +strcpy (policy->last_governor,"\0" ); } +read_unlock_irqrestore (&cpufreq_driver_lock, flags);
And we don't need to take writelock to policy->rwsem here as the policy is already inactive.
Quoted 1 line
Nack due to lock.
Just to clear the concepts, and not fighting with you :)
I don't think you used Nack correctly here. You have raised some mistakes in the patch here, and its not that the patch and the idea behind it is not acceptable.
So, probably Nack was a bit harsh :)
Viresh Kumar
maintainerPATCH v2 12/20 cpufreq: Mark policy->governor = NULL for inactive policies
In reply to Viresh KumarLater commits would change the way policies are managed today. Policies wouldn't be freed on cpu hotplug (currently they aren't freed on suspend), and while the CPU is offline, the sysfs cpufreq files would still be present.
Because we don't mark policy->governor as NULL, it still contains pointer of the last governor it used. And when we read the 'scaling_governor' file, it shows the old value.
To prevent from this, mark policy->governor as NULL after we have issued CPUFREQ_GOV_POLICY_EXIT event for its governor.
--- drivers/cpufreq/cpufreq.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/cpufreq/cpufreq.c +2 -1
@@ -1094,7 +1094,6 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)if (likely (policy)) {/* Policy should be inactive here */ WARN_ON (!policy_is_inactive (policy)); - policy->governor =NULL ; }return policy;
@@ -1497,6 +1496,8 @@ static int __cpufreq_remove_dev_finish(struct device *dev,if (!cpufreq_suspended)cpufreq_policy_free (policy); +else + policy->governor =NULL ; }else if (has_target ()) { ret =__cpufreq_governor (policy,CPUFREQ_GOV_START );if (!ret)
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 12/20] cpufreq: Mark policy->governor = NULL for inactive policies
Quoted 7 lines · Viresh Kumar
Later commits would change the way policies are managed today. Policies wouldn't be freed on cpu hotplug (currently they aren't freed on suspend), and while the CPU is offline, the sysfs cpufreq files would still be present.
Because we don't mark
policy->governoras NULL, it still contains pointer of the last governor it used. And when we read the 'scaling_governor' file, it shows the old value.
What's wrong with this behavior? If you cat scaling_min/max_freq, it's going to show the last minimum and maximum freqs too. I don't think this needs to be set to NULL until the governor is unloaded. Which you are already taking care of in the previous patch.
It's handy to be able to tell what governor will be restored when a cluster if offline.
Nacked because I think this patch is not helping.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 12/20] cpufreq: Mark policy->governor = NULL for inactive policies
Quoted 9 lines · Saravana Kannan
What's wrong with this behavior? If you cat scaling_min/max_freq, it's going to show the last minimum and maximum freqs too. I don't think this needs to be set to NULL until the governor is unloaded. Which you are already taking care of in the previous patch.
It's handy to be able to tell what governor will be restored when a cluster if offline.
Nacked because I think this patch is not helping.
Oh, I really believe this patch makes sense. :)
Don't confuse it with the 'last-governor' thing or scaling_min/max stuff.. Its different.
Consider a scenario:
- Cluster was using ONDEMAND governor.
- Cluster hotplugged out
- Governor removed, but the pointer policy->governor isn't updated.
-> At this point accessing 'scaling_governor' or 'scaling_setspeed' can get called and that will result in a crash.
- We go ahead and insert the governor again
-> At this point also the same problem will happen as governor will get allocated again.
Now, what we are talking about here is a pointer to the governor, not its name which is stored in 'last_governor' in the previous patch.
We store that name as it is used for internal working of the core, not for userspace.
What we expose to userspace must be consistent, and so if the governor is removed, we will not be able to provide 'scaling_setspeed' or 'scaling_governor' to userspace.
Also, it is scaling-governor, not what the next governor is going to be. Plus, what should we print on scaling_setspeed of a cluster not running anymore ?
And so why keep something that we can't provide to userspace all the time. That may break userspace if it relies on it..
Saravana Kannan
NAKRe: [PATCH V2 12/20] cpufreq: Mark policy->governor = NULL for inactive policies
Quoted 9 lines · Viresh Kumar
Quoted 8 lines · Saravana Kannan
What's wrong with this behavior? If you cat scaling_min/max_freq, it's going to show the last minimum and maximum freqs too. I don't think this needs to be set to NULL until the governor is unloaded. Which you are already taking care of in the previous patch.
It's handy to be able to tell what governor will be restored when a cluster if offline.
Quoted 6 lines
Quoted 1 line
Nacked because I think this patch is not helping.
Oh, I really believe this patch makes sense. :)
Don't confuse it with the 'last-governor' thing or scaling_min/max stuff.. Its different.
Ok, I agree that I misread your other patch and mentally read it as setting both last_governor to "" and the governor pointer to NULL. I think if you set the governor to NULL in the same for loop, you would solve the issues you mention below without compromising the case where the governor is not removed and you still let userspace query what the last governor is without having to online a CPU.
I think that's way more user friendly without and real negatives. We shouldn't make it any less user friendly than it needs to be.
Quoted 20 lines
Consider a scenario:
- Cluster was using ONDEMAND governor.
- Cluster hotplugged out
- Governor removed, but the pointerpolicy->governorisn't updated.-> At this point accessing '
scaling_governor' or 'scaling_setspeed' can get called and that will result in a crash.- We go ahead and insert the governor again
-> At this point also the same problem will happen as governor will get allocated again.
Now, what we are talking about here is a pointer to the governor, not its name which is stored in '
last_governor' in the previous patch.We store that name as it is used for internal working of the core, not for userspace.
Quoted 7 lines
What we expose to userspace must be consistent, and so if the governor is removed, we will not be able to provide '
scaling_setspeed' or 'scaling_governor' to userspace.Also, it is scaling-governor, not what the next governor is going to be. Plus, what should we print on
scaling_setspeedof a cluster not running anymore ?
The same could be said for all the files in that directory. What does it mean to say scaling_min_freq for a CPU that's hotplugged out. And the argument can be make for cases when the CPU is power collapsed too. What does it mean to ask for "current frequency" when it's powered off. The point is that these files are representing what will be done when the CPUs are clocked. Not what's happening at this absolute instance.
Quoted 2 lines
And so why keep something that we can't provide to userspace all the time. That may break userspace if it relies on it..
Because there are plenty of use cases where the module isn't going to be removed since it's not even a module. So, still letting userspace figure out what the last used governor was and what will it come up with is still useful. And this isn't going to break userspace any more than returning an error all the time.
I would strongly prefer that this is not done and just clear out the pointer if/when the governor module is removed.
Nack
Thanks,
Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 12/20] cpufreq: Mark policy->governor = NULL for inactive policies
Quoted 2 lines · Saravana Kannan
I would strongly prefer that this is not done and just clear out the pointer if/when the governor module is removed.
Okay, I give up ..
This looks fine ?
drivers/cpufreq/cpufreq.c +0 -0
@@ -1094,7 +1094,6 @@ static struct cpufreq_policy
*cpufreq_policy_restore(unsigned int cpu)
if (likely(policy)) {
/* Policy should be inactive here */
WARN_ON(!policy_is_inactive(policy));
- policy->governor = NULL;
}
return policy;
@@ -2156,8 +2155,10 @@ void cpufreq_unregister_governor(struct
cpufreq_governor *governor)
/* clear last_governor for all inactive policies */
read_lock_irqsave(&cpufreq_driver_lock, flags);
for_each_inactive_policy(policy, tpolicy) {
- if (!strcmp(policy->last_governor, governor->name))
+ if (!strcmp(policy->last_governor, governor->name)) {
+ policy->governor = NULL;
strcpy(policy->last_governor, "\0");
+ }
}
read_unlock_irqrestore(&cpufreq_driver_lock, flags);Viresh Kumar
maintainerPATCH v2 13/20 cpufreq: Don't allow updating inactive-policies from sysfs
In reply to Viresh KumarLater commits would change the way policies are managed today. Policies wouldn't be freed on cpu hotplug (currently they aren't freed only for suspend), and while the CPU is offline, the sysfs cpufreq files would still be present.
User may accidentally try to update the sysfs files in following directory:
'/sys/devices/system/cpu/cpuX/cpufreq/'. And that would result in undefined behavior as policy wouldn't be active then.
Apart from updating the store() routine, we also update __cpufreq_get() which can call cpufreq_out_of_sync(). The later routine tries to update policy->cur and starts notifying kernel about it.
--- drivers/cpufreq/cpufreq.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
drivers/cpufreq/cpufreq.c +19 -0
@@ -886,11 +886,22 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,down_write (&policy->rwsem); +/* +* Policy might not be active currently, and so we shouldn't try +* updating any values here. policy->cpus is cleared for inactive policy +* and so cpufreq_cpu_get_raw() should fail. +*/ +if (unlikely (policy_is_inactive (policy))) { + ret = -EPERM ; + goto unlock_policy_rwsem; + } +if (fattr->store) ret = fattr->store (policy, buf, count);else ret = -EIO ; +unlock_policy_rwsem:up_write (&policy->rwsem);up_read (&cpufreq_rwsem);
@@ -1620,6 +1631,14 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy) ret_freq = cpufreq_driver->get (policy->cpu); +/* +* Policy might not be active currently, and so we shouldn't try +* updating any values here. policy->cpus is cleared for inactive policy +* and so cpufreq_cpu_get_raw() should fail. +*/ +if (unlikely (policy_is_inactive (policy))) +return ret_freq; +if (ret_freq && policy->cur && !(cpufreq_driver->flags &CPUFREQ_CONST_LOOPS )) {/* verify no discrepancy between actual and
Signature · 1 line
2.3.0.rc0.44.ga94655d
Viresh Kumar
maintainerPATCH v2 14/20 cpufreq: Track cpu managing sysfs kobjects separately
In reply to Viresh KumarFrom: Saravana Kannan <[email protected]>
In order to prepare for the next few commits, that will stop migrating sysfs files on cpu hotplug, this patch starts managing sysfs-cpu separately.
The behavior is still the same as we are still migrating sysfs files on hotplug, later commits would change that.
--- drivers/cpufreq/cpufreq.c | 11 +++++++---- include/linux/cpufreq.h | 4 +++- 2 files changed, 10 insertions(+), 5 deletions(-)
drivers/cpufreq/cpufreq.c +7 -4
@@ -981,7 +981,7 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)for_each_cpu (j, policy->cpus) {struct device *cpu_dev; -if (j == policy->cpu) +if (j == policy->kobj_cpu)continue ;pr_debug ("Adding link for CPU: %u\n" , j);
@@ -1200,6 +1200,7 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,down_write (&policy->rwsem); policy->cpu = cpu; + policy->kobj_cpu = cpu;up_write (&policy->rwsem);return 0 ;
@@ -1257,10 +1258,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)* the creation of a brand new one. So we need to perform this update * by invoking update_policy_cpu(). */ -if (recover_policy && cpu != policy->cpu) +if (recover_policy && cpu != policy->cpu) {WARN_ON (update_policy_cpu (policy, cpu, dev)); -else + }else { policy->cpu = cpu; + policy->kobj_cpu = cpu; + }cpumask_copy (policy->cpus,cpumask_of (cpu));
@@ -1439,7 +1442,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, CPUFREQ_NAME_LEN);up_write (&policy->rwsem); -if (cpu != policy->cpu) { +if (cpu != policy->kobj_cpu) {sysfs_remove_link (&dev->kobj,"cpufreq" ); }else if (cpus >1 ) {/* Nominate new CPU */
include/linux/cpufreq.h +3 -1
@@ -65,7 +65,9 @@ struct cpufreq_policy {unsigned int shared_type;/* ACPI: ANY or ALL affected CPUs should set cpufreq */ -unsigned int cpu;/* cpu nr of CPU managing this policy */ +unsigned int cpu;/* cpu managing this policy, must be online */ +unsigned int kobj_cpu;/* cpu managing sysfs files, can be offline */ +struct clk *clk;struct cpufreq_cpuinfo cpuinfo;/* see above */
Signature · 1 line
2.3.0.rc0.44.ga94655d
Re: [PATCH V2 14/20] cpufreq: Track cpu managing sysfs kobjects separately
Quoted 66 lines · Viresh Kumar
From: Saravana Kannan <[email protected]>
In order to prepare for the next few commits, that will stop migrating sysfs files on cpu hotplug, this patch starts managing sysfs-cpu separately.
The behavior is still the same as we are still migrating sysfs files on hotplug, later commits would change that.
Signed-off-by Saravana Kannan <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 11 +++++++---- include/linux/cpufreq.h | 4 +++- 2 files changed, 10 insertions(+), 5 deletions(-)drivers/cpufreq/cpufreq.c +7
-4@@ -981,7 +981,7 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)for_each_cpu (j, policy->cpus) {struct device *cpu_dev; -if (j == policy->cpu) +if (j == policy->kobj_cpu)continue ;pr_debug ("Adding link for CPU: %u\n" , j);@@ -1200,6 +1200,7 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,down_write (&policy->rwsem); policy->cpu = cpu; + policy->kobj_cpu = cpu;up_write (&policy->rwsem);return 0 ;@@ -1257,10 +1258,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)* the creation of a brand new one. So we need to perform this update * by invoking update_policy_cpu(). */ -if (recover_policy && cpu != policy->cpu) +if (recover_policy && cpu != policy->cpu) {WARN_ON (update_policy_cpu (policy, cpu, dev)); -else + }else { policy->cpu = cpu; + policy->kobj_cpu = cpu; + }cpumask_copy (policy->cpus,cpumask_of (cpu));@@ -1439,7 +1442,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, CPUFREQ_NAME_LEN);up_write (&policy->rwsem); -if (cpu != policy->cpu) { +if (cpu != policy->kobj_cpu) {sysfs_remove_link (&dev->kobj,"cpufreq" ); }else if (cpus >1 ) {/* Nominate new CPU */ include/linux/cpufreq.h +0
-0
include/linux/cpufreq.h struct cpufreq_policy { · line 73
Quoted 10 lines
include/linux/cpufreq.h +3
-1@@ -65,7 +65,9 @@ struct cpufreq_policy {unsigned int shared_type;/* ACPI: ANY or ALL affected CPUs should set cpufreq */ -unsigned int cpu;/* cpu nr of CPU managing this policy */ +unsigned int cpu;/* cpu managing this policy, must be online */ +unsigned int kobj_cpu;/* cpu managing sysfs files, can be offline */ +struct clk *clk;struct cpufreq_cpuinfo cpuinfo;/* see above */
Acked?
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerRe: [PATCH V2 14/20] cpufreq: Track cpu managing sysfs kobjects separately
Quoted 1 line · Saravana Kannan
Acked?
Yeah, you can Ack the final version..
Viresh Kumar
maintainerPATCH v2 15/20 cpufreq: Stop migrating sysfs files on hotplug
In reply to Viresh KumarWhen we hot-unplug a cpu, we remove its sysfs cpufreq directory and if the outgoing cpu was the owner of policy->kobj earlier then we migrate the sysfs directory to under another online cpu.
There are few disadvantages this brings:
- Code Complexity
- Slower hotplug/suspend/resume
- sysfs file permissions are reset after all policy->cpus are offlined
- CPUFreq stats history lost after all policy->cpus are offlined
- Special management of sysfs stuff during suspend/resume
To overcome these, this patch modifies the way sysfs directories are managed:
- Select sysfs kobjects owner while initializing policy and don't change it during hotplugs. Track it with kobj_cpu created earlier.
- Create symlinks for all related CPUs (can be offline) instead of affected CPUs on policy initialization and remove them only when the policy is freed.
- Free policy structure only on the removal of cpufreq-driver and not during hotplug/suspend/resume, detected by checking 'struct subsys_interface *' (Valid only when called from subsys_interface_unregister() while unregistering driver).
[ Something similar attempted by Saravana earlier ]
--- drivers/cpufreq/cpufreq.c | 158 ++++++++++++++++++++++------------------------ 1 file changed, 74 insertions(+), 84 deletions(-)
drivers/cpufreq/cpufreq.c +74 -84
@@ -972,25 +972,36 @@ void cpufreq_sysfs_remove_file(const struct attribute *attr) }EXPORT_SYMBOL (cpufreq_sysfs_remove_file); -/* symlink affected CPUs */ -static int cpufreq_add_dev_symlink (struct cpufreq_policy *policy) +/* Add/remove symlinks for all related CPUs */ +static int cpufreq_add_remove_dev_symlink (struct cpufreq_policy *policy, +bool add) {unsigned int j;int ret =0 ; -for_each_cpu (j, policy->cpus) { +for_each_cpu (j, policy->related_cpus) {struct device *cpu_dev;if (j == policy->kobj_cpu)continue ; -pr_debug ("Adding link for CPU: %u\n" , j); +pr_debug ("%s: %s symlink for CPU: %u\n" , __func__, + add ?"Adding" :"Removing" , j); + cpu_dev =get_cpu_device (j); - ret =sysfs_create_link (&cpu_dev->kobj, &policy->kobj, -"cpufreq" ); -if (ret) -break ; +if (WARN_ON (!cpu_dev)) +continue ; + +if (add) { + ret =sysfs_create_link (&cpu_dev->kobj, &policy->kobj, +"cpufreq" ); +if (ret) +break ; + }else { +sysfs_remove_link (&cpu_dev->kobj,"cpufreq" ); + } } +return ret; }
@@ -1024,7 +1035,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,return ret; } -return cpufreq_add_dev_symlink (policy); +return cpufreq_add_remove_dev_symlink (policy, true); }static void cpufreq_init_policy (struct cpufreq_policy *policy)
@@ -1090,7 +1101,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, } } -return sysfs_create_link (&dev->kobj, &policy->kobj,"cpufreq" ); +return 0 ; }static struct cpufreq_policy *cpufreq_policy_restore (unsigned int cpu)
@@ -1110,7 +1121,7 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)return policy; } -static struct cpufreq_policy *cpufreq_policy_alloc (void ) +static struct cpufreq_policy *cpufreq_policy_alloc (int cpu) {struct cpufreq_policy *policy;
@@ -1131,6 +1142,11 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)init_completion (&policy->kobj_unregister);INIT_WORK (&policy->update, handle_update); + policy->cpu = cpu; + +/* Set this once on allocation */ + policy->kobj_cpu = cpu; +return policy; err_free_cpumask:
@@ -1149,10 +1165,11 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)blocking_notifier_call_chain (&cpufreq_policy_notifier_list,CPUFREQ_REMOVE_POLICY , policy); -down_read (&policy->rwsem); +down_write (&policy->rwsem); +cpufreq_add_remove_dev_symlink (policy, false); kobj = &policy->kobj; cmp = &policy->kobj_unregister; -up_read (&policy->rwsem); +up_write (&policy->rwsem);kobject_put (kobj);/*
@@ -1183,27 +1200,14 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)kfree (policy); } -static int update_policy_cpu (struct cpufreq_policy *policy,unsigned int cpu, -struct device *cpu_dev) +static void update_policy_cpu (struct cpufreq_policy *policy,unsigned int cpu) { -int ret; -if (WARN_ON (cpu == policy->cpu)) -return 0 ; - -/* Move kobject to the new policy->cpu */ - ret =kobject_move (&policy->kobj, &cpu_dev->kobj); -if (ret) { -pr_err ("%s: Failed to move kobj: %d\n" , __func__, ret); -return ret; - } +return ;down_write (&policy->rwsem); policy->cpu = cpu; - policy->kobj_cpu = cpu;up_write (&policy->rwsem); - -return 0 ; }/**
@@ -1221,7 +1225,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)int ret = -ENOMEM ;struct cpufreq_policy *policy;unsigned long flags; -bool recover_policy = cpufreq_suspended; +bool recover_policy = !sif;if (cpu_is_offline (cpu))return 0 ;
@@ -1247,7 +1251,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) policy = recover_policy ?cpufreq_policy_restore (cpu) :NULL ;if (!policy) { recover_policy = false; - policy =cpufreq_policy_alloc (); + policy =cpufreq_policy_alloc (cpu);if (!policy) goto nomem_out; }
@@ -1258,12 +1262,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)* the creation of a brand new one. So we need to perform this update * by invoking update_policy_cpu(). */ -if (recover_policy && cpu != policy->cpu) { -WARN_ON (update_policy_cpu (policy, cpu, dev)); - }else { - policy->cpu = cpu; - policy->kobj_cpu = cpu; - } +if (recover_policy && cpu != policy->cpu) +update_policy_cpu (policy, cpu);cpumask_copy (policy->cpus,cpumask_of (cpu));
@@ -1442,29 +1442,14 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, CPUFREQ_NAME_LEN);up_write (&policy->rwsem); -if (cpu != policy->kobj_cpu) { -sysfs_remove_link (&dev->kobj,"cpufreq" ); - }else if (cpus >1 ) { -/* Nominate new CPU */ -int new_cpu =cpumask_any_but (policy->cpus, cpu); -struct device *cpu_dev =get_cpu_device (new_cpu); - -sysfs_remove_link (&cpu_dev->kobj,"cpufreq" ); - ret =update_policy_cpu (policy, new_cpu, cpu_dev); -if (ret) { -if (sysfs_create_link (&cpu_dev->kobj, &policy->kobj, -"cpufreq" )) -pr_err ("%s: Failed to restore kobj link to cpu:%d\n" , - __func__, cpu_dev->id); -return ret; - } +if (cpu != policy->cpu) +return 0 ; -if (!cpufreq_suspended) -pr_debug ("%s: policy Kobject moved to cpu: %d from: %d\n" , - __func__, new_cpu, cpu); - }else if (cpufreq_driver->stop_cpu) { +if (cpus >1 ) +/* Nominate new CPU */ +update_policy_cpu (policy,cpumask_any_but (policy->cpus, cpu)); +else if (cpufreq_driver->stop_cpu) cpufreq_driver->stop_cpu (policy); - }return 0 ; }
@@ -1485,34 +1470,11 @@ static int __cpufreq_remove_dev_finish(struct device *dev,cpumask_clear_cpu (cpu, policy->cpus);up_write (&policy->rwsem); -/* If cpu is last user of policy, free policy */ -if (policy_is_inactive (policy)) { -if (has_target ()) { - ret =__cpufreq_governor (policy, -CPUFREQ_GOV_POLICY_EXIT ); -if (ret) { -pr_err ("%s: Failed to exit governor\n" , - __func__); -return ret; - } - } +/* Not the last cpu of policy, start governor again ? */ +if (!policy_is_inactive (policy)) { +if (!has_target ()) +return 0 ; -if (!cpufreq_suspended) -cpufreq_policy_put_kobj (policy); - -/* -* Perform the ->exit() even during light-weight tear-down, -* since this is a core component, and is essential for the -* subsequent light-weight ->init() to succeed. -*/ -if (cpufreq_driver->exit) - cpufreq_driver->exit (policy); - -if (!cpufreq_suspended) -cpufreq_policy_free (policy); -else - policy->governor =NULL ; - }else if (has_target ()) { ret =__cpufreq_governor (policy,CPUFREQ_GOV_START );if (!ret) ret =__cpufreq_governor (policy,CPUFREQ_GOV_LIMITS );
@@ -1521,8 +1483,36 @@ static int __cpufreq_remove_dev_finish(struct device *dev,pr_err ("%s: Failed to start governor\n" , __func__);return ret; } + +return 0 ; + } + +/* If cpu is last user of policy, free policy */ +if (has_target ()) { + ret =__cpufreq_governor (policy,CPUFREQ_GOV_POLICY_EXIT ); +if (ret) { +pr_err ("%s: Failed to exit governor\n" , __func__); +return ret; + } } +/* Free the policy kobjects only if the driver is getting removed. */ +if (sif) +cpufreq_policy_put_kobj (policy); + +/* +* Perform the ->exit() even during light-weight tear-down, +* since this is a core component, and is essential for the +* subsequent light-weight ->init() to succeed. +*/ +if (cpufreq_driver->exit) + cpufreq_driver->exit (policy); + +if (sif) +cpufreq_policy_free (policy); +else + policy->governor =NULL ; +return 0 ; }
Signature · 1 line
2.3.0.rc0.44.ga94655d
Viresh Kumar
maintainerPATCH v2 16/20 cpufreq: Remove cpufreq_update_policy()
In reply to Viresh Kumarcpufreq_update_policy() was kept as a separate routine earlier as it was handling migration of sysfs directories, which isn't done anymore. It is only updating policy->cpu now and can be removed.
--- drivers/cpufreq/cpufreq.c | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-)
drivers/cpufreq/cpufreq.c +10 -22
@@ -1116,6 +1116,10 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)if (likely (policy)) {/* Policy should be inactive here */ WARN_ON (!policy_is_inactive (policy)); + +down_write (&policy->rwsem); + policy->cpu = cpu; +up_write (&policy->rwsem); }return policy;
@@ -1200,16 +1204,6 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)kfree (policy); } -static void update_policy_cpu (struct cpufreq_policy *policy,unsigned int cpu) -{ -if (WARN_ON (cpu == policy->cpu)) -return ; - -down_write (&policy->rwsem); - policy->cpu = cpu; -up_write (&policy->rwsem); -} -/** * cpufreq_add_dev - add a CPU device *
@@ -1256,15 +1250,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) goto nomem_out; } -/* -* In the resume path, since we restore a saved policy, the assignment -* to policy->cpu is like an update of the existing policy, rather than -* the creation of a brand new one. So we need to perform this update -* by invoking update_policy_cpu(). -*/ -if (recover_policy && cpu != policy->cpu) -update_policy_cpu (policy, cpu); -cpumask_copy (policy->cpus,cpumask_of (cpu));/* call driver. From then on the cpufreq must be able
@@ -1445,11 +1430,14 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,if (cpu != policy->cpu)return 0 ; -if (cpus >1 ) +if (cpus >1 ) {/* Nominate new CPU */ -update_policy_cpu (policy,cpumask_any_but (policy->cpus, cpu)); -else if (cpufreq_driver->stop_cpu) +down_write (&policy->rwsem); + policy->cpu =cpumask_any_but (policy->cpus, cpu); +up_write (&policy->rwsem); + }else if (cpufreq_driver->stop_cpu) { cpufreq_driver->stop_cpu (policy); + }return 0 ; }
Signature · 1 line
2.3.0.rc0.44.ga94655d
Viresh Kumar
maintainerPATCH v2 17/20 cpufreq: Initialize policy->kobj while allocating policy
In reply to Viresh Kumarpolicy->kobj is required to be initialized once in the lifetime of a policy. Currently we are initializing it from __cpufreq_add_dev() and that doesn't look to be the best place for doing so as we have to do this on special cases (like:
!recover_policy).
We can initialize it from a more obvious place cpufreq_policy_alloc() and that will make code look cleaner, specially the error handling part.
The error handling part of __cpufreq_add_dev() was doing almost the same thing while recover_policy is true or false. Fix that as well by always calling cpufreq_policy_put_kobj() with an additional parameter to skip notification part of it.
--- drivers/cpufreq/cpufreq.c | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-)
drivers/cpufreq/cpufreq.c +20 -24
@@ -1125,9 +1125,10 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)return policy; } -static struct cpufreq_policy *cpufreq_policy_alloc (int cpu) +static struct cpufreq_policy *cpufreq_policy_alloc (struct device *dev) {struct cpufreq_policy *policy; +int ret; policy =kzalloc (sizeof (*policy),GFP_KERNEL );if (!policy)
@@ -1139,6 +1140,13 @@ static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)if (!zalloc_cpumask_var (&policy->related_cpus,GFP_KERNEL )) goto err_free_cpumask; + ret =kobject_init_and_add (&policy->kobj, &ktype_cpufreq, &dev->kobj, +"cpufreq" ); +if (ret) { +pr_err ("%s: failed to init policy->kobj: %d\n" , __func__, ret); + goto err_free_rcpumask; + } +INIT_LIST_HEAD (&policy->policy_list);init_rwsem (&policy->rwsem);spin_lock_init (&policy->transition_lock);
@@ -1146,13 +1154,15 @@ static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)init_completion (&policy->kobj_unregister);INIT_WORK (&policy->update, handle_update); - policy->cpu = cpu; + policy->cpu = dev->id;/* Set this once on allocation */ - policy->kobj_cpu = cpu; + policy->kobj_cpu = dev->id;return policy; +err_free_rcpumask: +free_cpumask_var (policy->related_cpus); err_free_cpumask:free_cpumask_var (policy->cpus); err_free_policy:
@@ -1161,13 +1171,14 @@ static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)return NULL ; } -static void cpufreq_policy_put_kobj (struct cpufreq_policy *policy) +static void cpufreq_policy_put_kobj (struct cpufreq_policy *policy,bool notify) {struct kobject *kobj;struct completion *cmp; -blocking_notifier_call_chain (&cpufreq_policy_notifier_list, -CPUFREQ_REMOVE_POLICY , policy); +if (notify) +blocking_notifier_call_chain (&cpufreq_policy_notifier_list, +CPUFREQ_REMOVE_POLICY , policy);down_write (&policy->rwsem);cpufreq_add_remove_dev_symlink (policy, false);
@@ -1245,7 +1256,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) policy = recover_policy ?cpufreq_policy_restore (cpu) :NULL ;if (!policy) { recover_policy = false; - policy =cpufreq_policy_alloc (cpu); + policy =cpufreq_policy_alloc (dev);if (!policy) goto nomem_out; }
@@ -1276,15 +1287,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) policy->user_policy.min = policy->min; policy->user_policy.max = policy->max; -/* prepare interface data */ - ret =kobject_init_and_add (&policy->kobj, &ktype_cpufreq, - &dev->kobj,"cpufreq" ); -if (ret) { -pr_err ("%s: failed to init policy->kobj: %d\n" , - __func__, ret); - goto err_init_policy_kobj; - } -write_lock_irqsave (&cpufreq_driver_lock, flags);for_each_cpu (j, policy->related_cpus)per_cpu (cpufreq_cpu_data, j) = policy;
@@ -1376,18 +1378,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) err_out_unregister: err_get_freq: -if (!recover_policy) { -kobject_put (&policy->kobj); -wait_for_completion (&policy->kobj_unregister); - } -err_init_policy_kobj:up_write (&policy->rwsem);if (cpufreq_driver->exit) cpufreq_driver->exit (policy); err_set_policy_cpu: -if (recover_policy) -cpufreq_policy_put_kobj (policy); +cpufreq_policy_put_kobj (policy, recover_policy);cpufreq_policy_free (policy); nomem_out:
@@ -1486,7 +1482,7 @@ static int __cpufreq_remove_dev_finish(struct device *dev,/* Free the policy kobjects only if the driver is getting removed. */ if (sif) -cpufreq_policy_put_kobj (policy); +cpufreq_policy_put_kobj (policy, true);/* * Perform the ->exit() even during light-weight tear-down,
Signature · 1 line
2.3.0.rc0.44.ga94655d
Viresh Kumar
maintainerPATCH v2 18/20 cpufreq: Call cpufreq_policy_put_kobj() from cpufreq_policy_free()
In reply to Viresh Kumarcpufreq_policy_put_kobj() is actually part of freeing the policy and can be called from cpufreq_policy_free() directly instead of a separate call.
--- drivers/cpufreq/cpufreq.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq.c +5 -9
@@ -1197,7 +1197,7 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify)pr_debug ("wait complete\n" ); } -static void cpufreq_policy_free (struct cpufreq_policy *policy) +static void cpufreq_policy_free (struct cpufreq_policy *policy,bool notify) {unsigned long flags;int cpu;
@@ -1210,6 +1210,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)per_cpu (cpufreq_cpu_data, cpu) =NULL ;write_unlock_irqrestore (&cpufreq_driver_lock, flags); +cpufreq_policy_put_kobj (policy, notify);free_cpumask_var (policy->related_cpus);free_cpumask_var (policy->cpus);kfree (policy);
@@ -1383,9 +1384,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)if (cpufreq_driver->exit) cpufreq_driver->exit (policy); err_set_policy_cpu: -cpufreq_policy_put_kobj (policy, recover_policy); -cpufreq_policy_free (policy); - +cpufreq_policy_free (policy, recover_policy); nomem_out:up_read (&cpufreq_rwsem);
@@ -1480,10 +1479,6 @@ static int __cpufreq_remove_dev_finish(struct device *dev, } } -/* Free the policy kobjects only if the driver is getting removed. */ -if (sif) -cpufreq_policy_put_kobj (policy, true); -/* * Perform the ->exit() even during light-weight tear-down, * since this is a core component, and is essential for the
@@ -1492,8 +1487,9 @@ static int __cpufreq_remove_dev_finish(struct device *dev,if (cpufreq_driver->exit) cpufreq_driver->exit (policy); +/* Free the policy only if the driver is getting removed. */ if (sif) -cpufreq_policy_free (policy); +cpufreq_policy_free (policy, true);else policy->governor =NULL ;
Signature · 1 line
2.3.0.rc0.44.ga94655d
Viresh Kumar
maintainerPATCH v2 19/20 cpufreq: Restart governor as soon as possible
In reply to Viresh KumarOn cpu hot-unplug, we don't need to wait for POST_DEAD notification to restart the governor if the policy has atleast one online cpu left. We can restart the governor right from the DOWN_PREPARE notification instead.
[ Something similar attempted by Saravana earlier ]
--- drivers/cpufreq/cpufreq.c | 58 ++++++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 34 deletions(-)
drivers/cpufreq/cpufreq.c +24 -34
@@ -1394,8 +1394,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)static int __cpufreq_remove_dev_prepare (struct device *dev,struct subsys_interface *sif) { -unsigned int cpu = dev->id, cpus; -int ret; +unsigned int cpu = dev->id; +int ret =0 ;struct cpufreq_policy *policy;pr_debug ("%s: unregistering CPU %u\n" , __func__, cpu);
@@ -1415,26 +1415,33 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, }down_write (&policy->rwsem); - cpus =cpumask_weight (policy->cpus); +cpumask_clear_cpu (cpu, policy->cpus); -if (has_target () && cpus ==1 ) -strncpy (policy->last_governor, policy->governor->name, -CPUFREQ_NAME_LEN ); +if (policy_is_inactive (policy)) { +if (has_target ()) +strncpy (policy->last_governor, policy->governor->name, +CPUFREQ_NAME_LEN ); + }else if (cpu == policy->cpu) { +/* Nominate new CPU */ + policy->cpu =cpumask_any (policy->cpus); + }up_write (&policy->rwsem); -if (cpu != policy->cpu) -return 0 ; +/* Start governor again for active policy */ +if (!policy_is_inactive (policy)) { +if (has_target ()) { + ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ); +if (!ret) + ret =__cpufreq_governor (policy,CPUFREQ_GOV_LIMITS ); -if (cpus >1 ) { -/* Nominate new CPU */ -down_write (&policy->rwsem); - policy->cpu =cpumask_any_but (policy->cpus, cpu); -up_write (&policy->rwsem); +if (ret) +pr_err ("%s: Failed to start governor\n" , __func__); + } }else if (cpufreq_driver->stop_cpu) { cpufreq_driver->stop_cpu (policy); } -return 0 ; +return ret; } static int __cpufreq_remove_dev_finish(struct device *dev,
@@ -1442,33 +1449,16 @@ static int __cpufreq_remove_dev_finish(struct device *dev, {unsigned int cpu = dev->id;int ret; -struct cpufreq_policy *policy =cpufreq_cpu_get_raw (cpu); +struct cpufreq_policy *policy =per_cpu (cpufreq_cpu_data, cpu);if (!policy) {pr_debug ("%s: No cpu_data found\n" , __func__);return -EINVAL ; } -down_write (&policy->rwsem); -cpumask_clear_cpu (cpu, policy->cpus); -up_write (&policy->rwsem); - -/* Not the last cpu of policy, start governor again ? */ -if (!policy_is_inactive (policy)) { -if (!has_target ()) -return 0 ; - - ret =__cpufreq_governor (policy,CPUFREQ_GOV_START ); -if (!ret) - ret =__cpufreq_governor (policy,CPUFREQ_GOV_LIMITS ); - -if (ret) { -pr_err ("%s: Failed to start governor\n" , __func__); -return ret; - } - +/* Only proceed for inactive policies */ +if (!policy_is_inactive (policy))return 0 ; - }/* If cpu is last user of policy, free policy */ if (has_target ()) {
Signature · 1 line
2.3.0.rc0.44.ga94655d
Viresh Kumar
maintainerPATCH v2 20/20 cpufreq: Add support for physical hoplug of CPUs
In reply to Viresh KumarIt is possible to physically hotplug the CPUs and it happens in this sequence.
Hot removal:
- CPU is offlined first, ~ 'echo 0 > /sys/devices/system/cpu/cpuX/online'
- Then its device is removed along with all sysfs files, cpufreq core notified with cpufreq_remove_dev() callback from subsys-interface..
Hot addition:
- First the device along with its sysfs files is added, cpufreq core notified with cpufreq_add_dev() callback from subsys-interface..
- CPU is onlined, ~ 'echo 1 > /sys/devices/system/cpu/cpuX/online'
This needs to be handled specially as current code isn't taking care of this case. Because we will hit the same routines with both hotplug and subsys callbacks, we can handle most of the stuff with regular hotplug callback paths. We only need to take care of adding/removing cpufreq sysfs links or freeing policy from subsys callbacks.
And that can be sensed easily as cpu would be offline in those cases. This patch adds special code in those paths to take care of policy and its links.
cpufreq_add_remove_dev_symlink() is also broken into another routine add_remove_cpu_dev_symlink() to reuse code at several places.
--- drivers/cpufreq/cpufreq.c | 87 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 65 insertions(+), 22 deletions(-)
drivers/cpufreq/cpufreq.c +65 -22
@@ -972,6 +972,26 @@ void cpufreq_sysfs_remove_file(const struct attribute *attr) }EXPORT_SYMBOL (cpufreq_sysfs_remove_file); +static inline int add_remove_cpu_dev_symlink (struct cpufreq_policy *policy, +int cpu,bool add) +{ +struct device *cpu_dev; + +pr_debug ("%s: %s symlink for CPU: %u\n" , __func__, + add ?"Adding" :"Removing" , cpu); + + cpu_dev =get_cpu_device (cpu); +if (WARN_ON (!cpu_dev)) +return 0 ; + +if (add) +return sysfs_create_link (&cpu_dev->kobj, &policy->kobj, +"cpufreq" ); + +sysfs_remove_link (&cpu_dev->kobj,"cpufreq" ); +return 0 ; +} +/* Add/remove symlinks for all related CPUs */ static int cpufreq_add_remove_dev_symlink (struct cpufreq_policy *policy,bool add)
@@ -979,27 +999,14 @@ static int cpufreq_add_remove_dev_symlink(struct cpufreq_policy *policy,unsigned int j;int ret =0 ; -for_each_cpu (j, policy->related_cpus) { -struct device *cpu_dev; - +/* Some related CPUs might not be present (physically hotplugged) */ +for_each_cpu_and (j, policy->related_cpus, cpu_present_mask) {if (j == policy->kobj_cpu)continue ; -pr_debug ("%s: %s symlink for CPU: %u\n" , __func__, - add ?"Adding" :"Removing" , j); - - cpu_dev =get_cpu_device (j); -if (WARN_ON (!cpu_dev)) -continue ; - -if (add) { - ret =sysfs_create_link (&cpu_dev->kobj, &policy->kobj, -"cpufreq" ); -if (ret) -break ; - }else { -sysfs_remove_link (&cpu_dev->kobj,"cpufreq" ); - } + ret =add_remove_cpu_dev_symlink (policy, j, add); +if (ret) +break ; }return ret;
@@ -1233,11 +1240,23 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)unsigned long flags;bool recover_policy = !sif; -if (cpu_is_offline (cpu)) -return 0 ; -pr_debug ("adding CPU %u\n" , cpu); +/* +* Only possible if 'cpu' wasn't physically present earlier and we are +* here from subsys_interface add callback. A hotplug notifier will +* follow and we will handle it like logical CPU hotplug then. For now, +* just create the sysfs link. +*/ +if (cpu_is_offline (cpu)) { + policy =per_cpu (cpufreq_cpu_data, cpu); +/* No need to create link of the first cpu of a policy */ +if (!policy) +return 0 ; + +return add_remove_cpu_dev_symlink (policy, cpu, true); + } +if (!down_read_trylock (&cpufreq_rwsem))return 0 ;
@@ -1496,8 +1515,32 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)unsigned int cpu = dev->id;int ret; -if (cpu_is_offline (cpu)) +/* +* Only possible if 'cpu' is getting physically removed now. A hotplug +* notifier should have already been called and we just need to remove +* link or free policy here. +*/ +if (cpu_is_offline (cpu)) { +struct cpufreq_policy *policy =per_cpu (cpufreq_cpu_data, cpu); +struct cpumask mask; + +if (!policy) +return 0 ; + +/* Prepare mask similar to related-cpus without this 'cpu' */ +cpumask_copy (&mask, policy->related_cpus); +cpumask_clear_cpu (cpu, &mask); + +/* +* Remove link if few CPUs are still present physically, else +* free policy when all are gone. +*/ +if (cpumask_intersects (&mask, cpu_present_mask)) +return add_remove_cpu_dev_symlink (policy, cpu, false); + +cpufreq_policy_free (policy, true);return 0 ; + } ret =__cpufreq_remove_dev_prepare (dev, sif);
Signature · 1 line
2.3.0.rc0.44.ga94655d
Viresh Kumar
maintainerQuoted 28 lines · Viresh Kumar
Hi Rafael,
The aim of this series is to stop managing cpufreq sysfs directories on CPU hotplugs.
Currently on removal of a 'cpu !=
policy->cpu', we remove its sysfs directories by removing the soft-link. And on removal ofpolicy->cpu, we migrate the sysfs directories to the next cpu. But ifpolicy->cpuwas the last CPU, we remove the policy completely and allocate it again as soon as the CPUs come back. This has shortcomings:- Code Complexity
- Slower hotplug
- sysfs file permissions are reset after allpolicy->cpusare offlined
- CPUFreq stats history lost after allpolicy->cpusare offlined
- Special management of sysfs stuff during suspend/resumeTo make things simple we stop playing with sysfs files unless the driver is getting removed. Also the policy is kept intact to be used later.
First few patches provide a clean base for others *more important* patches.
Rebased-over: your bleeding edge branch as there were dependencies on my earlier patches.
Pushed here:
Quoted 8 lines
git://git.linaro.org/people/viresh.kumar/linux.gitcpufreq/core/sysfs
v1->V2:
- Dropped the idea of using policy-lists for getting policy for any cpu
- Also dropped fallback list and its per-cpu variable
- Stopped cleaningcpufreq_cpu_dataand doinglist_del(policy)on logical hotplug.
- Added support for physical hotplug of CPUs (Untested).
Gentle reminder for reviews !! :)
Quoted 31 lines · Viresh Kumar
Quoted 30 lines · Viresh Kumar
Hi Rafael,
The aim of this series is to stop managing cpufreq sysfs directories on CPU hotplugs.
Currently on removal of a 'cpu !=
policy->cpu', we remove its sysfs directories by removing the soft-link. And on removal ofpolicy->cpu, we migrate the sysfs directories to the next cpu. But ifpolicy->cpuwas the last CPU, we remove the policy completely and allocate it again as soon as the CPUs come back. This has shortcomings:- Code Complexity
- Slower hotplug
- sysfs file permissions are reset after allpolicy->cpusare offlined
- CPUFreq stats history lost after allpolicy->cpusare offlined
- Special management of sysfs stuff during suspend/resumeTo make things simple we stop playing with sysfs files unless the driver is getting removed. Also the policy is kept intact to be used later.
First few patches provide a clean base for others *more important* patches.
Rebased-over: your bleeding edge branch as there were dependencies on my earlier patches.
Pushed here:
git://git.linaro.org/people/viresh.kumar/linux.gitcpufreq/core/sysfs
Quoted 9 lines
Quoted 6 lines
v1->V2:
- Dropped the idea of using policy-lists for getting policy for any cpu
- Also dropped fallback list and its per-cpu variable
- Stopped cleaningcpufreq_cpu_dataand doinglist_del(policy)on logical hotplug.
- Added support for physical hotplug of CPUs (Untested).Gentle reminder for reviews !! :)
Sorry, caught up on some internal work. Should be able to get to this by Wednesday.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerQuoted 6 lines · Saravana Kannan
Quoted 2 lines · Viresh Kumar
Gentle reminder for reviews !! :)
Sorry, caught up on some internal work. Should be able to get to this by Wednesday.
Will it be possible to get some reviews on this?
@Rafael: Can you do them if Saravana isn't available? I don't want to miss another kernel release for this stuff.
Quoted 12 lines · Viresh Kumar
Quoted 6 lines · Saravana Kannan
Quoted 2 lines · Viresh Kumar
Gentle reminder for reviews !! :)
Sorry, caught up on some internal work. Should be able to get to this by Wednesday.
Will it be possible to get some reviews on this?
@Rafael: Can you do them if Saravana isn't available? I don't want to miss another kernel release for this stuff.
Sorry for the delay. I'm a little bit more available. I'm trying to look at this right now. But obviously, more eyes the better. So, others should feel free to review/chime in.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerQuoted 3 lines · Saravana Kannan
Sorry for the delay. I'm a little bit more available. I'm trying to look at this right now. But obviously, more eyes the better. So, others should feel free to review/chime in.
Hi Saravana/Rafael,
Can we please finish the reviews here ?
Quoted 9 lines · Viresh Kumar
Quoted 3 lines · Saravana Kannan
Sorry for the delay. I'm a little bit more available. I'm trying to look at this right now. But obviously, more eyes the better. So, others should feel free to review/chime in.
Hi Saravana/Rafael,
Can we please finish the reviews here ?
My impression was that there had been some comments to address already. Isn't that the case?
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Quoted 7 lines · Rafael J. Wysocki
Quoted 6 lines · Viresh Kumar
Quoted 3 lines · Saravana Kannan
Sorry for the delay. I'm a little bit more available. I'm trying to look at this right now. But obviously, more eyes the better. So, others should feel free to review/chime in.
Quoted 8 lines
Quoted 3 lines
Hi Saravana/Rafael,
Can we please finish the reviews here ?
My impression was that there had been some comments to address already. Isn't that the case?
I'm not waiting on those. Just some internal fires to put out. Will review more today/tomorrow.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Viresh Kumar
maintainerQuoted 2 lines · Rafael J. Wysocki
My impression was that there had been some comments to address already. Isn't that the case?
It was just about coding style, nothing more. Use do-while instead of while(1) :)
Quoted 68 lines · Viresh Kumar
Hi Rafael,
The aim of this series is to stop managing cpufreq sysfs directories on CPU hotplugs.
Currently on removal of a 'cpu !=
policy->cpu', we remove its sysfs directories by removing the soft-link. And on removal ofpolicy->cpu, we migrate the sysfs directories to the next cpu. But ifpolicy->cpuwas the last CPU, we remove the policy completely and allocate it again as soon as the CPUs come back. This has shortcomings:- Code Complexity
- Slower hotplug
- sysfs file permissions are reset after allpolicy->cpusare offlined
- CPUFreq stats history lost after allpolicy->cpusare offlined
- Special management of sysfs stuff during suspend/resumeTo make things simple we stop playing with sysfs files unless the driver is getting removed. Also the policy is kept intact to be used later.
First few patches provide a clean base for others *more important* patches.
Rebased-over: your bleeding edge branch as there were dependencies on my earlier patches.
Pushed here:
git://git.linaro.org/people/viresh.kumar/linux.gitcpufreq/core/sysfs
v1->V2:
- Dropped the idea of using policy-lists for getting policy for any cpu
- Also dropped fallback list and its per-cpu variable
- Stopped cleaningcpufreq_cpu_dataand doinglist_del(policy)on logical hotplug.
- Added support for physical hotplug of CPUs (Untested).@Srivatsa: Can you please have a look at the above change? I have cc'd you only on this one.
Saravana Kannan (1):
cpufreq: Track cpu managing sysfs kobjects separatelyCc Srivatsa Bhat <[email protected]>Viresh Kumar (19):
cpufreq: Add doc style comment about cpufreq_cpu_{get|put}()
cpufreq: Merge__cpufreq_add_dev()andcpufreq_add_dev()
cpufreq: Throw warning when we try to get policy for an invalid CPU
cpufreq: Keep a single path for adding managed CPUs
cpufreq: Clearpolicy->cpuseven for the last CPU
cpufreq: Create for_each_{in}active_policy()
cpufreq: Callschedule_work()for the last active policy
cpufreq: Don't clearcpufreq_cpu_dataand policy list for inactive
policies
cpufreq: Get rid ofcpufreq_cpu_data_fallback
cpufreq: Don't traverse list of all policies for adding policy for a
cpu
cpufreq: Manage governor usage history with 'policy->last_governor'
cpufreq: Markpolicy->governor= NULL for inactive policies
cpufreq: Don't allow updating inactive-policies from sysfs
cpufreq: Stop migrating sysfs files on hotplug
cpufreq: Removecpufreq_update_policy()
cpufreq: Initializepolicy->kobjwhile allocating policy
cpufreq: Callcpufreq_policy_put_kobj()fromcpufreq_policy_free()
cpufreq: Restart governor as soon as possible
cpufreq: Add support for physical hoplug of CPUs
Quoted 4 lines
drivers/cpufreq/cpufreq.c | 593 ++++++++++++++++++++++++++-------------------- include/linux/cpufreq.h | 5 +- 2 files changed, 340 insertions(+), 258 deletions(-)
Just so it's clear I'm looking at these patches, I'm acking each patch as and when I look at them.
I might come back and nack some of them if I find an issue with patch N while looking at patch N + M. When I ack the last patch in the series, all my previous acks can be considered as final as they can be.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 68 lines · Viresh Kumar
Hi Rafael,
The aim of this series is to stop managing cpufreq sysfs directories on CPU hotplugs.
Currently on removal of a 'cpu !=
policy->cpu', we remove its sysfs directories by removing the soft-link. And on removal ofpolicy->cpu, we migrate the sysfs directories to the next cpu. But ifpolicy->cpuwas the last CPU, we remove the policy completely and allocate it again as soon as the CPUs come back. This has shortcomings:- Code Complexity
- Slower hotplug
- sysfs file permissions are reset after allpolicy->cpusare offlined
- CPUFreq stats history lost after allpolicy->cpusare offlined
- Special management of sysfs stuff during suspend/resumeTo make things simple we stop playing with sysfs files unless the driver is getting removed. Also the policy is kept intact to be used later.
First few patches provide a clean base for others *more important* patches.
Rebased-over: your bleeding edge branch as there were dependencies on my earlier patches.
Pushed here:
git://git.linaro.org/people/viresh.kumar/linux.gitcpufreq/core/sysfs
v1->V2:
- Dropped the idea of using policy-lists for getting policy for any cpu
- Also dropped fallback list and its per-cpu variable
- Stopped cleaningcpufreq_cpu_dataand doinglist_del(policy)on logical hotplug.
- Added support for physical hotplug of CPUs (Untested).@Srivatsa: Can you please have a look at the above change? I have cc'd you only on this one.
Saravana Kannan (1):
cpufreq: Track cpu managing sysfs kobjects separatelyCc Srivatsa Bhat <[email protected]>Viresh Kumar (19):
cpufreq: Add doc style comment about cpufreq_cpu_{get|put}()
cpufreq: Merge__cpufreq_add_dev()andcpufreq_add_dev()
cpufreq: Throw warning when we try to get policy for an invalid CPU
cpufreq: Keep a single path for adding managed CPUs
cpufreq: Clearpolicy->cpuseven for the last CPU
cpufreq: Create for_each_{in}active_policy()
cpufreq: Callschedule_work()for the last active policy
cpufreq: Don't clearcpufreq_cpu_dataand policy list for inactive
policies
cpufreq: Get rid ofcpufreq_cpu_data_fallback
cpufreq: Don't traverse list of all policies for adding policy for a
cpu
cpufreq: Manage governor usage history with 'policy->last_governor'
cpufreq: Markpolicy->governor= NULL for inactive policies
cpufreq: Don't allow updating inactive-policies from sysfs
cpufreq: Stop migrating sysfs files on hotplug
cpufreq: Removecpufreq_update_policy()
cpufreq: Initializepolicy->kobjwhile allocating policy
cpufreq: Callcpufreq_policy_put_kobj()fromcpufreq_policy_free()
cpufreq: Restart governor as soon as possible
cpufreq: Add support for physical hoplug of CPUs
Quoted 3 lines
drivers/cpufreq/cpufreq.c | 593 ++++++++++++++++++++++++++-------------------- include/linux/cpufreq.h | 5 +- 2 files changed, 340 insertions(+), 258 deletions(-)
I've queued up the first 5 for 4.2, but the rest needs refreshing at least.
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.