cpufreq: Record stats with fast-switching

PATCH Superseded Viresh Kumar 4 participants 10 messages Sep 2, 2020 – Sep 16, 2020

Subsystems (2)

Scheduler CPU frequency scaling framework

lore

Share & tools

lore

Viresh Kumar

author maintainer

2020-09-02 07:25 UTC #0 lore raw

cover cpufreq: Record stats with fast-switching

Hi,

We disabled recording cpufreq stats when fast switching was introduced to the cpufreq core as the cpufreq stats required to take a spinlock and that can't be allowed (for performance reasons) on scheduler's hot path.

Here is an attempt to get rid of the lock and bring back the support.

--
Viresh

Viresh Kumar (4):
cpufreq: stats: Defer stats update to
cpufreq_stats_record_transition()
cpufreq: stats: Remove locking
cpufreq: stats: Enable stats for fast-switch as well
cpufreq: Move traces and update to policy->cur to cpufreq core

 drivers/cpufreq/cpufreq.c        | 16 +++++-
 drivers/cpufreq/cpufreq_stats.c  | 87 ++++++++++++++++++++------------
 kernel/sched/cpufreq_schedutil.c | 12 +----
 3 files changed, 72 insertions(+), 43 deletions(-)

Signature · 1 line

2.25.0.rc1.19.g042ed3e048af

Viresh Kumar

author maintainer

2020-09-02 07:25 UTC #1 lore raw

PATCH 1/4 cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

In order to prepare for lock-less stats update, add support to defer any updates to it until cpufreq_stats_record_transition() is called.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq_stats.c | 75 ++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 19 deletions(-)

drivers/cpufreq/cpufreq_stats.c +56 ~~-19~~

index 94d959a8e954..fdf9e8556a49 100644
@@ -22,17 +22,22 @@ struct cpufreq_stats {
 	spinlock_t lock;
 	unsigned int *freq_table;
 	unsigned int *trans_table;
+
+	/* Deferred reset */
+	atomic_t reset_pending;
+	unsigned long long reset_time;
 };
 
-static void cpufreq_stats_update(struct cpufreq_stats *stats)
+static void cpufreq_stats_update(struct cpufreq_stats *stats,
+				 unsigned long long time)
 {
 	unsigned long long cur_time = get_jiffies_64();
 
-	stats->time_in_state[stats->last_index] += cur_time - stats->last_time;
+	stats->time_in_state[stats->last_index] += cur_time - time;
 	stats->last_time = cur_time;
 }
 
-static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)
+static void cpufreq_stats_reset_table(struct cpufreq_stats *stats)
 {
 	unsigned int count = stats->max_state;

@@ -41,42 +46,67 @@ static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)
 	memset(stats->trans_table, 0, count * count * sizeof(int));
 	stats->last_time = get_jiffies_64();
 	stats->total_trans = 0;
+
+	/* Adjust for the time elapsed since reset was requested */
+	atomic_set(&stats->reset_pending, 0);
+	cpufreq_stats_update(stats, stats->reset_time);
 	spin_unlock(&stats->lock);
 }
 
 static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
 {
-	return sprintf(buf, "%d\n", policy->stats->total_trans);
+	struct cpufreq_stats *stats = policy->stats;
+
+	if (atomic_read(&stats->reset_pending))
+		return sprintf(buf, "%d\n", 0);
+	else
+		return sprintf(buf, "%d\n", stats->total_trans);
 }
 cpufreq_freq_attr_ro(total_trans);
 
 static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 {
 	struct cpufreq_stats *stats = policy->stats;
+	bool pending = atomic_read(&stats->reset_pending);
+	unsigned long long time;
 	ssize_t len = 0;
 	int i;
 
 	if (policy->fast_switch_enabled)
 		return 0;
 
-	spin_lock(&stats->lock);
-	cpufreq_stats_update(stats);
-	spin_unlock(&stats->lock);
-
 	for (i = 0; i < stats->state_num; i++) {
+		if (pending) {
+			if (i == stats->last_index)
+				time = get_jiffies_64() - stats->reset_time;
+			else
+				time = 0;
+		} else {
+			time = stats->time_in_state[i];
+			if (i == stats->last_index)
+				time += get_jiffies_64() - stats->last_time;
+		}
+
 		len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
-			(unsigned long long)
-			jiffies_64_to_clock_t(stats->time_in_state[i]));
+			       jiffies_64_to_clock_t(time));
 	}
 	return len;
 }
 cpufreq_freq_attr_ro(time_in_state);
 
