[PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[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/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 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 |  19 +++-
 drivers/cpufreq/cpufreq_stats.c    | 192 +++++++++++++++++++++++++++++++++----
 include/linux/cpufreq.h            |   6 ++
 5 files changed, 206 insertions(+), 21 deletions(-)

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 534fcb8..8a429b3 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -45,6 +45,12 @@ config CPU_FREQ_STAT_DETAILS
 
 	  If in doubt, say N.
 
+config NR_CPU_LOAD_STORAGE
+	int "Maximum storage size to save CPU load (10-100)"
+	range 10 100
+	depends on CPU_FREQ_STAT_DETAILS
+	default "10"
+
 choice
 	prompt "Default CPUFreq governor"
 	default CPU_FREQ_DEFAULT_GOV_USERSPACE if ARM_SA1100_CPUFREQ || ARM_SA1110_CPUFREQ
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2d53f47..cbaaff0 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -292,6 +292,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..bca341b 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -90,6 +90,9 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 	unsigned int max_load = 0;
 	unsigned int ignore_nice;
 	unsigned int j;
+#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
+	struct cpufreq_freqs freq;
+#endif
 
 	if (dbs_data->cdata->governor == GOV_ONDEMAND)
 		ignore_nice = od_tuners->ignore_nice;
@@ -144,11 +147,17 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 			idle_time += jiffies_to_usecs(cur_nice_jiffies);
 		}
 
-		if (unlikely(!wall_time || wall_time < idle_time))
+		if (unlikely(!wall_time || wall_time < idle_time)) {
+#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
+			freq.load[j] = 0;
+#endif
 			continue;
+		}
 
 		load = 100 * (wall_time - idle_time) / wall_time;
-
+#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
+		freq.load[j] = load;
+#endif
 		if (dbs_data->cdata->governor == GOV_ONDEMAND) {
 			int freq_avg = __cpufreq_driver_getavg(policy, j);
 			if (freq_avg <= 0)
@@ -161,6 +170,12 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 			max_load = load;
 	}
 
+#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
+	freq.time = ktime_to_ms(ktime_get());
+	freq.old = freq.new = policy->cur;
+
+	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..289d675 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>
@@ -23,6 +24,7 @@
 #include <asm/cputime.h>
 
 static spinlock_t cpufreq_stats_lock;
+static spinlock_t cpufreq_stats_lock_load_table;
 
 struct cpufreq_stats {
 	unsigned int cpu;
@@ -35,6 +37,12 @@ struct cpufreq_stats {
 	unsigned int *freq_table;
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	unsigned int *trans_table;
+
+	/* debugfs file for load_table */
+	struct cpufreq_freqs *load_table;
+	unsigned int load_last_index;
+	unsigned int load_max_index;
+	struct dentry *debugfs_cpufreq;
 #endif
 };
 
@@ -149,6 +157,134 @@ static struct attribute_group stats_attr_group = {
 	.name = "stats"
 };
 
+#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
+#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;
+	unsigned int alloc_size_buf;
+	ssize_t len = 0;
+	char *buf;
+	int i, j, ret;
+
+	alloc_size_buf = MAX_LINE_SIZE * stat->load_max_index;
+	buf = kzalloc(alloc_size_buf, GFP_KERNEL);
+	if (!buf)
+		return 0;
+
+	spin_lock(&cpufreq_stats_lock_load_table);
+	len += sprintf(buf + len, "%10s %10s", "Time", "Frequency");
+	for (j = 0; j < NR_CPUS; j++)
+		len += sprintf(buf + len, " %3s%d", "cpu", j);
+	len += sprintf(buf + len, "\n");
+
+	i = stat->load_last_index;
+	do {
+		len += sprintf(buf + len, "%10lld %10d",
+				load_table[i].time,
+				load_table[i].old);
+
+		for (j = 0; j < NR_CPUS; j++)
+			len += sprintf(buf + len, " %4d",
+					load_table[i].load[j]);
+		len += sprintf(buf + len, "\n");
+
+		if (++i == stat->load_max_index)
+			i = 0;
+	} while (i != stat->load_last_index);
+	spin_unlock(&cpufreq_stats_lock_load_table);
+
+	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 void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq)
+{
+	struct cpufreq_stats *stat;
+	int i, last_index;
+
+	stat = per_cpu(cpufreq_stats_table, freq->cpu);
+	if (!stat)
+		return;
+
+	spin_lock(&cpufreq_stats_lock_load_table);
+	last_index = stat->load_last_index;
+	stat->load_table[last_index].old = freq->old;
+	stat->load_table[last_index].time = freq->time;
+	for (i = 0; i < NR_CPUS; i++)
+		stat->load_table[last_index].load[i] = freq->load[i];
+
+	if (++stat->load_last_index == stat->load_max_index)
+		stat->load_last_index = 0;
+	spin_unlock(&cpufreq_stats_lock_load_table);
+}
+
+static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
+{
+	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
+	unsigned int alloc_size_load;
+	int ret = 0;
+
+	stat->load_last_index = 0;
+	stat->load_max_index = CONFIG_NR_CPU_LOAD_STORAGE;
+	alloc_size_load = sizeof(*stat->load_table) * stat->load_max_index;
+	stat->load_table = kzalloc(alloc_size_load, GFP_KERNEL);
+	if (!stat->load_table) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	stat->debugfs_cpufreq = debugfs_create_dir("cpufreq", NULL);
+	if (!stat->debugfs_cpufreq) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	debugfs_create_file("load_table", S_IWUSR, stat->debugfs_cpufreq,
+				(void *)policy, &load_table_fops);
+err:
+	return ret;
+}
+
+static void cpufreq_stats_free_debugfs(unsigned int cpu)
+{
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
+
+	if (!policy)
+		return;
+
+	if (!policy_is_shared(policy)) {
+		pr_debug("%s: Free debugfs stat\n", __func__);
+		debugfs_remove(stat->debugfs_cpufreq);
+	}
+}
+#else
+static void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq)
+{
+	return 0;
+}
+static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
+{
+	return 0;
+}
+static void cpufreq_stats_free_debugfs(unsigned int cpu)
+{
+	return;
+}
+#endif
+
 static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
 {
 	int index;
@@ -167,6 +303,9 @@ static void cpufreq_stats_free_table(unsigned int cpu)
 
 	if (stat) {
 		pr_debug("%s: Free stat table\n", __func__);
+#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
+		kfree(stat->load_table);
+#endif
 		kfree(stat->time_in_state);
 		kfree(stat);
 		per_cpu(cpufreq_stats_table, cpu) = NULL;
@@ -257,6 +396,13 @@ 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) {
+		ret = -EINVAL;
+		goto error_out;
+	}
+
 	spin_unlock(&cpufreq_stats_lock);
 	cpufreq_cpu_put(data);
 	return 0;
@@ -312,32 +458,38 @@ 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;
-
-	stat = per_cpu(cpufreq_stats_table, freq->cpu);
-	if (!stat)
-		return 0;
+	switch (val) {
+	case CPUFREQ_POSTCHANGE:
+		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);
+
+		break;
+	case CPUFREQ_LOADCHECK:
+		cpufreq_stats_store_load_table(freq);
+		break;
+	}
+
 	return 0;
 }
 
