Hi Viresh,
On 07/22/2013 08:05 PM, Viresh Kumar wrote:
> 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 ??
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
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
