* [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it @ 2018-04-23 10:38 Viresh Kumar 2018-04-24 10:02 ` Valentin Schneider 0 siblings, 1 reply; 18+ messages in thread From: Viresh Kumar @ 2018-04-23 10:38 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra Cc: Viresh Kumar, Vincent Guittot, Daniel Lezcano, linux-kernel Rearrange select_task_rq_fair() a bit to avoid executing some conditional statements in few specific code-paths. That gets rid of the goto as well. This shouldn't result in any functional changes. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- kernel/sched/fair.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 54dc31e7ab9b..cacee15076a4 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6636,6 +6636,7 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f */ if (want_affine && (tmp->flags & SD_WAKE_AFFINE) && cpumask_test_cpu(prev_cpu, sched_domain_span(tmp))) { + sd = NULL; /* Prefer wake_affine over balance flags */ affine_sd = tmp; break; } @@ -6646,33 +6647,26 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f break; } - if (affine_sd) { - sd = NULL; /* Prefer wake_affine over balance flags */ - if (cpu == prev_cpu) - goto pick_cpu; - - new_cpu = wake_affine(affine_sd, p, cpu, prev_cpu, sync); - } - - if (sd && !(sd_flag & SD_BALANCE_FORK)) { + if (sd) { /* * We're going to need the task's util for capacity_spare_wake * in find_idlest_group. Sync it up to prev_cpu's * last_update_time. */ - sync_entity_load_avg(&p->se); - } + if (!(sd_flag & SD_BALANCE_FORK)) + sync_entity_load_avg(&p->se); + + new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag); + } else { + if (affine_sd && cpu != prev_cpu) + new_cpu = wake_affine(affine_sd, p, cpu, prev_cpu, sync); - if (!sd) { -pick_cpu: if (sd_flag & SD_BALANCE_WAKE) { /* XXX always ? */ new_cpu = select_idle_sibling(p, prev_cpu, new_cpu); if (want_affine) current->recent_used_cpu = cpu; } - } else { - new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag); } rcu_read_unlock(); -- 2.15.0.194.g9af6a3dea062 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-23 10:38 [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it Viresh Kumar @ 2018-04-24 10:02 ` Valentin Schneider 2018-04-24 10:30 ` Viresh Kumar 2018-04-24 10:43 ` Peter Zijlstra 0 siblings, 2 replies; 18+ messages in thread From: Valentin Schneider @ 2018-04-24 10:02 UTC (permalink / raw) To: Viresh Kumar, Ingo Molnar, Peter Zijlstra Cc: Vincent Guittot, Daniel Lezcano, linux-kernel Hi, On 23/04/18 11:38, Viresh Kumar wrote: > Rearrange select_task_rq_fair() a bit to avoid executing some > conditional statements in few specific code-paths. That gets rid of the > goto as well. > I'd argue making things easier to read is a non-negligible part as well. > This shouldn't result in any functional changes. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > kernel/sched/fair.c | 24 +++++++++--------------- > 1 file changed, 9 insertions(+), 15 deletions(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 54dc31e7ab9b..cacee15076a4 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -6636,6 +6636,7 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f > */ > if (want_affine && (tmp->flags & SD_WAKE_AFFINE) && > cpumask_test_cpu(prev_cpu, sched_domain_span(tmp))) { > + sd = NULL; /* Prefer wake_affine over balance flags */ > affine_sd = tmp; > break; > } > @@ -6646,33 +6647,26 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f > break; > } > > - if (affine_sd) { > - sd = NULL; /* Prefer wake_affine over balance flags */ > - if (cpu == prev_cpu) > - goto pick_cpu; > - > - new_cpu = wake_affine(affine_sd, p, cpu, prev_cpu, sync); > - } > - > - if (sd && !(sd_flag & SD_BALANCE_FORK)) { > + if (sd) { > /* > * We're going to need the task's util for capacity_spare_wake > * in find_idlest_group. Sync it up to prev_cpu's > * last_update_time. > */ > - sync_entity_load_avg(&p->se); > - } > + if (!(sd_flag & SD_BALANCE_FORK)) > + sync_entity_load_avg(&p->se); > + > + new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag); > + } else { > + if (affine_sd && cpu != prev_cpu) > + new_cpu = wake_affine(affine_sd, p, cpu, prev_cpu, sync); > > - if (!sd) { > -pick_cpu: > if (sd_flag & SD_BALANCE_WAKE) { /* XXX always ? */ > new_cpu = select_idle_sibling(p, prev_cpu, new_cpu); > > if (want_affine) > current->recent_used_cpu = cpu; > } > - } else { > - new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag); > } > rcu_read_unlock(); > > I stared at it for a bit and don't see anything wrong. I was just thinking that the original flow made it a bit clearer to follow the 'wake_affine' path. What about this ? It re-bumps up the number of conditionals and adds an indent level in the domain loop (that could be prevented by hiding the cpu != prev_cpu check in wake_affine()), which isn't great, but you get to squash some more if's. ---------------------->8------------------------- diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index cacee15..ad09b67 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6613,7 +6613,7 @@ static int wake_cap(struct task_struct *p, int cpu, int prev_cpu) static int select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_flags) { - struct sched_domain *tmp, *affine_sd = NULL, *sd = NULL; + struct sched_domain *tmp, *sd = NULL; int cpu = smp_processor_id(); int new_cpu = prev_cpu; int want_affine = 0; @@ -6636,8 +6636,10 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f */ if (want_affine && (tmp->flags & SD_WAKE_AFFINE) && cpumask_test_cpu(prev_cpu, sched_domain_span(tmp))) { + if (cpu != prev_cpu) + new_cpu = wake_affine(tmp, p, cpu, prev_cpu, sync); + sd = NULL; /* Prefer wake_affine over balance flags */ - affine_sd = tmp; break; } @@ -6657,16 +6659,11 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f sync_entity_load_avg(&p->se); new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag); - } else { - if (affine_sd && cpu != prev_cpu) - new_cpu = wake_affine(affine_sd, p, cpu, prev_cpu, sync); + } else if (sd_flag & SD_BALANCE_WAKE) { /* XXX always ? */ + new_cpu = select_idle_sibling(p, prev_cpu, new_cpu); - if (sd_flag & SD_BALANCE_WAKE) { /* XXX always ? */ - new_cpu = select_idle_sibling(p, prev_cpu, new_cpu); - - if (want_affine) - current->recent_used_cpu = cpu; - } + if (want_affine) + current->recent_used_cpu = cpu; } rcu_read_unlock(); ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 10:02 ` Valentin Schneider @ 2018-04-24 10:30 ` Viresh Kumar 2018-04-24 10:43 ` Peter Zijlstra 1 sibling, 0 replies; 18+ messages in thread From: Viresh Kumar @ 2018-04-24 10:30 UTC (permalink / raw) To: Valentin Schneider Cc: Ingo Molnar, Peter Zijlstra, Vincent Guittot, Daniel Lezcano, linux-kernel On 24-04-18, 11:02, Valentin Schneider wrote: > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index cacee15..ad09b67 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -6613,7 +6613,7 @@ static int wake_cap(struct task_struct *p, int cpu, int prev_cpu) > static int > select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_flags) > { > - struct sched_domain *tmp, *affine_sd = NULL, *sd = NULL; > + struct sched_domain *tmp, *sd = NULL; > int cpu = smp_processor_id(); > int new_cpu = prev_cpu; > int want_affine = 0; > @@ -6636,8 +6636,10 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f > */ > if (want_affine && (tmp->flags & SD_WAKE_AFFINE) && > cpumask_test_cpu(prev_cpu, sched_domain_span(tmp))) { > + if (cpu != prev_cpu) > + new_cpu = wake_affine(tmp, p, cpu, prev_cpu, sync); > + > sd = NULL; /* Prefer wake_affine over balance flags */ > - affine_sd = tmp; > break; > } > > @@ -6657,16 +6659,11 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f > sync_entity_load_avg(&p->se); > > new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag); > - } else { > - if (affine_sd && cpu != prev_cpu) > - new_cpu = wake_affine(affine_sd, p, cpu, prev_cpu, sync); > + } else if (sd_flag & SD_BALANCE_WAKE) { /* XXX always ? */ > + new_cpu = select_idle_sibling(p, prev_cpu, new_cpu); > > - if (sd_flag & SD_BALANCE_WAKE) { /* XXX always ? */ > - new_cpu = select_idle_sibling(p, prev_cpu, new_cpu); > - > - if (want_affine) > - current->recent_used_cpu = cpu; > - } > + if (want_affine) > + current->recent_used_cpu = cpu; > } > rcu_read_unlock(); LGTM. I will merge it as part of the current patch, but maybe wait for a few days before sending V2. -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 10:02 ` Valentin Schneider 2018-04-24 10:30 ` Viresh Kumar @ 2018-04-24 10:43 ` Peter Zijlstra 2018-04-24 11:19 ` Valentin Schneider 1 sibling, 1 reply; 18+ messages in thread From: Peter Zijlstra @ 2018-04-24 10:43 UTC (permalink / raw) To: Valentin Schneider Cc: Viresh Kumar, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel On Tue, Apr 24, 2018 at 11:02:26AM +0100, Valentin Schneider wrote: > I'd argue making things easier to read is a non-negligible part as well. Right, so I don't object to either of these (I think); but it would be good to see this in combination with that proposed EAS change. I think you (valentin) wanted to side-step the entire domain loop in that case or something. But yes, getting this code more readable is defninitely useful. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 10:43 ` Peter Zijlstra @ 2018-04-24 11:19 ` Valentin Schneider 2018-04-24 12:35 ` Peter Zijlstra 0 siblings, 1 reply; 18+ messages in thread From: Valentin Schneider @ 2018-04-24 11:19 UTC (permalink / raw) To: Peter Zijlstra Cc: Viresh Kumar, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel, Quentin Perret On 24/04/18 11:43, Peter Zijlstra wrote: > On Tue, Apr 24, 2018 at 11:02:26AM +0100, Valentin Schneider wrote: >> I'd argue making things easier to read is a non-negligible part as well. > > Right, so I don't object to either of these (I think); but it would be > good to see this in combination with that proposed EAS change. > True, I would've said the call to find_energy_efficient_cpu() ([1]) could simply be added to the if (sd) {} case, but... > I think you (valentin) wanted to side-step the entire domain loop in > that case or something. > ...this would change more things. Admittedly I've been sort of out of the loop (no pun intended) lately, but this doesn't ring a bell. That might have been the other frenchie (Quentin) :) > But yes, getting this code more readable is defninitely useful. > [1]: See [RFC PATCH v2 5/6] sched/fair: Select an energy-efficient CPU on task wake-up @ https://lkml.org/lkml/2018/4/6/856 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 11:19 ` Valentin Schneider @ 2018-04-24 12:35 ` Peter Zijlstra 2018-04-24 15:46 ` Joel Fernandes ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Peter Zijlstra @ 2018-04-24 12:35 UTC (permalink / raw) To: Valentin Schneider Cc: Viresh Kumar, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel, Quentin Perret, c On Tue, Apr 24, 2018 at 12:19:07PM +0100, Valentin Schneider wrote: > On 24/04/18 11:43, Peter Zijlstra wrote: > > On Tue, Apr 24, 2018 at 11:02:26AM +0100, Valentin Schneider wrote: > >> I'd argue making things easier to read is a non-negligible part as well. > > > > Right, so I don't object to either of these (I think); but it would be > > good to see this in combination with that proposed EAS change. > > > > True, I would've said the call to find_energy_efficient_cpu() ([1]) could > simply be added to the if (sd) {} case, but... I think the proposal was to put it before the for_each_domain() loop entirely, however... > > I think you (valentin) wanted to side-step the entire domain loop in > > that case or something. > > > > ...this would change more things. Admittedly I've been sort of out of the loop > (no pun intended) lately, but this doesn't ring a bell. That might have been > the other frenchie (Quentin) :) It does indeed appear I confused the two of you, it was Quentin playing with that. In any case, if there not going to be conflicts here, this all looks good. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 12:35 ` Peter Zijlstra @ 2018-04-24 15:46 ` Joel Fernandes 2018-04-24 15:47 ` Joel Fernandes 2018-04-25 5:15 ` Viresh Kumar 2018-04-25 8:12 ` Quentin Perret 2 siblings, 1 reply; 18+ messages in thread From: Joel Fernandes @ 2018-04-24 15:46 UTC (permalink / raw) To: Peter Zijlstra Cc: Valentin Schneider, Viresh Kumar, Ingo Molnar, Vincent Guittot, Daniel Lezcano, Linux Kernel Mailing List, Quentin Perret, c, Joel Fernandes On Tue, Apr 24, 2018 at 5:35 AM, Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Apr 24, 2018 at 12:19:07PM +0100, Valentin Schneider wrote: >> On 24/04/18 11:43, Peter Zijlstra wrote: >> > On Tue, Apr 24, 2018 at 11:02:26AM +0100, Valentin Schneider wrote: >> >> I'd argue making things easier to read is a non-negligible part as well. >> > >> > Right, so I don't object to either of these (I think); but it would be >> > good to see this in combination with that proposed EAS change. >> > >> >> True, I would've said the call to find_energy_efficient_cpu() ([1]) could >> simply be added to the if (sd) {} case, but... > > I think the proposal was to put it before the for_each_domain() loop > entirely, however... > >> > I think you (valentin) wanted to side-step the entire domain loop in >> > that case or something. >> > >> >> ...this would change more things. Admittedly I've been sort of out of the loop >> (no pun intended) lately, but this doesn't ring a bell. That might have been >> the other frenchie (Quentin) :) > > It does indeed appear I confused the two of you, it was Quentin playing > with that. > > In any case, if there not going to be conflicts here, this all looks > good. Both Viresh's and Valentin's patch looks lovely to me too. I couldn't spot anything wrong with them either. One suggestion I was thinking off is can we add better comments to this code (atleast label fast path vs slow path) ? Also, annotate the conditions for the fast/slow path with likely/unlikely since fast path is the common case? so like: if (unlikely(sd)) { /* Fast path, common case */ ... } else if (...) { /* Slow path */ } thanks, - Joel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 15:46 ` Joel Fernandes @ 2018-04-24 15:47 ` Joel Fernandes 2018-04-24 22:34 ` Rohit Jain 0 siblings, 1 reply; 18+ messages in thread From: Joel Fernandes @ 2018-04-24 15:47 UTC (permalink / raw) To: Peter Zijlstra Cc: Valentin Schneider, Viresh Kumar, Ingo Molnar, Vincent Guittot, Daniel Lezcano, Linux Kernel Mailing List, Quentin Perret, Joel Fernandes On Tue, Apr 24, 2018 at 8:46 AM, Joel Fernandes <joel.opensrc@gmail.com> wrote: > On Tue, Apr 24, 2018 at 5:35 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> On Tue, Apr 24, 2018 at 12:19:07PM +0100, Valentin Schneider wrote: >>> On 24/04/18 11:43, Peter Zijlstra wrote: >>> > On Tue, Apr 24, 2018 at 11:02:26AM +0100, Valentin Schneider wrote: >>> >> I'd argue making things easier to read is a non-negligible part as well. >>> > >>> > Right, so I don't object to either of these (I think); but it would be >>> > good to see this in combination with that proposed EAS change. >>> > >>> >>> True, I would've said the call to find_energy_efficient_cpu() ([1]) could >>> simply be added to the if (sd) {} case, but... >> >> I think the proposal was to put it before the for_each_domain() loop >> entirely, however... >> >>> > I think you (valentin) wanted to side-step the entire domain loop in >>> > that case or something. >>> > >>> >>> ...this would change more things. Admittedly I've been sort of out of the loop >>> (no pun intended) lately, but this doesn't ring a bell. That might have been >>> the other frenchie (Quentin) :) >> >> It does indeed appear I confused the two of you, it was Quentin playing >> with that. >> >> In any case, if there not going to be conflicts here, this all looks >> good. > > Both Viresh's and Valentin's patch looks lovely to me too. I couldn't > spot anything wrong with them either. One suggestion I was thinking > off is can we add better comments to this code (atleast label fast > path vs slow path) ? > > Also, annotate the conditions for the fast/slow path with > likely/unlikely since fast path is the common case? so like: > > if (unlikely(sd)) { > /* Fast path, common case */ > ... > } else if (...) { > /* Slow path */ > } Aargh, I messed that up, I meant: if (unlikely(sd)) { /* Slow path */ ... } else if (...) { /* Fast path */ } thanks, :-) - Joel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 15:47 ` Joel Fernandes @ 2018-04-24 22:34 ` Rohit Jain 2018-04-25 2:51 ` Viresh Kumar 0 siblings, 1 reply; 18+ messages in thread From: Rohit Jain @ 2018-04-24 22:34 UTC (permalink / raw) To: Joel Fernandes, Peter Zijlstra Cc: Valentin Schneider, Viresh Kumar, Ingo Molnar, Vincent Guittot, Daniel Lezcano, Linux Kernel Mailing List, Quentin Perret, Joel Fernandes On 04/24/2018 08:47 AM, Joel Fernandes wrote: > On Tue, Apr 24, 2018 at 8:46 AM, Joel Fernandes <joel.opensrc@gmail.com> wrote: >> On Tue, Apr 24, 2018 at 5:35 AM, Peter Zijlstra <peterz@infradead.org> wrote: >>> On Tue, Apr 24, 2018 at 12:19:07PM +0100, Valentin Schneider wrote: >>>> On 24/04/18 11:43, Peter Zijlstra wrote: >>>>> On Tue, Apr 24, 2018 at 11:02:26AM +0100, Valentin Schneider wrote: >>>>>> I'd argue making things easier to read is a non-negligible part as well. >>>>> Right, so I don't object to either of these (I think); but it would be >>>>> good to see this in combination with that proposed EAS change. >>>>> >>>> True, I would've said the call to find_energy_efficient_cpu() ([1]) could >>>> simply be added to the if (sd) {} case, but... >>> I think the proposal was to put it before the for_each_domain() loop >>> entirely, however... >>> >>>>> I think you (valentin) wanted to side-step the entire domain loop in >>>>> that case or something. >>>>> >>>> ...this would change more things. Admittedly I've been sort of out of the loop >>>> (no pun intended) lately, but this doesn't ring a bell. That might have been >>>> the other frenchie (Quentin) :) >>> It does indeed appear I confused the two of you, it was Quentin playing >>> with that. >>> >>> In any case, if there not going to be conflicts here, this all looks >>> good. >> Both Viresh's and Valentin's patch looks lovely to me too. I couldn't >> spot anything wrong with them either. One suggestion I was thinking >> off is can we add better comments to this code (atleast label fast >> path vs slow path) ? >> >> Also, annotate the conditions for the fast/slow path with >> likely/unlikely since fast path is the common case? so like: >> >> if (unlikely(sd)) { >> /* Fast path, common case */ >> ... >> } else if (...) { >> /* Slow path */ >> } > Aargh, I messed that up, I meant: > > if (unlikely(sd)) { > /* Slow path */ > ... > } else if (...) { > /* Fast path */ > } Including the "unlikely" suggestion and the original patch, as expected with a quick hackbench test on a 44 core 2 socket x86 machine causes no change in performance. Thanks, Rohit <snip> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 22:34 ` Rohit Jain @ 2018-04-25 2:51 ` Viresh Kumar 2018-04-25 16:48 ` Rohit Jain 0 siblings, 1 reply; 18+ messages in thread From: Viresh Kumar @ 2018-04-25 2:51 UTC (permalink / raw) To: Rohit Jain Cc: Joel Fernandes, Peter Zijlstra, Valentin Schneider, Ingo Molnar, Vincent Guittot, Daniel Lezcano, Linux Kernel Mailing List, Quentin Perret, Joel Fernandes On 24-04-18, 15:34, Rohit Jain wrote: > Including the "unlikely" suggestion and the original patch, as expected > with a quick hackbench test on a 44 core 2 socket x86 machine causes no > change in performance. Want me to include your Tested-by in next version ? -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-25 2:51 ` Viresh Kumar @ 2018-04-25 16:48 ` Rohit Jain 0 siblings, 0 replies; 18+ messages in thread From: Rohit Jain @ 2018-04-25 16:48 UTC (permalink / raw) To: Viresh Kumar Cc: Joel Fernandes, Peter Zijlstra, Valentin Schneider, Ingo Molnar, Vincent Guittot, Daniel Lezcano, Linux Kernel Mailing List, Quentin Perret, Joel Fernandes On 04/24/2018 07:51 PM, Viresh Kumar wrote: > On 24-04-18, 15:34, Rohit Jain wrote: >> Including the "unlikely" suggestion and the original patch, as expected >> with a quick hackbench test on a 44 core 2 socket x86 machine causes no >> change in performance. > Want me to include your Tested-by in next version ? > Please feel free to include it. I was not sure if this is too small a test to say tested by. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 12:35 ` Peter Zijlstra 2018-04-24 15:46 ` Joel Fernandes @ 2018-04-25 5:15 ` Viresh Kumar 2018-04-25 8:13 ` Quentin Perret 2018-04-25 8:12 ` Quentin Perret 2 siblings, 1 reply; 18+ messages in thread From: Viresh Kumar @ 2018-04-25 5:15 UTC (permalink / raw) To: Peter Zijlstra Cc: Valentin Schneider, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel, Quentin Perret, c On 24-04-18, 14:35, Peter Zijlstra wrote: > In any case, if there not going to be conflicts here, this all looks > good. Thanks Peter. I also had another patch and wasn't sure if that would be the right thing to do. The main purpose of this is to avoid calling sync_entity_load_avg() unnecessarily. +++ b/kernel/sched/fair.c @@ -6196,9 +6196,6 @@ static inline int find_idlest_cpu(struct sched_domain *sd, struct task_struct *p { int new_cpu = cpu; - if (!cpumask_intersects(sched_domain_span(sd), &p->cpus_allowed)) - return prev_cpu; - while (sd) { struct sched_group *group; struct sched_domain *tmp; @@ -6652,15 +6649,19 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f if (unlikely(sd)) { /* Slow path */ - /* - * We're going to need the task's util for capacity_spare_wake - * in find_idlest_group. Sync it up to prev_cpu's - * last_update_time. - */ - if (!(sd_flag & SD_BALANCE_FORK)) - sync_entity_load_avg(&p->se); + if (!cpumask_intersects(sched_domain_span(sd), &p->cpus_allowed)) { + new_cpu = prev_cpu; + } else { + /* + * We're going to need the task's util for + * capacity_spare_wake in find_idlest_group. Sync it up + * to prev_cpu's last_update_time. + */ + if (!(sd_flag & SD_BALANCE_FORK)) + sync_entity_load_avg(&p->se); - new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag); + new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag); + } } else if (sd_flag & SD_BALANCE_WAKE) { /* XXX always ? */ /* Fast path */ -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-25 5:15 ` Viresh Kumar @ 2018-04-25 8:13 ` Quentin Perret 2018-04-25 9:03 ` Viresh Kumar 0 siblings, 1 reply; 18+ messages in thread From: Quentin Perret @ 2018-04-25 8:13 UTC (permalink / raw) To: Viresh Kumar Cc: Peter Zijlstra, Valentin Schneider, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel, c On Wednesday 25 Apr 2018 at 10:45:09 (+0530), Viresh Kumar wrote: > On 24-04-18, 14:35, Peter Zijlstra wrote: > > In any case, if there not going to be conflicts here, this all looks > > good. > > Thanks Peter. > > I also had another patch and wasn't sure if that would be the right > thing to do. The main purpose of this is to avoid calling > sync_entity_load_avg() unnecessarily. While you're at it, you could probably remove the one in wake_cap() ? I think having just one in select_task_rq_fair() should be enough. Thanks, Quentin ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-25 8:13 ` Quentin Perret @ 2018-04-25 9:03 ` Viresh Kumar 2018-04-25 9:39 ` Quentin Perret 0 siblings, 1 reply; 18+ messages in thread From: Viresh Kumar @ 2018-04-25 9:03 UTC (permalink / raw) To: Quentin Perret Cc: Peter Zijlstra, Valentin Schneider, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel, c On 25-04-18, 09:13, Quentin Perret wrote: > While you're at it, you could probably remove the one in wake_cap() ? I > think having just one in select_task_rq_fair() should be enough. Just make it clear, you are asking me to remove sync_entity_load_avg() in wake_cap() ? But aren't we required to do that, as in the very next line we call task_util(p) ? -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-25 9:03 ` Viresh Kumar @ 2018-04-25 9:39 ` Quentin Perret 2018-04-25 10:13 ` Viresh Kumar 0 siblings, 1 reply; 18+ messages in thread From: Quentin Perret @ 2018-04-25 9:39 UTC (permalink / raw) To: Viresh Kumar Cc: Peter Zijlstra, Valentin Schneider, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel On Wednesday 25 Apr 2018 at 14:33:27 (+0530), Viresh Kumar wrote: > On 25-04-18, 09:13, Quentin Perret wrote: > > While you're at it, you could probably remove the one in wake_cap() ? I > > think having just one in select_task_rq_fair() should be enough. > > Just make it clear, you are asking me to remove sync_entity_load_avg() > in wake_cap() ? But aren't we required to do that, as in the very next > line we call task_util(p) ? Right, we do need to call sync_entity_load_avg() at some point before calling task_util(), but we don't need to re-call it in strf() after in this case. So my point was just that if you want to re-work the wake-up path and make sure we don't call sync_entity_load_avg() if not needed then this might need fixing as well ... Or maybe we don't care since re-calling sync_entity_load_avg() should be really cheap ... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-25 9:39 ` Quentin Perret @ 2018-04-25 10:13 ` Viresh Kumar 2018-04-25 10:55 ` Quentin Perret 0 siblings, 1 reply; 18+ messages in thread From: Viresh Kumar @ 2018-04-25 10:13 UTC (permalink / raw) To: Quentin Perret Cc: Peter Zijlstra, Valentin Schneider, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel On 25-04-18, 10:39, Quentin Perret wrote: > On Wednesday 25 Apr 2018 at 14:33:27 (+0530), Viresh Kumar wrote: > > On 25-04-18, 09:13, Quentin Perret wrote: > > > While you're at it, you could probably remove the one in wake_cap() ? I > > > think having just one in select_task_rq_fair() should be enough. > > > > Just make it clear, you are asking me to remove sync_entity_load_avg() > > in wake_cap() ? But aren't we required to do that, as in the very next > > line we call task_util(p) ? > > Right, we do need to call sync_entity_load_avg() at some point before > calling task_util(), but we don't need to re-call it in strf() > after in this case. So my point was just that if you want to re-work > the wake-up path and make sure we don't call sync_entity_load_avg() > if not needed then this might need fixing as well ... Or maybe we don't > care since re-calling sync_entity_load_avg() should be really cheap ... These are in two very different paths and I am not sure of a clean way to avoid calling sync_entity_load_avg() again. Maybe will leave it as is for now. -- viresh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-25 10:13 ` Viresh Kumar @ 2018-04-25 10:55 ` Quentin Perret 0 siblings, 0 replies; 18+ messages in thread From: Quentin Perret @ 2018-04-25 10:55 UTC (permalink / raw) To: Viresh Kumar Cc: Peter Zijlstra, Valentin Schneider, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel On Wednesday 25 Apr 2018 at 15:43:13 (+0530), Viresh Kumar wrote: > On 25-04-18, 10:39, Quentin Perret wrote: > > On Wednesday 25 Apr 2018 at 14:33:27 (+0530), Viresh Kumar wrote: > > > On 25-04-18, 09:13, Quentin Perret wrote: > > > > While you're at it, you could probably remove the one in wake_cap() ? I > > > > think having just one in select_task_rq_fair() should be enough. > > > > > > Just make it clear, you are asking me to remove sync_entity_load_avg() > > > in wake_cap() ? But aren't we required to do that, as in the very next > > > line we call task_util(p) ? > > > > Right, we do need to call sync_entity_load_avg() at some point before > > calling task_util(), but we don't need to re-call it in strf() > > after in this case. So my point was just that if you want to re-work > > the wake-up path and make sure we don't call sync_entity_load_avg() > > if not needed then this might need fixing as well ... Or maybe we don't > > care since re-calling sync_entity_load_avg() should be really cheap ... > > These are in two very different paths and I am not sure of a clean way > to avoid calling sync_entity_load_avg() again. Maybe will leave it as > is for now. Fair enough, I don't really like this double call but, looking into more details, I'm not sure how to avoid it cleanly either ... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it 2018-04-24 12:35 ` Peter Zijlstra 2018-04-24 15:46 ` Joel Fernandes 2018-04-25 5:15 ` Viresh Kumar @ 2018-04-25 8:12 ` Quentin Perret 2 siblings, 0 replies; 18+ messages in thread From: Quentin Perret @ 2018-04-25 8:12 UTC (permalink / raw) To: Peter Zijlstra Cc: Valentin Schneider, Viresh Kumar, Ingo Molnar, Vincent Guittot, Daniel Lezcano, linux-kernel, c On Tuesday 24 Apr 2018 at 14:35:23 (+0200), Peter Zijlstra wrote: > On Tue, Apr 24, 2018 at 12:19:07PM +0100, Valentin Schneider wrote: > > On 24/04/18 11:43, Peter Zijlstra wrote: > > > On Tue, Apr 24, 2018 at 11:02:26AM +0100, Valentin Schneider wrote: > > >> I'd argue making things easier to read is a non-negligible part as well. > > > > > > Right, so I don't object to either of these (I think); but it would be > > > good to see this in combination with that proposed EAS change. > > > > > > > True, I would've said the call to find_energy_efficient_cpu() ([1]) could > > simply be added to the if (sd) {} case, but... > > I think the proposal was to put it before the for_each_domain() loop > entirely, however... > > > > I think you (valentin) wanted to side-step the entire domain loop in > > > that case or something. > > > > > > > ...this would change more things. Admittedly I've been sort of out of the loop > > (no pun intended) lately, but this doesn't ring a bell. That might have been > > the other frenchie (Quentin) :) > > It does indeed appear I confused the two of you, it was Quentin playing > with that. > > In any case, if there not going to be conflicts here, this all looks > good. So, the proposal was to re-use the loop to find a non-overutilized sched domain in which we can use EAS. But yes, I don't see why this would conflict with this patch so I don't have objections against it. Thanks, Quentin ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-04-25 16:49 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-23 10:38 [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it Viresh Kumar 2018-04-24 10:02 ` Valentin Schneider 2018-04-24 10:30 ` Viresh Kumar 2018-04-24 10:43 ` Peter Zijlstra 2018-04-24 11:19 ` Valentin Schneider 2018-04-24 12:35 ` Peter Zijlstra 2018-04-24 15:46 ` Joel Fernandes 2018-04-24 15:47 ` Joel Fernandes 2018-04-24 22:34 ` Rohit Jain 2018-04-25 2:51 ` Viresh Kumar 2018-04-25 16:48 ` Rohit Jain 2018-04-25 5:15 ` Viresh Kumar 2018-04-25 8:13 ` Quentin Perret 2018-04-25 9:03 ` Viresh Kumar 2018-04-25 9:39 ` Quentin Perret 2018-04-25 10:13 ` Viresh Kumar 2018-04-25 10:55 ` Quentin Perret 2018-04-25 8:12 ` Quentin Perret
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).