@@ -352,12 +504,14 @@ 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_table(cpu);
 		break;
 	case CPU_UP_CANCELED_FROZEN:
+		cpufreq_stats_free_debugfs(cpu);
 		cpufreq_stats_free_sysfs(cpu);
 		cpufreq_stats_free_table(cpu);
 		break;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 037d36a..f780645 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -140,12 +140,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_DETAILS
+	int64_t time;
+	unsigned int load[NR_CPUS];
+#endif
 };
 
 
-- 
1.8.0

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Viresh Kumar]

On 20 June 2013 13:52, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index 534fcb8..8a429b3 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -45,6 +45,12 @@ config CPU_FREQ_STAT_DETAILS
>
>           If in doubt, say N.
>
> +config NR_CPU_LOAD_STORAGE
> +       int "Maximum storage size to save CPU load (10-100)"
> +       range 10 100
> +       depends on CPU_FREQ_STAT_DETAILS
> +       default "10"

As we are adding it in debugfs, probably we can use
CPU_FREQ_STAT instead of CPU_FREQ_STAT_DETAILS

@Rafael?

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2d53f47..cbaaff0 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -292,6 +292,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..bca341b 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -90,6 +90,9 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>         unsigned int max_load = 0;
>         unsigned int ignore_nice;
>         unsigned int j;
> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
> +       struct cpufreq_freqs freq;
> +#endif
>
>         if (dbs_data->cdata->governor == GOV_ONDEMAND)
>                 ignore_nice = od_tuners->ignore_nice;
> @@ -144,11 +147,17 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>                         idle_time += jiffies_to_usecs(cur_nice_jiffies);
>                 }
>
> -               if (unlikely(!wall_time || wall_time < idle_time))
> +               if (unlikely(!wall_time || wall_time < idle_time)) {
> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
> +                       freq.load[j] = 0;
> +#endif
>                         continue;
> +               }
>
>                 load = 100 * (wall_time - idle_time) / wall_time;
> -
> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
> +               freq.load[j] = load;
> +#endif

Add blank line here

>                 if (dbs_data->cdata->governor == GOV_ONDEMAND) {
>                         int freq_avg = __cpufreq_driver_getavg(policy, j);
>                         if (freq_avg <= 0)
> @@ -161,6 +170,12 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>                         max_load = load;
>         }
>
> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
> +       freq.time = ktime_to_ms(ktime_get());
> +       freq.old = freq.new = policy->cur;
> +
> +       cpufreq_notify_transition(policy, &freq, CPUFREQ_LOADCHECK);
> +#endif

