On Friday, January 02, 2015 11:16:39 AM Viresh Kumar wrote:

__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?

Reviewed-by: Prarit Bhargava prarit@redhat.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

---

drivers/cpufreq/cpufreq_stats.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index 81be4d637ab4..403671b1a5ee 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -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;

On Friday, January 02, 2015 11:16:41 AM Viresh Kumar wrote:

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' to cpufreq_stats_update().

What about:

"It is better to pass a struct cpufreq_stats pointer to cpufreq_stats_update() instead of a CPU number, because ..."

?

Reviewed-by: Prarit Bhargava prarit@redhat.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

---

drivers/cpufreq/cpufreq_stats.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index 62b6133f06aa..ebd9e4c5c124 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -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();

Unrelated change.

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;

On Friday, January 02, 2015 11:16:42 AM Viresh Kumar wrote:

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.

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."

Reviewed-by: Prarit Bhargava prarit@redhat.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

---

drivers/cpufreq/cpufreq_stats.c | 51 ++++++++++++++++++----------------------- include/linux/cpufreq.h | 3 +++ 2 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index ebd9e4c5c124..106c89ccef30 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -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);

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.

} 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;

Why don't you change this line to

policy->stats->cpu = policy->cpu;

and then you don't need the extra variable any more?

} @@ -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__);
  

Do we need the extra message here? If so, can it be pr_debug()?

• 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?

• stat = policy->stats_data;

Is stat guaraneed to be a valid pointer at this point?

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;

} diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 4d078cebafd2..3cb097b86d20 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -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;

?

• /* For cpufreq driver's internal use */ void *driver_data;

};

On Monday, January 05, 2015 09:53:45 AM Viresh Kumar wrote:

On 4 January 2015 at 03:50, Rafael J. Wysocki rjw@rjwysocki.net wrote:

All comments accepted before this ..

...

...

@@ -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__);
  

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.

...

...

•         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 ?

Looks correct to me.

...

...

• 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. :-)

On Friday, January 02, 2015 11:16:45 AM Viresh Kumar wrote:

CPUFREQ_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 :)

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?

Reviewed-by: Prarit Bhargava prarit@redhat.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

---

drivers/cpufreq/cpufreq.c | 3 --- include/linux/cpufreq.h | 5 ++--- 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b88894578fd1..6d97dffeaaf7 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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;

} diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 2d99f12e4199..43013eac26fd 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -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);

On Friday, January 02, 2015 11:16:51 AM Viresh Kumar wrote:

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."

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.

"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."

Reviewed-by: Prarit Bhargava prarit@redhat.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

---

drivers/cpufreq/cpufreq_stats.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index 95867985ea02..f1b234a09f66 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -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]++;