| Message ID | 1469453670-2660-10-git-send-email-morten.rasmussen@arm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Aug 15, 2016 at 03:39:49PM +0200, Peter Zijlstra wrote: > On Mon, Jul 25, 2016 at 02:34:26PM +0100, Morten Rasmussen wrote: > > > Because I forgot _again_, I added: > > /* > * Disable WAKE_AFFINE in the case where task @p doesn't fit in the > * capacity of either the waking CPU @cpu or the previous CPU @prev_cpu. > * > * In that case WAKE_AFFINE doesn't make sense and we'll let > * BALANCE_WAKE sort things out. > */ Thanks. > > > +static int wake_cap(struct task_struct *p, int cpu, int prev_cpu) > > +{ > > + long min_cap, max_cap; > > + > > + min_cap = min(capacity_orig_of(prev_cpu), capacity_orig_of(cpu)); > > + max_cap = cpu_rq(cpu)->rd->max_cpu_capacity; > > There's a tiny hole here, which I'm fairly sure we don't care about. If > @p last ran on @prev_cpu before @prev_cpu was split from @rd this > doesn't 'work' right. I hadn't considered that. What is 'working right' in this scenario? Ignoring @prev_cpu as it isn't a valid option anymore? In that case, since @prev_cpu is only used as part the min() it should only cause min_cap to be potentially smaller than it should be, not larger. It could lead us to let BALANCE_WAKE take over in scenarios where select_idle_sibling() would have been sufficient, but it should harm. However, as you say, I'm not sure if we care that much. Talking about @rd, I discussed with Juri and Dietmar the other week whether the root_domain is RCU protected, and if we therefore have to move the call to wake_cap() after the rcu_read_lock() below. I haven't yet done thorough investigation to find the answer. Should it be protected? > > > + /* Minimum capacity is close to max, no need to abort wake_affine */ > > + if (max_cap - min_cap < max_cap >> 3) > > + return 0; > > + > > + return min_cap * 1024 < task_util(p) * capacity_margin; > > +} > > + > > /* > > * select_task_rq_fair: Select target runqueue for the waking task in domains > > * that have the 'sd_flag' flag set. In practice, this is SD_BALANCE_WAKE, > > @@ -5389,7 +5414,8 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f > > > > if (sd_flag & SD_BALANCE_WAKE) { > > record_wakee(p); > > - want_affine = !wake_wide(p) && cpumask_test_cpu(cpu, tsk_cpus_allowed(p)); > > + want_affine = !wake_wide(p) && !wake_cap(p, cpu, prev_cpu) > > + && cpumask_test_cpu(cpu, tsk_cpus_allowed(p)); > > } > > > > rcu_read_lock(); > > -- > > 1.9.1 > >
On Mon, Aug 15, 2016 at 05:10:06PM +0200, Peter Zijlstra wrote: > On Mon, Aug 15, 2016 at 04:01:34PM +0100, Morten Rasmussen wrote: > > On Mon, Aug 15, 2016 at 03:39:49PM +0200, Peter Zijlstra wrote: > > > > +static int wake_cap(struct task_struct *p, int cpu, int prev_cpu) > > > > +{ > > > > + long min_cap, max_cap; > > > > + > > > > + min_cap = min(capacity_orig_of(prev_cpu), capacity_orig_of(cpu)); > > > > + max_cap = cpu_rq(cpu)->rd->max_cpu_capacity; > > > > > > There's a tiny hole here, which I'm fairly sure we don't care about. If > > > @p last ran on @prev_cpu before @prev_cpu was split from @rd this > > > doesn't 'work' right. > > > > I hadn't considered that. What is 'working right' in this scenario? > > Ignoring @prev_cpu as it isn't a valid option anymore? > > Probably, yeah. > > > In that case, since @prev_cpu is only used as part the min() it should > > only cause min_cap to be potentially smaller than it should be, not > > larger. It could lead us to let BALANCE_WAKE take over in scenarios > > where select_idle_sibling() would have been sufficient, but it should > > harm. > > +not, right? Yes :) > > > However, as you say, I'm not sure if we care that much. > > Yeah, don't think so, its extremely unlikely to happen, almost nobody > mucks about with root_domains anyway. And those that do, do so once to > setup things and then leave them be. > > > Talking about @rd, I discussed with Juri and Dietmar the other week > > whether the root_domain is RCU protected, and if we therefore have to > > move the call to wake_cap() after the rcu_read_lock() below. I haven't > > yet done thorough investigation to find the answer. Should it be > > protected? > > Yeah, I think either RCU or RCU-sched, I forever forget. Okay. Should I send an updated version?
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index c1c0aa8b300b..9e217eff3daf 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -114,6 +114,12 @@ unsigned int __read_mostly sysctl_sched_shares_window = 10000000UL; unsigned int sysctl_sched_cfs_bandwidth_slice = 5000UL; #endif +/* + * The margin used when comparing utilization with cpu capacity: + * util * 1024 < capacity * margin + */ +unsigned int capacity_margin = 1280; /* ~20% */ + static inline void update_load_add(struct load_weight *lw, unsigned long inc) { lw->weight += inc; @@ -5366,6 +5372,25 @@ static int cpu_util(int cpu) return (util >= capacity) ? capacity : util; } +static inline int task_util(struct task_struct *p) +{ + return p->se.avg.util_avg; +} + +static int wake_cap(struct task_struct *p, int cpu, int prev_cpu) +{ + long min_cap, max_cap; + + min_cap = min(capacity_orig_of(prev_cpu), capacity_orig_of(cpu)); + max_cap = cpu_rq(cpu)->rd->max_cpu_capacity; + + /* Minimum capacity is close to max, no need to abort wake_affine */ + if (max_cap - min_cap < max_cap >> 3) + return 0; + + return min_cap * 1024 < task_util(p) * capacity_margin; +} + /* * select_task_rq_fair: Select target runqueue for the waking task in domains * that have the 'sd_flag' flag set. In practice, this is SD_BALANCE_WAKE, @@ -5389,7 +5414,8 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f if (sd_flag & SD_BALANCE_WAKE) { record_wakee(p); - want_affine = !wake_wide(p) && cpumask_test_cpu(cpu, tsk_cpus_allowed(p)); + want_affine = !wake_wide(p) && !wake_cap(p, cpu, prev_cpu) + && cpumask_test_cpu(cpu, tsk_cpus_allowed(p)); } rcu_read_lock();
Currently, SD_WAKE_AFFINE always takes priority over wakeup balancing if SD_BALANCE_WAKE is set on the sched_domains. For asymmetric configurations SD_WAKE_AFFINE is only desirable if the waking task's compute demand (utilization) is suitable for the waking cpu and the previous cpu, and all cpus within their respective SD_SHARE_PKG_RESOURCES domains (sd_llc). If not, let wakeup balancing take over (find_idlest_{group, cpu}()). This patch makes affine wake-ups conditional on whether both the waker cpu and prev_cpu has sufficient capacity for the waking task, or not, assuming that the cpu capacities within an SD_SHARE_PKG_RESOURCES domain (sd_llc) are homogeneous. cc: Ingo Molnar <mingo@redhat.com> cc: Peter Zijlstra <peterz@infradead.org> Acked-by: Vincent Guittot <vincent.guittot@linaro.org> Signed-off-by: Morten Rasmussen <morten.rasmussen@arm.com> --- kernel/sched/fair.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) -- 1.9.1