Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1244168 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-11 19:30 +0200
    Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories Saravana Kannan <skannan@codeaurora.org> - 2015-10-12 21:40 +0200
      Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-13 05:40 +0200
        Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories Saravana Kannan <skannan@codeaurora.org> - 2015-10-13 21:30 +0200
          Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-15 09:00 +0200
            Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories Saravana Kannan <skannan@codeaurora.org> - 2015-10-15 21:30 +0200
      Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-13 08:20 +0200
        Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories Saravana Kannan <skannan@codeaurora.org> - 2015-10-13 21:30 +0200

#1244168 — [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

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(-)

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 */
-- 
2.4.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1245063

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

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245318

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246067

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

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247496

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.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248102

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

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245368

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.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246068

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

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web