| Message ID | dd7e6cab3e879e94305c0fca1ea6033e79d166ca.1444583718.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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.
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
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
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.
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
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 */
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(-)