All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks
@ 2017-06-29  5:26 Viresh Kumar
  2017-06-29  5:26 ` [PATCH V2 1/4] cpufreq: schedutil: Process remote callback for shared policies Viresh Kumar
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-06-29  5:26 UTC (permalink / raw)
  To: Rafael Wysocki, Ingo Molnar, Peter Zijlstra
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel,
	smuckle.linux, juri.lelli, Morten.Rasmussen, patrick.bellasi,
	eas-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH V2 1/4] cpufreq: schedutil: Process remote callback for shared policies
  2017-06-29  5:26 [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Viresh Kumar
@ 2017-06-29  5:26 ` Viresh Kumar
  2017-06-29  5:26 ` [PATCH V2 2/4] cpufreq: governor: " Viresh Kumar
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-06-29  5:26 UTC (permalink / raw)
  To: Rafael Wysocki, Ingo Molnar, Peter Zijlstra
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel,
	smuckle.linux, juri.lelli, Morten.Rasmussen, patrick.bellasi,
	eas-dev

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V2 2/4] cpufreq: governor: Process remote callback for shared policies
  2017-06-29  5:26 [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Viresh Kumar
  2017-06-29  5:26 ` [PATCH V2 1/4] cpufreq: schedutil: Process remote callback for shared policies Viresh Kumar