+/* We don't care what is written to the attribute */
 static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
 			   size_t count)
 {
-	/* We don't care what is written to the attribute. */
-	cpufreq_stats_clear_table(policy->stats);
+	struct cpufreq_stats *stats = policy->stats;
+
+	/*
+	 * Defer resetting of stats to cpufreq_stats_record_transition() to
+	 * avoid races.
+	 */
+	atomic_set(&stats->reset_pending, 1);
+	stats->reset_time = get_jiffies_64();
+
 	return count;
 }
 cpufreq_freq_attr_wo(reset);

@@ -84,8 +114,9 @@ cpufreq_freq_attr_wo(reset);
 static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 {
 	struct cpufreq_stats *stats = policy->stats;
+	bool pending = atomic_read(&stats->reset_pending);
 	ssize_t len = 0;
-	int i, j;
+	int i, j, count;
 
 	if (policy->fast_switch_enabled)
 		return 0;

@@ -113,8 +144,13 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 		for (j = 0; j < stats->state_num; j++) {
 			if (len >= PAGE_SIZE)
 				break;
-			len += scnprintf(buf + len, PAGE_SIZE - len, "%9u ",
-					stats->trans_table[i*stats->max_state+j]);
+
+			if (pending)
+				count = 0;
+			else
+				count = stats->trans_table[i * stats->max_state + j];
+
+			len += scnprintf(buf + len, PAGE_SIZE - len, "%9u ", count);
 		}
 		if (len >= PAGE_SIZE)
 			break;

@@ -228,10 +264,11 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
 	struct cpufreq_stats *stats = policy->stats;
 	int old_index, new_index;
 
-	if (!stats) {
-		pr_debug("%s: No stats found\n", __func__);
+	if (!stats)
 		return;
-	}
+
+	if (atomic_read(&stats->reset_pending))
+		cpufreq_stats_reset_table(stats);
 
 	old_index = stats->last_index;
 	new_index = freq_table_get_index(stats, new_freq);

@@ -241,7 +278,7 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
 		return;
 
 	spin_lock(&stats->lock);
-	cpufreq_stats_update(stats);
+	cpufreq_stats_update(stats, stats->last_time);
 
 	stats->last_index = new_index;
 	stats->trans_table[old_index * stats->max_state + new_index]++;

Signature · 1 line

2.25.0.rc1.19.g042ed3e048af

[email protected]

maintainer

2020-09-11 10:11 UTC #2 lore raw

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

drivers/cpufreq/cpufreq_stats.c static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)

Quoted 8 lines · Viresh Kumar

drivers/cpufreq/cpufreq_stats.c +7 -0

+	atomic_t reset_pending;
 
+	atomic_set(&stats->reset_pending, 0);
+	if (atomic_read(&stats->reset_pending))
+	bool pending = atomic_read(&stats->reset_pending);
+	atomic_set(&stats->reset_pending, 1);
+	bool pending = atomic_read(&stats->reset_pending);
+	if (atomic_read(&stats->reset_pending))

What do you think atomic_t is doing for you?

Viresh Kumar

author maintainer

2020-09-11 11:36 UTC #3 lore raw

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

Quoted 11 lines · On 11-09-20, 12:11, [email protected] wrote:

drivers/cpufreq/cpufreq_stats.c static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)
Quoted 8 lines · Viresh Kumar
drivers/cpufreq/cpufreq_stats.c +7 -0
+	atomic_t reset_pending;
 
+	atomic_set(&stats->reset_pending, 0);
+	if (atomic_read(&stats->reset_pending))
+	bool pending = atomic_read(&stats->reset_pending);
+	atomic_set(&stats->reset_pending, 1);
+	bool pending = atomic_read(&stats->reset_pending);
+	if (atomic_read(&stats->reset_pending))
What do you think atomic_t is doing for you?

I was trying to avoid races while two writes are going in parallel, but obviously as this isn't a RMW operation, it won't result in anything for me.

Maybe what I should be doing is just READ_ONCE()/WRITE_ONCE()? So the other side doesn't see any intermediate value that was never meant to be set/read ?

Signature · 1 line

viresh

Rafael J. Wysocki

maintainer

2020-09-11 12:18 UTC #4 lore raw

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

Quoted 14 lines · Viresh Kumar

Quoted 11 lines · On 11-09-20, 12:11, [email protected] wrote:
drivers/cpufreq/cpufreq_stats.c static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)
Quoted 8 lines · Viresh Kumar
drivers/cpufreq/cpufreq_stats.c +7 -0
+   atomic_t reset_pending;
 
+   atomic_set(&stats->reset_pending, 0);
+   if (atomic_read(&stats->reset_pending))
+   bool pending = atomic_read(&stats->reset_pending);
+   atomic_set(&stats->reset_pending, 1);
+   bool pending = atomic_read(&stats->reset_pending);
+   if (atomic_read(&stats->reset_pending))
What do you think atomic_t is doing for you?

Quoted 7 lines

