cpufreq: stats: cleanups
Viresh Kumar
maintainerv3 cover cpufreq: stats: cleanups
Hi Rafael,
This is V3 of the stats cleanup I sent earlier. Few things are improved based on the feedback received (mostly from you). Please see if it looks any better.
Pushed here:git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/stats/cleanups
V1->V2:
- New patches:
cpufreq: stats: remove unused cpufreq_stats_attribute
cpufreq: stats: initialize 'cur_time' on its definition
cpufreq: stats: rename 'struct cpufreq_stats' objects as 'stats'
cpufreq: stats: drop unnecessary locking
- Patches dropped:
cpufreq: stats: replace spinlock with mutex
cpufreq: stats: Fix locking
cpufreq: stats: call cpufreq_stats_update() with locks held
- Logs improved
- don't return error values notifiers on missing policy/stats
- rename stats_data as stats and make it cpufreq_stats pointer instead of void *
Viresh Kumar (16):
cpufreq: stats: Improve module description string
cpufreq: stats: return -EEXIST when stats are already allocated
cpufreq: stats: remove unused cpufreq_stats_attribute
cpufreq: stats: initialize 'cur_time' on its definition
cpufreq: stats: don't check for freq table while freeing stats
cpufreq: stats: pass 'stat' to cpufreq_stats_update()
cpufreq: stats: get rid of per-cpu cpufreq_stats_table
cpufreq: stats: rename 'struct cpufreq_stats' objects as 'stats'
cpufreq: Remove (now) unused 'last_cpu' from struct cpufreq_policy
cpufreq: stats: drop 'cpu' field of struct cpufreq_stats
cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications
cpufreq: stats: create sysfs group once we are ready
cpufreq: stats: time_in_state can't be NULL in cpufreq_stats_update()
cpufreq: stats: don't update stats from show_trans_table()
cpufreq: stats: don't update stats on false notifiers
cpufreq: stats: drop unnecessary locking
drivers/cpufreq/cpufreq.c | 6 -- drivers/cpufreq/cpufreq_stats.c | 211 ++++++++++++++++++---------------------- include/linux/cpufreq.h | 10 +- 3 files changed, 98 insertions(+), 129 deletions(-)
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 1/16 cpufreq: stats: Improve module description string
The MODULE_DESCRIPTION() string is just too long and then is broken into multiple lines just to make checkpatch happy.
Rewrite it to make it more precise.
--- drivers/cpufreq/cpufreq_stats.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/cpufreq/cpufreq_stats.c +1 -2
@@ -374,8 +374,7 @@ static void __exit cpufreq_stats_exit(void) }MODULE_AUTHOR ("Zou Nan hai <[email protected]>" ); -MODULE_DESCRIPTION ("'cpufreq_stats' - A driver to export cpufreq stats " -"through sysfs filesystem" ); +MODULE_DESCRIPTION ("Export cpufreq stats via sysfs" );MODULE_LICENSE ("GPL" );module_init (cpufreq_stats_init);
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 2/16 cpufreq: stats: return -EEXIST when stats are already allocated
In reply to Viresh Kumar__cpufreq_stats_create_table() is called from:
- cpufreq notifier on creation of a new policy. Stats will always be NULL here.
- cpufreq_stats_init() for all CPUs as cpufreq-stats might have been initialized after cpufreq driver. For any policy, 'stats' will be NULL for the first cpu only and will be valid for all other CPUs managed by the same policy.
While we return for other CPUs, we don't return the right error value. Its not that we would fail with -EBUSY. But generally, this is what these return values mean:
- EBUSY: we are busy right now, try again. And the retry attempt might be immediate.
- EEXIST: We already have what you are trying to create and there is no need to create it again, and so no more tries are required.
--- drivers/cpufreq/cpufreq_stats.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/cpufreq/cpufreq_stats.c +3 -1
@@ -192,8 +192,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)if (unlikely (!table))return 0 ; +/* stats already initialized */ if (per_cpu (cpufreq_stats_table, cpu)) -return -EBUSY ; +return -EEXIST ; + stat =kzalloc (sizeof (*stat),GFP_KERNEL );if ((stat) ==NULL )return -ENOMEM ;
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 3/16 cpufreq: stats: remove unused cpufreq_stats_attribute
In reply to Viresh KumarIt was never used, but is there since the first commit. Remove it.
--- drivers/cpufreq/cpufreq_stats.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/cpufreq/cpufreq_stats.c +0 -5
@@ -33,11 +33,6 @@ struct cpufreq_stats { static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table); -struct cpufreq_stats_attribute { -struct attribute attr; -ssize_t (*show) (struct cpufreq_stats *,char *); -}; -static int cpufreq_stats_update (unsigned int cpu) {struct cpufreq_stats *stat;
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 4/16 cpufreq: stats: initialize 'cur_time' on its definition
In reply to Viresh Kumar'cur_time' is defined in the first line and is then assigned a value in the next line. Initialize it while defining it.
--- drivers/cpufreq/cpufreq_stats.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/cpufreq/cpufreq_stats.c +1 -2
@@ -36,9 +36,8 @@ static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);static int cpufreq_stats_update (unsigned int cpu) {struct cpufreq_stats *stat; -unsigned long long cur_time; +unsigned long long cur_time =get_jiffies_64 (); - cur_time =get_jiffies_64 ();spin_lock (&cpufreq_stats_lock); stat =per_cpu (cpufreq_stats_table, cpu);if (stat->time_in_state)
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 5/16 cpufreq: stats: don't check for freq table while freeing stats
In reply to Viresh KumarWhile we allocate stats, we do need to check if freq-table is present or not as we need to use it then. But while freeing stats, all we need to know is if stats holds a valid pointer value. There is no use of testing if cpufreq table is present or not.
Don't check it.
--- drivers/cpufreq/cpufreq_stats.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/cpufreq/cpufreq_stats.c +1 -2
@@ -168,8 +168,7 @@ static void cpufreq_stats_free_table(unsigned int cpu)if (!policy)return ; -if (cpufreq_frequency_get_table (policy->cpu)) -__cpufreq_stats_free_table (policy); +__cpufreq_stats_free_table (policy);cpufreq_cpu_put (policy); }
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 6/16 cpufreq: stats: pass 'stat' to cpufreq_stats_update()
In reply to Viresh KumarIt is better to pass a struct cpufreq_stats pointer to cpufreq_stats_update() instead of a CPU number, because that's all it needs.
Even if we pass a cpu number to cpufreq_stats_update(), it reads the per-cpu variable to get 'stats' out of it. So we are doing these operations unnecessarily:
- First getting the cpu number to pass to cpufreq_stats_update(), stat->cpu.
- And then getting stats from the cpu, per_cpu(cpufreq_stats_table, cpu).
--- drivers/cpufreq/cpufreq_stats.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/cpufreq/cpufreq_stats.c +4 -6
@@ -33,13 +33,11 @@ struct cpufreq_stats { static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table); -static int cpufreq_stats_update (unsigned int cpu) +static int cpufreq_stats_update (struct cpufreq_stats *stat) { -struct cpufreq_stats *stat;unsigned long long cur_time =get_jiffies_64 ();spin_lock (&cpufreq_stats_lock); - stat =per_cpu (cpufreq_stats_table, cpu);if (stat->time_in_state) stat->time_in_state[stat->last_index] += cur_time - stat->last_time;
@@ -64,7 +62,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu);if (!stat)return 0 ; -cpufreq_stats_update (stat->cpu); +cpufreq_stats_update (stat);for (i =0 ; i < stat->state_num; i++) { len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i], (unsigned long long)
@@ -82,7 +80,7 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu);if (!stat)return 0 ; -cpufreq_stats_update (stat->cpu); +cpufreq_stats_update (stat); len +=snprintf (buf + len,PAGE_SIZE - len," From : To\n" ); len +=snprintf (buf + len,PAGE_SIZE - len," : " );for (i =0 ; i < stat->state_num; i++) {
@@ -307,7 +305,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,if (old_index ==-1 || new_index ==-1 )return 0 ; -cpufreq_stats_update (freq->cpu); +cpufreq_stats_update (stat);if (old_index == new_index)return 0 ;
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 7/16 cpufreq: stats: get rid of per-cpu cpufreq_stats_table
In reply to Viresh KumarAll CPUs sharing a cpufreq policy share stats too. For this reason, add a stats pointer to struct cpufreq_policy and drop per-CPU variable cpufreq_stats_table used for accessing cpufreq stats so as to reduce code complexity.
--- drivers/cpufreq/cpufreq_stats.c | 54 +++++++++++++++++++---------------------- include/linux/cpufreq.h | 3 +++ 2 files changed, 28 insertions(+), 29 deletions(-)
drivers/cpufreq/cpufreq_stats.c +25 -29
@@ -31,8 +31,6 @@ struct cpufreq_stats {#endif }; -static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table); -static int cpufreq_stats_update (struct cpufreq_stats *stat) {unsigned long long cur_time =get_jiffies_64 ();
@@ -48,20 +46,15 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)static ssize_t show_total_trans (struct cpufreq_policy *policy,char *buf) { -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu); -if (!stat) -return 0 ; -return sprintf (buf,"%d\n" , -per_cpu (cpufreq_stats_table, stat->cpu)->total_trans); +return sprintf (buf,"%d\n" , policy->stats->total_trans); }static ssize_t show_time_in_state (struct cpufreq_policy *policy,char *buf) { +struct cpufreq_stats *stat = policy->stats;ssize_t len =0 ;int i; -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu); -if (!stat) -return 0 ; +cpufreq_stats_update (stat);for (i =0 ; i < stat->state_num; i++) { len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
@@ -74,12 +67,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)#ifdef CONFIG_CPU_FREQ_STAT_DETAILS static ssize_t show_trans_table (struct cpufreq_policy *policy,char *buf) { +struct cpufreq_stats *stat = policy->stats;ssize_t len =0 ;int i, j; -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu); -if (!stat) -return 0 ;cpufreq_stats_update (stat); len +=snprintf (buf + len,PAGE_SIZE - len," From : To\n" ); len +=snprintf (buf + len,PAGE_SIZE - len," : " );
@@ -145,8 +136,9 @@ static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)static void __cpufreq_stats_free_table (struct cpufreq_policy *policy) { -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu); +struct cpufreq_stats *stat = policy->stats; +/* Already freed */ if (!stat)return ;
@@ -155,7 +147,7 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)sysfs_remove_group (&policy->kobj, &stats_attr_group);kfree (stat->time_in_state);kfree (stat); -per_cpu (cpufreq_stats_table, policy->cpu) =NULL ; + policy->stats =NULL ; }static void cpufreq_stats_free_table (unsigned int cpu)
@@ -184,7 +176,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)return 0 ;/* stats already initialized */ -if (per_cpu (cpufreq_stats_table, cpu)) +if (policy->stats)return -EEXIST ; stat =kzalloc (sizeof (*stat),GFP_KERNEL );
@@ -196,7 +188,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) goto error_out; stat->cpu = cpu; -per_cpu (cpufreq_stats_table, cpu) = stat; + policy->stats = stat;cpufreq_for_each_valid_entry (pos, table) count++;
@@ -231,7 +223,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)sysfs_remove_group (&policy->kobj, &stats_attr_group); error_out:kfree (stat); -per_cpu (cpufreq_stats_table, cpu) =NULL ; + policy->stats =NULL ;return ret; }
@@ -254,15 +246,7 @@ static void cpufreq_stats_create_table(unsigned int cpu)static void cpufreq_stats_update_policy_cpu (struct cpufreq_policy *policy) { -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, - policy->last_cpu); - -pr_debug ("Updating stats_table for new_cpu %u from last_cpu %u\n" , - policy->cpu, policy->last_cpu); -per_cpu (cpufreq_stats_table, policy->cpu) =per_cpu (cpufreq_stats_table, - policy->last_cpu); -per_cpu (cpufreq_stats_table, policy->last_cpu) =NULL ; - stat->cpu = policy->cpu; + policy->stats->cpu = policy->cpu; } static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
@@ -288,15 +272,24 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb, unsigned long val, void *data) { struct cpufreq_freqs *freq = data; +struct cpufreq_policy *policy =cpufreq_cpu_get (freq->cpu);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) +if (!policy) { +pr_err ("%s: No policy found\n" , __func__);return 0 ; + } + +if (!policy->stats) { +pr_debug ("%s: No stats found\n" , __func__); + goto put_policy; + } + + stat = policy->stats; old_index = stat->last_index; new_index =freq_table_get_index (stat, freq->new);
@@ -317,6 +310,9 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,#endif stat->total_trans++;spin_unlock (&cpufreq_stats_lock); + +put_policy: +cpufreq_cpu_put (policy);return 0 ; }
include/linux/cpufreq.h +3 -0
@@ -113,6 +113,9 @@ struct cpufreq_policy {wait_queue_head_t transition_wait;struct task_struct *transition_task;/* Task which is doing the transition */ +/* cpufreq-stats */ +struct cpufreq_stats *stats; +/* For cpufreq driver's internal use */ void *driver_data; };
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH RESEND v3 7/16 cpufreq: stats: get rid of per-cpu cpufreq_stats_table
All CPUs sharing a cpufreq policy share stats too. For this reason, add a stats pointer to struct cpufreq_policy and drop per-CPU variable cpufreq_stats_table used for accessing cpufreq stats so as to reduce code complexity.
---
Resending only a single patch as found a issue with it while testing with my newly written cpufreq-test suite. The problem was that we haven't done cpufreq_cpu_put() for few failure cases and that restricts the cpufreq driver module to unload.
Here is the diff to V3:
@@ -265,14 +265,14 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
struct cpufreq_stats *stats;
int old_index, new_index;- if (val != CPUFREQ_POSTCHANGE)
- return 0;
-
if (!policy) {
pr_err("%s: No policy found\n", __func__);
return 0;
}+ if (val != CPUFREQ_POSTCHANGE)
+ goto put_policy;
+
if (!policy->stats) {
pr_debug("%s: No stats found\n", __func__);
goto put_policy;@@ -285,10 +285,10 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
/* We can't do stats->time_in_state[-1]= .. */
if (old_index == -1 || new_index == -1)- return 0;
+ goto put_policy;
if (old_index == new_index)
- return 0;
+ goto put_policy;
cpufreq_stats_update(stats);
drivers/cpufreq/cpufreq_stats.c | 62 +++++++++++++++++++---------------------- include/linux/cpufreq.h | 3 ++ 2 files changed, 32 insertions(+), 33 deletions(-)
drivers/cpufreq/cpufreq_stats.c +29 -33
@@ -31,8 +31,6 @@ struct cpufreq_stats {#endif }; -static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table); -static int cpufreq_stats_update (struct cpufreq_stats *stat) {unsigned long long cur_time =get_jiffies_64 ();
@@ -48,20 +46,15 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)static ssize_t show_total_trans (struct cpufreq_policy *policy,char *buf) { -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu); -if (!stat) -return 0 ; -return sprintf (buf,"%d\n" , -per_cpu (cpufreq_stats_table, stat->cpu)->total_trans); +return sprintf (buf,"%d\n" , policy->stats->total_trans); }static ssize_t show_time_in_state (struct cpufreq_policy *policy,char *buf) { +struct cpufreq_stats *stat = policy->stats;ssize_t len =0 ;int i; -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu); -if (!stat) -return 0 ; +cpufreq_stats_update (stat);for (i =0 ; i < stat->state_num; i++) { len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
@@ -74,12 +67,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)#ifdef CONFIG_CPU_FREQ_STAT_DETAILS static ssize_t show_trans_table (struct cpufreq_policy *policy,char *buf) { +struct cpufreq_stats *stat = policy->stats;ssize_t len =0 ;int i, j; -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu); -if (!stat) -return 0 ;cpufreq_stats_update (stat); len +=snprintf (buf + len,PAGE_SIZE - len," From : To\n" ); len +=snprintf (buf + len,PAGE_SIZE - len," : " );
@@ -145,8 +136,9 @@ static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)static void __cpufreq_stats_free_table (struct cpufreq_policy *policy) { -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, policy->cpu); +struct cpufreq_stats *stat = policy->stats; +/* Already freed */ if (!stat)return ;
@@ -155,7 +147,7 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)sysfs_remove_group (&policy->kobj, &stats_attr_group);kfree (stat->time_in_state);kfree (stat); -per_cpu (cpufreq_stats_table, policy->cpu) =NULL ; + policy->stats =NULL ; }static void cpufreq_stats_free_table (unsigned int cpu)
@@ -184,7 +176,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)return 0 ;/* stats already initialized */ -if (per_cpu (cpufreq_stats_table, cpu)) +if (policy->stats)return -EEXIST ; stat =kzalloc (sizeof (*stat),GFP_KERNEL );
@@ -196,7 +188,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) goto error_out; stat->cpu = cpu; -per_cpu (cpufreq_stats_table, cpu) = stat; + policy->stats = stat;cpufreq_for_each_valid_entry (pos, table) count++;
@@ -231,7 +223,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)sysfs_remove_group (&policy->kobj, &stats_attr_group); error_out:kfree (stat); -per_cpu (cpufreq_stats_table, cpu) =NULL ; + policy->stats =NULL ;return ret; }
@@ -254,15 +246,7 @@ static void cpufreq_stats_create_table(unsigned int cpu)static void cpufreq_stats_update_policy_cpu (struct cpufreq_policy *policy) { -struct cpufreq_stats *stat =per_cpu (cpufreq_stats_table, - policy->last_cpu); - -pr_debug ("Updating stats_table for new_cpu %u from last_cpu %u\n" , - policy->cpu, policy->last_cpu); -per_cpu (cpufreq_stats_table, policy->cpu) =per_cpu (cpufreq_stats_table, - policy->last_cpu); -per_cpu (cpufreq_stats_table, policy->last_cpu) =NULL ; - stat->cpu = policy->cpu; + policy->stats->cpu = policy->cpu; } static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
@@ -288,27 +272,36 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb, unsigned long val, void *data) { struct cpufreq_freqs *freq = data; +struct cpufreq_policy *policy =cpufreq_cpu_get (freq->cpu);struct cpufreq_stats *stat;int old_index, new_index; -if (val !=CPUFREQ_POSTCHANGE ) +if (!policy) { +pr_err ("%s: No policy found\n" , __func__);return 0 ; + } - stat =per_cpu (cpufreq_stats_table, freq->cpu); -if (!stat) -return 0 ; +if (val !=CPUFREQ_POSTCHANGE ) + goto put_policy; + +if (!policy->stats) { +pr_debug ("%s: No stats found\n" , __func__); + goto put_policy; + } + + stat = policy->stats; 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 ; + goto put_policy;cpufreq_stats_update (stat);if (old_index == new_index) -return 0 ; + goto put_policy;spin_lock (&cpufreq_stats_lock); stat->last_index = new_index;
@@ -317,6 +310,9 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,#endif stat->total_trans++;spin_unlock (&cpufreq_stats_lock); + +put_policy: +cpufreq_cpu_put (policy);return 0 ; }
include/linux/cpufreq.h +3 -0
@@ -113,6 +113,9 @@ struct cpufreq_policy {wait_queue_head_t transition_wait;struct task_struct *transition_task;/* Task which is doing the transition */ +/* cpufreq-stats */ +struct cpufreq_stats *stats; +/* For cpufreq driver's internal use */ void *driver_data; };
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 8/16 cpufreq: stats: rename 'struct cpufreq_stats' objects as 'stats'
In reply to Viresh KumarCurrently we name objects of 'struct cpufreq_stats' as 'stat' and 'stats'. Use 'stats' to make it consistent.
--- drivers/cpufreq/cpufreq_stats.c | 102 ++++++++++++++++++++-------------------- 1 file changed, 51 insertions(+), 51 deletions(-)
drivers/cpufreq/cpufreq_stats.c +51 -51
@@ -31,15 +31,15 @@ struct cpufreq_stats {#endif }; -static int cpufreq_stats_update (struct cpufreq_stats *stat) +static int cpufreq_stats_update (struct cpufreq_stats *stats) {unsigned long long cur_time =get_jiffies_64 ();spin_lock (&cpufreq_stats_lock); -if (stat->time_in_state) - stat->time_in_state[stat->last_index] += - cur_time - stat->last_time; - stat->last_time = cur_time; +if (stats->time_in_state) + stats->time_in_state[stats->last_index] += + cur_time - stats->last_time; + stats->last_time = cur_time;spin_unlock (&cpufreq_stats_lock);return 0 ; }
@@ -51,15 +51,15 @@ static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)static ssize_t show_time_in_state (struct cpufreq_policy *policy,char *buf) { -struct cpufreq_stats *stat = policy->stats; +struct cpufreq_stats *stats = policy->stats;ssize_t len =0 ;int i; -cpufreq_stats_update (stat); -for (i =0 ; i < stat->state_num; i++) { - len +=sprintf (buf + len,"%u %llu\n" , stat->freq_table[i], +cpufreq_stats_update (stats); +for (i =0 ; i < stats->state_num; i++) { + len +=sprintf (buf + len,"%u %llu\n" , stats->freq_table[i], (unsigned long long ) -jiffies_64_to_clock_t (stat->time_in_state[i])); +jiffies_64_to_clock_t (stats->time_in_state[i])); }return len; }
@@ -67,36 +67,36 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)#ifdef CONFIG_CPU_FREQ_STAT_DETAILS static ssize_t show_trans_table (struct cpufreq_policy *policy,char *buf) { -struct cpufreq_stats *stat = policy->stats; +struct cpufreq_stats *stats = policy->stats;ssize_t len =0 ;int i, j; -cpufreq_stats_update (stat); +cpufreq_stats_update (stats); len +=snprintf (buf + len,PAGE_SIZE - len," From : To\n" ); len +=snprintf (buf + len,PAGE_SIZE - len," : " ); -for (i =0 ; i < stat->state_num; i++) { +for (i =0 ; i < stats->state_num; i++) {if (len >=PAGE_SIZE )break ; len +=snprintf (buf + len,PAGE_SIZE - len,"%9u " , - stat->freq_table[i]); + stats->freq_table[i]); }if (len >=PAGE_SIZE )return PAGE_SIZE ; len +=snprintf (buf + len,PAGE_SIZE - len,"\n" ); -for (i =0 ; i < stat->state_num; i++) { +for (i =0 ; i < stats->state_num; i++) {if (len >=PAGE_SIZE )break ; len +=snprintf (buf + len,PAGE_SIZE - len,"%9u: " , - stat->freq_table[i]); + stats->freq_table[i]); -for (j =0 ; j < stat->state_num; j++) { +for (j =0 ; j < stats->state_num; j++) {if (len >=PAGE_SIZE )break ; len +=snprintf (buf + len,PAGE_SIZE - len,"%9u " , - stat->trans_table[i*stat->max_state+j]); + stats->trans_table[i*stats->max_state+j]); }if (len >=PAGE_SIZE )break ;
@@ -125,28 +125,28 @@ static struct attribute_group stats_attr_group = { .name = "stats" }; -static int freq_table_get_index (struct cpufreq_stats *stat,unsigned int freq) +static int freq_table_get_index (struct cpufreq_stats *stats,unsigned int freq) {int index; -for (index =0 ; index < stat->max_state; index++) -if (stat->freq_table[index] == freq) +for (index =0 ; index < stats->max_state; index++) +if (stats->freq_table[index] == freq)return index;return -1 ; }static void __cpufreq_stats_free_table (struct cpufreq_policy *policy) { -struct cpufreq_stats *stat = policy->stats; +struct cpufreq_stats *stats = policy->stats;/* Already freed */ -if (!stat) +if (!stats)return ; -pr_debug ("%s: Free stat table\n" , __func__); +pr_debug ("%s: Free stats table\n" , __func__);sysfs_remove_group (&policy->kobj, &stats_attr_group); -kfree (stat->time_in_state); -kfree (stat); +kfree (stats->time_in_state); +kfree (stats); policy->stats =NULL ; }
@@ -166,7 +166,7 @@ static void cpufreq_stats_free_table(unsigned int cpu)static int __cpufreq_stats_create_table (struct cpufreq_policy *policy) {unsigned int i, count =0 , ret =0 ; -struct cpufreq_stats *stat; +struct cpufreq_stats *stats;unsigned int alloc_size;unsigned int cpu = policy->cpu;struct cpufreq_frequency_table *pos, *table;
@@ -179,16 +179,16 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)if (policy->stats)return -EEXIST ; - stat =kzalloc (sizeof (*stat),GFP_KERNEL ); -if ((stat) ==NULL ) + stats =kzalloc (sizeof (*stats),GFP_KERNEL ); +if ((stats) ==NULL )return -ENOMEM ; ret =sysfs_create_group (&policy->kobj, &stats_attr_group);if (ret) goto error_out; - stat->cpu = cpu; - policy->stats = stat; + stats->cpu = cpu; + policy->stats = stats;cpufreq_for_each_valid_entry (pos, table) count++;
@@ -198,31 +198,31 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)#ifdef CONFIG_CPU_FREQ_STAT_DETAILS alloc_size += count * count *sizeof (int );#endif - stat->max_state = count; - stat->time_in_state =kzalloc (alloc_size,GFP_KERNEL ); -if (!stat->time_in_state) { + stats->max_state = count; + stats->time_in_state =kzalloc (alloc_size,GFP_KERNEL ); +if (!stats->time_in_state) { ret = -ENOMEM ; goto error_alloc; } - stat->freq_table = (unsigned int *)(stat->time_in_state + count); + stats->freq_table = (unsigned int *)(stats->time_in_state + count);#ifdef CONFIG_CPU_FREQ_STAT_DETAILS - stat->trans_table = stat->freq_table + count; + stats->trans_table = stats->freq_table + count;#endif i =0 ;cpufreq_for_each_valid_entry (pos, table) -if (freq_table_get_index (stat, pos->frequency) ==-1 ) - stat->freq_table[i++] = pos->frequency; - stat->state_num = i; +if (freq_table_get_index (stats, pos->frequency) ==-1 ) + stats->freq_table[i++] = pos->frequency; + stats->state_num = i;spin_lock (&cpufreq_stats_lock); - stat->last_time =get_jiffies_64 (); - stat->last_index =freq_table_get_index (stat, policy->cur); + stats->last_time =get_jiffies_64 (); + stats->last_index =freq_table_get_index (stats, policy->cur);spin_unlock (&cpufreq_stats_lock);return 0 ; error_alloc:sysfs_remove_group (&policy->kobj, &stats_attr_group); error_out: -kfree (stat); +kfree (stats); policy->stats =NULL ;return ret; }
@@ -273,7 +273,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb, {struct cpufreq_freqs *freq = data;struct cpufreq_policy *policy =cpufreq_cpu_get (freq->cpu); -struct cpufreq_stats *stat; +struct cpufreq_stats *stats;int old_index, new_index;if (val !=CPUFREQ_POSTCHANGE )
@@ -289,26 +289,26 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb, goto put_policy; } - stat = policy->stats; + stats = policy->stats; - old_index = stat->last_index; - new_index =freq_table_get_index (stat, freq->new); + old_index = stats->last_index; + new_index =freq_table_get_index (stats, freq->new); -/* We can't do stat->time_in_state[-1]= .. */ +/* We can't do stats->time_in_state[-1]= .. */ if (old_index ==-1 || new_index ==-1 )return 0 ; -cpufreq_stats_update (stat); +cpufreq_stats_update (stats);if (old_index == new_index)return 0 ;spin_lock (&cpufreq_stats_lock); - stat->last_index = new_index; + stats->last_index = new_index;#ifdef CONFIG_CPU_FREQ_STAT_DETAILS - stat->trans_table[old_index * stat->max_state + new_index]++; + stats->trans_table[old_index * stats->max_state + new_index]++;#endif - stat->total_trans++; + stats->total_trans++;spin_unlock (&cpufreq_stats_lock); put_policy:
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 9/16 cpufreq: Remove (now) unused 'last_cpu' from struct cpufreq_policy
In reply to Viresh Kumar'last_cpu' was used only from cpufreq-stats and isn't used anymore. Get rid of it.
--- drivers/cpufreq/cpufreq.c | 3 --- include/linux/cpufreq.h | 2 -- 2 files changed, 5 deletions(-)
drivers/cpufreq/cpufreq.c +0 -3
@@ -1091,10 +1091,7 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, }down_write (&policy->rwsem); - - policy->last_cpu = policy->cpu; policy->cpu = cpu; -up_write (&policy->rwsem); blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
include/linux/cpufreq.h +0 -2
@@ -66,8 +66,6 @@ struct cpufreq_policy {unsigned int shared_type;/* ACPI: ANY or ALL affected CPUs should set cpufreq */ unsigned int cpu;/* cpu nr of CPU managing this policy */ -unsigned int last_cpu;/* cpu nr of previous CPU that managed -* this policy */ struct clk *clk;struct cpufreq_cpuinfo cpuinfo;/* see above */
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 10/16 cpufreq: stats: drop 'cpu' field of struct cpufreq_stats
In reply to Viresh Kumar'cpu' field of struct cpufreq_stats isn't used anymore and so can be dropped. This change makes cpufreq_stats_update_policy_cpu() empty and so that is removed as well.
--- drivers/cpufreq/cpufreq_stats.c | 12 ------------ 1 file changed, 12 deletions(-)
drivers/cpufreq/cpufreq_stats.c +0 -12
@@ -18,7 +18,6 @@static spinlock_t cpufreq_stats_lock;struct cpufreq_stats { -unsigned int cpu;unsigned int total_trans;unsigned long long last_time;unsigned int max_state;
@@ -187,7 +186,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)if (ret) goto error_out; - stats->cpu = cpu; policy->stats = stats;cpufreq_for_each_valid_entry (pos, table)
@@ -244,22 +242,12 @@ static void cpufreq_stats_create_table(unsigned int cpu)cpufreq_cpu_put (policy); } -static void cpufreq_stats_update_policy_cpu (struct cpufreq_policy *policy) -{ - policy->stats->cpu = policy->cpu; -} -static int cpufreq_stat_notifier_policy (struct notifier_block *nb,unsigned long val,void *data) {int ret =0 ;struct cpufreq_policy *policy = data; -if (val ==CPUFREQ_UPDATE_POLICY_CPU ) { -cpufreq_stats_update_policy_cpu (policy); -return 0 ; - } -if (val ==CPUFREQ_CREATE_POLICY ) ret =__cpufreq_stats_create_table (policy);else if (val ==CPUFREQ_REMOVE_POLICY )
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 11/16 cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications
In reply to Viresh KumarCPUFREQ_UPDATE_POLICY_CPU notifications were used only from cpufreq-stats which doesn't use it anymore. Remove them.
This also decrements values of other notification macros defined after CPUFREQ_UPDATE_POLICY_CPU by 1 to remove gaps. Hopefully all users are using macro's instead of direct numbers and so they wouldn't break as macro values are changed now.
--- drivers/cpufreq/cpufreq.c | 3 --- include/linux/cpufreq.h | 5 ++--- 2 files changed, 2 insertions(+), 6 deletions(-)
drivers/cpufreq/cpufreq.c +0 -3
@@ -1094,9 +1094,6 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, policy->cpu = cpu;up_write (&policy->rwsem); -blocking_notifier_call_chain (&cpufreq_policy_notifier_list, -CPUFREQ_UPDATE_POLICY_CPU , policy); -return 0 ; }
include/linux/cpufreq.h +2 -3
@@ -368,9 +368,8 @@ static inline void cpufreq_resume(void) {}#define CPUFREQ_INCOMPATIBLE (1)#define CPUFREQ_NOTIFY (2)#define CPUFREQ_START (3) -#define CPUFREQ_UPDATE_POLICY_CPU (4) -#define CPUFREQ_CREATE_POLICY (5) -#define CPUFREQ_REMOVE_POLICY (6) +#define CPUFREQ_CREATE_POLICY (4) +#define CPUFREQ_REMOVE_POLICY (5)#ifdef CONFIG_CPU_FREQ int cpufreq_register_notifier (struct notifier_block *nb,unsigned int list);
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 12/16 cpufreq: stats: create sysfs group once we are ready
In reply to Viresh KumarUserspace is free to read value of any file from cpufreq/stats directory once they are created. __cpufreq_stats_create_table() is creating the sysfs files first and then allocating resources for them. Though it would be quite difficult to trigger the racy situation here, but for the sake of keeping sensible code lets create sysfs entries only after we are ready to go.
This also does some makeup to the routine to make it look better.
--- drivers/cpufreq/cpufreq_stats.c | 44 +++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 19 deletions(-)
drivers/cpufreq/cpufreq_stats.c +25 -19
@@ -164,12 +164,13 @@ static void cpufreq_stats_free_table(unsigned int cpu)static int __cpufreq_stats_create_table (struct cpufreq_policy *policy) { -unsigned int i, count =0 , ret =0 ; +unsigned int i =0 , count =0 , ret = -ENOMEM ;struct cpufreq_stats *stats;unsigned int alloc_size;unsigned int cpu = policy->cpu;struct cpufreq_frequency_table *pos, *table; +/* We need cpufreq table for creating stats table */ table =cpufreq_frequency_get_table (cpu);if (unlikely (!table))return 0 ;
@@ -179,15 +180,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)return -EEXIST ; stats =kzalloc (sizeof (*stats),GFP_KERNEL ); -if ((stats) ==NULL ) +if (!stats)return -ENOMEM ; - ret =sysfs_create_group (&policy->kobj, &stats_attr_group); -if (ret) - goto error_out; - - policy->stats = stats; - +/* Find total allocation size */ cpufreq_for_each_valid_entry (pos, table) count++;
@@ -196,32 +192,42 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)#ifdef CONFIG_CPU_FREQ_STAT_DETAILS alloc_size += count * count *sizeof (int );#endif - stats->max_state = count; + +/* Allocate memory for time_in_state/freq_table/trans_table in one go */ stats->time_in_state =kzalloc (alloc_size,GFP_KERNEL ); -if (!stats->time_in_state) { - ret = -ENOMEM ; - goto error_alloc; - } +if (!stats->time_in_state) + goto free_stat; + stats->freq_table = (unsigned int *)(stats->time_in_state + count);#ifdef CONFIG_CPU_FREQ_STAT_DETAILS stats->trans_table = stats->freq_table + count;#endif - i =0 ; + + stats->max_state = count; + +/* Find valid-unique entries */ cpufreq_for_each_valid_entry (pos, table)if (freq_table_get_index (stats, pos->frequency) ==-1 ) stats->freq_table[i++] = pos->frequency; stats->state_num = i; +spin_lock (&cpufreq_stats_lock); stats->last_time =get_jiffies_64 (); stats->last_index =freq_table_get_index (stats, policy->cur);spin_unlock (&cpufreq_stats_lock); -return 0 ; -error_alloc: -sysfs_remove_group (&policy->kobj, &stats_attr_group); -error_out: -kfree (stats); + + policy->stats = stats; + ret =sysfs_create_group (&policy->kobj, &stats_attr_group); +if (!ret) +return 0 ; + +/* We failed, release resources */ policy->stats =NULL ; +kfree (stats->time_in_state); +free_stat: +kfree (stats); +return ret; }
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 13/16 cpufreq: stats: time_in_state can't be NULL in cpufreq_stats_update()
In reply to Viresh Kumar'time_in_state' can't be NULL if 'stats' is valid. These are allocated together and only if time_in_state is allocated successfully, we update policy->stats.
--- drivers/cpufreq/cpufreq_stats.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/cpufreq/cpufreq_stats.c +1 -3
@@ -35,9 +35,7 @@ static int cpufreq_stats_update(struct cpufreq_stats *stats)unsigned long long cur_time =get_jiffies_64 ();spin_lock (&cpufreq_stats_lock); -if (stats->time_in_state) - stats->time_in_state[stats->last_index] += - cur_time - stats->last_time; + stats->time_in_state[stats->last_index] += cur_time - stats->last_time; stats->last_time = cur_time;spin_unlock (&cpufreq_stats_lock);return 0 ;
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 14/16 cpufreq: stats: don't update stats from show_trans_table()
In reply to Viresh Kumarcpufreq_stats_update() updates time_in_state and nothing else. It should ideally be updated only in two cases:
- User requested for the current value of time_in_state.
- We have switched states and so need to update time for the last state.
Currently, we are also doing this while user asks for the transition table of frequencies. It wouldn't do any harm, but no good as well. Its useless here.
Remove it.
--- drivers/cpufreq/cpufreq_stats.c | 1 - 1 file changed, 1 deletion(-)
drivers/cpufreq/cpufreq_stats.c +0 -1
@@ -68,7 +68,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)ssize_t len =0 ;int i, j; -cpufreq_stats_update (stats); len +=snprintf (buf + len,PAGE_SIZE - len," From : To\n" ); len +=snprintf (buf + len,PAGE_SIZE - len," : " );for (i =0 ; i < stats->state_num; i++) {
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 15/16 cpufreq: stats: don't update stats on false notifiers
In reply to Viresh KumarWe need to call cpufreq_stats_update() to update 'time_in_state' for the last frequency. This is achieved by calling it from cpufreq_stat_notifier_trans(), which is called after frequency transition.
But if we detect that the cpu's frequency haven't really changed and its a false POSTCHANGE notification, we don't really need to update time_in_state.
It wouldn't cause any harm in calling cpufreq_stats_update() but we can avoid calling it here and call it when the frequency really changes. The result will be the same but more efficient.
--- drivers/cpufreq/cpufreq_stats.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/cpufreq/cpufreq_stats.c +2 -2
@@ -289,11 +289,11 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,if (old_index ==-1 || new_index ==-1 )return 0 ; -cpufreq_stats_update (stats); -if (old_index == new_index)return 0 ; +cpufreq_stats_update (stats); +spin_lock (&cpufreq_stats_lock); stats->last_index = new_index;#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v3 16/16 cpufreq: stats: drop unnecessary locking
In reply to Viresh KumarThere is no possibility of any race on updating last_index, trans_table or total_trans as these are updated only by cpufreq_stat_notifier_trans() which will be called sequentially.
The only place where locking is still relevant is: cpufreq_stats_update(), which updates time_in_state and last_time. This can be called by two thread in parallel, that may result in races.
The two threads being:
- sysfs read of time_in_state
- and frequency transition that calls cpufreq_stat_notifier_trans().
Remove locking from the first case mentioned above.
--- drivers/cpufreq/cpufreq_stats.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/cpufreq/cpufreq_stats.c +1 -5
@@ -207,12 +207,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)cpufreq_for_each_valid_entry (pos, table)if (freq_table_get_index (stats, pos->frequency) ==-1 ) stats->freq_table[i++] = pos->frequency; - stats->state_num = i; -spin_lock (&cpufreq_stats_lock); + stats->state_num = i; stats->last_time =get_jiffies_64 (); stats->last_index =freq_table_get_index (stats, policy->cur); -spin_unlock (&cpufreq_stats_lock); policy->stats = stats; ret =sysfs_create_group (&policy->kobj, &stats_attr_group);
@@ -294,13 +292,11 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,cpufreq_stats_update (stats); -spin_lock (&cpufreq_stats_lock); stats->last_index = new_index;#ifdef CONFIG_CPU_FREQ_STAT_DETAILS stats->trans_table[old_index * stats->max_state + new_index]++;#endif stats->total_trans++; -spin_unlock (&cpufreq_stats_lock); put_policy:cpufreq_cpu_put (policy);
Signature · 1 line
2.2.0
Viresh Kumar
maintainerQuoted 4 lines · Viresh Kumar
Hi Rafael,
This is V3 of the stats cleanup I sent earlier. Few things are improved based on the feedback received (mostly from you). Please see if it looks any better.
Any more inputs ?