Add blank line here

>         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..289d675 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>
> @@ -23,6 +24,7 @@
>  #include <asm/cputime.h>
>
>  static spinlock_t cpufreq_stats_lock;
> +static spinlock_t cpufreq_stats_lock_load_table;

Why need an extra lock?

>
>  struct cpufreq_stats {
>         unsigned int cpu;
> @@ -35,6 +37,12 @@ struct cpufreq_stats {
>         unsigned int *freq_table;
>  #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>         unsigned int *trans_table;
> +
> +       /* debugfs file for load_table */
> +       struct cpufreq_freqs *load_table;
> +       unsigned int load_last_index;
> +       unsigned int load_max_index;
> +       struct dentry *debugfs_cpufreq;
>  #endif
>  };
>
> @@ -149,6 +157,134 @@ static struct attribute_group stats_attr_group = {
>         .name = "stats"
>  };
>
> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
> +#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;
> +       unsigned int alloc_size_buf;
> +       ssize_t len = 0;
> +       char *buf;
> +       int i, j, ret;
> +
> +       alloc_size_buf = MAX_LINE_SIZE * stat->load_max_index;
> +       buf = kzalloc(alloc_size_buf, GFP_KERNEL);
> +       if (!buf)
> +               return 0;
> +
> +       spin_lock(&cpufreq_stats_lock_load_table);
> +       len += sprintf(buf + len, "%10s %10s", "Time", "Frequency");
> +       for (j = 0; j < NR_CPUS; j++)
> +               len += sprintf(buf + len, " %3s%d", "cpu", j);
> +       len += sprintf(buf + len, "\n");
> +
> +       i = stat->load_last_index;
> +       do {
> +               len += sprintf(buf + len, "%10lld %10d",
> +                               load_table[i].time,
> +                               load_table[i].old);
> +
> +               for (j = 0; j < NR_CPUS; j++)

Should we use, for_each_present_cpu() instead of NR_CPUS
for both loops above?

> +                       len += sprintf(buf + len, " %4d",
> +                                       load_table[i].load[j]);
> +               len += sprintf(buf + len, "\n");
> +
> +               if (++i == stat->load_max_index)
> +                       i = 0;
> +       } while (i != stat->load_last_index);
> +       spin_unlock(&cpufreq_stats_lock_load_table);
> +
> +       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 void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq)
> +{
> +       struct cpufreq_stats *stat;
> +       int i, last_index;
> +
> +       stat = per_cpu(cpufreq_stats_table, freq->cpu);
> +       if (!stat)
> +               return;
> +
> +       spin_lock(&cpufreq_stats_lock_load_table);
> +       last_index = stat->load_last_index;
> +       stat->load_table[last_index].old = freq->old;

what about keeping valid value of freq->new here too? I now
currently it is same as freq->old, but maybe we can fill it with
what we tried to go for? Or what actually was programmed?

Then this table will be even more useful.

> +       stat->load_table[last_index].time = freq->time;
> +       for (i = 0; i < NR_CPUS; i++)
> +               stat->load_table[last_index].load[i] = freq->load[i];
> +
> +       if (++stat->load_last_index == stat->load_max_index)
> +               stat->load_last_index = 0;
> +       spin_unlock(&cpufreq_stats_lock_load_table);
> +}
> +
> +static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
> +{
> +       struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
> +       unsigned int alloc_size_load;

s/alloc_size_load/size .. we don't need to name local variables
very meaningfully, i.e. they should be short and precise.

> +       int ret = 0;
> +
> +       stat->load_last_index = 0;
> +       stat->load_max_index = CONFIG_NR_CPU_LOAD_STORAGE;
> +       alloc_size_load = sizeof(*stat->load_table) * stat->load_max_index;
> +       stat->load_table = kzalloc(alloc_size_load, GFP_KERNEL);
> +       if (!stat->load_table) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       stat->debugfs_cpufreq = debugfs_create_dir("cpufreq", NULL);
> +       if (!stat->debugfs_cpufreq) {
> +               ret = -EINVAL;
> +               goto err;
> +       }
> +
> +       debugfs_create_file("load_table", S_IWUSR, stat->debugfs_cpufreq,
> +                               (void *)policy, &load_table_fops);
> +err:
> +       return ret;
> +}
> +
> +static void cpufreq_stats_free_debugfs(unsigned int cpu)
> +{
> +       struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);

you need to put it too

> +       struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
> +
> +       if (!policy)
> +               return;
> +
> +       if (!policy_is_shared(policy)) {

why ??

> +               pr_debug("%s: Free debugfs stat\n", __func__);
> +               debugfs_remove(stat->debugfs_cpufreq);
> +       }
> +}
> +#else
> +static void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq)
> +{
> +       return 0;
> +}
> +static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
> +{
> +       return 0;
> +}
> +static void cpufreq_stats_free_debugfs(unsigned int cpu)
> +{
> +       return;

you don't need this

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Chanwoo Choi]

