Re: [PATCH] sched/fair: Skip wake_affine() for core siblings

[Mike Galbraith <umgwanakikbuti@gmail.com>]

On Tue, 2015-09-29 at 19:00 +0300, Kirill Tkhai wrote:
> 
> On 29.09.2015 17:55, Mike Galbraith wrote:
> > On Mon, 2015-09-28 at 18:36 +0300, Kirill Tkhai wrote:
> > 
> >> ---
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 4df37a4..dfbe06b 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -4930,8 +4930,13 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f
> >>  	int want_affine = 0;
> >>  	int sync = wake_flags & WF_SYNC;
> >>  
> >> -	if (sd_flag & SD_BALANCE_WAKE)
> >> -		want_affine = !wake_wide(p) && cpumask_test_cpu(cpu, tsk_cpus_allowed(p));
> >> +	if (sd_flag & SD_BALANCE_WAKE) {
> >> +		want_affine = 1;
> >> +		if (cpu == prev_cpu || !cpumask_test_cpu(cpu, tsk_cpus_allowed(p)))
> >> +			goto want_affine;
> >> +		if (wake_wide(p))
> >> +			goto want_affine;
> >> +	}
> > 
> > That blew wake_wide() right out of the water.
> > 
> > It's not only about things like pgbench.  Drive multiple tasks in a Xen
> > guest (single event channel dom0 -> domu, and no select_idle_sibling()
> > to save the day) via network, and watch workers fail to be all they can
> > be because they keep being stacked up on the irq source.  Load balancing
> > yanks them apart, next irq stacks them right back up.  I met that in
> > enterprise land, thought wake_wide() should cure it, and indeed it did.
> 
> 1)Hm.. The patch makes select_task_rq_fair() to prefer old cpu instead of
> current, doesn't it? We more often don't set affine_sd. So, the skipped
> part of patch (skipped in quote) selects prev_cpu.

Not the way I read it..

>> -    if (affine_sd) {
>> +want_affine:
>> +    if (want_affine) {
>>              sd = NULL; /* Prefer wake_affine over balance flags */
>> -            if (cpu != prev_cpu && wake_affine(affine_sd, p, sync))
>> +            if (affine_sd && wake_affine(affine_sd, p, sync))
>>                      new_cpu = cpu;
>> -    }
>> -
>> -    if (!sd) {
>> -            if (sd_flag & SD_BALANCE_WAKE) /* XXX always ? */
>> -                    new_cpu = select_idle_sibling(p, new_cpu);
>> -
>> +            new_cpu = select_idle_sibling(p, new_cpu);

..it sets new_cpu = cpu if wake_affine() says Ok, wake_wide() has no say
in the matter.
 
> 2)I thought about waking by irq handler and even was going to ask why
> we use affine logic for such wakeups. Device handlers usually aren't
> bound, timers may migrate since NO_HZ logic presents. The only explanation
> I found is unbound timers is very unlikely case (I added statistics printk
> to my local sched_debug to check that). But if we have the situations like
> you described above, don't we have to disable affine logic for in_interrupt()
> cases?

BTDT.  In my experience, the more you try to differentiate sources, the
more corner cases you create.  I've tried doing special things for irq,
locks, wake_all, wake_one, and it always turned into a can of worms.
IMHO, the best policy for the fast patch is KISS.

> 3)I ask about just because (being outside of scheduler history) it's a little
> bit strange, we prefer smp_processor_id()'s sd_llc so much. Sync wakeup's
> profit is less or more clear: smp_processor_id()'s sd_llc may contain some
> data, which is interesting for a wakee, and this minimizes cache misses.
> But we do the same in other cases too, and at every migration we loose
> itlb, dtlb... Of course, it requires more accurate patches, then posted
> (not so rude patches).

IMHO, the sync wakeup hint is more often a big fat lie than anything
else, it really just gives us a bit more headroom for affine wakeups in
cases where that's likely to be a very good thing (affine in the cache
sense, not affine as in an individual CPU).  What it means is that waker
is likely to schedule RSN, but if you measure even very fast/light
things, there is an overlap win to be had by NOT waking CPU affine,
rather waking cache affine, that's why we cross core schedule so often.
A real network app doing a wakeup does is not necessarily gonna schedule
RSN, there is very often a latency win to be had by scheduling to a
nearby core, ie a thread pool worker doing a "sync" wakeup may very
instantly find that it has more work to do.  If a fast/light wakee can
slip into an idle crack and get to CPU instantly, it can generate more
work a little bit sooner.

	-Mike