cpufreq: schedutil: Allow remote wakeups

From: Steve Muckle <[email protected]>

Upcoming support for scheduler cpufreq callbacks on remote wakeups will require the client to know what the target CPU is that the callback is being invoked for. Add this information into the callback data structure.

Signed-off-by Steve Muckle <[email protected]>

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

---
 include/linux/sched/cpufreq.h | 1 +
 kernel/sched/cpufreq.c        | 1 +
 2 files changed, 2 insertions(+)

include/linux/sched/cpufreq.h +0 -0

index d2be2ccbb372..f798f63d93e8 100644

From: Steve Muckle <[email protected]>

Having irq_work_queue_on() available for !CONFIG_SMP can make some call sites cleaner.

Signed-off-by Steve Muckle <[email protected]>

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

---
 include/linux/irq_work.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/irq_work.h +1 -0

index 47b9ebd4a74f..0195c3502d6b 100644
@@ -1,6 +1,7 @@
 #ifndef _LINUX_IRQ_WORK_H
 #define _LINUX_IRQ_WORK_H
 
+#include <linux/bug.h>
 #include <linux/llist.h>
 
 /*

On many platforms any CPU (from any cpufreq policy) can perform DVFS on behalf of other CPUs. Add a flag to identify such cpufreq policies.

Also enable it for cpufreq-dt driver which is used only on ARM platforms currently.

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

---
 drivers/cpufreq/cpufreq-dt.c | 1 +
 include/linux/cpufreq.h      | 9 +++++++++
 2 files changed, 10 insertions(+)

drivers/cpufreq/cpufreq-dt.c +1 -0

index c943787d761e..e57b45f20544 100644
@@ -274,6 +274,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		transition_latency = CPUFREQ_ETERNAL;
 
 	policy->cpuinfo.transition_latency = transition_latency;
+	policy->dvfs_possible_from_any_cpu = true;
 
 	return 0;
 

include/linux/cpufreq.h +0 -0

index 87165f06a307..9490a314c515 100644
@@ -120,6 +120,15 @@ struct cpufreq_policy {
 	bool			fast_switch_possible;

From: Steve Muckle <[email protected]>

In preparation for schedutil receiving sched cpufreq callbacks for remote CPUs, extend the irq work in schedutil to support policies with fast switching enabled in addition to policies using the slow path.

Signed-off-by Steve Muckle <[email protected]>

[ vk: minor code updates ]

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

From: Steve Muckle <[email protected]>

Upcoming support for remote callbacks from the scheduler into schedutil requires that the CPU identified in the hook structure be used to indicate the CPU being operated on, rather than relying on smp_processor_id().

Note that policy->cpu is passed to trace_cpu_frequency() in fast switch path, as it is safe to use any CPU which is managed by the current policy.

Signed-off-by Steve Muckle <[email protected]>

[ vk: updated commit log, minor code cleanups and use policy->cpu for
traces ]

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

---
 kernel/sched/cpufreq_schedutil.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

kernel/sched/cpufreq_schedutil.c +5 -5

index a418544c51b1..b168c31f1c8f 100644
@@ -96,7 +96,7 @@ static void sugov_fast_switch(struct cpufreq_policy *policy,
 		return;
 
 	policy->cur = next_freq;
-	trace_cpu_frequency(next_freq, smp_processor_id());
+	trace_cpu_frequency(next_freq, policy->cpu);
 }
 
 static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,

@@ -106,7 +106,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
 
 	if (policy->fast_switch_enabled) {
 		if (sg_policy->next_freq == next_freq) {
-			trace_cpu_frequency(policy->cur, smp_processor_id());
+			trace_cpu_frequency(policy->cur, policy->cpu);
 			return;
 		}
 		sg_policy->next_freq = next_freq;

@@ -157,12 +157,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 	return cpufreq_driver_resolve_freq(policy, freq);
 }
 
-static void sugov_get_util(unsigned long *util, unsigned long *max)
+static void sugov_get_util(unsigned long *util, unsigned long *max, int cpu)
 {
-	struct rq *rq = this_rq();
+	struct rq *rq = cpu_rq(cpu);
 	unsigned long cfs_max;
 
-	cfs_max = arch_scale_cpu_capacity(NULL, smp_processor_id());
+	cfs_max = arch_scale_cpu_capacity(NULL, cpu);
 
 	*util = min(rq->cfs.avg.util_avg, cfs_max);
 	*max = cfs_max;

kernel/sched/cpufreq_schedutil.c static void sugov_update_single(struct update_util_data *hook, u64 time, · line 219

Quoted 6 lines · Viresh Kumar

kernel/sched/cpufreq_schedutil.c +1 -1

@@ -216,7 +216,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 	if (flags & SCHED_CPUFREQ_RT_DL) {
 		next_f = policy->cpuinfo.max_freq;
 	} else {
-		sugov_get_util(&util, &max);
+		sugov_get_util(&util, &max, hook->cpu);

Why is this not racy?

Quoted 7 lines

kernel/sched/cpufreq_schedutil.c +0 -0

 		sugov_iowait_boost(sg_cpu, &util, &max);
 		next_f = get_next_freq(sg_policy, util, max);
 	}

@@ -272,7 +272,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	unsigned long util, max;
 	unsigned int next_f;
 

kernel/sched/cpufreq_schedutil.c static void sugov_update_shared(struct update_util_data *hook, u64 time, · line 276

Quoted 3 lines

kernel/sched/cpufreq_schedutil.c +1 -1

-	sugov_get_util(&util, &max);
+	sugov_get_util(&util, &max, hook->cpu);
 

And here?

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +0 -0

 	raw_spin_lock(&sg_policy->update_lock);

Quoted 9 lines · Rafael J. Wysocki

kernel/sched/cpufreq_schedutil.c static void sugov_update_single(struct update_util_data *hook, u64 time, · line 219

Quoted 6 lines · Viresh Kumar

kernel/sched/cpufreq_schedutil.c +1 -1

@@ -216,7 +216,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
    if (flags & SCHED_CPUFREQ_RT_DL) {
            next_f = policy->cpuinfo.max_freq;
    } else {
-           sugov_get_util(&util, &max);
+           sugov_get_util(&util, &max, hook->cpu);

Why is this not racy?

Why would reading the utilization values be racy? The only dynamic value here is "util_avg" and I am not sure if reading it is racy.

But, this whole routine has races which I ignored as we may end up updating frequency simultaneously from two threads.

Quoted 13 lines

kernel/sched/cpufreq_schedutil.c static void sugov_update_shared(struct update_util_data *hook, u64 time, · line 276

Quoted 10 lines

kernel/sched/cpufreq_schedutil.c +1 -1

            sugov_iowait_boost(sg_cpu, &util, &max);
            next_f = get_next_freq(sg_policy, util, max);
    }

@@ -272,7 +272,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
    unsigned long util, max;
    unsigned int next_f;
 
-   sugov_get_util(&util, &max);
+   sugov_get_util(&util, &max, hook->cpu);
 

And here?

Quoted 1 line

kernel/sched/cpufreq_schedutil.c static void sugov_update_shared(struct update_util_data *hook, u64 time, · line 277

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +0 -0

    raw_spin_lock(&sg_policy->update_lock);

The lock prevents the same here though.

So, if we are going to use this series, then we can use the same update-lock in case of single cpu per policies as well.

Quoted 11 lines · Viresh Kumar

Quoted 9 lines · Rafael J. Wysocki

kernel/sched/cpufreq_schedutil.c static void sugov_update_single(struct update_util_data *hook, u64 time, · line 219

Quoted 6 lines · Viresh Kumar

kernel/sched/cpufreq_schedutil.c +1 -1

@@ -216,7 +216,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
    if (flags & SCHED_CPUFREQ_RT_DL) {
            next_f = policy->cpuinfo.max_freq;
    } else {
-           sugov_get_util(&util, &max);
+           sugov_get_util(&util, &max, hook->cpu);

Why is this not racy?

Quoted 5 lines

Why would reading the utilization values be racy? The only dynamic value here is "util_avg" and I am not sure if reading it is racy.

But, this whole routine has races which I ignored as we may end up updating frequency simultaneously from two threads.

Those races aren't there if we don't update cross-CPU, which is my point. :-)

Quoted 17 lines

Quoted 7 lines

Quoted 7 lines

kernel/sched/cpufreq_schedutil.c +0 -0

            sugov_iowait_boost(sg_cpu, &util, &max);
            next_f = get_next_freq(sg_policy, util, max);
    }

@@ -272,7 +272,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
    unsigned long util, max;
    unsigned int next_f;
 

Quoted 7 lines

kernel/sched/cpufreq_schedutil.c static void sugov_update_shared(struct update_util_data *hook, u64 time, · line 276

Quoted 3 lines

kernel/sched/cpufreq_schedutil.c +1 -1

-   sugov_get_util(&util, &max);
+   sugov_get_util(&util, &max, hook->cpu);
 

And here?

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +0 -0

    raw_spin_lock(&sg_policy->update_lock);

The lock prevents the same here though.

Quoted 2 lines

So, if we are going to use this series, then we can use the same update-lock in case of single cpu per policies as well.

No, we can't.

The lock is unavoidable in the mulit-CPU policies case, but there's no way I will agree on using a lock in the single-CPU case.

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Viresh Kumar

Quoted 9 lines · Rafael J. Wysocki

kernel/sched/cpufreq_schedutil.c static void sugov_update_single(struct update_util_data *hook, u64 time, · line 219

Quoted 6 lines · Viresh Kumar

kernel/sched/cpufreq_schedutil.c +1 -1

@@ -216,7 +216,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
    if (flags & SCHED_CPUFREQ_RT_DL) {
            next_f = policy->cpuinfo.max_freq;
    } else {
-           sugov_get_util(&util, &max);
+           sugov_get_util(&util, &max, hook->cpu);

Why is this not racy?

Quoted 7 lines

Quoted 5 lines

Why would reading the utilization values be racy? The only dynamic value here is "util_avg" and I am not sure if reading it is racy.

But, this whole routine has races which I ignored as we may end up updating frequency simultaneously from two threads.

Those races aren't there if we don't update cross-CPU, which is my point. :-)

Of course. There are no races without this series.

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

kernel/sched/cpufreq_schedutil.c +0 -0

            sugov_iowait_boost(sg_cpu, &util, &max);
            next_f = get_next_freq(sg_policy, util, max);
    }

@@ -272,7 +272,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
    unsigned long util, max;
    unsigned int next_f;
 

Quoted 7 lines

kernel/sched/cpufreq_schedutil.c static void sugov_update_shared(struct update_util_data *hook, u64 time, · line 276

Quoted 3 lines

kernel/sched/cpufreq_schedutil.c +1 -1

-   sugov_get_util(&util, &max);
+   sugov_get_util(&util, &max, hook->cpu);
 

And here?

Quoted 1 line

kernel/sched/cpufreq_schedutil.c +0 -0

    raw_spin_lock(&sg_policy->update_lock);

The lock prevents the same here though.

So, if we are going to use this series, then we can use the same update-lock in case of single cpu per policies as well.

Quoted 4 lines

No, we can't.

The lock is unavoidable in the mulit-CPU policies case, but there's no way I will agree on using a lock in the single-CPU case.

How do you suggest to avoid the locking here then ? Some atomic variable read/write as done in cpufreq_governor.c ?

From: Steve Muckle <[email protected]>

A callback is considered remote if the target CPU is not the current CPU and if it is not managed by the policy managing the current CPU or the current CPU can't do DVFS on its behalf.

Queue the irq work for remote callbacks on the destination CPU. The irq work will carry out the fast or slow switch as appropriate.

Signed-off-by Steve Muckle <[email protected]>

[ vk: commit log, code cleanups, introduce dvfs_possible_from_any_cpu
and drop late callback support to avoid IPIs on remote CPUs. ]

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

---
 kernel/sched/cpufreq_schedutil.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

kernel/sched/cpufreq_schedutil.c +7 -3

index b168c31f1c8f..9bad579b6b08 100644
@@ -100,11 +100,11 @@ static void sugov_fast_switch(struct cpufreq_policy *policy,
 }
 
 static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
-				unsigned int next_freq)
+				int cpu, bool remote, unsigned int next_freq)
 {
 	struct cpufreq_policy *policy = sg_policy->policy;
 
-	if (policy->fast_switch_enabled) {
+	if (policy->fast_switch_enabled && !remote) {
 		if (sg_policy->next_freq == next_freq) {
 			trace_cpu_frequency(policy->cur, policy->cpu);
 			return;

@@ -116,7 +116,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
 		sg_policy->next_freq = next_freq;
 		sg_policy->last_freq_update_time = time;
 		sg_policy->work_in_progress = true;
-		irq_work_queue(&sg_policy->irq_work);
+		irq_work_queue_on(&sg_policy->irq_work, cpu);
 	}
 }
 

@@ -206,6 +206,20 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 	struct cpufreq_policy *policy = sg_policy->policy;
 	unsigned long util, max;
 	unsigned int next_f;
+	int cpu, this_cpu = smp_processor_id();
+	bool remote;
+
+	if (policy->dvfs_possible_from_any_cpu) {

kernel/sched/cpufreq_schedutil.c +1 -1

 	sugov_set_iowait_boost(sg_cpu, time, flags);
 	sg_cpu->last_update = time;

@@ -220,7 +234,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 		sugov_iowait_boost(sg_cpu, &util, &max);
 		next_f = get_next_freq(sg_policy, util, max);
 	}
-	sugov_update_commit(sg_policy, time, next_f);
+	sugov_update_commit(sg_policy, time, cpu, remote, next_f);
 }
 
 static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu)

@@ -269,8 +283,24 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 {

From: Steve Muckle <[email protected]>

In preparation for the scheduler cpufreq callback happening on remote CPUs, add support for this in the legacy (ondemand and conservative) governors. The legacy governors make assumptions about the callback occurring on the CPU being updated.

Signed-off-by Steve Muckle <[email protected]>

[ vk: minor updates in commit log ]

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

---
 drivers/cpufreq/cpufreq_governor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/cpufreq_governor.c +0 -0

index 47e24b5384b3..c9e786e7ee1f 100644

Quoted 19 lines · Viresh Kumar

From: Steve Muckle <[email protected]>

In preparation for the scheduler cpufreq callback happening on remote CPUs, add support for this in the legacy (ondemand and conservative) governors. The legacy governors make assumptions about the callback occurring on the CPU being updated.

Signed-off-by Steve Muckle <[email protected]>

[ vk: minor updates in commit log ]

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

---
 drivers/cpufreq/cpufreq_governor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/cpufreq_governor.c +0 -0

index 47e24b5384b3..c9e786e7ee1f 100644

drivers/cpufreq/cpufreq_governor.c static void dbs_update_util_handler(struct update_util_data *data, u64 time, · line 318

Quoted 5 lines

drivers/cpufreq/cpufreq_governor.c +1 -1

@@ -315,7 +315,7 @@ static void dbs_update_util_handler(struct update_util_data *data, u64 time,
 
 	policy_dbs->last_sample_time = time;
 	policy_dbs->work_in_progress = true;
-	irq_work_queue(&policy_dbs->irq_work);
+	irq_work_queue_on(&policy_dbs->irq_work, data->cpu);

I'm totally unconvinced that this is sufficient.

This function carries out lockless computations with the assumption that it will always run on the CPU being updated.

For instance, how is it prevented from being run on two CPUs in parallel in the single-CPU policy case if cross-CPU updates are allowed to happen?

Second, is accessing rq_clock(rq) of a remote runqueue a good idea entirely?

From: Steve Muckle <[email protected]>

In preparation for the scheduler cpufreq callback happening on remote CPUs, check for this case in intel_pstate which currently requires the callback run on the local CPU. Such callbacks are ignored for now.

Signed-off-by Steve Muckle <[email protected]>

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

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

drivers/cpufreq/intel_pstate.c +0 -0

index 3d37219a0dd7..bd60f1cd7ea6 100644

Hi,

This is based of the work done by Steve Muckle [1] before he left Linaro and most of the patches are still under his authorship. I have done couple of improvements (detailed in individual patches) and removed the late callback support [2] as I wasn't sure of the value it adds. We can include it separately if others feel it is required. This series is based on pm/linux-next with patches [3] and [4] applied on top of it.

With Android UI and benchmarks the latency of cpufreq response to certain scheduling events can become very critical. Currently, callbacks into schedutil are only made from the scheduler if the target CPU of the event is the same as the current CPU. This means there are certain situations where a target CPU may not run schedutil for some time.

One testcase to show this behavior is where a task starts running on CPU0, then a new task is also spawned on CPU0 by a task on CPU1. If the system is configured such that new tasks should receive maximum demand initially, this should result in CPU0 increasing frequency immediately. Because of the above mentioned limitation though this does not occur. This is verified using ftrace with the sample [5] application.

This patchset updates the scheduler to call cpufreq callbacks for remote CPUs as well and updates schedutil governor to deal with it. An additional flag is added to cpufreq policies to avoid sending IPIs to remote CPUs to update the frequency, if CPUs on the platform can change frequency of any other CPU.

This series is tested with couple of usecases (Android: hackbench, recentfling, galleryfling, vellamo, Ubuntu: hackbench) on ARM hikey board (64 bit octa-core, single policy). Only galleryfling showed minor improvements, while others didn't had much deviation.

The reason being that this patchset only targets a corner case, where following are required to be true to improve performance and that doesn't happen too often with these tests:

- Task is migrated to another CPU.
- The task has maximum demand initially, and should take the CPU to higher OPPs.
- And the target CPU doesn't call into schedutil until the next tick, without this patchset.

From the first quick look patches [1-3/9] seem to be split out somewhat

From the first quick look patches [1-3/9] seem to be split out somewhat artificially.

Any chance to fold them into the patches where the new stuff is actually used?

I'll be looking at the rest of the patchset shortly.

Quoted 6 lines · Rafael J. Wysocki

From the first quick look patches [1-3/9] seem to be split out somewhat artificially.

Any chance to fold them into the patches where the new stuff is actually used?

I'll be looking at the rest of the patchset shortly.

I thought it would be better to keep them separate, but I can merge them to others in the next version if you want.