On 06/20/2013 06:03 PM, Viresh Kumar wrote:
> On 20 June 2013 13:52, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
>> index 534fcb8..8a429b3 100644
>> --- a/drivers/cpufreq/Kconfig
>> +++ b/drivers/cpufreq/Kconfig
>> @@ -45,6 +45,12 @@ config CPU_FREQ_STAT_DETAILS
>>
>>           If in doubt, say N.
>>
>> +config NR_CPU_LOAD_STORAGE
>> +       int "Maximum storage size to save CPU load (10-100)"
>> +       range 10 100
>> +       depends on CPU_FREQ_STAT_DETAILS
>> +       default "10"
> 
> As we are adding it in debugfs, probably we can use
> CPU_FREQ_STAT instead of CPU_FREQ_STAT_DETAILS
> 

OK, If Rafael agree about using CPU_FREQ_STAT instead of CPU_FREQ_STAT_DETAILS, I'll fix it. 

> @Rafael?
> 
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 2d53f47..cbaaff0 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -292,6 +292,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..bca341b 100644
>> --- a/drivers/cpufreq/cpufreq_governor.c
>> +++ b/drivers/cpufreq/cpufreq_governor.c
>> @@ -90,6 +90,9 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>>         unsigned int max_load = 0;
>>         unsigned int ignore_nice;
>>         unsigned int j;
>> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>> +       struct cpufreq_freqs freq;
>> +#endif
>>
>>         if (dbs_data->cdata->governor == GOV_ONDEMAND)
>>                 ignore_nice = od_tuners->ignore_nice;
>> @@ -144,11 +147,17 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>>                         idle_time += jiffies_to_usecs(cur_nice_jiffies);
>>                 }
>>
>> -               if (unlikely(!wall_time || wall_time < idle_time))
>> +               if (unlikely(!wall_time || wall_time < idle_time)) {
>> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>> +                       freq.load[j] = 0;
>> +#endif
>>                         continue;
>> +               }
>>
>>                 load = 100 * (wall_time - idle_time) / wall_time;
>> -
>> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>> +               freq.load[j] = load;
>> +#endif
> 
> Add blank line here

OK, I'll add blank.

> 
>>                 if (dbs_data->cdata->governor == GOV_ONDEMAND) {
>>                         int freq_avg = __cpufreq_driver_getavg(policy, j);
>>                         if (freq_avg <= 0)
>> @@ -161,6 +170,12 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
>>                         max_load = load;
>>         }
>>
>> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>> +       freq.time = ktime_to_ms(ktime_get());
>> +       freq.old = freq.new = policy->cur;
>> +
>> +       cpufreq_notify_transition(policy, &freq, CPUFREQ_LOADCHECK);
>> +#endif
> 
> Add blank line here

OK, I'll add blank.

> 
>>         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..289d675 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>
>> @@ -23,6 +24,7 @@
>>  #include <asm/cputime.h>
>>
>>  static spinlock_t cpufreq_stats_lock;
>> +static spinlock_t cpufreq_stats_lock_load_table;
> 
> Why need an extra lock?

You're right. The extra spinlock isn't necessary.
I'll use only one spinlock.

