cpufreq: Record stats with fast-switching

PATCH In review Viresh Kumar 2 participants 7 messages Oct 5, 2020

Subsystems (2)

Scheduler CPU frequency scaling framework

lore

Share & tools

lore

Viresh Kumar

author maintainer

2020-10-05 07:56 UTC #0 lore raw

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

V2->V3:
- Use READ/WRITE_ONCE() for reset-time as well.
- Use unlikely for few conditionals in the hot path.
- Better changelogs.
- Rebase changes.

V1-V2:
- Use READ_ONCE/WRITE_ONCE instead of atomic in the first patch.

--
Viresh

Viresh Kumar (5):
cpufreq: stats: Defer stats update to
cpufreq_stats_record_transition()
cpufreq: stats: Remove locking
cpufreq: stats: Mark few conditionals with unlikely()
cpufreq: stats: Enable stats for fast-switch as well
cpufreq: Move traces and update to policy->cur to cpufreq core

 drivers/cpufreq/cpufreq.c        | 11 ++++
 drivers/cpufreq/cpufreq_stats.c  | 89 ++++++++++++++++++++------------
 kernel/sched/cpufreq_schedutil.c | 12 +----
 3 files changed, 69 insertions(+), 43 deletions(-)

Signature · 1 line

2.25.0.rc1.19.g042ed3e048af

Viresh Kumar

author maintainer

2020-10-05 07:56 UTC #1 lore raw

PATCH v3 1/5 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.

The stats were updated from two places earlier:

- show_time_in_state(): This can be easily deferred, all we need is to calculate the delta duration again in this routine to show the current state's time-in-state.

- store_reset(): This is a bit tricky as we need to clear the stats here and avoid races with simultaneous call to cpufreq_stats_record_transition().

This patch fixes it by deferring the reset of the stats (within the code) to the next call to cpufreq_stats_record_transition(), but since we need to keep showing the right stats until that time, we capture the reset time and account for the time since last time reset was called until the time cpufreq_stats_record_transition() update the stats.

Userspace will continue seeing the stats correctly, everything will be 0 after the stats are reset, apart from the time-in-state of the current state, until the time a frequency switch happens.

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..498d962ba224 100644
@@ -22,17 +22,22 @@ struct cpufreq_stats {
 	spinlock_t lock;
 	unsigned int *freq_table;
 	unsigned int *trans_table;
+
+	/* Deferred reset */
+	unsigned int 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 */
+	WRITE_ONCE(stats->reset_pending, 0);
+	cpufreq_stats_update(stats, READ_ONCE(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 (READ_ONCE(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 = READ_ONCE(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() - READ_ONCE(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.
+	 */
+	WRITE_ONCE(stats->reset_time, get_jiffies_64());
+	WRITE_ONCE(stats->reset_pending, 1);
+
 	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 = READ_ONCE(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 (unlikely(READ_ONCE(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

Viresh Kumar

author maintainer

2020-10-05 07:56 UTC #2 lore raw

PATCH v3 2/5 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 498d962ba224..45a067855367 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 */
 	WRITE_ONCE(stats->reset_pending, 0);
 	cpufreq_stats_update(stats, READ_ONCE(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-10-05 07:56 UTC #3 lore raw

PATCH v3 3/5 cpufreq: stats: Mark few conditionals with unlikely()

In reply to Viresh Kumar

Since this will be part of scheduler's hotpath in some cases, use unlikely() for few of the obvious conditionals.

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

index 45a067855367..bba04da3a278 100644
@@ -260,7 +260,7 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
 	struct cpufreq_stats *stats = policy->stats;
 	int old_index, new_index;
 
-	if (!stats)
+	if (unlikely(!stats))
 		return;
 
 	if (unlikely(READ_ONCE(stats->reset_pending)))

@@ -270,7 +270,7 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
 	new_index = freq_table_get_index(stats, new_freq);
 
 	/* We can't do stats->time_in_state[-1]= .. */
-	if (old_index == -1 || new_index == -1 || old_index == new_index)
+	if (unlikely(old_index == -1 || new_index == -1 || old_index == new_index))
 		return;
 
 	cpufreq_stats_update(stats, stats->last_time);

Signature · 1 line

2.25.0.rc1.19.g042ed3e048af

Viresh Kumar

author maintainer

2020-10-05 07:56 UTC #4 lore raw

PATCH v3 4/5 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       | 4 ++++
 drivers/cpufreq/cpufreq_stats.c | 6 ------
 2 files changed, 4 insertions(+), 6 deletions(-)

drivers/cpufreq/cpufreq.c +4 -0

index 2ea245a6c0c0..2d0e2e464b14 100644
@@ -2072,8 +2072,12 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 	target_freq = clamp_val(target_freq, policy->min, policy->max);
 	freq = cpufreq_driver->fast_switch(policy, target_freq);
 
+	if (!freq)
+		return 0;
+
 	arch_set_freq_scale(policy->related_cpus, freq,
 			    policy->cpuinfo.max_freq);
+	cpufreq_stats_record_transition(policy, freq);
 
 	return freq;
 }

drivers/cpufreq/cpufreq_stats.c +0 -6

index bba04da3a278..8e7d64f34041 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-10-05 07:56 UTC #5 lore raw

PATCH v3 5/5 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        |  7 +++++++
 kernel/sched/cpufreq_schedutil.c | 12 +-----------
 2 files changed, 8 insertions(+), 11 deletions(-)

drivers/cpufreq/cpufreq.c +7 -0

index 2d0e2e464b14..db00693a586a 100644
@@ -2068,6 +2068,7 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 					unsigned int target_freq)
 {
 	unsigned int freq;
+	int cpu;
 
 	target_freq = clamp_val(target_freq, policy->min, policy->max);
 	freq = cpufreq_driver->fast_switch(policy, target_freq);

@@ -2075,10 +2076,16 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 	if (!freq)
 		return 0;
 
+	policy->cur = freq;
 	arch_set_freq_scale(policy->related_cpus, freq,
 			    policy->cpuinfo.max_freq);
 	cpufreq_stats_record_transition(policy, freq);
 
+	if (trace_cpu_frequency_enabled()) {
+		for_each_cpu(cpu, policy->cpus)
+			trace_cpu_frequency(freq, cpu);
+	}
+
 	return freq;
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);

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

Rafael J. Wysocki

maintainer

2020-10-05 13:25 UTC #6 lore raw

In reply to Viresh Kumar

Quoted 29 lines · Viresh Kumar

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.
V2->V3:
- Use READ/WRITE_ONCE() for reset-time as well.
- Use unlikely for few conditionals in the hot path.
- Better changelogs.
- Rebase changes.
V1-V2:
- Use READ_ONCE/WRITE_ONCE instead of atomic in the first patch.
--
Viresh
Viresh Kumar (5):
cpufreq: stats: Defer stats update to
cpufreq_stats_record_transition()
cpufreq: stats: Remove locking
cpufreq: stats: Mark few conditionals with unlikely()
cpufreq: stats: Enable stats for fast-switch as well
cpufreq: Move traces and update to policy->cur to cpufreq core

Quoted 6 lines

 drivers/cpufreq/cpufreq.c        | 11 ++++
 drivers/cpufreq/cpufreq_stats.c  | 89 ++++++++++++++++++++------------
 kernel/sched/cpufreq_schedutil.c | 12 +----
 3 files changed, 69 insertions(+), 43 deletions(-)

The entire series applied as 5.10 material with some minor changelog edits in patch [1/5], thanks!