Messages in this thread | | | From | Vincent Guittot <> | Date | Mon, 29 Nov 2021 16:49:58 +0100 | Subject | Re: [PATCH] sched/fair: Fix detection of per-CPU kthreads waking a task |
| |
On Fri, 26 Nov 2021 at 18:18, Vincent Donnefort <vincent.donnefort@arm.com> wrote: > > On Fri, Nov 26, 2021 at 04:49:12PM +0000, Valentin Schneider wrote: > > 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. > > From a quick experiment, even with the asym_fits_capacity(), I can trigger > the following: > > [ 0.118855] select_idle_sibling: wakee=kthreadd:2 nr_cpus_allowed=8 current=swapper/0:1 in_task=1 > [ 0.128214] select_idle_sibling: wakee=rcu_gp:3 nr_cpus_allowed=8 current=swapper/0:1 in_task=1 > [ 0.137327] select_idle_sibling: wakee=rcu_par_gp:4 nr_cpus_allowed=8 current=swapper/0:1 in_task=1 > [ 0.147221] select_idle_sibling: wakee=kworker/u16:0:7 nr_cpus_allowed=8 current=swapper/0:1 in_task=1 > [ 0.156994] select_idle_sibling: wakee=mm_percpu_wq:8 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
Timestamp shows its booting phase and thread name above shows per cpu thread. Could it happen just while creating per cpu thread at boot and as a result not relevant ?
Can you see similar things later after booting ?
I have tried to trigger the situation but failed to get wrong sequence. All are coming from interrupt while idle. After adding in_task() condition, I haven't been able to trigger the warn() that I added to catch the wrong situations on SMP, Heterogenous or NUMA system. Could you share more details on your setup ?
> [ 0.171943] select_idle_sibling: wakee=rcu_sched:10 nr_cpus_allowed=8 current=swapper/0:1 in_task=1 > > So the in_task() condition doesn't appear to be enough to filter wakeups > while we have the swapper as a current. > > > > > > > > >> + in_task() && > > >> prev == smp_processor_id() && > > >> this_rq()->nr_running <= 1) { > > >> return prev; > > >>
| |