cpufreq: stats: cleanups
Viresh Kumar
maintainerv2 cover cpufreq: stats: cleanups
This is a resend of cpufreq stats cleanups I had for sometime. Few issues were reported from Fenguang's bot and are all fixed now and are stable..
V1->V2:
- Reviewed-by Prarit
- Replaced lock with mutex as we may sleep from within critical region and so a new patch "cpufreq: stats: replace spinlock with mutex".
Pushed here:git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/stats/cleanups
Viresh Kumar (14):
cpufreq: stats: don't break strings into multiple lines
cpufreq: stats: return -EEXIST when stats are already allocated
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: Remove (now) unused 'last_cpu' from struct cpufreq_policy
cpufreq: stats: remove cpufreq_stats_update_policy_cpu()
cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications
cpufreq: stats: create sysfs group once we are ready
cpufreq: stats: don't update stats from show_trans_table()
cpufreq: stats: don't update stats on false notifiers
cpufreq: stats: replace spinlock with mutex
cpufreq: stats: Fix locking
cpufreq: stats: call cpufreq_stats_update() with locks held
drivers/cpufreq/cpufreq.c | 6 -- drivers/cpufreq/cpufreq_stats.c | 148 ++++++++++++++++++---------------------- include/linux/cpufreq.h | 10 +-- 3 files changed, 73 insertions(+), 91 deletions(-)
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v2 1/14 cpufreq: stats: don't break strings into multiple lines
The MODULE_DESCRIPTION() string is broken into multiple lines just to make checkpatch happy. Its less readable now and prone to errors. Fix it.
--- 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 ("'cpufreq_stats' - exports cpufreq stats through sysfs filesystem" );MODULE_LICENSE ("GPL" );module_init (cpufreq_stats_init);
Signature · 1 line
2.2.0
Re: [PATCH V2 01/14] cpufreq: stats: don't break strings into multiple lines
Quoted 14 lines · Viresh Kumar
The
MODULE_DESCRIPTION()string is broken into multiple lines just to make checkpatch happy. Its less readable now and prone to errors. Fix it.Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_stats.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/cpufreq/cpufreq_stats.c +0
-0
drivers/cpufreq/cpufreq_stats.c static void __exit cpufreq_stats_exit(void) · line 377
Quoted 6 lines
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 ("'cpufreq_stats' - exports cpufreq stats through sysfs filesystem" );
What about
+MODULE_DESCRIPTION("Export cpufreq stats via sysfs");
This doesn't seem to be substantially less information than the original to me.
Quoted 4 lines
drivers/cpufreq/cpufreq_stats.c +0
-0MODULE_LICENSE ("GPL" );module_init (cpufreq_stats_init);
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 01/14] cpufreq: stats: don't break strings into multiple lines
Quoted 5 lines · Rafael J. Wysocki
What about
+MODULE_DESCRIPTION("Export cpufreq stats via sysfs");
This doesn't seem to be substantially less information than the original to me.
Ack.
Viresh Kumar
maintainerPATCH v2 2/14 cpufreq: stats: return -EEXIST when stats are already allocated
In reply to Viresh Kumar__cpufreq_stats_create_table() is called from:
- policy create notifier: stats will always be NULL here
- cpufreq_stats_init() calls it for all CPUs as cpufreq-stats can be initialized after cpufreq driver. Because CPUs share clock lines, 'stats' will be NULL here for the first cpu only and will return back for others.
While we return for other CPUs, we don't return the right error value. We must return -EEXIST, as that is the case here.
--- 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
Re: [PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated
Quoted 8 lines · Viresh Kumar
__cpufreq_stats_create_table()is called from:
- policy create notifier: stats will always be NULL here
-cpufreq_stats_init()calls it for all CPUs as cpufreq-stats can be initialized after cpufreq driver. Because CPUs share clock lines, 'stats' will be NULL here for the first cpu only and will return back for others.While we return for other CPUs, we don't return the right error value. We must return
-EEXIST, as that is the case here.
What exactly is wrong with -EBUSY, then?
Quoted 23 lines
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- 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 · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated
Quoted 11 lines · Rafael J. Wysocki
Quoted 8 lines · Viresh Kumar
__cpufreq_stats_create_table()is called from:
- policy create notifier: stats will always be NULL here
-cpufreq_stats_init()calls it for all CPUs as cpufreq-stats can be initialized after cpufreq driver. Because CPUs share clock lines, 'stats' will be NULL here for the first cpu only and will return back for others.While we return for other CPUs, we don't return the right error value. We must return
-EEXIST, as that is the case here.What exactly is wrong with
-EBUSY, then?
Its not that could would fail with EBUSY but this is what I thought. Generally:
- Returning EBUSY means: we are busy right not try again. And that might be immediate.
- Returning EEXIST means: 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.
That's all..
Re: [PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated
Quoted 11 lines · Viresh Kumar
Quoted 10 lines · Rafael J. Wysocki
Quoted 8 lines · Viresh Kumar
__cpufreq_stats_create_table()is called from:
- policy create notifier: stats will always be NULL here
-cpufreq_stats_init()calls it for all CPUs as cpufreq-stats can be initialized after cpufreq driver. Because CPUs share clock lines, 'stats' will be NULL here for the first cpu only and will return back for others.While we return for other CPUs, we don't return the right error value. We must return
-EEXIST, as that is the case here.
Quoted 8 lines
Quoted 1 line
What exactly is wrong with
-EBUSY, then?Its not that could would fail with EBUSY but this is what I thought. Generally:
- Returning EBUSY means: we are busy right not try again. And that might be immediate.
- Returning EEXIST means: 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.
Now note how much useful the above is than the stuff in your original changelog.
I'd say all depends on whether or not the callers deal with the error code appropriately. If it is propagated all the way to user space, it really should *not* be -EBUSY, but otherwise this is a matter of how the callers handle the given error code.
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated
Quoted 1 line · Rafael J. Wysocki
Now note how much useful the above is than the stuff in your original changelog.
Yes, its all in logs now :)
Quoted 4 lines
I'd say all depends on whether or not the callers deal with the error code appropriately. If it is propagated all the way to user space, it really should *not* be
-EBUSY, but otherwise this is a matter of how the callers handle the given error code.
It might not get propagated to user space for now, but not sure if that wouldn't change in future. Even if it stays only within the kernel, we better return the error codes that fit the best for a particular situation and so I would still like
to get this through.
Viresh Kumar
maintainerPATCH v2 3/14 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
@@ -174,8 +174,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 v2 4/14 cpufreq: stats: pass 'stat' to cpufreq_stats_update()
In reply to Viresh Kumarcpufreq_stats_update() needs only 'stat' pointer for its use and nothing else. Currently it works fine as we have a per-cpu variable for storing stats. But later commits would remove it and hence we better pass 'stat' to cpufreq_stats_update().
--- drivers/cpufreq/cpufreq_stats.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/cpufreq/cpufreq_stats.c +5 -8
@@ -38,14 +38,11 @@ struct cpufreq_stats_attribute {ssize_t (*show) (struct cpufreq_stats *,char *); }; -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; +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) stat->time_in_state[stat->last_index] += cur_time - stat->last_time;
@@ -70,7 +67,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)
@@ -88,7 +85,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++) {
@@ -313,7 +310,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
Re: [PATCH V2 04/14] cpufreq: stats: pass 'stat' to cpufreq_stats_update()
Quoted 4 lines · Viresh Kumar
cpufreq_stats_update()needs only 'stat' pointer for its use and nothing else. Currently it works fine as we have a per-cpu variable for storing stats. But later commits would remove it and hence we better pass 'stat' tocpufreq_stats_update().
What about:
"It is better to pass a struct cpufreq_stats pointer to cpufreq_stats_update()
instead of a CPU number, because ..."
?
Quoted 12 lines
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_stats.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)drivers/cpufreq/cpufreq_stats.c +0
-0@@ -38,14 +38,11 @@ struct cpufreq_stats_attribute {ssize_t (*show) (struct cpufreq_stats *,char *);
drivers/cpufreq/cpufreq_stats.c struct cpufreq_stats_attribute { · line 46
Quoted 10 lines
drivers/cpufreq/cpufreq_stats.c +2
-4}; -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; +unsigned long long cur_time =get_jiffies_64 (); - cur_time =get_jiffies_64 ();
Unrelated change.
Quoted 33 lines
drivers/cpufreq/cpufreq_stats.c +3
-4spin_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;@@ -70,7 +67,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)@@ -88,7 +85,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++) {@@ -313,7 +310,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 · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 04/14] cpufreq: stats: pass 'stat' to cpufreq_stats_update()
Quoted 4 lines · Rafael J. Wysocki
What about:
"It is better to pass a struct
cpufreq_statspointer tocpufreq_stats_update()
instead of a CPU number, because ..."
Sure.
Quoted 9 lines
drivers/cpufreq/cpufreq_stats.c struct cpufreq_stats_attribute { · line 46
Quoted 7 lines
drivers/cpufreq/cpufreq_stats.c +2
-3+static int cpufreq_stats_update (struct cpufreq_stats *stat) { -struct cpufreq_stats *stat; -unsigned long long cur_time; +unsigned long long cur_time =get_jiffies_64 (); - cur_time =get_jiffies_64 ();Unrelated change.
Yeah, it was too small of a change so not that significant as well. And so folded it into this commit only :(
Will send it separately in v3..
A message that is not in the archive (sent off-list or not yet received): OF5E9CEA82.619493CC-ONC1257DC4.0027BC08-C1257DC4.0027BC14@lonatigroup.com
Viresh Kumar
maintainerRe: [PATCH V2 04/14] cpufreq: stats: pass 'stat' to cpufreq_stats_update() *
Quoted 6 lines · Luigi Fioretti
Hi,
thanks for your help but I don't think to be solution. I had apply this patch :http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036055.html
Quoted 7 lines
You can read the first problems here:
http://lists.linaro.org/pipermail/linaro-kernel/2014-March/012635.htmlI had found a solution to this problem, and I write to you into my first e-mail.
Now I have this problems:
When resume and run this function
What does all this has to do with the mail below ?
Quoted 39 lines
[email protected] ha scritto: -----
Per: "Rafael J. Wysocki" <[email protected]>
Da: Viresh Kumar
Inviato da: [email protected]
Data: 05/01/2015 05.05AM
Cc: Prarit Bhargava <[email protected]>, Lists linaro-kernel <[email protected]>, Saravana Kannan <[email protected]>, "[email protected]" <[email protected]>
Oggetto: Re: [PATCH V2 04/14] cpufreq: stats: pass 'stat' tocpufreq_stats_update()*Quoted 5 lines · Rafael J. Wysocki
What about:
"It is better to pass a struct
cpufreq_statspointer tocpufreq_stats_update()
instead of a CPU number, because ..."Sure.
Quoted 9 lines
drivers/cpufreq/cpufreq_stats.c struct cpufreq_stats_attribute { · line 46
Quoted 7 lines
drivers/cpufreq/cpufreq_stats.c +2
-3+static int cpufreq_stats_update (struct cpufreq_stats *stat) { -struct cpufreq_stats *stat; -unsigned long long cur_time; +unsigned long long cur_time =get_jiffies_64 (); - cur_time =get_jiffies_64 ();Unrelated change.
Yeah, it was too small of a change so not that significant as well. And so folded it into this commit only :(
Will send it separately in v3..
_______________________________________________
linaro-kernel mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/linaro-kernel
Viresh Kumar
maintainerPATCH v2 5/14 cpufreq: stats: get rid of per-cpu cpufreq_stats_table
In reply to Viresh KumarBecause all CPUs in a policy share clock line, they share stats as well. But cpufreq-stats maintains a per-cpu list of tables for managing this. Get rid of it by assigning a special field for stats in 'struct cpufreq_policy'.
--- drivers/cpufreq/cpufreq_stats.c | 51 ++++++++++++++++++----------------------- include/linux/cpufreq.h | 3 +++ 2 files changed, 25 insertions(+), 29 deletions(-)
drivers/cpufreq/cpufreq_stats.c +22 -29
@@ -31,8 +31,6 @@ struct cpufreq_stats {#endif }; -static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table); -struct cpufreq_stats_attribute {struct attribute attr;ssize_t (*show) (struct cpufreq_stats *,char *);
@@ -53,20 +51,17 @@ 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); +struct cpufreq_stats *stat = policy->stats_data; + +return sprintf (buf,"%d\n" , stat->total_trans); }static ssize_t show_time_in_state (struct cpufreq_policy *policy,char *buf) { +struct cpufreq_stats *stat = policy->stats_data;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],
@@ -79,12 +74,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_data;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," : " );
@@ -150,8 +143,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_data; +/* Already freed */ if (!stat)return ;
@@ -160,7 +154,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_data =NULL ; }static void cpufreq_stats_free_table (unsigned int cpu)
@@ -189,7 +183,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_data)return -EEXIST ; stat =kzalloc (sizeof (*stat),GFP_KERNEL );
@@ -201,7 +195,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_data = stat;cpufreq_for_each_valid_entry (pos, table) count++;
@@ -236,7 +230,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_data =NULL ;return ret; }
@@ -259,14 +253,8 @@ 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 ; +struct cpufreq_stats *stat = policy->stats_data; + stat->cpu = policy->cpu; }
@@ -293,15 +281,19 @@ 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) -return 0 ; +if (!policy) { +pr_err ("%s: No policy found\n" , __func__); +return -ENODEV ; + } + + stat = policy->stats_data; old_index = stat->last_index; new_index =freq_table_get_index (stat, freq->new);
@@ -322,6 +314,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,#endif stat->total_trans++;spin_unlock (&cpufreq_stats_lock); +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 data */ +void *stats_data; +/* For cpufreq driver's internal use */ void *driver_data; };
Signature · 1 line
2.2.0
Re: [PATCH V2 05/14] cpufreq: stats: get rid of per-cpu cpufreq_stats_table
Quoted 1 line · Viresh Kumar
Because all CPUs in a policy share clock line, they share stats as well.
"All CPUs sharing a cpufreq policy share stats too."
Sharing a clock line is irrelevant here.
Quoted 2 lines
But cpufreq-stats maintains a per-cpu list of tables for managing this. Get rid of it by assigning a special field for stats in 'struct
cpufreq_policy'.
"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."
Quoted 23 lines
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_stats.c | 51 ++++++++++++++++++----------------------- include/linux/cpufreq.h | 3 +++ 2 files changed, 25 insertions(+), 29 deletions(-)drivers/cpufreq/cpufreq_stats.c +0
-2@@ -31,8 +31,6 @@ struct cpufreq_stats {#endif }; -static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table); -struct cpufreq_stats_attribute {struct attribute attr;ssize_t (*show) (struct cpufreq_stats *,char *);@@ -53,20 +51,17 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stats_update(struct cpufreq_stats *stat) · line 56
Quoted 10 lines
drivers/cpufreq/cpufreq_stats.c +3
-5static 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); +struct cpufreq_stats *stat = policy->stats_data; + +return sprintf (buf,"%d\n" , stat->total_trans);
First off, why do you call the new field stats_data instead of stats? Moreover:
+ return sprintf(buf, "%d\n", policy->stats->total_trans);
and then you don't need the extra line above.
Quoted 80 lines
drivers/cpufreq/cpufreq_stats.c +9
-12}static ssize_t show_time_in_state (struct cpufreq_policy *policy,char *buf) { +struct cpufreq_stats *stat = policy->stats_data;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],@@ -79,12 +74,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_data;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," : " );@@ -150,8 +143,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_data; +/* Already freed */ if (!stat)return ;@@ -160,7 +154,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_data =NULL ; }static void cpufreq_stats_free_table (unsigned int cpu)@@ -189,7 +183,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_data)return -EEXIST ; stat =kzalloc (sizeof (*stat),GFP_KERNEL );@@ -201,7 +195,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_data = stat;cpufreq_for_each_valid_entry (pos, table) count++;@@ -236,7 +230,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_data =NULL ;return ret; }@@ -259,14 +253,8 @@ 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,
drivers/cpufreq/cpufreq_stats.c static void cpufreq_stats_create_table(unsigned int cpu) · line 258
Quoted 10 lines
drivers/cpufreq/cpufreq_stats.c +2
-7- 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 ; +struct cpufreq_stats *stat = policy->stats_data; + stat->cpu = policy->cpu;
Why don't you change this line to
policy->stats->cpu = policy->cpu;
and then you don't need the extra variable any more?
Quoted 8 lines
drivers/cpufreq/cpufreq_stats.c +1
-0}@@ -293,15 +281,19 @@ 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;
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 292
Quoted 10 lines
drivers/cpufreq/cpufreq_stats.c +2
-3int old_index, new_index;if (val !=CPUFREQ_POSTCHANGE )return 0 ; - stat =per_cpu (cpufreq_stats_table, freq->cpu); -if (!stat) -return 0 ; +if (!policy) { +pr_err ("%s: No policy found\n" , __func__);
Do we need the extra message here? If so, can it be pr_debug()?
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 294
Quoted 2 lines
drivers/cpufreq/cpufreq_stats.c +2
-0+return -ENODEV ; + }
This changes the behavior in a subtle way. Instead of returning 0 when stats are not present, we'll now return an error code for a missing policy. Are you sure that this isn't going to break anything?
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 296
Quoted 2 lines
drivers/cpufreq/cpufreq_stats.c +2
-0+ + stat = policy->stats_data;
Is stat guaraneed to be a valid pointer at this point?
Quoted 16 lines
drivers/cpufreq/cpufreq_stats.c +1
-0old_index = stat->last_index; new_index =freq_table_get_index (stat, freq->new);@@ -322,6 +314,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,#endif stat->total_trans++;spin_unlock (&cpufreq_stats_lock); +cpufreq_cpu_put (policy);return 0 ; }include/linux/cpufreq.h +0
-0
include/linux/cpufreq.h struct cpufreq_policy { · line 117
Quoted 5 lines
include/linux/cpufreq.h +2
-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 data */ +void *stats_data;
Why isn't that
struct cpufreq_stats *stats;
?
Quoted 5 lines
include/linux/cpufreq.h +1
-0+/* For cpufreq driver's internal use */ void *driver_data; };
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 05/14] cpufreq: stats: get rid of per-cpu cpufreq_stats_table
On 4 January 2015 at 03:50, Rafael J. Wysocki <[email protected]> wrote:
All comments accepted before this ..
Quoted 11 lines
Quoted 11 lines
drivers/cpufreq/cpufreq_stats.c +1
-0@@ -293,15 +281,19 @@ 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 ;
Quoted 7 lines
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 292
Quoted 5 lines
drivers/cpufreq/cpufreq_stats.c +2
-3- stat =per_cpu (cpufreq_stats_table, freq->cpu); -if (!stat) -return 0 ; +if (!policy) { +pr_err ("%s: No policy found\n" , __func__);Do we need the extra message here? If so, can it be
pr_debug()?
Because we are in the POSTCHANGE stage right now, the policy isn't allowed to be NULL. And so this is an error and I used pr_err.
Quoted 6 lines
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 294
Quoted 2 lines
drivers/cpufreq/cpufreq_stats.c +2
-0+return -ENODEV ; + }This changes the behavior in a subtle way. Instead of returning 0 when stats are not present, we'll now return an error code for a missing policy. Are you sure that this isn't going to break anything?
Something I didn't looked at earlier. This *can* break things a bit. If I understand it clearly now, the notifiers aren't allowed to return these errors. They should return:
NOTIFY_DONE or NOTIFY_OK or NOTIFY_BAD
And this is what the notifier chain does to the return value:
if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
break;So, if the return error has its 15th bit set, then we will break the loop. (NOTIFY_STOP_MASK 0x8000).
It wouldn't happen in this case, but probably we should just return NOTIFY_DONE (0)..
Right ?
Quoted 4 lines
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 296
Quoted 2 lines
drivers/cpufreq/cpufreq_stats.c +2
-0+ + stat = policy->stats_data;Is stat guaraneed to be a valid pointer at this point?
We better check it as well for some corner cases where we failed to create a table.
Quoted 5 lines
include/linux/cpufreq.h struct cpufreq_policy { · line 117
Quoted 1 line
include/linux/cpufreq.h +1
-0+void *stats_data;Why isn't that
struct cpufreq_stats *stats;
It will be.
Re: [PATCH V2 05/14] cpufreq: stats: get rid of per-cpu cpufreq_stats_table
Quoted 44 lines · Viresh Kumar
On 4 January 2015 at 03:50, Rafael J. Wysocki <[email protected]> wrote:
All comments accepted before this ..
Quoted 11 lines
Quoted 11 lines
drivers/cpufreq/cpufreq_stats.c +1
-0@@ -293,15 +281,19 @@ 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 ;Quoted 7 lines
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 292
Quoted 5 lines
drivers/cpufreq/cpufreq_stats.c +2
-3- stat =per_cpu (cpufreq_stats_table, freq->cpu); -if (!stat) -return 0 ; +if (!policy) { +pr_err ("%s: No policy found\n" , __func__);Do we need the extra message here? If so, can it be
pr_debug()?Because we are in the POSTCHANGE stage right now, the policy isn't allowed to be NULL. And so this is an error and I used
pr_err.Quoted 6 lines
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 294
Quoted 2 lines
drivers/cpufreq/cpufreq_stats.c +2
-0+return -ENODEV ; + }This changes the behavior in a subtle way. Instead of returning 0 when stats are not present, we'll now return an error code for a missing policy. Are you sure that this isn't going to break anything?
Something I didn't looked at earlier. This *can* break things a bit. If I understand it clearly now, the notifiers aren't allowed to return these errors. They should return:
NOTIFY_DONEorNOTIFY_OKorNOTIFY_BADAnd this is what the notifier chain does to the return value:
if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK) break;
Quoted 7 lines
So, if the return error has its 15th bit set, then we will break the loop. (
NOTIFY_STOP_MASK0x8000).It wouldn't happen in this case, but probably we should just return
NOTIFY_DONE(0)..Right ?
Looks correct to me.
Quoted 7 lines
Quoted 4 lines
drivers/cpufreq/cpufreq_stats.c static int cpufreq_stat_notifier_trans(struct notifier_block *nb, · line 296
Quoted 2 lines
drivers/cpufreq/cpufreq_stats.c +2
-0+ + stat = policy->stats_data;Is stat guaraneed to be a valid pointer at this point?
We better check it as well for some corner cases where we failed to create a table.
Exactly. :-)
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerPATCH v2 6/14 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 v2 7/14 cpufreq: stats: remove cpufreq_stats_update_policy_cpu()
In reply to Viresh Kumarcpufreq_stats_update_policy_cpu() doesn't do any useful stuff anymore and must be removed. Along with it, also remove 'cpu' field from 'struct cpufreq_stats' as that isn't used as well.
--- drivers/cpufreq/cpufreq_stats.c | 14 -------------- 1 file changed, 14 deletions(-)
drivers/cpufreq/cpufreq_stats.c +0 -14
@@ -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;
@@ -194,7 +193,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)if (ret) goto error_out; - stat->cpu = cpu; policy->stats_data = stat;cpufreq_for_each_valid_entry (pos, table)
@@ -251,24 +249,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) -{ -struct cpufreq_stats *stat = policy->stats_data; - - stat->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
Re: [PATCH V2 07/14] cpufreq: stats: remove cpufreq_stats_update_policy_cpu()
Quoted 2 lines · Viresh Kumar
cpufreq_stats_update_policy_cpu()doesn't do any useful stuff anymore and must be removed.
Well, "can be" sounds more appropriate than "must be". It wouldn't hurt to leave it as is, after all.
Quoted 2 lines
Along with it, also remove 'cpu' field from 'struct
cpufreq_stats' as that isn't used as well.
It's the reverse. Because the cpu field is not necessary any more, it can be dropped *along* *with* cpufreq_stats_update_policy_cpu().
Modifying data structures is more important than dropping a trivial function and it should be described this way in the changelog.
Quoted 27 lines
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_stats.c | 14 -------------- 1 file changed, 14 deletions(-)drivers/cpufreq/cpufreq_stats.c +0
-2@@ -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;@@ -194,7 +193,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)if (ret) goto error_out; - stat->cpu = cpu; policy->stats_data = stat;cpufreq_for_each_valid_entry (pos, table)
drivers/cpufreq/cpufreq_stats.c static void cpufreq_stats_create_table(unsigned int cpu) · line 258
Quoted 8 lines
drivers/cpufreq/cpufreq_stats.c +0
-5@@ -251,24 +249,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) -{ -struct cpufreq_stats *stat = policy->stats_data; - - stat->cpu = policy->cpu;
Well, if you wrote it the way I suggested in the previous patch, it would be more obvious ...
Quoted 17 lines
drivers/cpufreq/cpufreq_stats.c +0
-7-} -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 · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 07/14] cpufreq: stats: remove cpufreq_stats_update_policy_cpu()
Quoted 11 lines · Rafael J. Wysocki
Well, "can be" sounds more appropriate than "must be". It wouldn't hurt to leave it as is, after all.
It's the reverse. Because the cpu field is not necessary any more, it can be dropped *along* *with*
cpufreq_stats_update_policy_cpu().Modifying data structures is more important than dropping a trivial function and it should be described this way in the changelog.
Well, if you wrote it the way I suggested in the previous patch, it would be more obvious ...
Ack.
Viresh Kumar
maintainerPATCH v2 8/14 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 fixes the value of other notification macros, hopefully all users are using macro's instead of direct values and nothing will break :)
--- 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
Re: [PATCH V2 08/14] cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications
Quoted 5 lines · Viresh Kumar
CPUFREQ_UPDATE_POLICY_CPUnotifications were used only from cpufreq-stats which doesn't use it anymore. Remove them.This also fixes the value of other notification macros, hopefully all users are using macro's instead of direct values and nothing will break :)
The paragraph above is completely unclear to me. Do you mean that it changes the definitions of CPUFREQ_CREATE_POLICY and CPUFREQ_REMOVE_POLICY too which may affect code that uses numbers instead of these symbols?
Quoted 38 lines
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- 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 · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 08/14] cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications
Quoted 10 lines · Rafael J. Wysocki
Quoted 5 lines · Viresh Kumar
CPUFREQ_UPDATE_POLICY_CPUnotifications were used only from cpufreq-stats which doesn't use it anymore. Remove them.This also fixes the value of other notification macros, hopefully all users are using macro's instead of direct values and nothing will break :)
The paragraph above is completely unclear to me. Do you mean that it changes the definitions of
CPUFREQ_CREATE_POLICYandCPUFREQ_REMOVE_POLICYtoo which may affect code that uses numbers instead of these symbols?
Yeah. I will try to rewrite it a bit..
Viresh Kumar
maintainerPATCH v2 9/14 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
@@ -171,12 +171,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 *stat;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 ;
@@ -186,15 +187,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)return -EEXIST ; stat =kzalloc (sizeof (*stat),GFP_KERNEL ); -if ((stat) ==NULL ) +if (!stat)return -ENOMEM ; - ret =sysfs_create_group (&policy->kobj, &stats_attr_group); -if (ret) - goto error_out; - - policy->stats_data = stat; - +/* Find total allocation size */ cpufreq_for_each_valid_entry (pos, table) count++;
@@ -203,32 +199,42 @@ 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; + +/* Allocate memory for time_in_state/freq_table/trans_table in one go */ stat->time_in_state =kzalloc (alloc_size,GFP_KERNEL ); -if (!stat->time_in_state) { - ret = -ENOMEM ; - goto error_alloc; - } +if (!stat->time_in_state) + goto free_stat; + stat->freq_table = (unsigned int *)(stat->time_in_state + count);#ifdef CONFIG_CPU_FREQ_STAT_DETAILS stat->trans_table = stat->freq_table + count;#endif - i =0 ; + + stat->max_state = count; + +/* Find valid-unique entries */ 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; +spin_lock (&cpufreq_stats_lock); stat->last_time =get_jiffies_64 (); stat->last_index =freq_table_get_index (stat, policy->cur);spin_unlock (&cpufreq_stats_lock); -return 0 ; -error_alloc: -sysfs_remove_group (&policy->kobj, &stats_attr_group); -error_out: -kfree (stat); + + policy->stats_data = stat; + ret =sysfs_create_group (&policy->kobj, &stats_attr_group); +if (!ret) +return 0 ; + +/* We failed, release resources */ policy->stats_data =NULL ; +kfree (stat->time_in_state); +free_stat: +kfree (stat); +return ret; }
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v2 10/14 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
@@ -77,7 +77,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)ssize_t len =0 ;int i, j; -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++) {
Signature · 1 line
2.2.0
Viresh Kumar
maintainerPATCH v2 11/14 cpufreq: stats: don't update stats on false notifiers
In reply to Viresh KumarNo idea when 'old_index == new_index' in transition notifier (probably never?), but if this happens, we aren't supposed to update time_in_state by calling cpufreq_stats_update(). Its only relevant when we are changing frequencies.
So, move the call after matching indexes.
--- drivers/cpufreq/cpufreq_stats.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/cpufreq/cpufreq_stats.c +2 -2
@@ -293,11 +293,11 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,if (old_index ==-1 || new_index ==-1 )return 0 ; -cpufreq_stats_update (stat); -if (old_index == new_index)return 0 ; +cpufreq_stats_update (stat); +spin_lock (&cpufreq_stats_lock); stat->last_index = new_index;#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
Signature · 1 line
2.2.0
Re: [PATCH V2 11/14] cpufreq: stats: don't update stats on false notifiers
Quoted 1 line · Viresh Kumar
No idea when '
old_index==new_index' in transition notifier (probably never?),
This piece of information is completely irrelevant here.
Quoted 2 lines
but if this happens, we aren't supposed to update
time_in_stateby callingcpufreq_stats_update().
"..., because ...".
Quoted 29 lines
Its only relevant when we are changing frequencies.
So, move the call after matching indexes.
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_stats.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/cpufreq/cpufreq_stats.c +2
-2@@ -293,11 +293,11 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,if (old_index ==-1 || new_index ==-1 )return 0 ; -cpufreq_stats_update (stat); -if (old_index == new_index)return 0 ; +cpufreq_stats_update (stat); +spin_lock (&cpufreq_stats_lock); stat->last_index = new_index;#ifdef CONFIG_CPU_FREQ_STAT_DETAILS
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerPATCH v2 12/14 cpufreq: stats: replace spinlock with mutex
In reply to Viresh KumarLater commit will allocate memory from within the spinlock as that would be part of critical region. kzalloc() might sleep and we can't sleep from inside spinlocks critical region, so convert the spinlock into a mutex.
--- drivers/cpufreq/cpufreq_stats.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/cpufreq/cpufreq_stats.c +8 -8
@@ -12,10 +12,11 @@#include <linux/cpu.h> #include <linux/cpufreq.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/slab.h> #include <linux/cputime.h> -static spinlock_t cpufreq_stats_lock; +static DEFINE_MUTEX (cpufreq_stats_lock);struct cpufreq_stats {unsigned int total_trans;
@@ -39,12 +40,12 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat) {unsigned long long cur_time =get_jiffies_64 (); -spin_lock (&cpufreq_stats_lock); +mutex_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; -spin_unlock (&cpufreq_stats_lock); +mutex_unlock (&cpufreq_stats_lock);return 0 ; }
@@ -218,10 +219,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) stat->freq_table[i++] = pos->frequency; stat->state_num = i; -spin_lock (&cpufreq_stats_lock); +mutex_lock (&cpufreq_stats_lock); stat->last_time =get_jiffies_64 (); stat->last_index =freq_table_get_index (stat, policy->cur); -spin_unlock (&cpufreq_stats_lock); +mutex_unlock (&cpufreq_stats_lock); policy->stats_data = stat; ret =sysfs_create_group (&policy->kobj, &stats_attr_group);
@@ -298,13 +299,13 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,cpufreq_stats_update (stat); -spin_lock (&cpufreq_stats_lock); +mutex_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]++;#endif stat->total_trans++; -spin_unlock (&cpufreq_stats_lock); +mutex_unlock (&cpufreq_stats_lock);cpufreq_cpu_put (policy);return 0 ; }
@@ -322,7 +323,6 @@ static int __init cpufreq_stats_init(void)int ret;unsigned int cpu; -spin_lock_init (&cpufreq_stats_lock); ret =cpufreq_register_notifier (¬ifier_policy_block,CPUFREQ_POLICY_NOTIFIER );if (ret)
Signature · 1 line
2.2.0
Re: [PATCH V2 12/14] cpufreq: stats: replace spinlock with mutex
Quoted 3 lines · Viresh Kumar
Later commit will allocate memory from within the spinlock as that would be part of critical region.
kzalloc()might sleep and we can't sleep from inside spinlocks critical region, so convert the spinlock into a mutex.
"We will need to allocate memory under cpufreq_stats_lock, so it cannot be
a spinlock any more. Convert it to a mutex."
Quoted 76 lines
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_stats.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)drivers/cpufreq/cpufreq_stats.c +8
-8@@ -12,10 +12,11 @@#include <linux/cpu.h> #include <linux/cpufreq.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/slab.h> #include <linux/cputime.h> -static spinlock_t cpufreq_stats_lock; +static DEFINE_MUTEX (cpufreq_stats_lock);struct cpufreq_stats {unsigned int total_trans;@@ -39,12 +40,12 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat) {unsigned long long cur_time =get_jiffies_64 (); -spin_lock (&cpufreq_stats_lock); +mutex_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; -spin_unlock (&cpufreq_stats_lock); +mutex_unlock (&cpufreq_stats_lock);return 0 ; }@@ -218,10 +219,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) stat->freq_table[i++] = pos->frequency; stat->state_num = i; -spin_lock (&cpufreq_stats_lock); +mutex_lock (&cpufreq_stats_lock); stat->last_time =get_jiffies_64 (); stat->last_index =freq_table_get_index (stat, policy->cur); -spin_unlock (&cpufreq_stats_lock); +mutex_unlock (&cpufreq_stats_lock); policy->stats_data = stat; ret =sysfs_create_group (&policy->kobj, &stats_attr_group);@@ -298,13 +299,13 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,cpufreq_stats_update (stat); -spin_lock (&cpufreq_stats_lock); +mutex_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]++;#endif stat->total_trans++; -spin_unlock (&cpufreq_stats_lock); +mutex_unlock (&cpufreq_stats_lock);cpufreq_cpu_put (policy);return 0 ; }@@ -322,7 +323,6 @@ static int __init cpufreq_stats_init(void)int ret;unsigned int cpu; -spin_lock_init (&cpufreq_stats_lock); ret =cpufreq_register_notifier (¬ifier_policy_block,CPUFREQ_POLICY_NOTIFIER );if (ret)
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerPATCH v2 13/14 cpufreq: stats: Fix locking
In reply to Viresh KumarLocking wasn't handled properly at places and this patch is an attempt to fix it. Specially while creating/removing stat tables.
--- drivers/cpufreq/cpufreq_stats.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/cpufreq/cpufreq_stats.c +12 -6
@@ -150,10 +150,12 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)pr_debug ("%s: Free stat table\n" , __func__); +mutex_lock (&cpufreq_stats_lock);sysfs_remove_group (&policy->kobj, &stats_attr_group);kfree (stat->time_in_state);kfree (stat); policy->stats_data =NULL ; +mutex_unlock (&cpufreq_stats_lock); }static void cpufreq_stats_free_table (unsigned int cpu)
@@ -182,13 +184,17 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)if (unlikely (!table))return 0 ; +mutex_lock (&cpufreq_stats_lock); +/* stats already initialized */ -if (policy->stats_data) -return -EEXIST ; +if (policy->stats_data) { + ret = -EEXIST ; + goto unlock; + } stat =kzalloc (sizeof (*stat),GFP_KERNEL );if (!stat) -return -ENOMEM ; + goto unlock;/* Find total allocation size */ cpufreq_for_each_valid_entry (pos, table)
@@ -219,21 +225,21 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) stat->freq_table[i++] = pos->frequency; stat->state_num = i; -mutex_lock (&cpufreq_stats_lock); stat->last_time =get_jiffies_64 (); stat->last_index =freq_table_get_index (stat, policy->cur); -mutex_unlock (&cpufreq_stats_lock); policy->stats_data = stat; ret =sysfs_create_group (&policy->kobj, &stats_attr_group);if (!ret) -return 0 ; + goto unlock;/* We failed, release resources */ policy->stats_data =NULL ;kfree (stat->time_in_state); free_stat:kfree (stat); +unlock: +mutex_unlock (&cpufreq_stats_lock);return ret; }
Signature · 1 line
2.2.0
Re: [PATCH V2 13/14] cpufreq: stats: Fix locking
Quoted 2 lines · Viresh Kumar
Locking wasn't handled properly at places and this patch is an attempt to fix it. Specially while creating/removing stat tables.
This is pants, sorry.
Whenever you change locking, you need to know exactly (a) what is wrong with the way it is currently done and (b) how you are going to improve it. All of that needs to be described here.
For example, you seem to be thinking that locking is missing from __cpufreq_stats_free_table(), so you are adding it in there. Thus you need to describe (a) why it is missing (eg. what races are possible because of that) and (b) why adding it the way you do is going to improve the situation.
Quoted 70 lines
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_stats.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)drivers/cpufreq/cpufreq_stats.c +12
-6@@ -150,10 +150,12 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)pr_debug ("%s: Free stat table\n" , __func__); +mutex_lock (&cpufreq_stats_lock);sysfs_remove_group (&policy->kobj, &stats_attr_group);kfree (stat->time_in_state);kfree (stat); policy->stats_data =NULL ; +mutex_unlock (&cpufreq_stats_lock); }static void cpufreq_stats_free_table (unsigned int cpu)@@ -182,13 +184,17 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)if (unlikely (!table))return 0 ; +mutex_lock (&cpufreq_stats_lock); +/* stats already initialized */ -if (policy->stats_data) -return -EEXIST ; +if (policy->stats_data) { + ret = -EEXIST ; + goto unlock; + } stat =kzalloc (sizeof (*stat),GFP_KERNEL );if (!stat) -return -ENOMEM ; + goto unlock;/* Find total allocation size */ cpufreq_for_each_valid_entry (pos, table)@@ -219,21 +225,21 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) stat->freq_table[i++] = pos->frequency; stat->state_num = i; -mutex_lock (&cpufreq_stats_lock); stat->last_time =get_jiffies_64 (); stat->last_index =freq_table_get_index (stat, policy->cur); -mutex_unlock (&cpufreq_stats_lock); policy->stats_data = stat; ret =sysfs_create_group (&policy->kobj, &stats_attr_group);if (!ret) -return 0 ; + goto unlock;/* We failed, release resources */ policy->stats_data =NULL ;kfree (stat->time_in_state); free_stat:kfree (stat); +unlock: +mutex_unlock (&cpufreq_stats_lock);return ret; }
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 13/14] cpufreq: stats: Fix locking
Quoted 1 line · Rafael J. Wysocki
This is pants, sorry.
I am sorry :(
Quoted 3 lines
Whenever you change locking, you need to know exactly (a) what is wrong with the way it is currently done and (b) how you are going to improve it. All of that needs to be described here.
Thanks for kicking me here..
Quoted 4 lines
For example, you seem to be thinking that locking is missing from
__cpufreq_stats_free_table(), so you are adding it in there. Thus you need to describe (a) why it is missing (eg. what races are possible because of that) and (b) why adding it the way you do is going to improve the situation.
Yeah, that's what I thought. That race between create/free of stats will be eliminated by this..
But now as you forced me to write it properly, I am failing to understand why do we need to have any locking in place for stats ?
These are the reasons why I think we can remove all locking stuff from stats:
1.) create/free calls to stats can't run in parallel. They are all sequential. It happens with:
- cpu hotplug: which is sequential with proper locking in place.
- driver unregister: again sequential
- stats unregister: again sequential
So, actually we will never try to allocate stats for a single policy in parallel threads. Same goes for freeing as well..
2.) Any race possible with sysfs reads ?
These routines are called from show() routine from cpufreq.c and it has policy locks on the way. So, policy can't go away and so will stats.
Also, if stats unregister we will unregister the notifiers first. And that will again make things fall in line.
What else are we protecting? Maybe the calls to cpufreq_stats_update() can happen in parallel and so we may need locking only within that routine.
I might be missing the obvious logic, but then what exactly ?
--
viresh
Re: [PATCH V2 13/14] cpufreq: stats: Fix locking
Quoted 34 lines · Viresh Kumar
Quoted 1 line · Rafael J. Wysocki
This is pants, sorry.
I am sorry :(
Quoted 3 lines
Whenever you change locking, you need to know exactly (a) what is wrong with the way it is currently done and (b) how you are going to improve it. All of that needs to be described here.
Thanks for kicking me here..
Quoted 4 lines
For example, you seem to be thinking that locking is missing from
__cpufreq_stats_free_table(), so you are adding it in there. Thus you need to describe (a) why it is missing (eg. what races are possible because of that) and (b) why adding it the way you do is going to improve the situation.Yeah, that's what I thought. That race between create/free of stats will be eliminated by this..
But now as you forced me to write it properly, I am failing to understand why do we need to have any locking in place for stats ?
These are the reasons why I think we can remove all locking stuff from stats:
1.) create/free calls to stats can't run in parallel. They are all sequential. It happens with:
- cpu hotplug: which is sequential with proper locking in place.
- driver unregister: again sequential
- stats unregister: again sequentialSo, actually we will never try to allocate stats for a single policy in parallel threads. Same goes for freeing as well..
Quoted 7 lines
2.) Any race possible with sysfs reads ?
These routines are called from
show()routine fromcpufreq.cand it has policy locks on the way. So, policy can't go away and so will stats.Also, if stats unregister we will unregister the notifiers first. And that will again make things fall in line.
That sounds about right.
Quoted 4 lines
What else are we protecting? Maybe the calls to
cpufreq_stats_update()can happen in parallel and so we may need locking only within that routine.I might be missing the obvious logic, but then what exactly ?
Hard to say from the top of my head and I'm not sure if I have the time to have a deeper look into that during the next few days.
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Viresh Kumar
maintainerRe: [PATCH V2 13/14] cpufreq: stats: Fix locking
Quoted 2 lines · Rafael J. Wysocki
Hard to say from the top of my head and I'm not sure if I have the time to have a deeper look into that during the next few days.
Ok, let me resend the patches then with better improved logs. And lets see if something is still missing.
Viresh Kumar
maintainerPATCH v2 14/14 cpufreq: stats: call cpufreq_stats_update() with locks held
In reply to Viresh Kumarcpufreq_stats_update() has only two callers, one of them is already taking locks and other one don't.
To make locking efficient for cpufreq_stat_notifier_trans(), lets call cpufreq_stats_update() from within locks. Also update show_time_in_state() to call cpufreq_stats_update() after taking locks.
--- drivers/cpufreq/cpufreq_stats.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/cpufreq/cpufreq_stats.c +4 -3
@@ -40,12 +40,10 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat) {unsigned long long cur_time =get_jiffies_64 (); -mutex_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; -mutex_unlock (&cpufreq_stats_lock);return 0 ; }
@@ -62,7 +60,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)ssize_t len =0 ;int i; +mutex_lock (&cpufreq_stats_lock);cpufreq_stats_update (stat); +mutex_unlock (&cpufreq_stats_lock); +for (i =0 ; i < stat->state_num; i++) { len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i], (unsigned long long)
@@ -303,9 +304,9 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,if (old_index == new_index)return 0 ; +mutex_lock (&cpufreq_stats_lock);cpufreq_stats_update (stat); -mutex_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]++;
Signature · 1 line
2.2.0
Re: [PATCH V2 14/14] cpufreq: stats: call cpufreq_stats_update() with locks held
Quoted 2 lines · Viresh Kumar
cpufreq_stats_update()has only two callers, one of them is already taking locks and other one don't.
"and the other one doesn't."
Quoted 3 lines
To make locking efficient for
cpufreq_stat_notifier_trans(), lets callcpufreq_stats_update()from within locks. Also updateshow_time_in_state()to callcpufreq_stats_update()after taking locks.
"It doesn't make much sense to drop the lock in cpufreq_stats_update() and
re-acquire it immediately after that in cpufreq_stat_notifier_trans(), so make callers of cpufreq_stats_update() responsible for acquiring the lock around it as appropriate and modify cpufreq_stat_notifier_trans() to call it under the lock it already uses."
Quoted 46 lines
Reviewed-by Prarit Bhargava <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/cpufreq_stats.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)drivers/cpufreq/cpufreq_stats.c +4
-3@@ -40,12 +40,10 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat) {unsigned long long cur_time =get_jiffies_64 (); -mutex_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; -mutex_unlock (&cpufreq_stats_lock);return 0 ; }@@ -62,7 +60,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)ssize_t len =0 ;int i; +mutex_lock (&cpufreq_stats_lock);cpufreq_stats_update (stat); +mutex_unlock (&cpufreq_stats_lock); +for (i =0 ; i < stat->state_num; i++) { len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i], (unsigned long long)@@ -303,9 +304,9 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,if (old_index == new_index)return 0 ; +mutex_lock (&cpufreq_stats_lock);cpufreq_stats_update (stat); -mutex_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]++;
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Quoted 8 lines · Viresh Kumar
This is a resend of cpufreq stats cleanups I had for sometime. Few issues were reported from Fenguang's bot and are all fixed now and are stable..
V1->V2:
- Reviewed-by Prarit
- Replaced lock with mutex as we may sleep from within critical region and so a new patch "cpufreq: stats: replace spinlock with mutex".
P.
Quoted 24 lines
Pushed here:
git://git.linaro.org/people/viresh.kumar/linux.gitcpufreq/stats/cleanupsViresh Kumar (14):
cpufreq: stats: don't break strings into multiple lines
cpufreq: stats: return-EEXISTwhen stats are already allocated
cpufreq: stats: don't check for freq table while freeing stats
cpufreq: stats: pass 'stat' tocpufreq_stats_update()
cpufreq: stats: get rid of per-cpucpufreq_stats_table
cpufreq: Remove (now) unused 'last_cpu' from structcpufreq_policy
cpufreq: stats: removecpufreq_stats_update_policy_cpu()
cpufreq: removeCPUFREQ_UPDATE_POLICY_CPUnotifications
cpufreq: stats: create sysfs group once we are ready
cpufreq: stats: don't update stats fromshow_trans_table()
cpufreq: stats: don't update stats on false notifiers
cpufreq: stats: replace spinlock with mutex
cpufreq: stats: Fix locking
cpufreq: stats: callcpufreq_stats_update()with locks helddrivers/cpufreq/cpufreq.c | 6 -- drivers/cpufreq/cpufreq_stats.c | 148 ++++++++++++++++++---------------------- include/linux/cpufreq.h | 10 +-- 3 files changed, 73 insertions(+), 91 deletions(-)