I was trying to avoid races while two writes are going in parallel, but obviously as this isn't a RMW operation, it won't result in anything for me.
Maybe what I should be doing is just READ_ONCE()/WRITE_ONCE()? So the other side doesn't see any intermediate value that was never meant to be set/read ?

If the value in question is a pointer or an int (or equivalent), READ_ONCE()/WRITE_ONCE() should be sufficient, and should be used at least as a matter of annotation of the sensitive code IMO.

IIRC, atomic_set() and atomic_read() are pretty much the same as WRITE_ONCE() and READ_ONCE(), respectively, anyway.

Cheers!

Lukasz Luba

2020-09-15 10:04 UTC #5 lore raw

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

In reply to Viresh Kumar

Hi Viresh,

drivers/cpufreq/cpufreq_stats.c struct cpufreq_stats {

Quoted 13 lines · Viresh Kumar

In order to prepare for lock-less stats update, add support to defer any updates to it until cpufreq_stats_record_transition() is called.
Signed-off-by Viresh Kumar <[email protected]>
---
  drivers/cpufreq/cpufreq_stats.c | 75 ++++++++++++++++++++++++---------
  1 file changed, 56 insertions(+), 19 deletions(-)
drivers/cpufreq/cpufreq_stats.c +0 -0
index 94d959a8e954..fdf9e8556a49 100644
@@ -22,17 +22,22 @@ struct cpufreq_stats {

Would it be possible to move this structure in the linux/cpufreq.h header? Any subsystem could have access to it, like to the cpuidle stats.

Apart from that (and the comment regarding the 'atomic_t' field) I don't see any issues.

Regards,
Lukasz

Viresh Kumar

author maintainer

2020-09-16 05:56 UTC #6 lore raw

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

Quoted 11 lines · Lukasz Luba

Hi Viresh,
Quoted 8 lines · Viresh Kumar
In order to prepare for lock-less stats update, add support to defer any updates to it until cpufreq_stats_record_transition() is called.
Signed-off-by Viresh Kumar <[email protected]>
---
  drivers/cpufreq/cpufreq_stats.c | 75 ++++++++++++++++++++++++---------
  1 file changed, 56 insertions(+), 19 deletions(-)

Quoted 9 lines

drivers/cpufreq/cpufreq_stats.c struct cpufreq_stats {
Quoted 5 lines
drivers/cpufreq/cpufreq_stats.c +0 -0
index 94d959a8e954..fdf9e8556a49 100644
@@ -22,17 +22,22 @@ struct cpufreq_stats {
Would it be possible to move this structure in the linux/cpufreq.h header? Any subsystem could have access to it, like to the cpuidle stats.

Hmm, I am not sure why we should be doing it. In case of cpuidle many parts of the kernel are playing with cpuidle code, like drivers/idle/, drivers/cpuidle, etc.

Something should land in include/ only if you want others to use it, but in case of cpufreq no one should be using cpufreq stats.

So unless you have a real case where that might be beneficial, I am going to keep it as is.

Quoted 2 lines

Apart from that (and the comment regarding the 'atomic_t' field) I don't see any issues.

Thanks.

Signature · 1 line

viresh

Viresh Kumar

author maintainer

2020-09-02 07:25 UTC #7 lore raw

PATCH 2/4 cpufreq: stats: Remove locking

In reply to Viresh Kumar

The locking isn't required anymore as stats can get updated only from one place at a time. Remove it.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq_stats.c | 6 ------
 1 file changed, 6 deletions(-)

drivers/cpufreq/cpufreq_stats.c +0 -6

index fdf9e8556a49..d86ea9744649 100644
@@ -19,7 +19,6 @@ struct cpufreq_stats {
 	unsigned int state_num;
 	unsigned int last_index;
 	u64 *time_in_state;
-	spinlock_t lock;
 	unsigned int *freq_table;
 	unsigned int *trans_table;

@@ -41,7 +40,6 @@ static void cpufreq_stats_reset_table(struct cpufreq_stats *stats)
 {
 	unsigned int count = stats->max_state;
 
-	spin_lock(&stats->lock);
 	memset(stats->time_in_state, 0, count * sizeof(u64));
 	memset(stats->trans_table, 0, count * count * sizeof(int));
 	stats->last_time = get_jiffies_64();

@@ -50,7 +48,6 @@ static void cpufreq_stats_reset_table(struct cpufreq_stats *stats)
 	/* Adjust for the time elapsed since reset was requested */
 	atomic_set(&stats->reset_pending, 0);
 	cpufreq_stats_update(stats, stats->reset_time);
-	spin_unlock(&stats->lock);
 }
 
 static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)

@@ -244,7 +241,6 @@ void cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	stats->state_num = i;
 	stats->last_time = get_jiffies_64();
 	stats->last_index = freq_table_get_index(stats, policy->cur);
-	spin_lock_init(&stats->lock);
 
 	policy->stats = stats;
 	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);

@@ -277,11 +273,9 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
 	if (old_index == -1 || new_index == -1 || old_index == new_index)
 		return;
 
-	spin_lock(&stats->lock);
 	cpufreq_stats_update(stats, stats->last_time);
 
 	stats->last_index = new_index;
 	stats->trans_table[old_index * stats->max_state + new_index]++;
 	stats->total_trans++;
-	spin_unlock(&stats->lock);
 }

Signature · 1 line

2.25.0.rc1.19.g042ed3e048af

Viresh Kumar

author maintainer

2020-09-02 07:25 UTC #8 lore raw

PATCH 3/4 cpufreq: stats: Enable stats for fast-switch as well

In reply to Viresh Kumar

Now that all the blockers are gone for enabling stats in fast-switching case, enable it.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c       | 6 +++++-
 drivers/cpufreq/cpufreq_stats.c | 6 ------
 2 files changed, 5 insertions(+), 7 deletions(-)

drivers/cpufreq/cpufreq.c +5 -1

index 47aa90f9a7c2..d5fe64e96be9 100644
@@ -2057,8 +2057,12 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 					unsigned int target_freq)
 {
 	target_freq = clamp_val(target_freq, policy->min, policy->max);
+	target_freq = cpufreq_driver->fast_switch(policy, target_freq);
 
-	return cpufreq_driver->fast_switch(policy, target_freq);
+	if (target_freq)
+		cpufreq_stats_record_transition(policy, target_freq);
+
+	return target_freq;
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);

drivers/cpufreq/cpufreq_stats.c +0 -6

index d86ea9744649..06b5ee12f3b2 100644
@@ -69,9 +69,6 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 	ssize_t len = 0;
 	int i;
 
-	if (policy->fast_switch_enabled)
-		return 0;
-
 	for (i = 0; i < stats->state_num; i++) {
 		if (pending) {
 			if (i == stats->last_index)

@@ -115,9 +112,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 	ssize_t len = 0;
 	int i, j, count;
 
-	if (policy->fast_switch_enabled)
-		return 0;
-
 	len += scnprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += scnprintf(buf + len, PAGE_SIZE - len, "         : ");
 	for (i = 0; i < stats->state_num; i++) {

Signature · 1 line

2.25.0.rc1.19.g042ed3e048af

Viresh Kumar

author maintainer

2020-09-02 07:25 UTC #9 lore raw

PATCH 4/4 cpufreq: Move traces and update to policy->cur to cpufreq core

In reply to Viresh Kumar

The cpufreq core handles the updates to policy->cur and recording of cpufreq trace events for all the governors except schedutil's fast switch case.

Move that as well to cpufreq core for consistency and readability.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c        | 14 ++++++++++++--
 kernel/sched/cpufreq_schedutil.c | 12 +-----------
 2 files changed, 13 insertions(+), 13 deletions(-)

drivers/cpufreq/cpufreq.c +12 -2

index d5fe64e96be9..bc930f6ecff6 100644
@@ -2056,11 +2056,21 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 					unsigned int target_freq)
 {
+	int cpu;
+
 	target_freq = clamp_val(target_freq, policy->min, policy->max);
 	target_freq = cpufreq_driver->fast_switch(policy, target_freq);
 
-	if (target_freq)
-		cpufreq_stats_record_transition(policy, target_freq);
+	if (!target_freq)
+		return 0;
+
+	policy->cur = target_freq;
+	cpufreq_stats_record_transition(policy, target_freq);
+
+	if (trace_cpu_frequency_enabled()) {
+		for_each_cpu(cpu, policy->cpus)
+			trace_cpu_frequency(target_freq, cpu);
+	}
 
 	return target_freq;
 }

kernel/sched/cpufreq_schedutil.c +1 ~~-11~~

index e39008242cf4..28f6d1ad608b 100644
@@ -115,21 +115,11 @@ static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
 			      unsigned int next_freq)
 {
 	struct cpufreq_policy *policy = sg_policy->policy;
-	int cpu;
 
 	if (!sugov_update_next_freq(sg_policy, time, next_freq))
 		return;
 
-	next_freq = cpufreq_driver_fast_switch(policy, next_freq);
-	if (!next_freq)
-		return;
-
-	policy->cur = next_freq;
-
-	if (trace_cpu_frequency_enabled()) {
-		for_each_cpu(cpu, policy->cpus)
-			trace_cpu_frequency(next_freq, cpu);
-	}
+	cpufreq_driver_fast_switch(policy, next_freq);
 }
 
 static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,

Signature · 1 line

2.25.0.rc1.19.g042ed3e048af