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
