Messages in this thread | | | From | Joel Fernandes <> | Date | Tue, 24 Apr 2018 08:47:35 -0700 | Subject | Re: [PATCH] sched/fair: Rearrange select_task_rq_fair() to optimize it |
| |
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
| |