[4/5] cpufreq: create cpu/cpufreq/policyX directories

Message ID dd7e6cab3e879e94305c0fca1ea6033e79d166ca.1444583718.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar Oct. 11, 2015, 5:21 p.m.
The cpufreq sysfs interface had been a bit inconsistent as one of the
CPUs for a policy had a real directory within its sysfs 'cpuX' directory
and all other CPUs had links to it. That also made the code a bit
complex as we need to take care of moving the sysfs directory if the CPU
containing the real directory is getting physically hot-unplugged.

Solve this by creating 'policyX' directories (per-policy) in
/sys/devices/system/cpu/cpufreq/ directory, where X is the CPU for which
the policy was first created.

This also removes the need of keeping kobj_cpu and we can remove it now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 34 ++++------------------------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 4 insertions(+), 31 deletions(-)

Comments

Saravana Kannan Oct. 12, 2015, 7:31 p.m. | #1
On 10/11/2015 10:21 AM, Viresh Kumar wrote:
> The cpufreq sysfs interface had been a bit inconsistent as one of the
> CPUs for a policy had a real directory within its sysfs 'cpuX' directory
> and all other CPUs had links to it. That also made the code a bit
> complex as we need to take care of moving the sysfs directory if the CPU
> containing the real directory is getting physically hot-unplugged.

This should actually make hotplug a bit faster too.

> Solve this by creating 'policyX' directories (per-policy) in
> /sys/devices/system/cpu/cpufreq/ directory, where X is the CPU for which
> the policy was first created.
Can we use the first CPU in the related CPUs mask? Instead of the first 
CPU that the policy got created on? The policyX numbering would be a bit 
more consistent that way.

>
> This also removes the need of keeping kobj_cpu and we can remove it now.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Suggested-by: ?

> ---
>   drivers/cpufreq/cpufreq.c | 34 ++++------------------------------
>   include/linux/cpufreq.h   |  1 -
>   2 files changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2cb0e3b8170e..58aabe0f2d2c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -917,9 +917,6 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
>
>   	/* Some related CPUs might not be present (physically hotplugged) */
>   	for_each_cpu(j, policy->real_cpus) {
Didn't notice when this got added. Do we really need this anymore if we 
don't care about moving the directory and creating symlinks? I don't 
think we need it anymore. And if we really need to know related - 
offline, we can use for_each_cpu_and(related, online/present mask)

> -		if (j == policy->kobj_cpu)
> -			continue;
> -
>   		ret = add_cpu_dev_symlink(policy, j);
>   		if (ret)
>   			break;
> @@ -933,12 +930,8 @@ static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
>   	unsigned int j;
>
>   	/* Some related CPUs might not be present (physically hotplugged) */
> -	for_each_cpu(j, policy->real_cpus) {
> -		if (j == policy->kobj_cpu)
> -			continue;
> -
> +	for_each_cpu(j, policy->real_cpus)
>   		remove_cpu_dev_symlink(policy, j);
> -	}
>   }
>
>   static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
> @@ -1054,8 +1047,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
>   		goto err_free_rcpumask;
>
> -	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq, &dev->kobj,
> -				   "cpufreq");
> +	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
> +				   cpufreq_global_kobject, "policy%u", cpu);
>   	if (ret) {
>   		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
>   		goto err_free_real_cpus;
> @@ -1069,10 +1062,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   	INIT_WORK(&policy->update, handle_update);
>
>   	policy->cpu = cpu;
> -
> -	/* Set this once on allocation */
> -	policy->kobj_cpu = cpu;
> -
>   	return policy;
>
>   err_free_real_cpus:
> @@ -1424,22 +1413,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>   		return;
>   	}
>
> -	if (cpu != policy->kobj_cpu) {
> -		remove_cpu_dev_symlink(policy, cpu);
> -	} else {
> -		/*
> -		 * The CPU owning the policy object is going away.  Move it to
> -		 * another suitable CPU.
> -		 */
> -		unsigned int new_cpu = cpumask_first(policy->real_cpus);
> -		struct device *new_dev = get_cpu_device(new_cpu);
> -
> -		dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu);
> -
> -		sysfs_remove_link(&new_dev->kobj, "cpufreq");
> -		policy->kobj_cpu = new_cpu;
> -		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
> -	}
> +	remove_cpu_dev_symlink(policy, cpu);
>   }
>
>   static void handle_update(struct work_struct *work)
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 9623218d996a..ef4c5b1a860f 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -65,7 +65,6 @@ struct cpufreq_policy {
>   	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
>   						should set cpufreq */
>   	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 */
>

-Saravana
Viresh Kumar Oct. 13, 2015, 3:39 a.m. | #2
On 12-10-15, 12:31, Saravana Kannan wrote:
> Can we use the first CPU in the related CPUs mask? Instead of the
> first CPU that the policy got created on? The policyX numbering
> would be a bit more consistent that way.

Okay..

> Suggested-by: ?

Will add. Though me/Rafael thought about it long back, but then
dropped the idea :)

> Didn't notice when this got added. Do we really need this anymore if
> we don't care about moving the directory and creating symlinks? I
> don't think we need it anymore. And if we really need to know
> related - offline, we can use for_each_cpu_and(related,
> online/present mask)

Its about tracking present-cpus, for which the link is present. So, it
is still required.
Viresh Kumar Oct. 13, 2015, 6:15 a.m. | #3
On 12-10-15, 12:31, Saravana Kannan wrote:
> Can we use the first CPU in the related CPUs mask? Instead of the
> first CPU that the policy got created on? The policyX numbering
> would be a bit more consistent that way.

