Messages in this thread | | | Subject | Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it | From | Rohit Jain <> | Date | Tue, 24 Apr 2018 15:34:37 -0700 |
| |
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>
| |