Re: [PATCH 2/3 v6] cpufreq: stats: Add 'load_table' debugfs file toshow accumulated data of CPUs

[Viresh Kumar]

On 18 July 2013 16:47, Chanwoo Choi <cw00.choi@xxxxxxxxxxx> 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;
>  }
--
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/