> 
>>
>>  struct cpufreq_stats {
>>         unsigned int cpu;
>> @@ -35,6 +37,12 @@ struct cpufreq_stats {
>>         unsigned int *freq_table;
>>  #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>>         unsigned int *trans_table;
>> +
>> +       /* debugfs file for load_table */
>> +       struct cpufreq_freqs *load_table;
>> +       unsigned int load_last_index;
>> +       unsigned int load_max_index;
>> +       struct dentry *debugfs_cpufreq;
>>  #endif
>>  };
>>
>> @@ -149,6 +157,134 @@ static struct attribute_group stats_attr_group = {
>>         .name = "stats"
>>  };
>>
>> +#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>> +#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;
>> +       unsigned int alloc_size_buf;
>> +       ssize_t len = 0;
>> +       char *buf;
>> +       int i, j, ret;
>> +
>> +       alloc_size_buf = MAX_LINE_SIZE * stat->load_max_index;
>> +       buf = kzalloc(alloc_size_buf, GFP_KERNEL);
>> +       if (!buf)
>> +               return 0;
>> +
>> +       spin_lock(&cpufreq_stats_lock_load_table);
>> +       len += sprintf(buf + len, "%10s %10s", "Time", "Frequency");
>> +       for (j = 0; j < NR_CPUS; j++)
>> +               len += sprintf(buf + len, " %3s%d", "cpu", j);
>> +       len += sprintf(buf + len, "\n");
>> +
>> +       i = stat->load_last_index;
>> +       do {
>> +               len += sprintf(buf + len, "%10lld %10d",
>> +                               load_table[i].time,
>> +                               load_table[i].old);
>> +
>> +               for (j = 0; j < NR_CPUS; j++)
> 
> Should we use, for_each_present_cpu() instead of NR_CPUS
> for both loops above?

OK, I'll use for_each_present_cpu() macro.

> 
>> +                       len += sprintf(buf + len, " %4d",
>> +                                       load_table[i].load[j]);
>> +               len += sprintf(buf + len, "\n");
>> +
>> +               if (++i == stat->load_max_index)
>> +                       i = 0;
>> +       } while (i != stat->load_last_index);
>> +       spin_unlock(&cpufreq_stats_lock_load_table);
>> +
>> +       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 void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq)
>> +{
>> +       struct cpufreq_stats *stat;
>> +       int i, last_index;
>> +
>> +       stat = per_cpu(cpufreq_stats_table, freq->cpu);
>> +       if (!stat)
>> +               return;
>> +
>> +       spin_lock(&cpufreq_stats_lock_load_table);
>> +       last_index = stat->load_last_index;
>> +       stat->load_table[last_index].old = freq->old;
> 
> what about keeping valid value of freq->new here too? I now
> currently it is same as freq->old, but maybe we can fill it with
> what we tried to go for? Or what actually was programmed?
> 
Yes, freq->old is same freq->new.

The cpufreq gorvernor(dbs_check_cpu()) send CPUFREQ_LOADCHECK noti
right after calculating CPUs load, regardless of changing CPU frequency.
So, I use only freq->old value without using freq->new because load_table
debugfs file need current cpu frequency.

Now, I can't think of any proper usage for freq->new.
Do you have good way for using freq->new to include more useful data in load_table?

> Then this table will be even more useful.
> 
>> +       stat->load_table[last_index].time = freq->time;
>> +       for (i = 0; i < NR_CPUS; i++)
>> +               stat->load_table[last_index].load[i] = freq->load[i];
>> +
>> +       if (++stat->load_last_index == stat->load_max_index)
>> +               stat->load_last_index = 0;
>> +       spin_unlock(&cpufreq_stats_lock_load_table);
>> +}
>> +
>> +static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
>> +{
>> +       struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
>> +       unsigned int alloc_size_load;
> 
> s/alloc_size_load/size .. we don't need to name local variables
> very meaningfully, i.e. they should be short and precise.
> 

OK, I'll fix from 'alloc_size_load' to 'size'.

>> +       int ret = 0;
>> +
>> +       stat->load_last_index = 0;
>> +       stat->load_max_index = CONFIG_NR_CPU_LOAD_STORAGE;
>> +       alloc_size_load = sizeof(*stat->load_table) * stat->load_max_index;
>> +       stat->load_table = kzalloc(alloc_size_load, GFP_KERNEL);
>> +       if (!stat->load_table) {
>> +               ret = -ENOMEM;
>> +               goto err;
>> +       }
>> +
>> +       stat->debugfs_cpufreq = debugfs_create_dir("cpufreq", NULL);
>> +       if (!stat->debugfs_cpufreq) {
>> +               ret = -EINVAL;
>> +               goto err;
>> +       }
>> +
>> +       debugfs_create_file("load_table", S_IWUSR, stat->debugfs_cpufreq,
>> +                               (void *)policy, &load_table_fops);
>> +err:
>> +       return ret;
>> +}
>> +
>> +static void cpufreq_stats_free_debugfs(unsigned int cpu)
>> +{
>> +       struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> 
> you need to put it too

OK. I'll put cpufreq_policy.

> 
>> +       struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
>> +
>> +       if (!policy)
>> +               return;
>> +
>> +       if (!policy_is_shared(policy)) {
> 
> why ??
> 

I thought wrong. It isn't necessary for debugfs file.
I'll remove it.

>> +               pr_debug("%s: Free debugfs stat\n", __func__);
>> +               debugfs_remove(stat->debugfs_cpufreq);
>> +       }
>> +}
>> +#else
>> +static void cpufreq_stats_store_load_table(struct cpufreq_freqs *freq)
>> +{
>> +       return 0;
>> +}
>> +static int cpufreq_stats_create_debugfs(struct cpufreq_policy *policy)
>> +{
>> +       return 0;
>> +}
>> +static void cpufreq_stats_free_debugfs(unsigned int cpu)
>> +{
>> +       return;
> 
> you don't need this
> 

OK, I'll remove it.

Thanks for your comment.

Best Regards,
Chanwoo Choi

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Viresh Kumar]

