* [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-18 11:17 [PATCH 0/3 v6] cpufreq: Add 'load_table' debugfs file to show colleced CPUs load Chanwoo Choi
@ 2013-07-18 11:17 ` Chanwoo Choi
2013-07-22 10:11 ` Viresh Kumar
2013-07-18 11:17 ` [PATCH 2/3 v6] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs Chanwoo Choi
2013-07-18 11:17 ` [PATCH 3/3 v6] Documentation: cpufreq: load_table: Update load_table debugfs file documentation Chanwoo Choi
2 siblings, 1 reply; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-18 11:17 UTC (permalink / raw)
To: viresh.kumar, rjw, linux-kernel
Cc: linux-pm, cpufreq, kyungmin.park, myungjoo.ham, Chanwoo Choi
This patch create debugfs root directory and child directory according to
the number of CPUs for CPUFreq as below debugfs directory path:
- /sys/kernel/debug/cpufreq/cpuX
If many CPUs share only one cpufreq policy, other CPUs(except for first CPU)
create a symbolic link for debugfs directory of CPU0.
- link: /sys/kernel/debug/cpufreq/cpu[1-(N-1)] -> /sys/kernel/debug/cpufreq/cpu0
And then cpufreq may need to create debugfs specific file below of debugfs
directory of cpufreq. (e.g., /sys/kernel/debug/cpufreq/cpu0/load_table)
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
---
Changes since v5:
- Refactoring patch v4
- Create again symbolic link of debugfs directory when first CPU dev is removed
(In this case, many CPUs share only one cpufreq policy)
drivers/cpufreq/cpufreq.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/cpufreq.h | 1 +
2 files changed, 178 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2d53f47..924d654 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -23,6 +23,7 @@
#include <linux/notifier.h>
#include <linux/cpufreq.h>
#include <linux/delay.h>
+#include <linux/debugfs.h>
#include <linux/interrupt.h>
#include <linux/spinlock.h>
#include <linux/device.h>
@@ -703,6 +704,164 @@ static struct kobj_type ktype_cpufreq = {
.release = cpufreq_sysfs_release,
};
+#ifdef CONFIG_CPU_FREQ_STAT
+/* The cpufreq_debugfs is used to create debugfs root directory for CPUFreq. */
+static struct dentry *cpufreq_debugfs;
+
+static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
+ struct device *dev)
+{
+ char name[CPUFREQ_NAME_LEN];
+ unsigned int cpus, size, idx;
+
+ if (!cpufreq_debugfs)
+ return -EINVAL;
+
+ cpus = cpumask_weight(policy->cpus);
+ idx = cpus > 1 ? policy->cpu : 0;
+
+ size = sizeof(struct dentry*) * cpus;
+ policy->cpu_debugfs = devm_kzalloc(dev, size, GFP_KERNEL);
+ if (!policy->cpu_debugfs) {
+ pr_err("allocating debugfs memory failed\n");
+ return -ENOMEM;
+ }
+
+ sprintf(name, "cpu%d", policy->cpu);
+ policy->cpu_debugfs[idx] = debugfs_create_dir(name, cpufreq_debugfs);
+ if (!policy->cpu_debugfs[idx]) {
+ pr_err("creating debugfs directory failed\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
+ unsigned int src_cpu,
+ unsigned int dest_cpu)
+{
+ char symlink_name[CPUFREQ_NAME_LEN];
+ char target_name[CPUFREQ_NAME_LEN];
+
+ if (!cpufreq_debugfs)
+ return -EINVAL;
+
+ if (!policy->cpu_debugfs[src_cpu])
+ return -EINVAL;
+
+ sprintf(symlink_name, "cpu%d", dest_cpu);
+ sprintf(target_name, "./cpu%d", src_cpu);
+ policy->cpu_debugfs[dest_cpu] = debugfs_create_symlink(
+ symlink_name,
+ cpufreq_debugfs,
+ target_name);
+ if (!policy->cpu_debugfs[dest_cpu]) {
+ pr_err("creating debugfs symlink failed\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
+ unsigned int cpu)
+{
+ unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;
+
+ if (!policy->cpu_debugfs[idx])
+ return;
+
+ debugfs_remove_recursive(policy->cpu_debugfs[idx]);
+}
+
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
+ unsigned int new_cpu)
+{
+ struct dentry *old_entry, *new_entry;
+ char new_dir_name[CPUFREQ_NAME_LEN];
+ unsigned int j, old_cpu = policy->cpu;
+
+ if (!policy->cpu_debugfs[new_cpu])
+ return;
+
+ /*
+ * Remove symbolic link of debugfs directory except for debugfs
+ * directory of old_cpu.
+ */
+ for_each_present_cpu(j) {
+ if (old_cpu == j)
+ continue;
+
+ debugfs_remove(policy->cpu_debugfs[j]);
+ }
+
+ /*
+ * Change debugfs directory name from as following:
+ * - old debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${old_cpu}
+ * - new debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${new_cpu}
+ */
+ sprintf(new_dir_name, "cpu%d", new_cpu);
+ old_entry = policy->cpu_debugfs[old_cpu];
+ new_entry = debugfs_rename(cpufreq_debugfs, old_entry,
+ cpufreq_debugfs, new_dir_name);
+ if (!new_entry) {
+ pr_err("changing debugfs directory name failed\n");
+ goto err_rename;
+ }
+
+ policy->cpu_debugfs[new_cpu] = new_entry;
+ policy->cpu_debugfs[old_cpu] = NULL;
+
+ /* Create again symbolic link of debugfs directory */
+ for_each_present_cpu(j) {
+ if (new_cpu == j)
+ continue;
+
+ cpufreq_create_debugfs_symlink(policy, new_cpu, j);
+ }
+
+ return;
+
+err_rename:
+ for_each_present_cpu(j) {
+ if (old_cpu == j)
+ continue;
+
+ cpufreq_create_debugfs_symlink(policy, old_cpu, j);
+ }
+}
+
+static int cpufreq_create_debugfs(void)
+{
+ cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);
+ if (!cpufreq_debugfs) {
+ pr_err("creating debugfs root failed\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static void cpufreq_remove_debugfs(void)
+{
+ if (cpufreq_debugfs)
+ debugfs_remove_recursive(cpufreq_debugfs);
+}
+#else
+static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
+ struct device *dev) { return 0; }
+static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
+ unsigned int src_cpu,
+ unsigned int dest_cpu) { return 0;}
+static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
+ unsigned int cpu) { }
+static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
+ unsigned int new_cpu) { }
+static int cpufreq_create_debugfs(void) { return 0; }
+static void cpufreq_remove_debugfs(void) { }
+#endif
+
/* symlink affected CPUs */
static int cpufreq_add_dev_symlink(unsigned int cpu,
struct cpufreq_policy *policy)
@@ -726,6 +885,8 @@ static int cpufreq_add_dev_symlink(unsigned int cpu,
cpufreq_cpu_put(managed_policy);
return ret;
}
+
+ cpufreq_create_debugfs_symlink(policy, cpu, j);
}
return ret;
}
@@ -777,6 +938,9 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
}
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+ /* prepare interface data for debugfs */
+ cpufreq_create_debugfs_dir(policy, dev);
+
ret = cpufreq_add_dev_symlink(cpu, policy);
if (ret)
goto err_out_kobj_put;
@@ -839,6 +1003,8 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
return ret;
}
+ cpufreq_create_debugfs_symlink(policy, sibling, cpu);
+
return 0;
}
#endif
@@ -1046,6 +1212,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
if (cpu != data->cpu) {
sysfs_remove_link(&dev->kobj, "cpufreq");
+ cpufreq_remove_debugfs_dir(data, cpu);
} else if (cpus > 1) {
/* first sibling now owns the new sysfs dir */
cpu_dev = get_cpu_device(cpumask_first(data->cpus));
@@ -1068,6 +1235,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
return -EINVAL;
}
+ cpufreq_move_debugfs_dir(data, cpu_dev->id);
+
WARN_ON(lock_policy_rwsem_write(cpu));
update_policy_cpu(data, cpu_dev->id);
unlock_policy_rwsem_write(cpu);
@@ -1089,6 +1258,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
unlock_policy_rwsem_read(cpu);
kobject_put(kobj);
+ cpufreq_remove_debugfs_dir(data, cpu);
+
/* we need to make sure that the underlying kobj is actually
* not referenced anymore by anybody before we proceed with
* unloading.
@@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
cpufreq_driver = driver_data;
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+ cpufreq_create_debugfs();
+
ret = subsys_interface_register(&cpufreq_interface);
if (ret)
goto err_null_driver;
@@ -1918,12 +2091,14 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
}
register_hotcpu_notifier(&cpufreq_cpu_notifier);
+
pr_debug("driver %s up and running\n", driver_data->name);
return 0;
err_if_unreg:
subsys_interface_unregister(&cpufreq_interface);
err_null_driver:
+ cpufreq_remove_debugfs();
write_lock_irqsave(&cpufreq_driver_lock, flags);
cpufreq_driver = NULL;
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
@@ -1949,6 +2124,8 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
pr_debug("unregistering driver %s\n", driver->name);
+ cpufreq_remove_debugfs();
+
subsys_interface_unregister(&cpufreq_interface);
unregister_hotcpu_notifier(&cpufreq_cpu_notifier);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 037d36a..825f379 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -115,6 +115,7 @@ struct cpufreq_policy {
struct kobject kobj;
struct completion kobj_unregister;
+ struct dentry **cpu_debugfs;
};
#define CPUFREQ_ADJUST (0)
--
1.8.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-18 11:17 ` [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq Chanwoo Choi
@ 2013-07-22 10:11 ` Viresh Kumar
2013-07-24 1:25 ` Chanwoo Choi
0 siblings, 1 reply; 20+ messages in thread
From: Viresh Kumar @ 2013-07-22 10:11 UTC (permalink / raw)
To: Chanwoo Choi
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> +#ifdef CONFIG_CPU_FREQ_STAT
> +/* The cpufreq_debugfs is used to create debugfs root directory for CPUFreq. */
> +static struct dentry *cpufreq_debugfs;
> +
> +static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
> + struct device *dev)
> +{
> + char name[CPUFREQ_NAME_LEN];
> + unsigned int cpus, size, idx;
> +
> + if (!cpufreq_debugfs)
> + return -EINVAL;
> +
> + cpus = cpumask_weight(policy->cpus);
I remember I told you not to use policy->cpus for this purpose?? But
related_cpus.
> + idx = cpus > 1 ? policy->cpu : 0;
> + policy->cpu_debugfs[idx] = debugfs_create_dir(name, cpufreq_debugfs);
This is broken. A policy may contain cpus 9,10 only.. You will allocate array
for 2 cpus and try to access cpu_debugfs[9] :)
> + if (!policy->cpu_debugfs[idx]) {
> + pr_err("creating debugfs directory failed\n");
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
> + unsigned int src_cpu,
> + unsigned int dest_cpu)
Only use policy and cpu for which symlink has to be created as param
to this routine. And create link to policy->cpu.
> +{
> + char symlink_name[CPUFREQ_NAME_LEN];
> + char target_name[CPUFREQ_NAME_LEN];
> +
> + if (!cpufreq_debugfs)
> + return -EINVAL;
> +
> + if (!policy->cpu_debugfs[src_cpu])
> + return -EINVAL;
> +
> + sprintf(symlink_name, "cpu%d", dest_cpu);
> + sprintf(target_name, "./cpu%d", src_cpu);
> + policy->cpu_debugfs[dest_cpu] = debugfs_create_symlink(
> + symlink_name,
> + cpufreq_debugfs,
> + target_name);
> + if (!policy->cpu_debugfs[dest_cpu]) {
> + pr_err("creating debugfs symlink failed\n");
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
> + unsigned int cpu)
> +{
> + unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;
> +
> + if (!policy->cpu_debugfs[idx])
> + return;
> +
> + debugfs_remove_recursive(policy->cpu_debugfs[idx]);
Whey do we need recursive here? And what exactly does recursive will
do?
> +}
> +
same problem here too.
> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
> + unsigned int new_cpu)
> +{
> + struct dentry *old_entry, *new_entry;
> + char new_dir_name[CPUFREQ_NAME_LEN];
> + unsigned int j, old_cpu = policy->cpu;
> +
> + if (!policy->cpu_debugfs[new_cpu])
> + return;
> +
> + /*
> + * Remove symbolic link of debugfs directory except for debugfs
> + * directory of old_cpu.
> + */
> + for_each_present_cpu(j) {
> + if (old_cpu == j)
> + continue;
> +
> + debugfs_remove(policy->cpu_debugfs[j]);
Why you need this? We aren't removing the earlier dentry at all here.
> + }
> +
> + /*
> + * Change debugfs directory name from as following:
> + * - old debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${old_cpu}
> + * - new debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${new_cpu}
> + */
> + sprintf(new_dir_name, "cpu%d", new_cpu);
> + old_entry = policy->cpu_debugfs[old_cpu];
> + new_entry = debugfs_rename(cpufreq_debugfs, old_entry,
> + cpufreq_debugfs, new_dir_name);
This routine returns old_entry only.. and so you can simply create a
single routine with name dentry.
> + if (!new_entry) {
> + pr_err("changing debugfs directory name failed\n");
> + goto err_rename;
> + }
> +
> + policy->cpu_debugfs[new_cpu] = new_entry;
> + policy->cpu_debugfs[old_cpu] = NULL;
> +
> + /* Create again symbolic link of debugfs directory */
> + for_each_present_cpu(j) {
present_cpu?? We discussed this before.. You will break multi cluster
systems.
> + if (new_cpu == j)
> + continue;
> +
> + cpufreq_create_debugfs_symlink(policy, new_cpu, j);
> + }
> +
> + return;
> +
> +err_rename:
> + for_each_present_cpu(j) {
> + if (old_cpu == j)
> + continue;
> +
> + cpufreq_create_debugfs_symlink(policy, old_cpu, j);
> + }
> +}
> +
> +static int cpufreq_create_debugfs(void)
> +{
> + cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);
> + if (!cpufreq_debugfs) {
> + pr_err("creating debugfs root failed\n");
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static void cpufreq_remove_debugfs(void)
> +{
> + if (cpufreq_debugfs)
> + debugfs_remove_recursive(cpufreq_debugfs);
> +}
> +#else
> +static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
> + struct device *dev) { return 0; }
> +static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
> + unsigned int src_cpu,
> + unsigned int dest_cpu) { return 0;}
> +static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
> + unsigned int cpu) { }
> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
> + unsigned int new_cpu) { }
> +static int cpufreq_create_debugfs(void) { return 0; }
> +static void cpufreq_remove_debugfs(void) { }
make all above #else part routines inline.
> +#endif
> +
> /* symlink affected CPUs */
> static int cpufreq_add_dev_symlink(unsigned int cpu,
> struct cpufreq_policy *policy)
> @@ -726,6 +885,8 @@ static int cpufreq_add_dev_symlink(unsigned int cpu,
> cpufreq_cpu_put(managed_policy);
> return ret;
> }
> +
> + cpufreq_create_debugfs_symlink(policy, cpu, j);
> }
> return ret;
> }
> @@ -777,6 +938,9 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
> }
> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>
> + /* prepare interface data for debugfs */
> + cpufreq_create_debugfs_dir(policy, dev);
> +
> ret = cpufreq_add_dev_symlink(cpu, policy);
> if (ret)
> goto err_out_kobj_put;
> @@ -839,6 +1003,8 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
> return ret;
> }
>
> + cpufreq_create_debugfs_symlink(policy, sibling, cpu);
> +
> return 0;
> }
> #endif
> @@ -1046,6 +1212,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
>
> if (cpu != data->cpu) {
> sysfs_remove_link(&dev->kobj, "cpufreq");
> + cpufreq_remove_debugfs_dir(data, cpu);
> } else if (cpus > 1) {
> /* first sibling now owns the new sysfs dir */
> cpu_dev = get_cpu_device(cpumask_first(data->cpus));
> @@ -1068,6 +1235,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
> return -EINVAL;
> }
>
> + cpufreq_move_debugfs_dir(data, cpu_dev->id);
> +
> WARN_ON(lock_policy_rwsem_write(cpu));
> update_policy_cpu(data, cpu_dev->id);
> unlock_policy_rwsem_write(cpu);
> @@ -1089,6 +1258,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
> unlock_policy_rwsem_read(cpu);
> kobject_put(kobj);
>
> + cpufreq_remove_debugfs_dir(data, cpu);
> +
> /* we need to make sure that the underlying kobj is actually
> * not referenced anymore by anybody before we proceed with
> * unloading.
> @@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> cpufreq_driver = driver_data;
> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>
> + cpufreq_create_debugfs();
Why you moved this to register_driver? It was fine at cpufreq_core_init()
> ret = subsys_interface_register(&cpufreq_interface);
> if (ret)
> goto err_null_driver;
> @@ -1918,12 +2091,14 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> }
>
> register_hotcpu_notifier(&cpufreq_cpu_notifier);
> +
unrelated change.
> pr_debug("driver %s up and running\n", driver_data->name);
>
> return 0;
> err_if_unreg:
> subsys_interface_unregister(&cpufreq_interface);
> err_null_driver:
> + cpufreq_remove_debugfs();
> write_lock_irqsave(&cpufreq_driver_lock, flags);
> cpufreq_driver = NULL;
> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> @@ -1949,6 +2124,8 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
>
> pr_debug("unregistering driver %s\n", driver->name);
>
> + cpufreq_remove_debugfs();
And so you don't need this.
> subsys_interface_unregister(&cpufreq_interface);
> unregister_hotcpu_notifier(&cpufreq_cpu_notifier);
>
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 037d36a..825f379 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -115,6 +115,7 @@ struct cpufreq_policy {
>
> struct kobject kobj;
> struct completion kobj_unregister;
> + struct dentry **cpu_debugfs;
> };
>
> #define CPUFREQ_ADJUST (0)
> --
> 1.8.0
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-22 10:11 ` Viresh Kumar
@ 2013-07-24 1:25 ` Chanwoo Choi
2013-07-24 5:05 ` Viresh Kumar
2013-07-24 6:14 ` Chanwoo Choi
0 siblings, 2 replies; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-24 1:25 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
Hi Viresh,
On 07/22/2013 07:11 PM, Viresh Kumar wrote:
> On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> +#ifdef CONFIG_CPU_FREQ_STAT
>> +/* The cpufreq_debugfs is used to create debugfs root directory for CPUFreq. */
>> +static struct dentry *cpufreq_debugfs;
>> +
>> +static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
>> + struct device *dev)
>> +{
>> + char name[CPUFREQ_NAME_LEN];
>> + unsigned int cpus, size, idx;
>> +
>> + if (!cpufreq_debugfs)
>> + return -EINVAL;
>> +
>> + cpus = cpumask_weight(policy->cpus);
>
> I remember I told you not to use policy->cpus for this purpose?? But
> related_cpus.
You're right. I'll use policy->related_cpus instead of policy->cpus.
>
>> + idx = cpus > 1 ? policy->cpu : 0;
>
>> + policy->cpu_debugfs[idx] = debugfs_create_dir(name, cpufreq_debugfs);
>
> This is broken. A policy may contain cpus 9,10 only.. You will allocate array
> for 2 cpus and try to access cpu_debugfs[9] :)
Right, I'll consider other method to resolve issue related to index of array.
>
>> + if (!policy->cpu_debugfs[idx]) {
>> + pr_err("creating debugfs directory failed\n");
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
>> + unsigned int src_cpu,
>> + unsigned int dest_cpu)
>
> Only use policy and cpu for which symlink has to be created as param
> to this routine. And create link to policy->cpu.
>
OK, I'll simplify function prototype(cpufreq_create_debugfs_symlink) by removing
unnecessary parameter.
>> +{
>> + char symlink_name[CPUFREQ_NAME_LEN];
>> + char target_name[CPUFREQ_NAME_LEN];
>> +
>> + if (!cpufreq_debugfs)
>> + return -EINVAL;
>> +
>> + if (!policy->cpu_debugfs[src_cpu])
>> + return -EINVAL;
>> +
>> + sprintf(symlink_name, "cpu%d", dest_cpu);
>> + sprintf(target_name, "./cpu%d", src_cpu);
>> + policy->cpu_debugfs[dest_cpu] = debugfs_create_symlink(
>> + symlink_name,
>> + cpufreq_debugfs,
>> + target_name);
>> + if (!policy->cpu_debugfs[dest_cpu]) {
>> + pr_err("creating debugfs symlink failed\n");
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
>> + unsigned int cpu)
>> +{
>> + unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;
>> +
>> + if (!policy->cpu_debugfs[idx])
>> + return;
>> +
>> + debugfs_remove_recursive(policy->cpu_debugfs[idx]);
>
> Whey do we need recursive here? And what exactly does recursive will
> do?
>
If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory
and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
>> +}
>> +
>
> same problem here too.
>> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
>> + unsigned int new_cpu)
>> +{
>> + struct dentry *old_entry, *new_entry;
>> + char new_dir_name[CPUFREQ_NAME_LEN];
>> + unsigned int j, old_cpu = policy->cpu;
>> +
>> + if (!policy->cpu_debugfs[new_cpu])
>> + return;
>> +
>> + /*
>> + * Remove symbolic link of debugfs directory except for debugfs
>> + * directory of old_cpu.
>> + */
>> + for_each_present_cpu(j) {
>> + if (old_cpu == j)
>> + continue;
>> +
>> + debugfs_remove(policy->cpu_debugfs[j]);
>
> Why you need this? We aren't removing the earlier dentry at all here.
>
>> + }
>> +
>> + /*
>> + * Change debugfs directory name from as following:
>> + * - old debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${old_cpu}
>> + * - new debugfs dir name : /sys/kernel/debugfs/cpufreq/cpu${new_cpu}
>> + */
>> + sprintf(new_dir_name, "cpu%d", new_cpu);
>> + old_entry = policy->cpu_debugfs[old_cpu];
>> + new_entry = debugfs_rename(cpufreq_debugfs, old_entry,
>> + cpufreq_debugfs, new_dir_name);
>
> This routine returns old_entry only.. and so you can simply create a
> single routine with name dentry.
I used 'new_entry' variable to improve readability to distinguish between old_entry and new_entry.
But, as your comment, I'll simplify this statement to remove unnecessary code.
>
>> + if (!new_entry) {
>> + pr_err("changing debugfs directory name failed\n");
>> + goto err_rename;
>> + }
>> +
>> + policy->cpu_debugfs[new_cpu] = new_entry;
>> + policy->cpu_debugfs[old_cpu] = NULL;
>> +
>> + /* Create again symbolic link of debugfs directory */
>> + for_each_present_cpu(j) {
>
> present_cpu?? We discussed this before.. You will break multi cluster
> systems.
My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
>
>> + if (new_cpu == j)
>> + continue;
>> +
>> + cpufreq_create_debugfs_symlink(policy, new_cpu, j);
>> + }
>> +
>> + return;
>> +
>> +err_rename:
>> + for_each_present_cpu(j) {
>> + if (old_cpu == j)
>> + continue;
>> +
>> + cpufreq_create_debugfs_symlink(policy, old_cpu, j);
>> + }
>> +}
>> +
>> +static int cpufreq_create_debugfs(void)
>> +{
>> + cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);
>> + if (!cpufreq_debugfs) {
>> + pr_err("creating debugfs root failed\n");
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cpufreq_remove_debugfs(void)
>> +{
>> + if (cpufreq_debugfs)
>> + debugfs_remove_recursive(cpufreq_debugfs);
>> +}
>> +#else
>> +static int cpufreq_create_debugfs_dir(struct cpufreq_policy *policy,
>> + struct device *dev) { return 0; }
>> +static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
>> + unsigned int src_cpu,
>> + unsigned int dest_cpu) { return 0;}
>> +static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
>> + unsigned int cpu) { }
>> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
>> + unsigned int new_cpu) { }
>> +static int cpufreq_create_debugfs(void) { return 0; }
>> +static void cpufreq_remove_debugfs(void) { }
>
> make all above #else part routines inline.
OK.
>
>> +#endif
>> +
>> /* symlink affected CPUs */
>> static int cpufreq_add_dev_symlink(unsigned int cpu,
>> struct cpufreq_policy *policy)
>> @@ -726,6 +885,8 @@ static int cpufreq_add_dev_symlink(unsigned int cpu,
>> cpufreq_cpu_put(managed_policy);
>> return ret;
>> }
>> +
>> + cpufreq_create_debugfs_symlink(policy, cpu, j);
>> }
>> return ret;
>> }
>> @@ -777,6 +938,9 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
>> }
>> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>
>> + /* prepare interface data for debugfs */
>> + cpufreq_create_debugfs_dir(policy, dev);
>> +
>> ret = cpufreq_add_dev_symlink(cpu, policy);
>> if (ret)
>> goto err_out_kobj_put;
>> @@ -839,6 +1003,8 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
>> return ret;
>> }
>>
>> + cpufreq_create_debugfs_symlink(policy, sibling, cpu);
>> +
>> return 0;
>> }
>> #endif
>> @@ -1046,6 +1212,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
>>
>> if (cpu != data->cpu) {
>> sysfs_remove_link(&dev->kobj, "cpufreq");
>> + cpufreq_remove_debugfs_dir(data, cpu);
>> } else if (cpus > 1) {
>> /* first sibling now owns the new sysfs dir */
>> cpu_dev = get_cpu_device(cpumask_first(data->cpus));
>> @@ -1068,6 +1235,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
>> return -EINVAL;
>> }
>>
>> + cpufreq_move_debugfs_dir(data, cpu_dev->id);
>> +
>> WARN_ON(lock_policy_rwsem_write(cpu));
>> update_policy_cpu(data, cpu_dev->id);
>> unlock_policy_rwsem_write(cpu);
>> @@ -1089,6 +1258,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
>> unlock_policy_rwsem_read(cpu);
>> kobject_put(kobj);
>>
>> + cpufreq_remove_debugfs_dir(data, cpu);
>> +
>> /* we need to make sure that the underlying kobj is actually
>> * not referenced anymore by anybody before we proceed with
>> * unloading.
>> @@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>> cpufreq_driver = driver_data;
>> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>
>> + cpufreq_create_debugfs();
>
> Why you moved this to register_driver? It was fine at cpufreq_core_init()
If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit().
Do you agree about creating cpufreq_core_exit()(?
>
>> ret = subsys_interface_register(&cpufreq_interface);
>> if (ret)
>> goto err_null_driver;
>> @@ -1918,12 +2091,14 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>> }
>>
>> register_hotcpu_notifier(&cpufreq_cpu_notifier);
>> +
>
> unrelated change.
OK, I'll remove it.
>
>> pr_debug("driver %s up and running\n", driver_data->name);
>>
>> return 0;
>> err_if_unreg:
>> subsys_interface_unregister(&cpufreq_interface);
>> err_null_driver:
>> + cpufreq_remove_debugfs();
>> write_lock_irqsave(&cpufreq_driver_lock, flags);
>> cpufreq_driver = NULL;
>> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>> @@ -1949,6 +2124,8 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
>>
>> pr_debug("unregistering driver %s\n", driver->name);
>>
>> + cpufreq_remove_debugfs();
>
> And so you don't need this.
>
>> subsys_interface_unregister(&cpufreq_interface);
>> unregister_hotcpu_notifier(&cpufreq_cpu_notifier);
>>
>> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
>> index 037d36a..825f379 100644
>> --- a/include/linux/cpufreq.h
>> +++ b/include/linux/cpufreq.h
>> @@ -115,6 +115,7 @@ struct cpufreq_policy {
>>
>> struct kobject kobj;
>> struct completion kobj_unregister;
>> + struct dentry **cpu_debugfs;
>> };
>>
>> #define CPUFREQ_ADJUST (0)
>> --
>> 1.8.0
>>
>
Thanks for your comment.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 1:25 ` Chanwoo Choi
@ 2013-07-24 5:05 ` Viresh Kumar
2013-07-24 7:43 ` Chanwoo Choi
2013-07-24 6:14 ` Chanwoo Choi
1 sibling, 1 reply; 20+ messages in thread
From: Viresh Kumar @ 2013-07-24 5:05 UTC (permalink / raw)
To: Chanwoo Choi
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 24 July 2013 06:55, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 07/22/2013 07:11 PM, Viresh Kumar wrote:
>> On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> +static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
>>> + unsigned int cpu)
>>> +{
>>> + unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;
>>> +
>>> + if (!policy->cpu_debugfs[idx])
>>> + return;
>>> +
>>> + debugfs_remove_recursive(policy->cpu_debugfs[idx]);
>>
>> Whey do we need recursive here? And what exactly does recursive will
>> do?
>>
>
> If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory
> and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
You are calling this routine even when we aren't at the last cpu of a policy.
And so, eventually you are calling this routine for a link you have created.
Have you actually tested your code? What kind of platform? What is cpu
topology ?? And what exactly you tested..
We are already on v6 and this patch still looks like the v1.. It still has lots
of basic mistakes, which I don't expect so later in the series..
Its very difficult for me to review the same patchset again and again.. So,
normally people might not review it well after v3-v4 and just trust the sender..
But I am nowhere close to getting that.. And so discouraged to review it..
Please review/test it well on multiple kind of systems if possible. Test on
your intel laptop and see if it has multiple policy structures with
multiple cpus
in it.. cpuX/cpufreq/related_cpus gives you all cpus that share policy
structure.
>>> +}
>>> +
>>
>> same problem here too.
>>> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
>>> + unsigned int new_cpu)
>>> +{
>>> + struct dentry *old_entry, *new_entry;
>>> + char new_dir_name[CPUFREQ_NAME_LEN];
>>> + unsigned int j, old_cpu = policy->cpu;
>>> +
>>> + if (!policy->cpu_debugfs[new_cpu])
>>> + return;
>>> +
>>> + /*
>>> + * Remove symbolic link of debugfs directory except for debugfs
>>> + * directory of old_cpu.
>>> + */
>>> + for_each_present_cpu(j) {
>>> + if (old_cpu == j)
>>> + continue;
>>> +
>>> + debugfs_remove(policy->cpu_debugfs[j]);
>>
>> Why you need this? We aren't removing the earlier dentry at all here.
haven't answered this.
>>> + if (!new_entry) {
>>> + pr_err("changing debugfs directory name failed\n");
>>> + goto err_rename;
>>> + }
>>> +
>>> + policy->cpu_debugfs[new_cpu] = new_entry;
>>> + policy->cpu_debugfs[old_cpu] = NULL;
>>> +
>>> + /* Create again symbolic link of debugfs directory */
>>> + for_each_present_cpu(j) {
>>
>> present_cpu?? We discussed this before.. You will break multi cluster
>> systems.
>
> My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
Go through earlier comments about this.. you are still wrong.. You need to
run over cpus that are in this policy.. i.e. policy->cpus.
>>> + if (new_cpu == j)
>>> + continue;
>>> +
>>> @@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>>> cpufreq_driver = driver_data;
>>> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>>
>>> + cpufreq_create_debugfs();
>>
>> Why you moved this to register_driver? It was fine at cpufreq_core_init()
>
> If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit().
> Do you agree about creating cpufreq_core_exit()(?
No you don't need that routine. Or in other words there isn't any exit
for cpufreq core and so this directory must not be removed.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 5:05 ` Viresh Kumar
@ 2013-07-24 7:43 ` Chanwoo Choi
2013-07-24 7:51 ` Viresh Kumar
2013-07-24 8:07 ` Viresh Kumar
0 siblings, 2 replies; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-24 7:43 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 07/24/2013 02:05 PM, Viresh Kumar wrote:
> On 24 July 2013 06:55, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> On 07/22/2013 07:11 PM, Viresh Kumar wrote:
>>> On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>
>>>> +static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
>>>> + unsigned int cpu)
>>>> +{
>>>> + unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;
>>>> +
>>>> + if (!policy->cpu_debugfs[idx])
>>>> + return;
>>>> +
>>>> + debugfs_remove_recursive(policy->cpu_debugfs[idx]);
>>>
>>> Whey do we need recursive here? And what exactly does recursive will
>>> do?
>>>
>>
>> If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory
>> and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
>
> You are calling this routine even when we aren't at the last cpu of a policy.
> And so, eventually you are calling this routine for a link you have created.
I'll call proper debugfs_remove*() function according to type of debugfs pointer.
- if cpu is last user of policy, call debugfs_remove_recursive()
- else, call debugfs_remove().
>
> Have you actually tested your code? What kind of platform? What is cpu
> topology ?? And what exactly you tested..
I tested quad-core EXYNOS4412 SoC based on Cortex-A9 with Tizen platform.
It is opereated on this environment but as you commnet, this test and environment
isn't enough to verify this patchset.
- Testcase1 : Change cpufreq governor on runtime
- Testcase2 : Turn on/off CPU state on runtme
>
> We are already on v6 and this patch still looks like the v1.. It still has lots
> of basic mistakes, which I don't expect so later in the series..
>
> Its very difficult for me to review the same patchset again and again.. So,
> normally people might not review it well after v3-v4 and just trust the sender..
> But I am nowhere close to getting that.. And so discouraged to review it..
>
I'm so sorry about this and thanks for previous your review sincerely.
> Please review/test it well on multiple kind of systems if possible. Test on
> your intel laptop and see if it has multiple policy structures with
> multiple cpus
> in it.. cpuX/cpufreq/related_cpus gives you all cpus that share policy
> structure.
As you comment, I'll modify/test this patchset on various system with enough testcase
and resend this patchset after a thorough review.
>
>>>> +}
>>>> +
>>>
>>> same problem here too.
>>>> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
>>>> + unsigned int new_cpu)
>>>> +{
>>>> + struct dentry *old_entry, *new_entry;
>>>> + char new_dir_name[CPUFREQ_NAME_LEN];
>>>> + unsigned int j, old_cpu = policy->cpu;
>>>> +
>>>> + if (!policy->cpu_debugfs[new_cpu])
>>>> + return;
>>>> +
>>>> + /*
>>>> + * Remove symbolic link of debugfs directory except for debugfs
>>>> + * directory of old_cpu.
>>>> + */
>>>> + for_each_present_cpu(j) {
>>>> + if (old_cpu == j)
>>>> + continue;
>>>> +
>>>> + debugfs_remove(policy->cpu_debugfs[j]);
>>>
>>> Why you need this? We aren't removing the earlier dentry at all here.
>
> haven't answered this.
The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table)
If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu,
core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and
child data for 'old_cpu' to debugfs directory for 'new_cpu'.
If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file.
So I didn't remove the earlier dentry of 'old_cpu'.
>
>>>> + if (!new_entry) {
>>>> + pr_err("changing debugfs directory name failed\n");
>>>> + goto err_rename;
>>>> + }
>>>> +
>>>> + policy->cpu_debugfs[new_cpu] = new_entry;
>>>> + policy->cpu_debugfs[old_cpu] = NULL;
>>>> +
>>>> + /* Create again symbolic link of debugfs directory */
>>>> + for_each_present_cpu(j) {
>>>
>>> present_cpu?? We discussed this before.. You will break multi cluster
>>> systems.
>>
>> My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
>
> Go through earlier comments about this.. you are still wrong.. You need to
> run over cpus that are in this policy.. i.e. policy->cpus.
>
OK.
>>>> + if (new_cpu == j)
>>>> + continue;
>>>> +
>
>>>> @@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>>>> cpufreq_driver = driver_data;
>>>> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>>>
>>>> + cpufreq_create_debugfs();
>>>
>>> Why you moved this to register_driver? It was fine at cpufreq_core_init()
>>
>> If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit().
>> Do you agree about creating cpufreq_core_exit()(?
>
> No you don't need that routine. Or in other words there isn't any exit
> for cpufreq core and so this directory must not be removed.
>
I understood on your previous comment as You said that I had to remove 'cpufreq' debugfs directory
when cpufreq isn't used.
If the core execute cpufreq_create_debugfs() in cpufreq_core_init(),
don't I need to remove 'cpufreq' debugfs directory without cpufreq_core_exit()?
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 7:43 ` Chanwoo Choi
@ 2013-07-24 7:51 ` Viresh Kumar
2013-07-24 8:01 ` Chanwoo Choi
2013-07-24 8:07 ` Viresh Kumar
1 sibling, 1 reply; 20+ messages in thread
From: Viresh Kumar @ 2013-07-24 7:51 UTC (permalink / raw)
To: Chanwoo Choi
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 24 July 2013 13:13, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 07/24/2013 02:05 PM, Viresh Kumar wrote:
>> On 24 July 2013 06:55, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> On 07/22/2013 07:11 PM, Viresh Kumar wrote:
>>>> On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>
>>>>> +static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
>>>>> + unsigned int cpu)
>>>>> +{
>>>>> + unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;
>>>>> +
>>>>> + if (!policy->cpu_debugfs[idx])
>>>>> + return;
>>>>> +
>>>>> + debugfs_remove_recursive(policy->cpu_debugfs[idx]);
>>>>
>>>> Whey do we need recursive here? And what exactly does recursive will
>>>> do?
>>>>
>>>
>>> If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory
>>> and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
>>
>> You are calling this routine even when we aren't at the last cpu of a policy.
>> And so, eventually you are calling this routine for a link you have created.
>
> I'll call proper debugfs_remove*() function according to type of debugfs pointer.
> - if cpu is last user of policy, call debugfs_remove_recursive()
> - else, call debugfs_remove().
>
>>
>> Have you actually tested your code? What kind of platform? What is cpu
>> topology ?? And what exactly you tested..
>
> I tested quad-core EXYNOS4412 SoC based on Cortex-A9 with Tizen platform.
> It is opereated on this environment but as you commnet, this test and environment
> isn't enough to verify this patchset.
> - Testcase1 : Change cpufreq governor on runtime
> - Testcase2 : Turn on/off CPU state on runtme
>
>>
>> We are already on v6 and this patch still looks like the v1.. It still has lots
>> of basic mistakes, which I don't expect so later in the series..
>>
>> Its very difficult for me to review the same patchset again and again.. So,
>> normally people might not review it well after v3-v4 and just trust the sender..
>> But I am nowhere close to getting that.. And so discouraged to review it..
>>
>
> I'm so sorry about this and thanks for previous your review sincerely.
>
>> Please review/test it well on multiple kind of systems if possible. Test on
>> your intel laptop and see if it has multiple policy structures with
>> multiple cpus
>> in it.. cpuX/cpufreq/related_cpus gives you all cpus that share policy
>> structure.
>
> As you comment, I'll modify/test this patchset on various system with enough testcase
> and resend this patchset after a thorough review.
>
>
>>
>>>>> +}
>>>>> +
>>>>
>>>> same problem here too.
>>>>> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
>>>>> + unsigned int new_cpu)
>>>>> +{
>>>>> + struct dentry *old_entry, *new_entry;
>>>>> + char new_dir_name[CPUFREQ_NAME_LEN];
>>>>> + unsigned int j, old_cpu = policy->cpu;
>>>>> +
>>>>> + if (!policy->cpu_debugfs[new_cpu])
>>>>> + return;
>>>>> +
>>>>> + /*
>>>>> + * Remove symbolic link of debugfs directory except for debugfs
>>>>> + * directory of old_cpu.
>>>>> + */
>>>>> + for_each_present_cpu(j) {
>>>>> + if (old_cpu == j)
>>>>> + continue;
>>>>> +
>>>>> + debugfs_remove(policy->cpu_debugfs[j]);
>>>>
>>>> Why you need this? We aren't removing the earlier dentry at all here.
>>
>> haven't answered this.
>
> The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table)
> If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu,
> core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and
> child data for 'old_cpu' to debugfs directory for 'new_cpu'.
>
> If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file.
> So I didn't remove the earlier dentry of 'old_cpu'.
>
>>
>>>>> + if (!new_entry) {
>>>>> + pr_err("changing debugfs directory name failed\n");
>>>>> + goto err_rename;
>>>>> + }
>>>>> +
>>>>> + policy->cpu_debugfs[new_cpu] = new_entry;
>>>>> + policy->cpu_debugfs[old_cpu] = NULL;
>>>>> +
>>>>> + /* Create again symbolic link of debugfs directory */
>>>>> + for_each_present_cpu(j) {
>>>>
>>>> present_cpu?? We discussed this before.. You will break multi cluster
>>>> systems.
>>>
>>> My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
>>
>> Go through earlier comments about this.. you are still wrong.. You need to
>> run over cpus that are in this policy.. i.e. policy->cpus.
>>
>
> OK.
>
>>>>> + if (new_cpu == j)
>>>>> + continue;
>>>>> +
>>
>>>>> @@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>>>>> cpufreq_driver = driver_data;
>>>>> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>>>>
>>>>> + cpufreq_create_debugfs();
>>>>
>>>> Why you moved this to register_driver? It was fine at cpufreq_core_init()
>>>
>>> If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit().
>>> Do you agree about creating cpufreq_core_exit()(?
>>
>> No you don't need that routine. Or in other words there isn't any exit
>> for cpufreq core and so this directory must not be removed.
>>
>
> I understood on your previous comment as You said that I had to remove 'cpufreq' debugfs directory
> when cpufreq isn't used.
>
> If the core execute cpufreq_create_debugfs() in cpufreq_core_init(),
> don't I need to remove 'cpufreq' debugfs directory without cpufreq_core_exit()?
I copied following from your patch sent on 5th july.. It didn't had any version
number and so is difficult to distinguish..
> @@ -1976,6 +2029,10 @@ static int __init cpufreq_core_init(void)
> BUG_ON(!cpufreq_global_kobject);
> register_syscore_ops(&cpufreq_syscore_ops);
>
> + cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);
> + if (!cpufreq_debugfs)
> + pr_debug("creating debugfs root failed\n");
So, you just added this directory once.. So you must not
remove it.
Where did I say you remove this directory..
To be clear, don't remove cpufreq debugfs directory at all. Play
only with cpu directories inside this debugfs directory.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 7:51 ` Viresh Kumar
@ 2013-07-24 8:01 ` Chanwoo Choi
0 siblings, 0 replies; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-24 8:01 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 07/24/2013 04:51 PM, Viresh Kumar wrote:
> On 24 July 2013 13:13, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> On 07/24/2013 02:05 PM, Viresh Kumar wrote:
>>> On 24 July 2013 06:55, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>> On 07/22/2013 07:11 PM, Viresh Kumar wrote:
>>>>> On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>
>>>>>> +static void cpufreq_remove_debugfs_dir(struct cpufreq_policy *policy,
>>>>>> + unsigned int cpu)
>>>>>> +{
>>>>>> + unsigned int idx = cpumask_weight(policy->cpus) > 1 ? cpu : 0;
>>>>>> +
>>>>>> + if (!policy->cpu_debugfs[idx])
>>>>>> + return;
>>>>>> +
>>>>>> + debugfs_remove_recursive(policy->cpu_debugfs[idx]);
>>>>>
>>>>> Whey do we need recursive here? And what exactly does recursive will
>>>>> do?
>>>>>
>>>>
>>>> If cpu is last user of policy, __cpufreq_remove_dev() have to remove debugfs directory
>>>> and child file/directory of root debugfs directory. So, I used debugfs_remove_recursive() function.
>>>
>>> You are calling this routine even when we aren't at the last cpu of a policy.
>>> And so, eventually you are calling this routine for a link you have created.
>>
>> I'll call proper debugfs_remove*() function according to type of debugfs pointer.
>> - if cpu is last user of policy, call debugfs_remove_recursive()
>> - else, call debugfs_remove().
>>
>>>
>>> Have you actually tested your code? What kind of platform? What is cpu
>>> topology ?? And what exactly you tested..
>>
>> I tested quad-core EXYNOS4412 SoC based on Cortex-A9 with Tizen platform.
>> It is opereated on this environment but as you commnet, this test and environment
>> isn't enough to verify this patchset.
>> - Testcase1 : Change cpufreq governor on runtime
>> - Testcase2 : Turn on/off CPU state on runtme
>>
>>>
>>> We are already on v6 and this patch still looks like the v1.. It still has lots
>>> of basic mistakes, which I don't expect so later in the series..
>>>
>>> Its very difficult for me to review the same patchset again and again.. So,
>>> normally people might not review it well after v3-v4 and just trust the sender..
>>> But I am nowhere close to getting that.. And so discouraged to review it..
>>>
>>
>> I'm so sorry about this and thanks for previous your review sincerely.
>>
>>> Please review/test it well on multiple kind of systems if possible. Test on
>>> your intel laptop and see if it has multiple policy structures with
>>> multiple cpus
>>> in it.. cpuX/cpufreq/related_cpus gives you all cpus that share policy
>>> structure.
>>
>> As you comment, I'll modify/test this patchset on various system with enough testcase
>> and resend this patchset after a thorough review.
>>
>>
>>>
>>>>>> +}
>>>>>> +
>>>>>
>>>>> same problem here too.
>>>>>> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
>>>>>> + unsigned int new_cpu)
>>>>>> +{
>>>>>> + struct dentry *old_entry, *new_entry;
>>>>>> + char new_dir_name[CPUFREQ_NAME_LEN];
>>>>>> + unsigned int j, old_cpu = policy->cpu;
>>>>>> +
>>>>>> + if (!policy->cpu_debugfs[new_cpu])
>>>>>> + return;
>>>>>> +
>>>>>> + /*
>>>>>> + * Remove symbolic link of debugfs directory except for debugfs
>>>>>> + * directory of old_cpu.
>>>>>> + */
>>>>>> + for_each_present_cpu(j) {
>>>>>> + if (old_cpu == j)
>>>>>> + continue;
>>>>>> +
>>>>>> + debugfs_remove(policy->cpu_debugfs[j]);
>>>>>
>>>>> Why you need this? We aren't removing the earlier dentry at all here.
>>>
>>> haven't answered this.
>>
>> The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table)
>> If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu,
>> core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and
>> child data for 'old_cpu' to debugfs directory for 'new_cpu'.
>>
>> If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file.
>> So I didn't remove the earlier dentry of 'old_cpu'.
>>
>>>
>>>>>> + if (!new_entry) {
>>>>>> + pr_err("changing debugfs directory name failed\n");
>>>>>> + goto err_rename;
>>>>>> + }
>>>>>> +
>>>>>> + policy->cpu_debugfs[new_cpu] = new_entry;
>>>>>> + policy->cpu_debugfs[old_cpu] = NULL;
>>>>>> +
>>>>>> + /* Create again symbolic link of debugfs directory */
>>>>>> + for_each_present_cpu(j) {
>>>>>
>>>>> present_cpu?? We discussed this before.. You will break multi cluster
>>>>> systems.
>>>>
>>>> My mistake. I'll use for_each_cpu() macro instead of for_each_present_cpu().
>>>
>>> Go through earlier comments about this.. you are still wrong.. You need to
>>> run over cpus that are in this policy.. i.e. policy->cpus.
>>>
>>
>> OK.
>>
>>>>>> + if (new_cpu == j)
>>>>>> + continue;
>>>>>> +
>>>
>>>>>> @@ -1894,6 +2065,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>>>>>> cpufreq_driver = driver_data;
>>>>>> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>>>>>
>>>>>> + cpufreq_create_debugfs();
>>>>>
>>>>> Why you moved this to register_driver? It was fine at cpufreq_core_init()
>>>>
>>>> If we moved this to cpufreq_core_int(), I have to create cpufreq_core_exit().
>>>> Do you agree about creating cpufreq_core_exit()(?
>>>
>>> No you don't need that routine. Or in other words there isn't any exit
>>> for cpufreq core and so this directory must not be removed.
>>>
>>
>> I understood on your previous comment as You said that I had to remove 'cpufreq' debugfs directory
>> when cpufreq isn't used.
>>
>> If the core execute cpufreq_create_debugfs() in cpufreq_core_init(),
>> don't I need to remove 'cpufreq' debugfs directory without cpufreq_core_exit()?
>
> I copied following from your patch sent on 5th july.. It didn't had any version
> number and so is difficult to distinguish..
>
>> @@ -1976,6 +2029,10 @@ static int __init cpufreq_core_init(void)
>> BUG_ON(!cpufreq_global_kobject);
>> register_syscore_ops(&cpufreq_syscore_ops);
>>
>> + cpufreq_debugfs = debugfs_create_dir("cpufreq", NULL);
>> + if (!cpufreq_debugfs)
>> + pr_debug("creating debugfs root failed\n");
>
> So, you just added this directory once.. So you must not
> remove it.
>
>
> Where did I say you remove this directory..
> To be clear, don't remove cpufreq debugfs directory at all. Play
> only with cpu directories inside this debugfs directory.
>
You're right. I'm sorry and I misunderstood.
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 7:43 ` Chanwoo Choi
2013-07-24 7:51 ` Viresh Kumar
@ 2013-07-24 8:07 ` Viresh Kumar
2013-07-24 8:46 ` Chanwoo Choi
1 sibling, 1 reply; 20+ messages in thread
From: Viresh Kumar @ 2013-07-24 8:07 UTC (permalink / raw)
To: Chanwoo Choi
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
I just realized I missed answering few questions:
On 24 July 2013 13:13, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 07/24/2013 02:05 PM, Viresh Kumar wrote:
>> On 24 July 2013 06:55, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> On 07/22/2013 07:11 PM, Viresh Kumar wrote:
>>>> On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
>>>>> + unsigned int new_cpu)
>>>>> +{
>>>>> + struct dentry *old_entry, *new_entry;
>>>>> + char new_dir_name[CPUFREQ_NAME_LEN];
>>>>> + unsigned int j, old_cpu = policy->cpu;
>>>>> +
>>>>> + if (!policy->cpu_debugfs[new_cpu])
>>>>> + return;
>>>>> +
>>>>> + /*
>>>>> + * Remove symbolic link of debugfs directory except for debugfs
>>>>> + * directory of old_cpu.
>>>>> + */
>>>>> + for_each_present_cpu(j) {
>>>>> + if (old_cpu == j)
>>>>> + continue;
>>>>> +
>>>>> + debugfs_remove(policy->cpu_debugfs[j]);
>>>>
>>>> Why you need this? We aren't removing the earlier dentry at all here.
>>
>> haven't answered this.
>
> The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table)
> If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu,
> core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and
> child data for 'old_cpu' to debugfs directory for 'new_cpu'.
>
> If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file.
> So I didn't remove the earlier dentry of 'old_cpu'.
Okay.. The original question was: why do you need to remove & add
entries or links for cpus other than policy->cpu? Because we are renaming
the entry, wouldn't that work straight away?
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 8:07 ` Viresh Kumar
@ 2013-07-24 8:46 ` Chanwoo Choi
2013-07-24 8:51 ` Viresh Kumar
0 siblings, 1 reply; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-24 8:46 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 07/24/2013 05:07 PM, Viresh Kumar wrote:
> I just realized I missed answering few questions:
>
> On 24 July 2013 13:13, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> On 07/24/2013 02:05 PM, Viresh Kumar wrote:
>>> On 24 July 2013 06:55, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>> On 07/22/2013 07:11 PM, Viresh Kumar wrote:
>>>>> On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>
>>>>>> +static void cpufreq_move_debugfs_dir(struct cpufreq_policy *policy,
>>>>>> + unsigned int new_cpu)
>>>>>> +{
>>>>>> + struct dentry *old_entry, *new_entry;
>>>>>> + char new_dir_name[CPUFREQ_NAME_LEN];
>>>>>> + unsigned int j, old_cpu = policy->cpu;
>>>>>> +
>>>>>> + if (!policy->cpu_debugfs[new_cpu])
>>>>>> + return;
>>>>>> +
>>>>>> + /*
>>>>>> + * Remove symbolic link of debugfs directory except for debugfs
>>>>>> + * directory of old_cpu.
>>>>>> + */
>>>>>> + for_each_present_cpu(j) {
>>>>>> + if (old_cpu == j)
>>>>>> + continue;
>>>>>> +
>>>>>> + debugfs_remove(policy->cpu_debugfs[j]);
>>>>>
>>>>> Why you need this? We aren't removing the earlier dentry at all here.
>>>
>>> haven't answered this.
>>
>> The debugfs entry of 'old_cpu' include child debugfs file(e.g., load_table)
>> If cpu is last user of policy and core call __cpufre_remove_dev() to remove last cpu,
>> core call cpufreq_move_debugfs_dir(). I have to move the data of debugfs directory/file and
>> child data for 'old_cpu' to debugfs directory for 'new_cpu'.
>>
>> If I remove earlier dentry of 'old_cpu', I can't get the child debugfs dir/file.
>> So I didn't remove the earlier dentry of 'old_cpu'.
>
> Okay.. The original question was: why do you need to remove & add
> entries or links for cpus other than policy->cpu? Because we are renaming
> the entry, wouldn't that work straight away?
>
In case that all CPUs share same cpufreq policy. Each debugfs dentry of CPU[1-3]
except for CPU0 has symbolic link to CPU0's debugfs directory as following.
-sh-4.1# ls -al /sys/kernel/debug/cpufreq/
total 0
drwxr-xr-x 3 root root 0 Jan 1 09:00 .
drwx------ 28 root root 0 Jan 1 09:00 ..
drwxr-xr-x 2 root root 0 Jan 1 09:00 cpu0 (policy->cpu is 0)
lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu1 -> ./cpu0
lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu2 -> ./cpu0
lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu3 -> ./cpu0
If turn off CPU0 state, I have to move debugfs directory data from cpu0 to cpu1
and again create link to cpu1's debugfs directory for CPU[2-3] debugfs directory.
So, I removed dentry link of CPU[1-3] before creating link again.
cpu1
cpu2 -> ./cpu1
cpu3 -> ./cpu1
But I can rewrite new link of CPU[2-3] to previous dentry link(policy->cpu_debugfs[2] or policy->cpu_debugfs[3])
for reducing unnecessary code without revmoval sequence.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 8:46 ` Chanwoo Choi
@ 2013-07-24 8:51 ` Viresh Kumar
2013-07-24 9:05 ` Chanwoo Choi
0 siblings, 1 reply; 20+ messages in thread
From: Viresh Kumar @ 2013-07-24 8:51 UTC (permalink / raw)
To: Chanwoo Choi
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 24 July 2013 14:16, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> In case that all CPUs share same cpufreq policy. Each debugfs dentry of CPU[1-3]
> except for CPU0 has symbolic link to CPU0's debugfs directory as following.
>
> -sh-4.1# ls -al /sys/kernel/debug/cpufreq/
> total 0
> drwxr-xr-x 3 root root 0 Jan 1 09:00 .
> drwx------ 28 root root 0 Jan 1 09:00 ..
> drwxr-xr-x 2 root root 0 Jan 1 09:00 cpu0 (policy->cpu is 0)
> lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu1 -> ./cpu0
> lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu2 -> ./cpu0
> lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu3 -> ./cpu0
>
> If turn off CPU0 state, I have to move debugfs directory data from cpu0 to cpu1
> and again create link to cpu1's debugfs directory for CPU[2-3] debugfs directory.
> So, I removed dentry link of CPU[1-3] before creating link again.
>
> cpu1
> cpu2 -> ./cpu1
> cpu3 -> ./cpu1
>
> But I can rewrite new link of CPU[2-3] to previous dentry link(policy->cpu_debugfs[2] or policy->cpu_debugfs[3])
> for reducing unnecessary code without revmoval sequence.
Because we aren't freeing the debugfs node at all (just renaming
it), the links might still be good after renaming.. But please check
if it is true.
So, according to me you need to do this:
- Remove symlink for new policy->cpu, i..e cpu1 in your example
- rename debugfs entry to give it to cpu1 instead of cpu0.
- Set cpu0 pointer to NULL.
Probably that's it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 8:51 ` Viresh Kumar
@ 2013-07-24 9:05 ` Chanwoo Choi
2013-07-24 9:09 ` Viresh Kumar
0 siblings, 1 reply; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-24 9:05 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 07/24/2013 05:51 PM, Viresh Kumar wrote:
> On 24 July 2013 14:16, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> In case that all CPUs share same cpufreq policy. Each debugfs dentry of CPU[1-3]
>> except for CPU0 has symbolic link to CPU0's debugfs directory as following.
>>
>> -sh-4.1# ls -al /sys/kernel/debug/cpufreq/
>> total 0
>> drwxr-xr-x 3 root root 0 Jan 1 09:00 .
>> drwx------ 28 root root 0 Jan 1 09:00 ..
>> drwxr-xr-x 2 root root 0 Jan 1 09:00 cpu0 (policy->cpu is 0)
>> lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu1 -> ./cpu0
>> lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu2 -> ./cpu0
>> lrwxrwxrwx 1 root root 0 Jan 1 09:00 cpu3 -> ./cpu0
>>
>> If turn off CPU0 state, I have to move debugfs directory data from cpu0 to cpu1
>> and again create link to cpu1's debugfs directory for CPU[2-3] debugfs directory.
>> So, I removed dentry link of CPU[1-3] before creating link again.
>>
>> cpu1
>> cpu2 -> ./cpu1
>> cpu3 -> ./cpu1
>>
>> But I can rewrite new link of CPU[2-3] to previous dentry link(policy->cpu_debugfs[2] or policy->cpu_debugfs[3])
>> for reducing unnecessary code without revmoval sequence.
>
> Because we aren't freeing the debugfs node at all (just renaming
> it), the links might still be good after renaming.. But please check
> if it is true.
>
> So, according to me you need to do this:
> - Remove symlink for new policy->cpu, i..e cpu1 in your example
> - rename debugfs entry to give it to cpu1 instead of cpu0.
> - Set cpu0 pointer to NULL.
>
> Probably that's it.
>
And, I add additional step on below:
> - Remove symlink for new policy->cpu, i..e cpu1 in your example
> - rename debugfs entry to give it to cpu1 instead of cpu0.
- Store renamed cpu0 pointer to cpu1 pointer
- Create new link for CPU[2-3] to CPU1's debugfs directory
because debugfs use string path to create symbolic link.
It isn't automatically connected with new CPU1 debugfs directory.
> - Set cpu0 pointer to NULL.
>
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 9:05 ` Chanwoo Choi
@ 2013-07-24 9:09 ` Viresh Kumar
2013-07-24 9:14 ` Chanwoo Choi
0 siblings, 1 reply; 20+ messages in thread
From: Viresh Kumar @ 2013-07-24 9:09 UTC (permalink / raw)
To: Chanwoo Choi
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 24 July 2013 14:35, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> And, I add additional step on below:
>
>> - Remove symlink for new policy->cpu, i..e cpu1 in your example
>> - rename debugfs entry to give it to cpu1 instead of cpu0.
> - Store renamed cpu0 pointer to cpu1 pointer
> - Create new link for CPU[2-3] to CPU1's debugfs directory
> because debugfs use string path to create symbolic link.
> It isn't automatically connected with new CPU1 debugfs directory.
Honestly speaking I am not the best at debugfs core, but I still think
the link is connected to a struct debugfs node and not to any path..
This node is connected to a path though. And so even after rename
things should stay stable without your new step...
Just try and see if I am right or wrong.. Otherwise what you already
did is correct as you need to remove links for 2-3 as well..
>> - Set cpu0 pointer to NULL.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 9:09 ` Viresh Kumar
@ 2013-07-24 9:14 ` Chanwoo Choi
0 siblings, 0 replies; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-24 9:14 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On 07/24/2013 06:09 PM, Viresh Kumar wrote:
> On 24 July 2013 14:35, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> And, I add additional step on below:
>>
>>> - Remove symlink for new policy->cpu, i..e cpu1 in your example
>>> - rename debugfs entry to give it to cpu1 instead of cpu0.
>> - Store renamed cpu0 pointer to cpu1 pointer
>> - Create new link for CPU[2-3] to CPU1's debugfs directory
>> because debugfs use string path to create symbolic link.
>> It isn't automatically connected with new CPU1 debugfs directory.
>
> Honestly speaking I am not the best at debugfs core, but I still think
> the link is connected to a struct debugfs node and not to any path..
> This node is connected to a path though. And so even after rename
> things should stay stable without your new step...
>
> Just try and see if I am right or wrong.. Otherwise what you already
> did is correct as you need to remove links for 2-3 as well..
>
OK, I'll try it according to your suggestion. Thanks.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 1:25 ` Chanwoo Choi
2013-07-24 5:05 ` Viresh Kumar
@ 2013-07-24 6:14 ` Chanwoo Choi
2013-07-24 6:16 ` Viresh Kumar
1 sibling, 1 reply; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-24 6:14 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
Hi Viresh,
>>> + if (!policy->cpu_debugfs[idx]) {
>>> + pr_err("creating debugfs directory failed\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int cpufreq_create_debugfs_symlink(struct cpufreq_policy *policy,
>>> + unsigned int src_cpu,
>>> + unsigned int dest_cpu)
>>
>> Only use policy and cpu for which symlink has to be created as param
>> to this routine. And create link to policy->cpu.
>>
>
> OK, I'll simplify function prototype(cpufreq_create_debugfs_symlink) by removing
> unnecessary parameter.
>
I considered the parameter type of cpufreq_create_debugfs_symlink() and then
I need following function declaration because this function didn't always need to
create symbolic link to policy->cpu. This function declaration is capable of creating
symbolic link as 'dest_cpu -> src_cpu'
+static int cpufreq_create_debugfs_symlink(unsigned int src_cpu,
+ unsigned int dest_cpu)
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq
2013-07-24 6:14 ` Chanwoo Choi
@ 2013-07-24 6:16 ` Viresh Kumar
0 siblings, 0 replies; 20+ messages in thread
From: Viresh Kumar @ 2013-07-24 6:16 UTC (permalink / raw)
To: Chanwoo Choi
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park,
myungjoo.ham, Lists linaro-kernel
On Wed, Jul 24, 2013 at 11:44 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> I considered the parameter type of cpufreq_create_debugfs_symlink() and then
> I need following function declaration because this function didn't always need to
> create symbolic link to policy->cpu. This function declaration is capable of creating
> symbolic link as 'dest_cpu -> src_cpu'
>
> +static int cpufreq_create_debugfs_symlink(unsigned int src_cpu,
> + unsigned int dest_cpu)
You need policy structure as well, right? And src_cpu is always going to
be policy->cpu. So you can have policy and cpu as two parameters.. cpu
is dest cpu btw.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/3 v6] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs
2013-07-18 11:17 [PATCH 0/3 v6] cpufreq: Add 'load_table' debugfs file to show colleced CPUs load Chanwoo Choi
2013-07-18 11:17 ` [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq Chanwoo Choi
@ 2013-07-18 11:17 ` Chanwoo Choi
2013-07-22 11:05 ` Viresh Kumar
2013-07-18 11:17 ` [PATCH 3/3 v6] Documentation: cpufreq: load_table: Update load_table debugfs file documentation Chanwoo Choi
2 siblings, 1 reply; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-18 11:17 UTC (permalink / raw)
To: viresh.kumar, rjw, linux-kernel
Cc: linux-pm, cpufreq, kyungmin.park, myungjoo.ham, Chanwoo Choi
This patch add new 'load_table' debugfs file to show previous accumulated data
of CPUs load as following path and add CPUFREQ_LOADCHECK notification to
CPUFREQ_TRANSITION_NOTIFIER notifier chain.
- /sys/kernel/debug/cpufreq/cpuX/load_table
When governor calculates CPUs load on dbs_check_cpu(), governor send
CPUFREQ_LOADCHECK notification with CPUs load, so that cpufreq_stats
accumulates calculated CPUs load on 'load_table' storage.
This debugfs file is used to judge the correct system state or determine
suitable system resource according to current CPUs load on user-space.
This debugfs file include following data:
- Measurement point of time
- CPU frequency
- Per-CPU load
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
---
Changes since v5:
- Determine index value of policy->cpu_debugfs[] according to
cpumask_weight(policy->cpus) value
- Bug fix, store 'policy->cpu' to 'freq->cpu' before notify
CPUFREQ_LOADCHECK notification
Changes since v4:
- Reset the data of CPUs load when cpufreq governor is changed
- Move code about creating debugfs directory to below first patch
: [PATCH 1/2] cpufreq: Add debugfs directory for cpufreq
Changes since v3:
- Extend a range of accumulated data (10 ~ 1000)
- Add unit information of time/freq and align 'Time' field as left for readability
- Use CONFIG_CPU_FREQ_STAT depdendency instead of CONFIG_CPU_FREQ_STAT_DETATILS
- Initialize load of offline CPUx as zero(0)
- Create/remove debugfs root directory on cpufreq_stats_init/exit() because
debugfs root is used on all CPUs.
Changes since v2:
- Code clean according to Viresh Kumar's comment
- Show both old frequency and new frequency on 'load_table' debugfs file
- Change debufs file patch as below
old: /sys/kernel/debugfs/cpufreq/load_table
new: /sys/kernel/debugfs/cpufreq/cpuX/load_table
Changes since v1:
- Set maximum storage size to save CPUs load on Kconfig
- Use spinlock to synchronize read/write operation for CPUs load
- Use local variable instead of global variable(struct cpufreq_freqs *freqs)
- Use pointer of data structure to get correct size of data structure
in sizeof() macro instead of structure name
: sizeof(struct cpufreq_freqs) -> sizeof(*stat->load_table)
- Change time unit from nanosecond to microsecond
- Remove unnecessary memory copy
drivers/cpufreq/Kconfig | 6 +
drivers/cpufreq/cpufreq.c | 4 +
drivers/cpufreq/cpufreq_governor.c | 14 ++
drivers/cpufreq/cpufreq_stats.c | 258 +++++++++++++++++++++++++++++++++----
include/linux/cpufreq.h | 6 +
5 files changed, 264 insertions(+), 24 deletions(-)
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 534fcb8..5c3f406 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -36,6 +36,12 @@ config CPU_FREQ_STAT
If in doubt, say N.
+config NR_CPU_LOAD_STORAGE
+ int "Maximum storage size to save CPU load (10-1000)"
+ range 10 1000
+ depends on CPU_FREQ_STAT
+ default "10"
+
config CPU_FREQ_STAT_DETAILS
bool "CPU frequency translation statistics details"
depends on CPU_FREQ_STAT
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 924d654..cb68873 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -293,6 +293,10 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
if (likely(policy) && likely(policy->cpu == freqs->cpu))
policy->cur = freqs->new;
break;
+ case CPUFREQ_LOADCHECK:
+ srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
+ CPUFREQ_LOADCHECK, freqs);
+ break;
}
}
/**
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index dc9b72e..decabcb 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -87,6 +87,9 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
struct od_dbs_tuners *od_tuners = dbs_data->tuners;
struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
struct cpufreq_policy *policy;
+#ifdef CONFIG_CPU_FREQ_STAT
+ struct cpufreq_freqs freq = {0};
+#endif
unsigned int max_load = 0;
unsigned int ignore_nice;
unsigned int j;
@@ -148,6 +151,9 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
continue;
load = 100 * (wall_time - idle_time) / wall_time;
+#ifdef CONFIG_CPU_FREQ_STAT
+ freq.load[j] = load;
+#endif
if (dbs_data->cdata->governor == GOV_ONDEMAND) {
int freq_avg = __cpufreq_driver_getavg(policy, j);
@@ -161,6 +167,14 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
max_load = load;
}
+#ifdef CONFIG_CPU_FREQ_STAT
+ freq.time = ktime_to_ms(ktime_get());
+ freq.old = policy->cur;
+ freq.cpu = policy->cpu;
+
+ cpufreq_notify_transition(policy, &freq, CPUFREQ_LOADCHECK);
+#endif
+
dbs_data->cdata->gov_check_cpu(cpu, max_load);
}
EXPORT_SYMBOL_GPL(dbs_check_cpu);
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index fb65dec..fd171ba 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -12,6 +12,7 @@
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/cpu.h>
+#include <linux/debugfs.h>
#include <linux/sysfs.h>
#include <linux/cpufreq.h>
#include <linux/module.h>
@@ -36,6 +37,12 @@ struct cpufreq_stats {
#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
unsigned int *trans_table;
#endif
+
+ /* Debugfs file for load_table */
+ struct cpufreq_freqs *load_table;
+ unsigned int load_last_index;
+ unsigned int load_max_index;
+ struct dentry *debugfs_load_table;
};
static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);
@@ -149,6 +156,181 @@ static struct attribute_group stats_attr_group = {
.name = "stats"
};
+#define MAX_LINE_SIZE 255
+static ssize_t load_table_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct cpufreq_policy *policy = file->private_data;
+ struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
+ struct cpufreq_freqs *load_table = stat->load_table;
+ ssize_t len = 0;
+ char *buf;
+ int i, cpu, ret;
+
+ buf = kzalloc(MAX_LINE_SIZE * stat->load_max_index, GFP_KERNEL);
+ if (!buf)
+ return 0;
+
+ spin_lock(&cpufreq_stats_lock);
+ len += sprintf(buf + len, "%-10s %-12s %-12s ", "Time(ms)",
+ "Old Freq(Hz)",
+ "New Freq(Hz)");
+ for_each_cpu(cpu, policy->cpus)
+ len += sprintf(buf + len, "%3s%d ", "CPU", cpu);
+ len += sprintf(buf + len, "\n");
+
+ i = stat->load_last_index;
+ do {
+ len += sprintf(buf + len, "%-10lld %-12d %-12d ",
+ load_table[i].time,
+ load_table[i].old,
+ load_table[i].new);
+
+ for_each_cpu(cpu, policy->cpus)
+ len += sprintf(buf + len, "%-4d ",
+ load_table[i].load[cpu]);
+ len += sprintf(buf + len, "\n");
+
+ if (++i == stat->load_max_index)
+ i = 0;
+ } while (i != stat->load_last_index);
+ spin_unlock(&cpufreq_stats_lock);
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, len);
+ kfree(buf);
+
+ return ret;
+}
+
+static const struct file_operations load_table_fops = {
+ .read = load_table_read,
+ .open = simple_open,
+ .llseek = no_llseek,
+};
+
+static int cpufreq_stats_reset_debugfs(struct cpufreq_policy *policy)
+{
+ struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
+ int size;
+
+ if (!stat)
+ return -EINVAL;
+
+ if (stat->load_table)
+ kfree(stat->load_table);
+ stat->load_last_index = 0;
+
+ size = sizeof(*stat->load_table) * stat->load_max_index;
+ stat->load_table = kzalloc(size, GFP_KERNEL);
+ if (!stat->load_table)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
+{
+ struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
+ unsigned int idx, size;
+ int ret = 0;
+
+ if (!stat)
+ return -EINVAL;
+
+ if (!policy->cpu_debugfs)
+ return -EINVAL;
+
+ stat->load_last_index = 0;
+ stat->load_max_index = CONFIG_NR_CPU_LOAD_STORAGE;
+
+ /* Allocate memory for storage of CPUs load */
+ size = sizeof(*stat->load_table) * stat->load_max_index;
+ stat->load_table = kzalloc(size, GFP_KERNEL);
+ if (!stat->load_table)
+ return -ENOMEM;
+
+ /* Create debugfs directory and file for cpufreq */
+ idx = cpumask_weight(policy->cpus) > 1 ? policy->cpu : 0;
+ stat->debugfs_load_table = debugfs_create_file("load_table", S_IWUSR,
+ policy->cpu_debugfs[idx],
+ policy, &load_table_fops);
+ if (!stat->debugfs_load_table) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ pr_debug("Creating debugfs file for CPU%d \n", policy->cpu);
+
+ return 0;
+err:
+ kfree(stat->load_table);
+ return ret;
+}
+
+/* should be called late in the CPU removal sequence so that the stats
+ * memory is still available in case someone tries to use it.
+ */
+static void cpufreq_stats_free_load_table(unsigned int cpu)
+{
+ struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
+
+ if (stat) {
+ pr_debug("Free memory of load_table\n");
+ kfree(stat->load_table);
+ }
+}
+
+/* must be called early in the CPU removal sequence (before
+ * cpufreq_remove_dev) so that policy is still valid.
+ */
+static void cpufreq_stats_free_debugfs(unsigned int cpu)
+{
+ struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
+
+ if (stat) {
+ pr_debug("Remove load_table debugfs file\n");
+ debugfs_remove(stat->debugfs_load_table);
+ }
+}
+
+static void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq,
+ unsigned long val)
+{
+ struct cpufreq_stats *stat;
+ int cpu, last_idx;
+
+ stat = per_cpu(cpufreq_stats_table, freq->cpu);
+ if (!stat)
+ return;
+
+ spin_lock(&cpufreq_stats_lock);
+
+ switch (val) {
+ case CPUFREQ_POSTCHANGE:
+ if (!stat->load_last_index)
+ last_idx = stat->load_max_index;
+ else
+ last_idx = stat->load_last_index - 1;
+
+ stat->load_table[last_idx].new = freq->new;
+ break;
+ case CPUFREQ_LOADCHECK:
+ last_idx = stat->load_last_index;
+
+ stat->load_table[last_idx].time = freq->time;
+ stat->load_table[last_idx].old = freq->old;
+ stat->load_table[last_idx].new = freq->old;
+ for_each_present_cpu(cpu)
+ stat->load_table[last_idx].load[cpu] = freq->load[cpu];
+
+ if (++stat->load_last_index == stat->load_max_index)
+ stat->load_last_index = 0;
+ break;
+ }
+
+ spin_unlock(&cpufreq_stats_lock);
+}
+
static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
{
int index;
@@ -204,7 +386,7 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy,
unsigned int alloc_size;
unsigned int cpu = policy->cpu;
if (per_cpu(cpufreq_stats_table, cpu))
- return -EBUSY;
+ return 0;
stat = kzalloc(sizeof(struct cpufreq_stats), GFP_KERNEL);
if ((stat) == NULL)
return -ENOMEM;
@@ -257,6 +439,14 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy,
spin_lock(&cpufreq_stats_lock);
stat->last_time = get_jiffies_64();
stat->last_index = freq_table_get_index(stat, policy->cur);
+
+ ret = cpufreq_stats_create_debugfs(data);
+ if (ret < 0) {
+ spin_unlock(&cpufreq_stats_lock);
+ ret = -EINVAL;
+ goto error_out;
+ }
+
spin_unlock(&cpufreq_stats_lock);
cpufreq_cpu_put(data);
return 0;
@@ -297,11 +487,18 @@ static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
if (val != CPUFREQ_NOTIFY)
return 0;
table = cpufreq_frequency_get_table(cpu);
- if (!table)
- return 0;
- ret = cpufreq_stats_create_table(policy, table);
- if (ret)
+ if (table) {
+ ret = cpufreq_stats_create_table(policy, table);
+ if (ret)
+ return ret;
+ }
+
+ ret = cpufreq_stats_reset_debugfs(policy);
+ if (ret < 0) {
+ pr_debug("Failed to reset CPUs data of debugfs\n");
return ret;
+ }
+
return 0;
}
@@ -312,32 +509,40 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
struct cpufreq_stats *stat;
int old_index, new_index;
- if (val != CPUFREQ_POSTCHANGE)
- return 0;
+ switch (val) {
+ case CPUFREQ_POSTCHANGE:
+ stat = per_cpu(cpufreq_stats_table, freq->cpu);
+ if (!stat)
+ return 0;
- stat = per_cpu(cpufreq_stats_table, freq->cpu);
- if (!stat)
- return 0;
+ old_index = stat->last_index;
+ new_index = freq_table_get_index(stat, freq->new);
- old_index = stat->last_index;
- new_index = freq_table_get_index(stat, freq->new);
+ /* We can't do stat->time_in_state[-1]= .. */
+ if (old_index == -1 || new_index == -1)
+ return 0;
- /* We can't do stat->time_in_state[-1]= .. */
- if (old_index == -1 || new_index == -1)
- return 0;
+ cpufreq_stats_update(freq->cpu);
- cpufreq_stats_update(freq->cpu);
+ if (old_index == new_index)
+ return 0;
- if (old_index == new_index)
- return 0;
-
- spin_lock(&cpufreq_stats_lock);
- stat->last_index = new_index;
+ spin_lock(&cpufreq_stats_lock);
+ stat->last_index = new_index;
#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
- stat->trans_table[old_index * stat->max_state + new_index]++;
+ stat->trans_table[old_index * stat->max_state + new_index]++;
#endif
- stat->total_trans++;
- spin_unlock(&cpufreq_stats_lock);
+ stat->total_trans++;
+ spin_unlock(&cpufreq_stats_lock);
+
+ cpufreq_stats_store_load_table(freq, CPUFREQ_POSTCHANGE);
+
+ break;
+ case CPUFREQ_LOADCHECK:
+ cpufreq_stats_store_load_table(freq, CPUFREQ_LOADCHECK);
+ break;
+ }
+
return 0;
}
@@ -352,12 +557,16 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
cpufreq_update_policy(cpu);
break;
case CPU_DOWN_PREPARE:
+ cpufreq_stats_free_debugfs(cpu);
cpufreq_stats_free_sysfs(cpu);
break;
case CPU_DEAD:
+ cpufreq_stats_free_load_table(cpu);
cpufreq_stats_free_table(cpu);
break;
case CPU_UP_CANCELED_FROZEN:
+ cpufreq_stats_free_debugfs(cpu);
+ cpufreq_stats_free_load_table(cpu);
cpufreq_stats_free_sysfs(cpu);
cpufreq_stats_free_table(cpu);
break;
@@ -418,6 +627,7 @@ static void __exit cpufreq_stats_exit(void)
unregister_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
for_each_online_cpu(cpu) {
cpufreq_stats_free_table(cpu);
+ cpufreq_stats_free_debugfs(cpu);
cpufreq_stats_free_sysfs(cpu);
}
}
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 825f379..4d0a4fd 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -141,12 +141,18 @@ static inline bool policy_is_shared(struct cpufreq_policy *policy)
#define CPUFREQ_POSTCHANGE (1)
#define CPUFREQ_RESUMECHANGE (8)
#define CPUFREQ_SUSPENDCHANGE (9)
+#define CPUFREQ_LOADCHECK (10)
struct cpufreq_freqs {
unsigned int cpu; /* cpu nr */
unsigned int old;
unsigned int new;
u8 flags; /* flags of cpufreq_driver, see below. */
+
+#ifdef CONFIG_CPU_FREQ_STAT
+ int64_t time;
+ unsigned int load[NR_CPUS];
+#endif
};
--
1.8.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/3 v6] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs
2013-07-18 11:17 ` [PATCH 2/3 v6] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs Chanwoo Choi
@ 2013-07-22 11:05 ` Viresh Kumar
2013-07-24 1:56 ` Chanwoo Choi
0 siblings, 1 reply; 20+ messages in thread
From: Viresh Kumar @ 2013-07-22 11:05 UTC (permalink / raw)
To: Chanwoo Choi
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park, myungjoo.ham
On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> +static int cpufreq_stats_reset_debugfs(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
> + int size;
> +
> + if (!stat)
> + return -EINVAL;
> +
> + if (stat->load_table)
> + kfree(stat->load_table);
> + stat->load_last_index = 0;
> +
> + size = sizeof(*stat->load_table) * stat->load_max_index;
> + stat->load_table = kzalloc(size, GFP_KERNEL);
> + if (!stat->load_table)
> + return -ENOMEM;
Why are you freeing memory and allocating it again ??
> + return 0;
> +}
> +
> +static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
> + unsigned int idx, size;
> + int ret = 0;
> +
> + if (!stat)
> + return -EINVAL;
> +
> + if (!policy->cpu_debugfs)
> + return -EINVAL;
> +
> + stat->load_last_index = 0;
> + stat->load_max_index = CONFIG_NR_CPU_LOAD_STORAGE;
> +
> + /* Allocate memory for storage of CPUs load */
> + size = sizeof(*stat->load_table) * stat->load_max_index;
> + stat->load_table = kzalloc(size, GFP_KERNEL);
> + if (!stat->load_table)
> + return -ENOMEM;
> +
> + /* Create debugfs directory and file for cpufreq */
> + idx = cpumask_weight(policy->cpus) > 1 ? policy->cpu : 0;
idx is broken again..
> + stat->debugfs_load_table = debugfs_create_file("load_table", S_IWUSR,
> + policy->cpu_debugfs[idx],
> + policy, &load_table_fops);
> + if (!stat->debugfs_load_table) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + pr_debug("Creating debugfs file for CPU%d \n", policy->cpu);
s/Creating/Created
> +
> + return 0;
> +err:
> + kfree(stat->load_table);
> + return ret;
> +}
> +
> +/* should be called late in the CPU removal sequence so that the stats
> + * memory is still available in case someone tries to use it.
> + */
Please write multiline comment correctly..
> +static void cpufreq_stats_free_load_table(unsigned int cpu)
> +{
> + struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
> +
> + if (stat) {
> + pr_debug("Free memory of load_table\n");
> + kfree(stat->load_table);
> + }
> +}
> +
> +/* must be called early in the CPU removal sequence (before
> + * cpufreq_remove_dev) so that policy is still valid.
> + */
> +static void cpufreq_stats_free_debugfs(unsigned int cpu)
> +{
> + struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
> +
> + if (stat) {
> + pr_debug("Remove load_table debugfs file\n");
> + debugfs_remove(stat->debugfs_load_table);
> + }
> +}
> +
> +static void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq,
> + unsigned long val)
> +{
> + struct cpufreq_stats *stat;
> + int cpu, last_idx;
> +
> + stat = per_cpu(cpufreq_stats_table, freq->cpu);
> + if (!stat)
> + return;
> +
> + spin_lock(&cpufreq_stats_lock);
> +
> + switch (val) {
> + case CPUFREQ_POSTCHANGE:
> + if (!stat->load_last_index)
> + last_idx = stat->load_max_index;
> + else
> + last_idx = stat->load_last_index - 1;
> +
> + stat->load_table[last_idx].new = freq->new;
> + break;
> + case CPUFREQ_LOADCHECK:
> + last_idx = stat->load_last_index;
> +
> + stat->load_table[last_idx].time = freq->time;
> + stat->load_table[last_idx].old = freq->old;
> + stat->load_table[last_idx].new = freq->old;
> + for_each_present_cpu(cpu)
> + stat->load_table[last_idx].load[cpu] = freq->load[cpu];
> +
> + if (++stat->load_last_index == stat->load_max_index)
> + stat->load_last_index = 0;
> + break;
> + }
> +
> + spin_unlock(&cpufreq_stats_lock);
> +}
> +
> static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
> {
> int index;
> @@ -204,7 +386,7 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy,
> unsigned int alloc_size;
> unsigned int cpu = policy->cpu;
> if (per_cpu(cpufreq_stats_table, cpu))
> - return -EBUSY;
> + return 0;
> stat = kzalloc(sizeof(struct cpufreq_stats), GFP_KERNEL);
> if ((stat) == NULL)
> return -ENOMEM;
> @@ -257,6 +439,14 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy,
> spin_lock(&cpufreq_stats_lock);
> stat->last_time = get_jiffies_64();
> stat->last_index = freq_table_get_index(stat, policy->cur);
> +
> + ret = cpufreq_stats_create_debugfs(data);
> + if (ret < 0) {
> + spin_unlock(&cpufreq_stats_lock);
> + ret = -EINVAL;
> + goto error_out;
> + }
> +
> spin_unlock(&cpufreq_stats_lock);
> cpufreq_cpu_put(data);
> return 0;
> @@ -297,11 +487,18 @@ static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
> if (val != CPUFREQ_NOTIFY)
> return 0;
> table = cpufreq_frequency_get_table(cpu);
> - if (!table)
> - return 0;
> - ret = cpufreq_stats_create_table(policy, table);
> - if (ret)
> + if (table) {
> + ret = cpufreq_stats_create_table(policy, table);
> + if (ret)
> + return ret;
> + }
> +
> + ret = cpufreq_stats_reset_debugfs(policy);
Why?
> + if (ret < 0) {
> + pr_debug("Failed to reset CPUs data of debugfs\n");
> return ret;
> + }
> +
> return 0;
> }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/3 v6] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs
2013-07-22 11:05 ` Viresh Kumar
@ 2013-07-24 1:56 ` Chanwoo Choi
0 siblings, 0 replies; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-24 1:56 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, linux-kernel, linux-pm, cpufreq, kyungmin.park, myungjoo.ham
Hi Viresh,
On 07/22/2013 08:05 PM, Viresh Kumar wrote:
> On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
>
>> +static int cpufreq_stats_reset_debugfs(struct cpufreq_policy *policy)
>> +{
>> + struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
>> + int size;
>> +
>> + if (!stat)
>> + return -EINVAL;
>> +
>> + if (stat->load_table)
>> + kfree(stat->load_table);
>> + stat->load_last_index = 0;
>> +
>> + size = sizeof(*stat->load_table) * stat->load_max_index;
>> + stat->load_table = kzalloc(size, GFP_KERNEL);
>> + if (!stat->load_table)
>> + return -ENOMEM;
>
> Why are you freeing memory and allocating it again ??
This purpose is reseting the data of stat->load_table.
If you don't agree this, I'll initizliae stat->load_table array as zero(0) with loop statement.
>
>> + return 0;
>> +}
>> +
>> +static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
>> +{
>> + struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
>> + unsigned int idx, size;
>> + int ret = 0;
>> +
>> + if (!stat)
>> + return -EINVAL;
>> +
>> + if (!policy->cpu_debugfs)
>> + return -EINVAL;
>> +
>> + stat->load_last_index = 0;
>> + stat->load_max_index = CONFIG_NR_CPU_LOAD_STORAGE;
>> +
>> + /* Allocate memory for storage of CPUs load */
>> + size = sizeof(*stat->load_table) * stat->load_max_index;
>> + stat->load_table = kzalloc(size, GFP_KERNEL);
>> + if (!stat->load_table)
>> + return -ENOMEM;
>> +
>> + /* Create debugfs directory and file for cpufreq */
>> + idx = cpumask_weight(policy->cpus) > 1 ? policy->cpu : 0;
>
> idx is broken again..
OK, I'll fix it.
>
>> + stat->debugfs_load_table = debugfs_create_file("load_table", S_IWUSR,
>> + policy->cpu_debugfs[idx],
>> + policy, &load_table_fops);
>> + if (!stat->debugfs_load_table) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + pr_debug("Creating debugfs file for CPU%d \n", policy->cpu);
>
> s/Creating/Created
OK, I'll fixt it.
>
>> +
>> + return 0;
>> +err:
>> + kfree(stat->load_table);
>> + return ret;
>> +}
>> +
>> +/* should be called late in the CPU removal sequence so that the stats
>> + * memory is still available in case someone tries to use it.
>> + */
>
> Please write multiline comment correctly..
OK.
>
>> +static void cpufreq_stats_free_load_table(unsigned int cpu)
>> +{
>> + struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
>> +
>> + if (stat) {
>> + pr_debug("Free memory of load_table\n");
>> + kfree(stat->load_table);
>> + }
>> +}
>> +
>> +/* must be called early in the CPU removal sequence (before
>> + * cpufreq_remove_dev) so that policy is still valid.
>> + */
>> +static void cpufreq_stats_free_debugfs(unsigned int cpu)
>> +{
>> + struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
>> +
>> + if (stat) {
>> + pr_debug("Remove load_table debugfs file\n");
>> + debugfs_remove(stat->debugfs_load_table);
>> + }
>> +}
>> +
>> +static void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq,
>> + unsigned long val)
>> +{
>> + struct cpufreq_stats *stat;
>> + int cpu, last_idx;
>> +
>> + stat = per_cpu(cpufreq_stats_table, freq->cpu);
>> + if (!stat)
>> + return;
>> +
>> + spin_lock(&cpufreq_stats_lock);
>> +
>> + switch (val) {
>> + case CPUFREQ_POSTCHANGE:
>> + if (!stat->load_last_index)
>> + last_idx = stat->load_max_index;
>> + else
>> + last_idx = stat->load_last_index - 1;
>> +
>> + stat->load_table[last_idx].new = freq->new;
>> + break;
>> + case CPUFREQ_LOADCHECK:
>> + last_idx = stat->load_last_index;
>> +
>> + stat->load_table[last_idx].time = freq->time;
>> + stat->load_table[last_idx].old = freq->old;
>> + stat->load_table[last_idx].new = freq->old;
>> + for_each_present_cpu(cpu)
>> + stat->load_table[last_idx].load[cpu] = freq->load[cpu];
>> +
>> + if (++stat->load_last_index == stat->load_max_index)
>> + stat->load_last_index = 0;
>> + break;
>> + }
>> +
>> + spin_unlock(&cpufreq_stats_lock);
>> +}
>> +
>> static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
>> {
>> int index;
>> @@ -204,7 +386,7 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy,
>> unsigned int alloc_size;
>> unsigned int cpu = policy->cpu;
>> if (per_cpu(cpufreq_stats_table, cpu))
>> - return -EBUSY;
>> + return 0;
>> stat = kzalloc(sizeof(struct cpufreq_stats), GFP_KERNEL);
>> if ((stat) == NULL)
>> return -ENOMEM;
>> @@ -257,6 +439,14 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy,
>> spin_lock(&cpufreq_stats_lock);
>> stat->last_time = get_jiffies_64();
>> stat->last_index = freq_table_get_index(stat, policy->cur);
>> +
>> + ret = cpufreq_stats_create_debugfs(data);
>> + if (ret < 0) {
>> + spin_unlock(&cpufreq_stats_lock);
>> + ret = -EINVAL;
>> + goto error_out;
>> + }
>> +
>> spin_unlock(&cpufreq_stats_lock);
>> cpufreq_cpu_put(data);
>> return 0;
>> @@ -297,11 +487,18 @@ static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
>> if (val != CPUFREQ_NOTIFY)
>> return 0;
>> table = cpufreq_frequency_get_table(cpu);
>> - if (!table)
>> - return 0;
>> - ret = cpufreq_stats_create_table(policy, table);
>> - if (ret)
>> + if (table) {
>> + ret = cpufreq_stats_create_table(policy, table);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + ret = cpufreq_stats_reset_debugfs(policy);
>
> Why?
When cpufreq governor is changed, always reset stats->load_table for performance/powersave/userspace governor.
-sh-4.1# cat /sys/kernel/debug/cpufreq/cpu0/load_table
Time(ms) Old Freq(Hz) New Freq(Hz) CPU0 CPU1 CPU3
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
>
>> + if (ret < 0) {
>> + pr_debug("Failed to reset CPUs data of debugfs\n");
>> return ret;
>> + }
>> +
>> return 0;
>> }
>
Thanks for your comment.
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/3 v6] Documentation: cpufreq: load_table: Update load_table debugfs file documentation
2013-07-18 11:17 [PATCH 0/3 v6] cpufreq: Add 'load_table' debugfs file to show colleced CPUs load Chanwoo Choi
2013-07-18 11:17 ` [PATCH 1/3 v6] cpufreq: Add debugfs directory for cpufreq Chanwoo Choi
2013-07-18 11:17 ` [PATCH 2/3 v6] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs Chanwoo Choi
@ 2013-07-18 11:17 ` Chanwoo Choi
2 siblings, 0 replies; 20+ messages in thread
From: Chanwoo Choi @ 2013-07-18 11:17 UTC (permalink / raw)
To: viresh.kumar, rjw, linux-kernel
Cc: linux-pm, cpufreq, kyungmin.park, myungjoo.ham, Chanwoo Choi
This patch add the detailed description of 'load_table' debugfs file to show
collected CPUs load and the change of CPU frequency.
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
---
Changes since v5:
- Add description of test case
Documentation/cpu-freq/cpufreq-stats.txt | 40 ++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/Documentation/cpu-freq/cpufreq-stats.txt b/Documentation/cpu-freq/cpufreq-stats.txt
index fc64749..f1d97d03 100644
--- a/Documentation/cpu-freq/cpufreq-stats.txt
+++ b/Documentation/cpu-freq/cpufreq-stats.txt
@@ -18,10 +18,12 @@ Contents
1. Introduction
cpufreq-stats is a driver that provides CPU frequency statistics for each CPU.
-These statistics are provided in /sysfs as a bunch of read_only interfaces. This
-interface (when configured) will appear in a separate directory under cpufreq
-in /sysfs (<sysfs root>/devices/system/cpu/cpuX/cpufreq/stats/) for each CPU.
-Various statistics will form read_only files under this directory.
+These statistics are provided in /sysfs and /debugfs as a bunch of read_only
+interfaces. This interface (when configured) will appear in a separate directory
+under cpufreq in below directory list for each CPU. Various statistics will form
+read_only files under this directory.
+- /sysfs (<sysfs root>/devices/system/cpu/cpuX/cpufreq/stats/)
+- /debugfs (<sysfs root>/kernel/debug/cpufreq/cpuX/)
This driver is designed to be independent of any particular cpufreq_driver
that may be running on your CPU. So, it will work with any cpufreq_driver.
@@ -33,6 +35,7 @@ cpufreq stats provides following statistics (explained in detail below).
- time_in_state
- total_trans
- trans_table
+- load_table
All the statistics will be from the time the stats driver has been inserted
to the time when a read of a particular statistic is done. Obviously, stats
@@ -95,6 +98,29 @@ contains the actual freq values for each row and column for better readability.
2800000: 0 0 0 2 0
--------------------------------------------------------------------------------
+- load_table
+This gives the collected CPUs data which include measured time, old CPU
+frequency, new CPU frequency and each CPU load. The cat output will have
+"<measured time> <old CPU frequency> <new CPU frequency> <CPUs load> ..." pair
+in each line, which will mean the change of CPU frequency according to CPUs load
+at <measured time>.
+
+--------------------------------------------------------------------------------
+- In case that all CPUs share only one voltage/frequency.
+<mysystem>:/sys/kernel/debug/cpufreq/cpu0 # cat load_table
+Time(ms) Old Freq(Hz) New Freq(Hz) CPU0 CPU1 CPU2 CPU3
+23820 500000 500000 53 86 2 73
+23920 500000 400000 66 40 0 42
+24020 400000 400000 71 71 10 52
+24120 400000 300000 33 27 45 65
+24220 300000 300000 4 37 71 34
+24320 300000 300000 1 85 38 16
+24420 300000 200000 6 41 15 51
+24520 200000 200000 12 62 1 51
+24620 200000 200000 9 51 0 58
+24720 200000 200000 32 32 11 27
+--------------------------------------------------------------------------------
+
3. Configuring cpufreq-stats
@@ -104,6 +130,7 @@ Config Main Menu
CPU Frequency scaling --->
[*] CPU Frequency scaling
<*> CPU frequency translation statistics
+ (10) Maximum storage size to save CPU load (10-1000)
[*] CPU frequency translation statistics details
@@ -113,6 +140,11 @@ cpufreq-stats.
"CPU frequency translation statistics" (CONFIG_CPU_FREQ_STAT) provides the
basic statistics which includes time_in_state and total_trans.
+"Maximum storage size to save CPU load (10-1000)" (depends on CONFIG_CPU_FREQ_
+STAT) indicates the total number of load_table data and provides collected data
+which include old cpu frequency, new cpu frequency and CPUs load. The user can
+determine the storage size of collected CPUs data. The range is from 10 to 1000.
+
"CPU frequency translation statistics details" (CONFIG_CPU_FREQ_STAT_DETAILS)
provides fine grained cpufreq stats by trans_table. The reason for having a
separate config option for trans_table is:
--
1.8.0
^ permalink raw reply related [flat|nested] 20+ messages in thread