Re: [PATCH RESEND] sched: prefer an idle cpu vs an idle sibling for BALANCE_WAKE

[Josef Bacik <jbacik@fb.com>]

On 06/01/2015 04:42 PM, Peter Zijlstra wrote:
> On Mon, 2015-06-01 at 15:38 -0400, Josef Bacik wrote:
>
>> Ok I got this patch to give me the same performance as all our other
>> crap, just need to apply this incremental
>>
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index b71eb2b..e11cfec 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -4761,13 +4761,10 @@ select_task_rq_fair(struct task_struct *p, int
>> prev_cpu, int sd_flag, int wake_f
>>
>>    		if (tmp->flags & sd_flag)
>>    			sd = tmp;
>> -		else if (!want_affine || (want_affine && affine_sd))
>> -			break;
>>    	}
>
> That bit worries me a bit, because that causes us to have a weird
> definition for what sd is.
>
> Without WAKE_AFFINE, sd is the biggest domain with BALANCE_WAKE (or any
> other sd_flag) set.
>
> But with WAKE_AFFINE, its the first domain that satisfies the wake
> affine constraint of covering both the previous and waking cpu. It
> basically reduces sd to affine_sd.
>

I'm just giving you what made the performance good for us, I'm relying 
on you to make it sane ;).  So with that bit we break out if 
!wake_affine as soon as we don't find an sd that matches sd_flag, that 
seems less than helpful.  I was going to change it to

else if (want_affine && affine_sd)

but the thing is we don't do anything with affine_sd unless the bit 
below matches

if (affine_sd && cpu != prev_cpu && wake_affine(affine_sd, p, sync))

so it seems like we're leaving ourselves without sd set in a few cases 
where we'd actually want it set, so I just nuked the whole thing and 
carried on.  I'm all ears for other ideas, I'm just letting you know 
what my results are since you are the guys who actually understand this 
stuff.  Thanks,

Josef