On 20 June 2013 16:15, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> Yes, freq->old is same freq->new.
>
> The cpufreq gorvernor(dbs_check_cpu()) send CPUFREQ_LOADCHECK noti
> right after calculating CPUs load, regardless of changing CPU frequency.
> So, I use only freq->old value without using freq->new because load_table
> debugfs file need current cpu frequency.
>
> Now, I can't think of any proper usage for freq->new.
> Do you have good way for using freq->new to include more useful data in load_table?

This information might be interesting.. At what load, we moved from
which freq to which one..

Just see if you can add the notification after frequency has been
changed.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Chanwoo Choi]

On 06/20/2013 07:55 PM, Viresh Kumar wrote:
> On 20 June 2013 16:15, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> Yes, freq->old is same freq->new.
>>
>> The cpufreq gorvernor(dbs_check_cpu()) send CPUFREQ_LOADCHECK noti
>> right after calculating CPUs load, regardless of changing CPU frequency.
>> So, I use only freq->old value without using freq->new because load_table
>> debugfs file need current cpu frequency.
>>
>> Now, I can't think of any proper usage for freq->new.
>> Do you have good way for using freq->new to include more useful data in load_table?
> 
> This information might be interesting.. At what load, we moved from
> which freq to which one..
> 
> Just see if you can add the notification after frequency has been
> changed.
> 
OK, I'll modify it and send test result. Thanks.

Best Regards,
Chanwoo Choi

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Chanwoo Choi]

On 06/20/2013 07:59 PM, Chanwoo Choi wrote:
> On 06/20/2013 07:55 PM, Viresh Kumar wrote:
>> On 20 June 2013 16:15, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> Yes, freq->old is same freq->new.
>>>
>>> The cpufreq gorvernor(dbs_check_cpu()) send CPUFREQ_LOADCHECK noti
>>> right after calculating CPUs load, regardless of changing CPU frequency.
>>> So, I use only freq->old value without using freq->new because load_table
>>> debugfs file need current cpu frequency.
>>>
>>> Now, I can't think of any proper usage for freq->new.
>>> Do you have good way for using freq->new to include more useful data in load_table?
>>
>> This information might be interesting.. At what load, we moved from
>> which freq to which one..
>>
>> Just see if you can add the notification after frequency has been
>> changed.
>>
> OK, I'll modify it and send test result. Thanks.
> 
> Best Regards,
> Chanwoo Choi
> 

But,
To show old frequency/new frequency on load_table debugfs file,
governor function(dbs_check_cpu()) pass calculated CPUs load to specific governor(e.g., ondemand)
as below function flow.

dbs_check_cpu() (in cpufreq.c)
-> od_check_cpu() (in cpufreq_ondemand.c)
-> __cpufreq_driver_target() (in cpufreq.c)
-> cpufreq_driver->target(policy)

Also, The __cpufreq_driver_target() is external function which can be called on other file
so I must consider exception case.

If send CPUFREQ_LOADCHECK noti after changed cpu frequency,
I think it is complicated and has quite a little difficulty.

What is your opinion?

Thanks,
Chanwoo Choi

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Viresh Kumar]

On 20 June 2013 16:48, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> But,
> To show old frequency/new frequency on load_table debugfs file,
> governor function(dbs_check_cpu()) pass calculated CPUs load to specific governor(e.g., ondemand)
> as below function flow.
>
> dbs_check_cpu() (in cpufreq.c)
> -> od_check_cpu() (in cpufreq_ondemand.c)
> -> __cpufreq_driver_target() (in cpufreq.c)
> -> cpufreq_driver->target(policy)
>
> Also, The __cpufreq_driver_target() is external function which can be called on other file
> so I must consider exception case.
>
> If send CPUFREQ_LOADCHECK noti after changed cpu frequency,
> I think it is complicated and has quite a little difficulty.
>
> What is your opinion?

What you can do is:
- create another routine: cpufreq_governor_driver_target()
- replace all __cpufreq_driver_target() from ondemand/conservative governors
with this one
- In cpufreq_governor_driver_target() call __cpufreq_driver_target() and
take a note of new freq.

Maybe you don't need to check the actual freq that is set (even
that would be simple to implement), but what is requested.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Chanwoo Choi]

On 06/21/2013 12:42 AM, Viresh Kumar wrote:
> On 20 June 2013 16:48, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> But,
>> To show old frequency/new frequency on load_table debugfs file,
>> governor function(dbs_check_cpu()) pass calculated CPUs load to specific governor(e.g., ondemand)
>> as below function flow.
>>
>> dbs_check_cpu() (in cpufreq.c)
>> -> od_check_cpu() (in cpufreq_ondemand.c)
>> -> __cpufreq_driver_target() (in cpufreq.c)
>> -> cpufreq_driver->target(policy)
>>
>> Also, The __cpufreq_driver_target() is external function which can be called on other file
>> so I must consider exception case.
>>
>> If send CPUFREQ_LOADCHECK noti after changed cpu frequency,
>> I think it is complicated and has quite a little difficulty.
>>
>> What is your opinion?
> 
> What you can do is:
> - create another routine: cpufreq_governor_driver_target()
> - replace all __cpufreq_driver_target() from ondemand/conservative governors
> with this one
> - In cpufreq_governor_driver_target() call __cpufreq_driver_target() and
> take a note of new freq.
> 
> Maybe you don't need to check the actual freq that is set (even
> that would be simple to implement), but what is requested.
> 

