Hi,
Here is the second version of this series. The first [1] version was sent several months back.
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 [2] application.
Maybe the ideal solution is to always allow remote callbacks but that has its own challenges:
o There is no protection required for single CPU per policy case today, and adding any kind of locking there, to supply remote callbacks, isn't really a good idea.
o If is local CPU isn't part of the same cpufreq policy as the target CPU, then we wouldn't be able to do fast switching at all and have to use some kind of bottom half to schedule work on the target CPU to do real switching. That may be overkill as well.
Taking above challenges into consideration, this version proposes a much simpler diff as compared to the first version.
This series only allows remote callbacks for target CPUs that share the cpufreq policy with the local CPU. Locking is mostly in place everywhere and we wouldn't be required to change a lot of things.
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.
V1->V2: - Don't support remote callbacks for unshared cpufreq policies. - Don't support remote callbacks where local CPU isn't part of the target CPU's cpufreq policy. - Dropped dvfs_possible_from_any_cpu flag.
-- viresh
[1] https://marc.info/?l=linux-pm&m=148906015927796&w=2 [2] http://pastebin.com/7LkMSRxE
Steve Muckle (1): intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs
Viresh Kumar (3): cpufreq: schedutil: Process remote callback for shared policies cpufreq: governor: Process remote callback for shared policies sched: cpufreq: Enable remote sched cpufreq callbacks
drivers/cpufreq/cpufreq_governor.c | 4 ++++ drivers/cpufreq/intel_pstate.c | 3 +++ include/linux/sched/cpufreq.h | 1 + kernel/sched/cpufreq.c | 1 + kernel/sched/cpufreq_schedutil.c | 19 ++++++++++++++----- kernel/sched/deadline.c | 2 +- kernel/sched/fair.c | 8 +++++--- kernel/sched/rt.c | 2 +- kernel/sched/sched.h | 10 ++-------- 9 files changed, 32 insertions(+), 18 deletions(-)
-- 2.13.0.71.gd7076ec9c9cb
This patch updates the schedutil governor to process cpufreq utilization update hooks called for remote CPUs (i.e. For updates to the runqueue of other non-local CPUs). For now, we only support remote callbacks for CPUs which share their cpufreq policy with the local CPU.
It may not be worth allowing remote callbacks in other cases, as we wouldn't be able to update the frequency on local CPU in that case.
The schedutil governor already has proper locking in place for shared policy update hooks.
This also adds a new field "cpu" in "struct update_util_data", to identify the remote CPU.
Based on initial work from Steve Muckle.
Signed-off-by: Steve Muckle smuckle.linux@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/sched/cpufreq.h | 1 + kernel/sched/cpufreq.c | 1 + kernel/sched/cpufreq_schedutil.c | 19 ++++++++++++++----- 3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h index d2be2ccbb372..8256a8f35f22 100644 --- a/include/linux/sched/cpufreq.h +++ b/include/linux/sched/cpufreq.h @@ -16,6 +16,7 @@ #ifdef CONFIG_CPU_FREQ struct update_util_data { void (*func)(struct update_util_data *data, u64 time, unsigned int flags); + unsigned int cpu; };
void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c index dbc51442ecbc..ee4c596b71b4 100644 --- a/kernel/sched/cpufreq.c +++ b/kernel/sched/cpufreq.c @@ -42,6 +42,7 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, return;
data->func = func; + data->cpu = cpu; rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); } EXPORT_SYMBOL_GPL(cpufreq_add_update_util_hook); diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index 076a2e31951c..3f9cae9ab326 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -154,12 +154,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; @@ -218,6 +218,10 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, unsigned int next_f; bool busy;
+ /* Remote callbacks aren't allowed for policies which aren't shared */ + if (smp_processor_id() != hook->cpu) + return; + sugov_set_iowait_boost(sg_cpu, time, flags); sg_cpu->last_update = time;
@@ -229,7 +233,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); sugov_iowait_boost(sg_cpu, &util, &max); next_f = get_next_freq(sg_policy, util, max); /* @@ -287,10 +291,15 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, { struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util); struct sugov_policy *sg_policy = sg_cpu->sg_policy; + struct cpufreq_policy *policy = sg_policy->policy; unsigned long util, max; unsigned int next_f;
- sugov_get_util(&util, &max); + /* Allow remote callbacks only on the CPUs sharing cpufreq policy */ + if (!cpumask_test_cpu(smp_processor_id(), policy->cpus)) + return; + + sugov_get_util(&util, &max, hook->cpu);
raw_spin_lock(&sg_policy->update_lock);
-- 2.13.0.71.gd7076ec9c9cb
This patch updates the legacy governors (ondemand/conservative) to process cpufreq utilization update hooks to be called for remote CPUs (i.e. For updates to the runqueue of other non-local CPUs).
Based on initial work from Steve Muckle.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq_governor.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 47e24b5384b3..0b49fc8bb91d 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -275,6 +275,10 @@ static void dbs_update_util_handler(struct update_util_data *data, u64 time, struct policy_dbs_info *policy_dbs = cdbs->policy_dbs; u64 delta_ns, lst;
+ /* Allow remote callbacks only on the CPUs sharing cpufreq policy */ + if (!cpumask_test_cpu(smp_processor_id(), policy_dbs->policy->cpus)) + return; + /* * The work may not be allowed to be queued up right now. * Possible reasons: -- 2.13.0.71.gd7076ec9c9cb
From: Steve Muckle smuckle.linux@gmail.com
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 smuckle.linux@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/intel_pstate.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index 4ce501148790..7a2a8ee579ef 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -1755,6 +1755,9 @@ static void intel_pstate_update_util(struct update_util_data *data, u64 time, struct cpudata *cpu = container_of(data, struct cpudata, update_util); u64 delta_ns;
+ if (smp_processor_id() != data->cpu) + return; + if (flags & SCHED_CPUFREQ_IOWAIT) { cpu->iowait_boost = int_tofp(1); } else if (cpu->iowait_boost) { -- 2.13.0.71.gd7076ec9c9cb
On Thu, 2017-06-29 at 10:56 +0530, Viresh Kumar wrote:
From: Steve Muckle smuckle.linux@gmail.com
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.
Is it possible that we miss a chance to calculate load periodically at a predefined interval (10ms default), because the callback happened on a different CPU?
Thanks, Srinivas
Signed-off-by: Steve Muckle smuckle.linux@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/intel_pstate.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index 4ce501148790..7a2a8ee579ef 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -1755,6 +1755,9 @@ static void intel_pstate_update_util(struct update_util_data *data, u64 time, struct cpudata *cpu = container_of(data, struct cpudata, update_util); u64 delta_ns;
- if (smp_processor_id() != data->cpu)
return;if (flags & SCHED_CPUFREQ_IOWAIT) { cpu->iowait_boost = int_tofp(1); } else if (cpu->iowait_boost) {
On 29-06-17, 14:23, Srinivas Pandruvada wrote:
On Thu, 2017-06-29 at 10:56 +0530, Viresh Kumar wrote:
From: Steve Muckle smuckle.linux@gmail.com
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.
Is it possible that we miss a chance to calculate load periodically at a predefined interval (10ms default), because the callback happened on a different CPU?
We aren't updating cpu->sample.time for remote callbacks here, so no we shouldn't miss anything.
-- viresh
On Thursday, June 29, 2017 02:23:58 PM Srinivas Pandruvada wrote:
On Thu, 2017-06-29 at 10:56 +0530, Viresh Kumar wrote:
From: Steve Muckle smuckle.linux@gmail.com
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.
Is it possible that we miss a chance to calculate load periodically at a predefined interval (10ms default), because the callback happened on a different CPU?
We won't.
The callback will just be invoked more often after this series and the check this patch is adding to it is actually there in the callers now, so effectively this just moves the check down the callchain.
It is confusing, though, because the two complementary changes are made by different patches.
Also, Viresh please note that we have more than one governor callback in intel_pstate now.
Thanks, Rafael
Now that all clients properly support (or ignore) remote scheduler cpufreq callbacks, remove the restriction that such callbacks only be made on the local CPU.
Also remove cpufreq_update_this_cpu() as all its users are migrated to use cpufreq_update_util() instead.
Based on initial work from Steve Muckle.
Signed-off-by: Steve Muckle smuckle.linux@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/sched/deadline.c | 2 +- kernel/sched/fair.c | 8 +++++--- kernel/sched/rt.c | 2 +- kernel/sched/sched.h | 10 ++-------- 4 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c index a2ce59015642..512d51226998 100644 --- a/kernel/sched/deadline.c +++ b/kernel/sched/deadline.c @@ -763,7 +763,7 @@ static void update_curr_dl(struct rq *rq) }
/* kick cpufreq (see the comment in kernel/sched/sched.h). */ - cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_DL); + cpufreq_update_util(rq, SCHED_CPUFREQ_DL);
schedstat_set(curr->se.statistics.exec_max, max(curr->se.statistics.exec_max, delta_exec)); diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index c77e4b1d51c0..77ef663e1380 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -3215,7 +3215,9 @@ static inline void set_tg_cfs_propagate(struct cfs_rq *cfs_rq) {}
static inline void cfs_rq_util_change(struct cfs_rq *cfs_rq) { - if (&this_rq()->cfs == cfs_rq) { + struct rq *rq = rq_of(cfs_rq); + + if (&rq->cfs == cfs_rq) { /* * There are a few boundary cases this might miss but it should * get called often enough that that should (hopefully) not be @@ -3232,7 +3234,7 @@ static inline void cfs_rq_util_change(struct cfs_rq *cfs_rq) * * See cpu_util(). */ - cpufreq_update_util(rq_of(cfs_rq), 0); + cpufreq_update_util(rq, 0); } }
@@ -4792,7 +4794,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags) * passed. */ if (p->in_iowait) - cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_IOWAIT); + cpufreq_update_util(rq, SCHED_CPUFREQ_IOWAIT);
for_each_sched_entity(se) { if (se->on_rq) diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c index 979b7341008a..1e626e49f7fc 100644 --- a/kernel/sched/rt.c +++ b/kernel/sched/rt.c @@ -959,7 +959,7 @@ static void update_curr_rt(struct rq *rq) return;
/* Kick cpufreq (see the comment in kernel/sched/sched.h). */ - cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_RT); + cpufreq_update_util(rq, SCHED_CPUFREQ_RT);
schedstat_set(curr->se.statistics.exec_max, max(curr->se.statistics.exec_max, delta_exec)); diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 6dda2aab731e..cce497b5837c 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -1987,19 +1987,13 @@ static inline void cpufreq_update_util(struct rq *rq, unsigned int flags) { struct update_util_data *data;
- data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data)); + data = rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data, + cpu_of(rq))); if (data) data->func(data, rq_clock(rq), flags); } - -static inline void cpufreq_update_this_cpu(struct rq *rq, unsigned int flags) -{ - if (cpu_of(rq) == smp_processor_id()) - cpufreq_update_util(rq, flags); -} #else static inline void cpufreq_update_util(struct rq *rq, unsigned int flags) {} -static inline void cpufreq_update_this_cpu(struct rq *rq, unsigned int flags) {} #endif /* CONFIG_CPU_FREQ */
#ifdef arch_scale_freq_capacity -- 2.13.0.71.gd7076ec9c9cb
Hi,
On Thu, Jun 29, 2017 at 7:26 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi,
Here is the second version of this series. The first [1] version was sent several months back.
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 [2] application.
Maybe the ideal solution is to always allow remote callbacks but that has its own challenges:
o There is no protection required for single CPU per policy case today, and adding any kind of locking there, to supply remote callbacks, isn't really a good idea.
o If is local CPU isn't part of the same cpufreq policy as the target CPU, then we wouldn't be able to do fast switching at all and have to use some kind of bottom half to schedule work on the target CPU to do real switching. That may be overkill as well.
Taking above challenges into consideration, this version proposes a much simpler diff as compared to the first version.
This series only allows remote callbacks for target CPUs that share the cpufreq policy with the local CPU. Locking is mostly in place everywhere and we wouldn't be required to change a lot of things.
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.
V1->V2:
- Don't support remote callbacks for unshared cpufreq policies.
- Don't support remote callbacks where local CPU isn't part of the target CPU's cpufreq policy.
- Dropped dvfs_possible_from_any_cpu flag.
There is no way I could consider this for inclusion into 4.13, so I'm not sure why you chose this specific timing.
Thanks, Rafael
On 29-06-17, 22:30, Rafael J. Wysocki wrote:
There is no way I could consider this for inclusion into 4.13, so I'm not sure why you chose this specific timing.
Sure, I don't aim at getting it merged for 4.13. I sent it now to get some early feedback, so that it is ready for inclusion for the 4.14 merge window.
Isn't it fine to send such patches anytime? Should I avoid sending such changes around merge window ?
-- viresh
On Thursday, June 29, 2017 10:56:29 AM Viresh Kumar wrote:
Hi,
Here is the second version of this series. The first [1] version was sent several months back.
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 [2] application.
Maybe the ideal solution is to always allow remote callbacks but that has its own challenges:
o There is no protection required for single CPU per policy case today, and adding any kind of locking there, to supply remote callbacks, isn't really a good idea.
o If is local CPU isn't part of the same cpufreq policy as the target CPU, then we wouldn't be able to do fast switching at all and have to use some kind of bottom half to schedule work on the target CPU to do real switching. That may be overkill as well.
Taking above challenges into consideration, this version proposes a much simpler diff as compared to the first version.
This series only allows remote callbacks for target CPUs that share the cpufreq policy with the local CPU. Locking is mostly in place everywhere and we wouldn't be required to change a lot of things.
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.
V1->V2:
- Don't support remote callbacks for unshared cpufreq policies.
- Don't support remote callbacks where local CPU isn't part of the target CPU's cpufreq policy.
- Dropped dvfs_possible_from_any_cpu flag.
I would rearrange the changes.
You need two patches for that IMO, one moving the smp_processor_id() check from the callers of cpufreq_update_util()/cpufreq_update_this_cpu() to the callbacks in all governors and the other one modifying schedutil to work with cross-CPU updates.
That at least would reduce the confusion factor somewhat. :-)
Thanks, Rafael
-
Rafael J. Wysocki -
Rafael J. Wysocki -
Srinivas Pandruvada -
Viresh Kumar