Skip to contentSkip to conversation

cpufreq: stats: cleanups

PATCH In review Viresh Kumar 1 participant 19 messages Jan 6, 2015 – Jan 13, 2015 CPU frequency scaling framework

lore
Share & tools
lore

Viresh Kumar

author maintainer
#0 lore raw

v3 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
author maintainer
#1 lore raw

PATCH 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
index 0cd9b4dcef99..80801f880dd8 100644
@@ -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
author maintainer
#2 lore raw

PATCH 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
index 80801f880dd8..d2299ca2fc2c 100644
@@ -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
author maintainer
#3 lore raw

PATCH v3 3/16 cpufreq: stats: remove unused cpufreq_stats_attribute

In reply to Viresh Kumar

It 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
index d2299ca2fc2c..14f8d858a63d 100644
@@ -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
author maintainer
#4 lore raw

PATCH 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
index 14f8d858a63d..21bec770569d 100644
@@ -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
author maintainer
#5 lore raw

PATCH v3 5/16 cpufreq: stats: don't check for freq table while freeing stats

In reply to Viresh Kumar

While 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
index 21bec770569d..e9d68420b876 100644
@@ -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
author maintainer
#6 lore raw

PATCH v3 6/16 cpufreq: stats: pass 'stat' to cpufreq_stats_update()

In reply to Viresh Kumar

It 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
index e9d68420b876..6c234f548601 100644
@@ -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
author maintainer
#7 lore raw

PATCH v3 7/16 cpufreq: stats: get rid of per-cpu cpufreq_stats_table

In reply to Viresh Kumar

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.

---
 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
index 6c234f548601..d6f5053d8ffb 100644
@@ -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
index 4d078cebafd2..60b7b496565d 100644
@@ -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
author maintainer
#8 lore raw

PATCH 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
index 6c234f548601..3792b2e2f4a8 100644
@@ -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
index 4d078cebafd2..60b7b496565d 100644
@@ -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
author maintainer
#9 lore raw

PATCH v3 8/16 cpufreq: stats: rename 'struct cpufreq_stats' objects as 'stats'

In reply to Viresh Kumar

Currently 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
index d6f5053d8ffb..ca3d5b71e828 100644
@@ -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
author maintainer
#10 lore raw

PATCH 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
index a09a29c312a9..b88894578fd1 100644
@@ -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
index 60b7b496565d..7e1a389b4e92 100644
@@ -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
author maintainer
#11 lore raw

PATCH 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
index ca3d5b71e828..1ed703a3e21e 100644
@@ -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
author maintainer
#12 lore raw

PATCH v3 11/16 cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications

In reply to Viresh Kumar

CPUFREQ_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
index b88894578fd1..6d97dffeaaf7 100644
@@ -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
index 7e1a389b4e92..2ee4888c1f47 100644
@@ -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
author maintainer
#13 lore raw

PATCH v3 12/16 cpufreq: stats: create sysfs group once we are ready

In reply to Viresh Kumar

Userspace 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
index 1ed703a3e21e..17df8c507594 100644
@@ -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
author maintainer
#14 lore raw

PATCH 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
index 17df8c507594..66ebdc4c1762 100644
@@ -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
author maintainer
#15 lore raw

PATCH v3 14/16 cpufreq: stats: don't update stats from show_trans_table()

In reply to Viresh Kumar

cpufreq_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
index 66ebdc4c1762..6e87dd9c4083 100644
@@ -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
author maintainer
#16 lore raw

PATCH v3 15/16 cpufreq: stats: don't update stats on false notifiers

In reply to Viresh Kumar

We 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
index 6e87dd9c4083..71fcc7c198a1 100644
@@ -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
author maintainer
#17 lore raw

PATCH v3 16/16 cpufreq: stats: drop unnecessary locking

In reply to Viresh Kumar

There 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
index 71fcc7c198a1..bce255568123 100644
@@ -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
author maintainer
#18 lore raw
In reply to Viresh Kumar
Quoted 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 ?