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

[Viresh Kumar]

On 20 June 2013 13:52, Chanwoo Choi <cw00.choi@xxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/