OK, I understand and will try to implement it.
Thanks.

@Rafael?
If possible, I want to know the your opinion about this.

Best Regards,
Chanwoo Choi

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Rafael J. Wysocki]

On Friday, June 21, 2013 01:01:35 PM Chanwoo Choi wrote:
> On 06/21/2013 12:42 AM, Viresh Kumar wrote:
> > On 20 June 2013 16:48, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> >> But,
> >> To show old frequency/new frequency on load_table debugfs file,
> >> governor function(dbs_check_cpu()) pass calculated CPUs load to specific governor(e.g., ondemand)
> >> as below function flow.
> >>
> >> dbs_check_cpu() (in cpufreq.c)
> >> -> od_check_cpu() (in cpufreq_ondemand.c)
> >> -> __cpufreq_driver_target() (in cpufreq.c)
> >> -> cpufreq_driver->target(policy)
> >>
> >> Also, The __cpufreq_driver_target() is external function which can be called on other file
> >> so I must consider exception case.
> >>
> >> If send CPUFREQ_LOADCHECK noti after changed cpu frequency,
> >> I think it is complicated and has quite a little difficulty.
> >>
> >> What is your opinion?
> > 
> > What you can do is:
> > - create another routine: cpufreq_governor_driver_target()
> > - replace all __cpufreq_driver_target() from ondemand/conservative governors
> > with this one
> > - In cpufreq_governor_driver_target() call __cpufreq_driver_target() and
> > take a note of new freq.
> > 
> > Maybe you don't need to check the actual freq that is set (even
> > that would be simple to implement), but what is requested.
> > 
> 
> OK, I understand and will try to implement it.
> Thanks.
> 
> @Rafael?
> If possible, I want to know the your opinion about this.

Well, to be honest, I don't like the whole thing.  The more changes you need
to make to the common code to support it, the worse.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Viresh Kumar]

On 21 June 2013 18:43, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Well, to be honest, I don't like the whole thing.  The more changes you need
> to make to the common code to support it, the worse.

You meant you didn't like this patch completely? Or the idea that
I gave?

Getting the new frequency would be very useful IMHO. That would
give us a relation between old/new freq and load on each cpu at
that time. That's why I suggested it.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Chanwoo Choi]

On 06/21/2013 10:13 PM, Rafael J. Wysocki wrote:
> On Friday, June 21, 2013 01:01:35 PM Chanwoo Choi wrote:
>> On 06/21/2013 12:42 AM, Viresh Kumar wrote:
>>> On 20 June 2013 16:48, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>> But,
>>>> To show old frequency/new frequency on load_table debugfs file,
>>>> governor function(dbs_check_cpu()) pass calculated CPUs load to specific governor(e.g., ondemand)
>>>> as below function flow.
>>>>
>>>> dbs_check_cpu() (in cpufreq.c)
>>>> -> od_check_cpu() (in cpufreq_ondemand.c)
>>>> -> __cpufreq_driver_target() (in cpufreq.c)
>>>> -> cpufreq_driver->target(policy)
>>>>
>>>> Also, The __cpufreq_driver_target() is external function which can be called on other file
>>>> so I must consider exception case.
>>>>
>>>> If send CPUFREQ_LOADCHECK noti after changed cpu frequency,
>>>> I think it is complicated and has quite a little difficulty.
>>>>
>>>> What is your opinion?
>>>
>>> What you can do is:
>>> - create another routine: cpufreq_governor_driver_target()
>>> - replace all __cpufreq_driver_target() from ondemand/conservative governors
>>> with this one
>>> - In cpufreq_governor_driver_target() call __cpufreq_driver_target() and
>>> take a note of new freq.
>>>
>>> Maybe you don't need to check the actual freq that is set (even
>>> that would be simple to implement), but what is requested.
>>>
>>
>> OK, I understand and will try to implement it.
>> Thanks.
>>
>> @Rafael?
>> If possible, I want to know the your opinion about this.
> 
> Well, to be honest, I don't like the whole thing.  The more changes you need
> to make to the common code to support it, the worse.
> 

To minimize the modification of cpufreq core, I implemented load_table debugfs file
by only using CPUFREQ_LOADCHECK and CPUFREQ_POSTCHANGE noti. So, I don't add additional
function for load_table debugfs file. I will send next version patch for load_table soon.

Best Regards,
Chanwoo Choi

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Rafael J. Wysocki]