Okay, checked this again. The problem is that ->init() isn't called
yet and we are very early in the initialization sequence. So, we can't
really know related_cpus yet. So I will keep it unchanged for now.
Saravana Kannan Oct. 13, 2015, 7:25 p.m. | #4
On 10/12/2015 11:15 PM, Viresh Kumar wrote:
> On 12-10-15, 12:31, Saravana Kannan wrote:
>> Can we use the first CPU in the related CPUs mask? Instead of the
>> first CPU that the policy got created on? The policyX numbering
>> would be a bit more consistent that way.
>
> Okay, checked this again. The problem is that ->init() isn't called
> yet and we are very early in the initialization sequence. So, we can't
> really know related_cpus yet. So I will keep it unchanged for now.
>

Can we move the sysfs add to the end so that by the time we add sysfs, 
we'll have all the details?

-Saravana
Saravana Kannan Oct. 13, 2015, 7:29 p.m. | #5
On 10/12/2015 08:39 PM, Viresh Kumar wrote:
> On 12-10-15, 12:31, Saravana Kannan wrote:
>> Can we use the first CPU in the related CPUs mask? Instead of the
>> first CPU that the policy got created on? The policyX numbering
>> would be a bit more consistent that way.
>
> Okay..
>
>> Suggested-by: ?
>
> Will add. Though me/Rafael thought about it long back, but then
> dropped the idea :)
>
>> Didn't notice when this got added. Do we really need this anymore if
>> we don't care about moving the directory and creating symlinks? I
>> don't think we need it anymore. And if we really need to know
>> related - offline, we can use for_each_cpu_and(related,
>> online/present mask)
>
> Its about tracking present-cpus, for which the link is present. So, it
> is still required.

But we don't need to track track of "present-cpus" separately though. We 
could do the for_each_cpu_and() when we create the symlinks for the 
first time. And after that, we can just use the subsystem interface 
callbacks (cpufreq_add_dev() and cpufreq_remove_dev()) to keep the 
symlinks updated.

I don't see any place where keeping track of this separately is more 
efficient. This would save some memory savings when the number of CPUs 
is large and also simplify the code because we won't have to keep 
another field up to date.

-Saravana
Viresh Kumar Oct. 15, 2015, 6:55 a.m. | #6
On 13-10-15, 12:29, Saravana Kannan wrote:
> But we don't need to track track of "present-cpus" separately
> though. We could do the for_each_cpu_and() when we create the
> symlinks for the first time. And after that, we can just use the
> subsystem interface callbacks (cpufreq_add_dev() and
> cpufreq_remove_dev()) to keep the symlinks updated.
> 
> I don't see any place where keeping track of this separately is more
> efficient. This would save some memory savings when the number of
> CPUs is large and also simplify the code because we won't have to
> keep another field up to date.

It is still required to track when can we free the policy.
Saravana Kannan Oct. 15, 2015, 7:28 p.m. | #7
On 10/14/2015 11:55 PM, Viresh Kumar wrote:
> On 13-10-15, 12:29, Saravana Kannan wrote:
>> But we don't need to track track of "present-cpus" separately
>> though. We could do the for_each_cpu_and() when we create the
>> symlinks for the first time. And after that, we can just use the
>> subsystem interface callbacks (cpufreq_add_dev() and
>> cpufreq_remove_dev()) to keep the symlinks updated.
>>
>> I don't see any place where keeping track of this separately is more
>> efficient. This would save some memory savings when the number of
>> CPUs is large and also simplify the code because we won't have to
>> keep another field up to date.
>
> It is still required to track when can we free the policy.
>

Ok, I'm not very familiar with this new field's uses. I'll take a closer 
look later and respond if I have other thoughts.

Thanks,
Saravana

Patch hide | download patch | download mbox

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2cb0e3b8170e..58aabe0f2d2c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -917,9 +917,6 @@  static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
 
 	/* Some related CPUs might not be present (physically hotplugged) */
 	for_each_cpu(j, policy->real_cpus) {
-		if (j == policy->kobj_cpu)
-			continue;
-
 		ret = add_cpu_dev_symlink(policy, j);
 		if (ret)
 			break;
@@ -933,12 +930,8 @@  static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
 	unsigned int j;
 
 	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus) {
-		if (j == policy->kobj_cpu)
-			continue;
-
+	for_each_cpu(j, policy->real_cpus)
 		remove_cpu_dev_symlink(policy, j);
-	}
 }
 
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
@@ -1054,8 +1047,8 @@  static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
 		goto err_free_rcpumask;
 
-	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq, &dev->kobj,
-				   "cpufreq");
+	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
+				   cpufreq_global_kobject, "policy%u", cpu);
 	if (ret) {
 		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
 		goto err_free_real_cpus;
@@ -1069,10 +1062,6 @@  static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	INIT_WORK(&policy->update, handle_update);
 
 	policy->cpu = cpu;
-
-	/* Set this once on allocation */
-	policy->kobj_cpu = cpu;
-
 	return policy;
 
 err_free_real_cpus:
@@ -1424,22 +1413,7 @@  static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 		return;
 	}
 
-	if (cpu != policy->kobj_cpu) {
-		remove_cpu_dev_symlink(policy, cpu);
-	} else {
-		/*
-		 * The CPU owning the policy object is going away.  Move it to
-		 * another suitable CPU.
-		 */
-		unsigned int new_cpu = cpumask_first(policy->real_cpus);
-		struct device *new_dev = get_cpu_device(new_cpu);
-
-		dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu);
-
-		sysfs_remove_link(&new_dev->kobj, "cpufreq");
-		policy->kobj_cpu = new_cpu;
-		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
-	}
+	remove_cpu_dev_symlink(policy, cpu);
 }
 
 static void handle_update(struct work_struct *work)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 9623218d996a..ef4c5b1a860f 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -65,7 +65,6 @@  struct cpufreq_policy {
 	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
 						should set cpufreq */
 	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 */