@ 2017-06-29  5:26 ` Viresh Kumar
  2017-06-29  5:26 ` [PATCH V2 3/4] intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs Viresh Kumar
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-06-29  5:26 UTC (permalink / raw)
  To: Rafael Wysocki, Ingo Molnar, Peter Zijlstra, Viresh Kumar
  Cc: linux-pm, Vincent Guittot, linux-kernel, smuckle.linux,
	juri.lelli, Morten.Rasmussen, patrick.bellasi, eas-dev

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V2 3/4] intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs
  2017-06-29  5:26 [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Viresh Kumar
  2017-06-29  5:26 ` [PATCH V2 1/4] cpufreq: schedutil: Process remote callback for shared policies Viresh Kumar
  2017-06-29  5:26 ` [PATCH V2 2/4] cpufreq: governor: " Viresh Kumar
@ 2017-06-29  5:26 ` Viresh Kumar
  2017-06-29 21:23   ` Srinivas Pandruvada
  2017-06-29  5:26 ` [PATCH V2 4/4] sched: cpufreq: Enable remote sched cpufreq callbacks Viresh Kumar
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Viresh Kumar @ 2017-06-29  5:26 UTC (permalink / raw)
  To: Rafael Wysocki, Ingo Molnar, Peter Zijlstra, Srinivas Pandruvada,
	Len Brown, Viresh Kumar
  Cc: linux-pm, Vincent Guittot, linux-kernel, smuckle.linux,
	juri.lelli, Morten.Rasmussen, patrick.bellasi, eas-dev

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V2 4/4] sched: cpufreq: Enable remote sched cpufreq callbacks
  2017-06-29  5:26 [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Viresh Kumar
                   ` (2 preceding siblings ...)
  2017-06-29  5:26 ` [PATCH V2 3/4] intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs Viresh Kumar
@ 2017-06-29  5:26 ` Viresh Kumar
  2017-06-29 20:30 ` [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Rafael J. Wysocki
  2017-07-12 23:22 ` Rafael J. Wysocki
  5 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-06-29  5:26 UTC (permalink / raw)
  To: Rafael Wysocki, Ingo Molnar, Peter Zijlstra
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel,
	smuckle.linux, juri.lelli, Morten.Rasmussen, patrick.bellasi,
	eas-dev

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks
  2017-06-29  5:26 [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Viresh Kumar
                   ` (3 preceding siblings ...)
  2017-06-29  5:26 ` [PATCH V2 4/4] sched: cpufreq: Enable remote sched cpufreq callbacks Viresh Kumar
@ 2017-06-29 20:30 ` Rafael J. Wysocki
  2017-06-30  3:24   ` Viresh Kumar
  2017-07-12 23:22 ` Rafael J. Wysocki
  5 siblings, 1 reply; 11+ messages in thread
From: Rafael J. Wysocki @ 2017-06-29 20:30 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Ingo Molnar, Peter Zijlstra, Linux PM,
	Vincent Guittot, Linux Kernel Mailing List, Steve Muckle,
	Juri Lelli, Morten Rasmussen, Patrick Bellasi, eas-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V2 3/4] intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs
  2017-06-29  5:26 ` [PATCH V2 3/4] intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs Viresh Kumar
@ 2017-06-29 21:23   ` Srinivas Pandruvada
  2017-06-30  3:27     ` Viresh Kumar
  2017-07-12 23:27     ` Rafael J. Wysocki
  0 siblings, 2 replies; 11+ messages in thread
From: Srinivas Pandruvada @ 2017-06-29 21:23 UTC (permalink / raw)
  To: Viresh Kumar, Rafael Wysocki, Ingo Molnar, Peter Zijlstra, Len Brown
  Cc: linux-pm, Vincent Guittot, linux-kernel, smuckle.linux,
	juri.lelli, Morten.Rasmussen, patrick.bellasi, eas-dev

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) {

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks
  2017-06-29 20:30 ` [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Rafael J. Wysocki
@ 2017-06-30  3:24   ` Viresh Kumar
  0 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-06-30  3:24 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael Wysocki, Ingo Molnar, Peter Zijlstra, Linux PM,
	Vincent Guittot, Linux Kernel Mailing List, Steve Muckle,
	Juri Lelli, Morten Rasmussen, Patrick Bellasi, eas-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V2 3/4] intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs
  2017-06-29 21:23   ` Srinivas Pandruvada
@ 2017-06-30  3:27     ` Viresh Kumar
  2017-07-12 23:27     ` Rafael J. Wysocki
  1 sibling, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-06-30  3:27 UTC (permalink / raw)
  To: Srinivas Pandruvada
  Cc: Rafael Wysocki, Ingo Molnar, Peter Zijlstra, Len Brown, linux-pm,
	Vincent Guittot, linux-kernel, smuckle.linux, juri.lelli,
	Morten.Rasmussen, patrick.bellasi, eas-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks
  2017-06-29  5:26 [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Viresh Kumar
                   ` (4 preceding siblings ...)
  2017-06-29 20:30 ` [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Rafael J. Wysocki
@ 2017-07-12 23:22 ` Rafael J. Wysocki
  5 siblings, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2017-07-12 23:22 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Ingo Molnar, Peter Zijlstra, linux-pm, Vincent Guittot,
	linux-kernel, smuckle.linux, juri.lelli, Morten.Rasmussen,
	patrick.bellasi, eas-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V2 3/4] intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs
  2017-06-29 21:23   ` Srinivas Pandruvada
  2017-06-30  3:27     ` Viresh Kumar
@ 2017-07-12 23:27     ` Rafael J. Wysocki
  1 sibling, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2017-07-12 23:27 UTC (permalink / raw)
  To: Srinivas Pandruvada, Viresh Kumar
  Cc: Ingo Molnar, Peter Zijlstra, Len Brown, linux-pm,
	Vincent Guittot, linux-kernel, smuckle.linux, juri.lelli,
	Morten.Rasmussen, patrick.bellasi, eas-dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-07-12 23:35 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-29  5:26 [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Viresh Kumar
2017-06-29  5:26 ` [PATCH V2 1/4] cpufreq: schedutil: Process remote callback for shared policies Viresh Kumar
2017-06-29  5:26 ` [PATCH V2 2/4] cpufreq: governor: " Viresh Kumar
2017-06-29  5:26 ` [PATCH V2 3/4] intel_pstate: Ignore scheduler cpufreq callbacks on remote CPUs Viresh Kumar
2017-06-29 21:23   ` Srinivas Pandruvada
2017-06-30  3:27     ` Viresh Kumar
2017-07-12 23:27     ` Rafael J. Wysocki
2017-06-29  5:26 ` [PATCH V2 4/4] sched: cpufreq: Enable remote sched cpufreq callbacks Viresh Kumar
2017-06-29 20:30 ` [PATCH V2 0/4] sched: cpufreq: Allow remote callbacks Rafael J. Wysocki
2017-06-30  3:24   ` Viresh Kumar
2017-07-12 23:22 ` Rafael J. Wysocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.