Re: [PATCH RFC 1/2] sched: Minimize the idle cpu selection race window.

[Uladzislau Rezki]

On Fri, Nov 24, 2017 at 07:46:30PM +0100, Mike Galbraith wrote:
> On Fri, 2017-11-24 at 11:26 +0100, Uladzislau Rezki wrote:
> > 
> > I guess there is misunderstanding here. The main goal is not to cover
> > pinned case, for sure. I was thinking more about below points:
> > 
> > - Extend a claim_wake_up logic for making an ILB/NO_HZ decision more
> >   predictable (that is good for mobile workloads). Because as it is
> >   right now it simply returns a first CPU in a "nohz" mask and if we
> >   know that CPU has been claimed i think it is worth to go with another
> >   ILB core, since waking up on a remote CPU + doing nohz_idle_balance
> >   does not improve wake-up latency and is a miss from ilb point of view.
> 
> Using unsynchronized access of a flag set by chaotic events all over
> the box?
Well, entering NO_HZ mode and checking nohz.idle_cpus_mask
nohz_balancer_kick() -> find_new_ilb() are obviously asynchronous
events and like you say is chaotic except of accessing to the
cpumask variable (i mean atomically). So do not see any issues
with that.

> 
> > If you have any proposal, i would be appreciated if you could
> > share your specific view.
> 
> My view is you're barking up the wrong tree: you're making the idle
> data SIS is using more accurate, but I question the benefit.  That it
> makes an imperfect placement decision occasionally due to raciness is
> nearly meaningless compared to the cost of frequent bounce.
Before sitting down and start testing, i just illustrated how we can
apply claim_wake_up to ilb asking community a specific view on it:
drawbacks, pros/cons, proposals etc.

>
> Better (but still not perfect, that requires atomics) state information
> doesn't meaningfully improve decision quality.
> 
I agree, it should be atomic, something like below:

core.c
+/* cpus with claim wake-up set bit */
+cpumask_t cpu_claim_wake_up_map;
...
@@ -1559,6 +1562,7 @@ int select_task_rq(struct task_struct *p, int cpu, int sd_flags, int wake_flags)
                     !cpu_online(cpu)))
                cpu = select_fallback_rq(task_cpu(p), p);

+       cpumask_test_and_set_cpu(cpu, &cpu_claim_wake_up_map);
        return cpu;
}
...
+       if (cpumask_test_cpu(cpu, &cpu_claim_wake_up_map))
+               return 0;
+
        return 1;
}

fair.c:
static inline int find_new_ilb(void)
{
-       int ilb = cpumask_first(nohz.idle_cpus_mask);
+       cpumask_t cpumask;
+       int ilb;
+
+       cpumask_andnot(&cpumask, nohz.idle_cpus_mask,
+                       &cpu_claim_wake_up_map);
+
+       ilb = cpumask_first(&cpumask);

> > Considering a core as not-idle when somebody tends to wake up a task on
> > it is a good point. If you have any specific example when it is bad,
> > please share it.
> 
> I'm not sure how that will work out.  Probably like most everything wrt
> SIS, first comes "woohoo" then "aw crap", and off to the trash it goes.
> 

--
Uladzislau Rezki