Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH] sched/fair: Fix detection of per-CPU kthreads waking a task | Date | Fri, 26 Nov 2021 16:49:12 +0000 |
| |
On 26/11/21 15:40, Vincent Guittot wrote: > On Fri, 26 Nov 2021 at 14:32, Valentin Schneider > <Valentin.Schneider@arm.com> wrote: >> /* >> - * Allow a per-cpu kthread to stack with the wakee if the >> - * kworker thread and the tasks previous CPUs are the same. >> - * The assumption is that the wakee queued work for the >> - * per-cpu kthread that is now complete and the wakeup is >> - * essentially a sync wakeup. An obvious example of this >> + * Allow a per-cpu kthread to stack with the wakee if the kworker thread >> + * and the tasks previous CPUs are the same. The assumption is that the >> + * wakee queued work for the per-cpu kthread that is now complete and >> + * the wakeup is essentially a sync wakeup. An obvious example of this >> * pattern is IO completions. >> + * >> + * Ensure the wakeup is issued by the kthread itself, and don't match >> + * against the idle task because that could override the >> + * available_idle_cpu(target) check done higher up. >> */ >> - if (is_per_cpu_kthread(current) && >> + if (is_per_cpu_kthread(current) && !is_idle_task(current) && > > still i don't see the need of !is_idle_task(current) >
Admittedly, belts and braces. The existing condition checks rq->nr_running <= 1 which can lead to coscheduling when the wakeup is issued by the idle task (or even if rq->nr_running == 0, you can have rq->ttwu_pending without having sent an IPI due to polling). Essentially this overrides the first check in sis() that uses idle_cpu(target) (prev == smp_processor_id() == target).
I couldn't prove such wakeups can happen right now, but if/when they do (AIUI it would just take someone to add a wake_up_process() down some smp_call_function() callback) then we'll need the above. If you're still not convinced by now, I won't push it further.
> >> + in_task() && >> prev == smp_processor_id() && >> this_rq()->nr_running <= 1) { >> return prev; >>
| |