On Monday, June 24, 2013 11:48:22 AM Viresh Kumar wrote:
> On 21 June 2013 18:43, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Well, to be honest, I don't like the whole thing.  The more changes you need
> > to make to the common code to support it, the worse.
> 
> You meant you didn't like this patch completely? Or the idea that
> I gave?

Well, kind of both ...

> Getting the new frequency would be very useful IMHO. That would
> give us a relation between old/new freq and load on each cpu at
> that time. That's why I suggested it.

Where "us" is user space or what?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Viresh Kumar]

On 24 June 2013 15:09, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, June 24, 2013 11:48:22 AM Viresh Kumar wrote:
>> On 21 June 2013 18:43, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > Well, to be honest, I don't like the whole thing.  The more changes you need
>> > to make to the common code to support it, the worse.
>>
>> You meant you didn't like this patch completely? Or the idea that
>> I gave?
>
> Well, kind of both ...
>
>> Getting the new frequency would be very useful IMHO. That would
>> give us a relation between old/new freq and load on each cpu at
>> that time. That's why I suggested it.
>
> Where "us" is user space or what?

User space as well as the developers who are accessing this information.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Rafael J. Wysocki]

On Monday, June 24, 2013 03:11:15 PM Viresh Kumar wrote:
> On 24 June 2013 15:09, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Monday, June 24, 2013 11:48:22 AM Viresh Kumar wrote:
> >> On 21 June 2013 18:43, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > Well, to be honest, I don't like the whole thing.  The more changes you need
> >> > to make to the common code to support it, the worse.
> >>
> >> You meant you didn't like this patch completely? Or the idea that
> >> I gave?
> >
> > Well, kind of both ...
> >
> >> Getting the new frequency would be very useful IMHO. That would
> >> give us a relation between old/new freq and load on each cpu at
> >> that time. That's why I suggested it.
> >
> > Where "us" is user space or what?
> 
> User space as well as the developers who are accessing this information.

My impression was that that new interface wouldn't be useful on all platforms
supported by cpufreq.  If that's the case, I'd rather not make changes in the
core code that are needed only to implement that new interface, at least to
start with.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Viresh Kumar]

On 24 June 2013 15:50, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> My impression was that that new interface wouldn't be useful on all platforms
> supported by cpufreq.  If that's the case, I'd rather not make changes in the
> core code that are needed only to implement that new interface, at least to
> start with.

Every platform might not have userspace software that would do some
actions based on this. But, these are nice and useful stats to have.

They tell you a lot about how the hardware/governor/policies are
behaving with load. And so will eventually help getting performance based
on governor tunables for your platform. That's why I asked to put this in
core as this should be pretty much useful.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Rafael J. Wysocki]

On Monday, June 24, 2013 04:03:15 PM Viresh Kumar wrote:
> On 24 June 2013 15:50, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > My impression was that that new interface wouldn't be useful on all platforms
> > supported by cpufreq.  If that's the case, I'd rather not make changes in the
> > core code that are needed only to implement that new interface, at least to
> > start with.
> 
> Every platform might not have userspace software that would do some
> actions based on this. But, these are nice and useful stats to have.
> 
> They tell you a lot about how the hardware/governor/policies are
> behaving with load. And so will eventually help getting performance based
> on governor tunables for your platform. That's why I asked to put this in
> core as this should be pretty much useful.

OK, let's see how the code will look like.

Chanwoo, please prepare a v4 with the changes suggested by Viresh.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2] cpufreq: stats: Add 'load_table' debugfs file to show accumulated data of CPUs

[Chanwoo Choi]

On 06/24/2013 08:00 PM, Rafael J. Wysocki wrote:
> On Monday, June 24, 2013 04:03:15 PM Viresh Kumar wrote:
>> On 24 June 2013 15:50, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> My impression was that that new interface wouldn't be useful on all platforms
>>> supported by cpufreq.  If that's the case, I'd rather not make changes in the
>>> core code that are needed only to implement that new interface, at least to
>>> start with.
>>
>> Every platform might not have userspace software that would do some
>> actions based on this. But, these are nice and useful stats to have.
>>
>> They tell you a lot about how the hardware/governor/policies are
>> behaving with load. And so will eventually help getting performance based
>> on governor tunables for your platform. That's why I asked to put this in
>> core as this should be pretty much useful.
> 
> OK, let's see how the code will look like.
> 
> Chanwoo, please prepare a v4 with the changes suggested by Viresh.
> 

@Viresh,
I implement v3 patch for load_table debugfs file. As you suggested, v3 pach include
old/new frequency data. But, I don't add additional function to minimize dependency
among cpufreq core functions. And then I use CPUFREQ_LOADCHECK and CPUFREQ_POSTCHANGE
notification to collect cpu data(time, old/new frequency, CPUs load).

Please review v3 patch for load_table debugfs file. Thanks.

Best Regards,
Chanwoo Choi