sched,numa: pinned task accounting
Peter Zijlstra
maintainercover sched,numa: pinned task accounting
Hi,
Here's a first few patches that provide pinned task accounting; they are boot tested only so far.
I don't think this is enough to address Artem's regression, but its a foundation to add some more smarts. In particular we should make it harder still to migrate well placed tasks away due to these pinned tasks.
Rostedt, Juri, please double check what I did to your resp. set_cpus_allowed methods.
--- include/linux/kthread.h | 1 + include/linux/sched.h | 7 ----- kernel/kthread.c | 20 ++++++++++++--- kernel/sched/core.c | 66 ++++++++++++++++++++++++++++++++++++++++++------ kernel/sched/deadline.c | 35 ++----------------------- kernel/sched/fair.c | 43 +++++++++++++++++++++++++------ kernel/sched/idle_task.c | 1 + kernel/sched/rt.c | 41 +----------------------------- kernel/sched/sched.h | 3 +++ kernel/sched/stop_task.c | 1 + kernel/workqueue.c | 11 ++------ 11 files changed, 121 insertions(+), 108 deletions(-)
Peter Zijlstra
maintainerRFC PATCH 1/4 sched: Fix a race between __kthread_bind() and sched_setaffinity()
Because sched_setscheduler() checks p->flags & PF_NO_SETAFFINITY without locks, a caller might observe an old value and race with the set_cpus_allowed_ptr() call from __kthread_bind() and effectively undo it.
__kthread_bind() do_set_cpus_allowed() <SYSCALL> sched_setaffinity() if (p->flags & PF_NO_SETAFFINITIY) set_cpus_allowed_ptr() p->flags |= PF_NO_SETAFFINITY
Fix the issue by putting everything under the regular scheduler locks.
This also closes a hole in the serialization of
task_struct::{nr_,}cpus_allowed.
--- include/linux/kthread.h | 1 + include/linux/sched.h | 7 ------- kernel/kthread.c | 21 ++++++++++++++++++--- kernel/sched/core.c | 30 ++++++++++++++++++++++++++---- kernel/workqueue.c | 11 ++--------- 5 files changed, 47 insertions(+), 23 deletions(-)
include/linux/kthread.h +1 -0
@@ -38,6 +38,7 @@ struct task_struct *kthread_create_on_cp })void kthread_bind (struct task_struct *k,unsigned int cpu); +void kthread_bind_mask (struct task_struct *k,const struct cpumask *mask);int kthread_stop (struct task_struct *k);bool kthread_should_stop (void );bool kthread_should_park (void );
include/linux/sched.h +0 -7
@@ -2215,13 +2215,6 @@ static inline void calc_load_enter_idle(static inline void calc_load_exit_idle (void ) { }#endif /* CONFIG_NO_HZ_COMMON */ -#ifndef CONFIG_CPUMASK_OFFSTACK -static inline int set_cpus_allowed (struct task_struct *p,cpumask_t new_mask) -{ -return set_cpus_allowed_ptr (p, &new_mask); -} -#endif -/* * Do not use outside of architecture code which knows its limitations. *
kernel/kthread.c +17 -3
@@ -325,16 +325,30 @@ struct task_struct *kthread_create_on_no }EXPORT_SYMBOL (kthread_create_on_node); -static void __kthread_bind (struct task_struct *p,unsigned int cpu,long state) +static void __kthread_bind_mask (struct task_struct *p,const struct cpumask *mask,long state) { -/* Must have done schedule() in kthread() before we set_task_cpu */ +unsigned long flags; +if (!wait_task_inactive (p, state)) {WARN_ON (1 );return ; } +/* It's safe because the task is inactive. */ -do_set_cpus_allowed (p,cpumask_of (cpu)); +raw_spin_lock_irqsave (&p->pi_lock, flags); +do_set_cpus_allowed (p, mask); p->flags |=PF_NO_SETAFFINITY ; +raw_spin_unlock_irqrestore (&p->pi_lock, flags); +} + +static void __kthread_bind (struct task_struct *p,unsigned int cpu,long state) +{ +__kthread_bind_mask (p,cpumask_of (cpu), state); +} + +void kthread_bind_mask (struct task_struct *p,const struct cpumask *mask) +{ +__kthread_bind_mask (p, mask,TASK_UNINTERRUPTIBLE ); }/**
kernel/sched/core.c +26 -4
@@ -4044,6 +4044,9 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pireturn retval; } +static int __set_cpus_allowed_ptr (struct task_struct *p, +const struct cpumask *new_mask,bool check); +long sched_setaffinity (pid_t pid,const struct cpumask *in_mask) {cpumask_var_t cpus_allowed, new_mask;
@@ -4110,7 +4113,7 @@ long sched_setaffinity(pid_t pid, const }#endif again: - retval =set_cpus_allowed_ptr (p, new_mask); + retval =__set_cpus_allowed_ptr (p, new_mask, true);if (!retval) {cpuset_cpus_allowed (p, cpus_allowed);
@@ -4638,7 +4641,8 @@ void init_idle(struct task_struct *idle,struct rq *rq =cpu_rq (cpu);unsigned long flags; -raw_spin_lock_irqsave (&rq->lock, flags); +raw_spin_lock_irqsave (&idle->pi_lock, flags); +raw_spin_lock (&rq->lock);__sched_fork (0 , idle); idle->state =TASK_RUNNING ;
@@ -4664,7 +4668,8 @@ void init_idle(struct task_struct *idle,#if defined(CONFIG_SMP ) idle->on_cpu =1 ;#endif -raw_spin_unlock_irqrestore (&rq->lock, flags); +raw_spin_unlock (&rq->lock); +raw_spin_unlock_irqrestore (&idle->pi_lock, flags);/* Set the preempt count _outside_ the spinlocks! */ init_idle_preempt_count (idle, cpu);
@@ -4788,6 +4793,8 @@ static struct rq *move_queued_task(strucvoid do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) { +lockdep_assert_held (&p->pi_lock); +if (p->sched_class->set_cpus_allowed) p->sched_class->set_cpus_allowed (p, new_mask);
@@ -4818,7 +4825,8 @@ void do_set_cpus_allowed(struct task_str* task must not exit() & deallocate itself prematurely. The * call is not atomic; no spinlocks may be held. */ -int set_cpus_allowed_ptr (struct task_struct *p,const struct cpumask *new_mask) +static int __set_cpus_allowed_ptr (struct task_struct *p, +const struct cpumask *new_mask,bool check) {unsigned long flags;struct rq *rq;
@@ -4827,6 +4835,15 @@ int set_cpus_allowed_ptr(struct task_str rq =task_rq_lock (p, &flags); +/* +* Must re-check here, to close a race against __kthread_bind(), +* sched_setaffinity() is not guaranteed to observe the flag. +*/ +if (check && (p->flags &PF_NO_SETAFFINITY )) { + ret = -EINVAL ; + goto out; + } +if (cpumask_equal (&p->cpus_allowed, new_mask)) goto out;
@@ -4856,6 +4873,11 @@ int set_cpus_allowed_ptr(struct task_strreturn ret; } + +int set_cpus_allowed_ptr (struct task_struct *p,const struct cpumask *new_mask) +{ +return __set_cpus_allowed_ptr (p, new_mask, false); +}EXPORT_SYMBOL_GPL (set_cpus_allowed_ptr);/*
kernel/workqueue.c +2 -9
@@ -1619,11 +1619,7 @@ static void worker_attach_to_pool(struct {mutex_lock (&pool->attach_mutex); -/* -* set_cpus_allowed_ptr() will fail if the cpumask doesn't have any -* online CPUs. It'll be re-applied when any of the CPUs come up. -*/ -set_cpus_allowed_ptr (worker->task, pool->attrs->cpumask); +kthread_bind_mask (worker->task, pool->attrs->cpumask);/* * The pool->attach_mutex ensures %POOL_DISASSOCIATED remains
@@ -1708,9 +1704,6 @@ static struct worker *create_worker(struset_user_nice (worker->task, pool->attrs->nice); -/* prevent userland from meddling with cpumask of workqueue workers */ - worker->task->flags |=PF_NO_SETAFFINITY ; -/* successful, attach the worker to the pool */ worker_attach_to_pool (worker, pool);
@@ -3832,7 +3825,7 @@ struct workqueue_struct *__alloc_workque } wq->rescuer = rescuer; - rescuer->task->flags |=PF_NO_SETAFFINITY ; +kthread_bind_mask (rescuer->task, cpu_possible_mask);wake_up_process (rescuer->task); }
Re: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and sched_setaffinity()
Quoted 13 lines · Peter Zijlstra
Because
sched_setscheduler()checksp->flags&PF_NO_SETAFFINITYwithout locks, a caller might observe an old value and race with theset_cpus_allowed_ptr()call from__kthread_bind()and effectively undo it.__kthread_bind() do_set_cpus_allowed() <SYSCALL> sched_setaffinity() if (p->flags & PF_NO_SETAFFINITIY) set_cpus_allowed_ptr() p->flags |= PF_NO_SETAFFINITY
Quoted 8 lines
Fix the issue by putting everything under the regular scheduler locks.
This also closes a hole in the serialization of
task_struct::{nr_,}cpus_allowed.Cc Tejun Heo <[email protected]>Cc Oleg Nesterov <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>
For workqueue part,
Please route anyway you see fit.
Thanks.
Signature · 1 line
tejun
Peter Zijlstra
maintainerRe: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and sched_setaffinity()
Quoted 19 lines · Tejun Heo
Quoted 18 lines · Peter Zijlstra
Because
sched_setscheduler()checksp->flags&PF_NO_SETAFFINITYwithout locks, a caller might observe an old value and race with theset_cpus_allowed_ptr()call from__kthread_bind()and effectively undo it.__kthread_bind() do_set_cpus_allowed() <SYSCALL> sched_setaffinity() if (p->flags & PF_NO_SETAFFINITIY) set_cpus_allowed_ptr() p->flags |= PF_NO_SETAFFINITYFix the issue by putting everything under the regular scheduler locks.
This also closes a hole in the serialization of
task_struct::{nr_,}cpus_allowed.
Quoted 7 lines
Quoted 3 lines
Cc Tejun Heo <[email protected]>Cc Oleg Nesterov <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>For workqueue part,
Acked-by Tejun Heo <[email protected]>
Sorry be being very late on this, got sidetracked with other bits.
This threw up a warning on testing:
[ 2.443944] WARNING: CPU: 0 PID: 10 at kernel/kthread.c:333 __kthread_bind_mask+0x34/0x6e() [ 2.446978] Modules linked in: [ 2.448359] CPU: 0 PID: 10 Comm: khelper Not tainted 4.1.0-rc6-00314-g6455666 #4 [ 2.450990] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 [ 2.454132] 0000000000000009 ffff88000f643d68 ffffffff81a3df14 0000000000000b02 [ 2.470295] 0000000000000000 ffff88000f643da8 ffffffff810f308f 000000000f643da8 [ 2.503291] ffffffff8110d116 ffff88000f55d580 ffff88000f5240c0 ffff88000f4936e0 [ 2.506510] Call Trace: [ 2.520770] [<ffffffff81a3df14>] dump_stack+0x4c/0x65 [ 2.522479] [<ffffffff810f308f>] warn_slowpath_common+0xa1/0xbb [ 2.524334] [<ffffffff8110d116>] ? __kthread_bind_mask+0x34/0x6e [ 2.526219] [<ffffffff810f314c>] warn_slowpath_null+0x1a/0x1c [ 2.528069] [<ffffffff8110d116>] __kthread_bind_mask+0x34/0x6e [ 2.529925] [<ffffffff8110d381>] kthread_bind_mask+0x13/0x15 [ 2.531738] [<ffffffff8110679d>] worker_attach_to_pool+0x39/0x7c [ 2.546650] [<ffffffff8110866b>] rescuer_thread+0x130/0x318 [ 2.548484] [<ffffffff8110853b>] ? cancel_delayed_work_sync+0x15/0x15 [ 2.550411] [<ffffffff8110853b>] ? cancel_delayed_work_sync+0x15/0x15 [ 2.552207] [<ffffffff8110cd0f>] kthread+0xf8/0x100 [ 2.553864] [<ffffffff8110cc17>] ? kthread_create_on_node+0x184/0x184 [ 2.555795] [<ffffffff81a457c2>] ret_from_fork+0x42/0x70 [ 2.557538] [<ffffffff8110cc17>] ? kthread_create_on_node+0x184/0x184 [ 2.572520] ---[ end trace 362b92c9255ab666 ]---
Which is the rescue thread attaching itself to a pool that needs help, and obviously the rescue thread isn't new so kthread_bind doesn't work right.
The best I could come up with is something like the below on top; does that work for you? I'll go give it some runtime.
kernel/workqueue.c +8 -4
@@ -1622,11 +1622,15 @@ static struct worker *alloc_worker(int n* cpu-[un]hotplugs. */ static void worker_attach_to_pool (struct worker *worker, -struct worker_pool *pool) +struct worker_pool *pool, +bool new) {mutex_lock (&pool->attach_mutex); -kthread_bind_mask (worker->task, pool->attrs->cpumask); +if (new) +kthread_bind_mask (worker->task, pool->attrs->cpumask); +else +set_cpus_allowed_ptr (worker->task, pool->attrs->cpumask);/* * The pool->attach_mutex ensures %POOL_DISASSOCIATED remains
@@ -1712,7 +1716,7 @@ static struct worker *create_worker(struset_user_nice (worker->task, pool->attrs->nice);/* successful, attach the worker to the pool */ -worker_attach_to_pool (worker, pool); +worker_attach_to_pool (worker, pool, true);/* start the newly created worker */ spin_lock_irq (&pool->lock);
@@ -2241,7 +2245,7 @@ static int rescuer_thread(void *__rescuespin_unlock_irq (&wq_mayday_lock); -worker_attach_to_pool (rescuer, pool); +worker_attach_to_pool (rescuer, pool, false);spin_lock_irq (&pool->lock); rescuer->pool = pool;
Tejun Heo
maintainerRe: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and sched_setaffinity()
Quoted 37 lines · Peter Zijlstra
Which is the rescue thread attaching itself to a pool that needs help, and obviously the rescue thread isn't new so
kthread_binddoesn't work right.The best I could come up with is something like the below on top; does that work for you? I'll go give it some runtime.
kernel/workqueue.c +7
-3@@ -1622,11 +1622,15 @@ static struct worker *alloc_worker(int n* cpu-[un]hotplugs. */ static void worker_attach_to_pool (struct worker *worker, -struct worker_pool *pool) +struct worker_pool *pool, +bool new) {mutex_lock (&pool->attach_mutex); -kthread_bind_mask (worker->task, pool->attrs->cpumask); +if (new) +kthread_bind_mask (worker->task, pool->attrs->cpumask); +else +set_cpus_allowed_ptr (worker->task, pool->attrs->cpumask);/* * The pool->attach_mutex ensures %POOL_DISASSOCIATED remains @@ -1712,7 +1716,7 @@ static struct worker *create_worker(struset_user_nice (worker->task, pool->attrs->nice);/* successful, attach the worker to the pool */ -worker_attach_to_pool (worker, pool); +worker_attach_to_pool (worker, pool, true);/* start the newly created worker */ spin_lock_irq (&pool->lock);
kernel/workqueue.c static int rescuer_thread(void *__rescue · line 2248
Quoted 5 lines
kernel/workqueue.c +1
-1@@ -2241,7 +2245,7 @@ static int rescuer_thread(void *__rescuespin_unlock_irq (&wq_mayday_lock); -worker_attach_to_pool (rescuer, pool); +worker_attach_to_pool (rescuer, pool, false);
Hmmm... the race condition didn't exist for workqueue in the first place, right? As long as the flag is set before the affinity is configured, there's no race condition. I think the code was better before. Can't we just revert workqueue.c part?
Thanks.
Signature · 1 line
tejun
Peter Zijlstra
maintainerRe: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and sched_setaffinity()
Quoted 39 lines · Tejun Heo
Quoted 38 lines · Peter Zijlstra
Which is the rescue thread attaching itself to a pool that needs help, and obviously the rescue thread isn't new so
kthread_binddoesn't work right.The best I could come up with is something like the below on top; does that work for you? I'll go give it some runtime.
kernel/workqueue.c +7
-3@@ -1622,11 +1622,15 @@ static struct worker *alloc_worker(int n* cpu-[un]hotplugs. */ static void worker_attach_to_pool (struct worker *worker, -struct worker_pool *pool) +struct worker_pool *pool, +bool new) {mutex_lock (&pool->attach_mutex); -kthread_bind_mask (worker->task, pool->attrs->cpumask); +if (new) +kthread_bind_mask (worker->task, pool->attrs->cpumask); +else +set_cpus_allowed_ptr (worker->task, pool->attrs->cpumask);/* * The pool->attach_mutex ensures %POOL_DISASSOCIATED remains @@ -1712,7 +1716,7 @@ static struct worker *create_worker(struset_user_nice (worker->task, pool->attrs->nice);/* successful, attach the worker to the pool */ -worker_attach_to_pool (worker, pool); +worker_attach_to_pool (worker, pool, true);/* start the newly created worker */ spin_lock_irq (&pool->lock);@@ -2241,7 +2245,7 @@ static int rescuer_thread(void *__rescue
Quoted 7 lines
kernel/workqueue.c static int rescuer_thread(void *__rescue · line 2248
Quoted 4 lines
kernel/workqueue.c +1
-1spin_unlock_irq (&wq_mayday_lock); -worker_attach_to_pool (rescuer, pool); +worker_attach_to_pool (rescuer, pool, false);Hmmm... the race condition didn't exist for workqueue in the first place, right?
No, I think workqueues are susceptible just the same as everybody else. By the time we call __kthread_bind() the task exists and is visible to userspace.
__kthread_bind()
do_set_cpus_allowed()
<SYSCALL>
sched_setaffinity()
if (p->flags & PF_NO_SETAFFINITIY) /* false-not-taken */
set_cpus_allowed_ptr()
p->flags |= PF_NO_SETAFFINITYQuoted 2 lines
As long as the flag is set before the affinity is configured, there's no race condition.
Even if we were to strictly order those stores you could have (note there is no matching barrier, as there is only the one load, so ordering cannot help):
__kthread_bind() <SYSCALL> sched_setaffinity() if (p->flags & PF_NO_SETAFFINITY) /* false-not-taken */ p->flags |= PF_NO_SETAFFINITY; smp_wmb(); do_set_cpus_allowed(); set_cpus_allowed_ptr()
Quoted 2 lines
I think the code was better before. Can't we just revert
workqueue.cpart?
I agree that the new argument isn't pretty, but I cannot see how workqueues would not be affected by this.
Tejun Heo
maintainerRe: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and sched_setaffinity()
Hello,
Quoted 13 lines · Peter Zijlstra
Even if we were to strictly order those stores you could have (note there is no matching barrier, as there is only the one load, so ordering cannot help):
__kthread_bind() <SYSCALL> sched_setaffinity() if (p->flags & PF_NO_SETAFFINITY) /* false-not-taken */ p->flags |= PF_NO_SETAFFINITY; smp_wmb(); do_set_cpus_allowed(); set_cpus_allowed_ptr()
Quoted 5 lines
Quoted 2 lines
I think the code was better before. Can't we just revert
workqueue.cpart?I agree that the new argument isn't pretty, but I cannot see how workqueues would not be affected by this.
So, the problem there is that __kthread_bind() doesn't grab the same lock that the syscall side grabs but workqueue used set_cpus_allowed_ptr() which goes through the rq locking, so as long as the check on syscall side is movied inside rq lock, it should be fine.
Thanks.
Signature · 1 line
tejun
Re: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and sched_setaffinity()
Quoted 19 lines · Tejun Heo
Hello,
Quoted 16 lines · Peter Zijlstra
Even if we were to strictly order those stores you could have (note there is no matching barrier, as there is only the one load, so ordering cannot help):
__kthread_bind() <SYSCALL> sched_setaffinity() if (p->flags & PF_NO_SETAFFINITY) /* false-not-taken */ p->flags |= PF_NO_SETAFFINITY; smp_wmb(); do_set_cpus_allowed(); set_cpus_allowed_ptr()Quoted 2 lines
I think the code was better before. Can't we just revert
workqueue.cpart?
Quoted 8 lines
Quoted 2 lines
I agree that the new argument isn't pretty, but I cannot see how workqueues would not be affected by this.
So, the problem there is that
__kthread_bind()doesn't grab the same lock that the syscall side grabs but workqueue usedset_cpus_allowed_ptr()which goes through the rq locking, so as long as the check on syscall side is movied inside rq lock, it should be fine.
Currently neither site uses any lock, and that is what the patch fixes (it uses the per-task ->pi_lock instead of the rq->lock, but that is immaterial).
What matters though is that you now must hold a scheduler lock while setting PF_NO_SETAFFINITY. In order to avoid spreading that knowledge around I've taught kthread_bind*() about this and made the workqueue code use that API (rather than having the workqueue code take scheduler locks).
Hmm.. a better solution. Have the worker thread creation call kthread_bind_mask() before attach_to_pool() and have attach_to_pool() keep using set_cpus_allowed_ptr(). Less ugly.
---
Subject: sched: Fix a race between __kthread_bind() and sched_setaffinity()
From: Peter Zijlstra <[email protected]>
Date: Fri, 15 May 2015 17:43:34 +0200
Because sched_setscheduler() checks p->flags & PF_NO_SETAFFINITY without locks, a caller might observe an old value and race with the set_cpus_allowed_ptr() call from __kthread_bind() and effectively undo it.
__kthread_bind() do_set_cpus_allowed() <SYSCALL> sched_setaffinity() if (p->flags & PF_NO_SETAFFINITIY) set_cpus_allowed_ptr() p->flags |= PF_NO_SETAFFINITY
Fix the issue by putting everything under the regular scheduler locks.
This also closes a hole in the serialization of
task_struct::{nr_,}cpus_allowed.
--- include/linux/kthread.h | 1 + include/linux/sched.h | 7 ------- kernel/kthread.c | 20 +++++++++++++++++--- kernel/sched/core.c | 36 ++++++++++++++++++++++++++++++++---- kernel/workqueue.c | 6 ++---- 5 files changed, 52 insertions(+), 18 deletions(-)
include/linux/kthread.h +1 -0
@@ -38,6 +38,7 @@ struct task_struct *kthread_create_on_cp })void kthread_bind (struct task_struct *k,unsigned int cpu); +void kthread_bind_mask (struct task_struct *k,const struct cpumask *mask);int kthread_stop (struct task_struct *k);bool kthread_should_stop (void );bool kthread_should_park (void );
include/linux/sched.h +0 -7
@@ -2215,13 +2215,6 @@ static inline void calc_load_enter_idle(static inline void calc_load_exit_idle (void ) { }#endif /* CONFIG_NO_HZ_COMMON */ -#ifndef CONFIG_CPUMASK_OFFSTACK -static inline int set_cpus_allowed (struct task_struct *p,cpumask_t new_mask) -{ -return set_cpus_allowed_ptr (p, &new_mask); -} -#endif -/* * Do not use outside of architecture code which knows its limitations. *
kernel/kthread.c +17 -3
@@ -325,16 +325,30 @@ struct task_struct *kthread_create_on_no }EXPORT_SYMBOL (kthread_create_on_node); -static void __kthread_bind (struct task_struct *p,unsigned int cpu,long state) +static void __kthread_bind_mask (struct task_struct *p,const struct cpumask *mask,long state) { -/* Must have done schedule() in kthread() before we set_task_cpu */ +unsigned long flags; +if (!wait_task_inactive (p, state)) {WARN_ON (1 );return ; } +/* It's safe because the task is inactive. */ -do_set_cpus_allowed (p,cpumask_of (cpu)); +raw_spin_lock_irqsave (&p->pi_lock, flags); +do_set_cpus_allowed (p, mask); p->flags |=PF_NO_SETAFFINITY ; +raw_spin_unlock_irqrestore (&p->pi_lock, flags); +} + +static void __kthread_bind (struct task_struct *p,unsigned int cpu,long state) +{ +__kthread_bind_mask (p,cpumask_of (cpu), state); +} + +void kthread_bind_mask (struct task_struct *p,const struct cpumask *mask) +{ +__kthread_bind_mask (p, mask,TASK_UNINTERRUPTIBLE ); }/**
kernel/sched/core.c +32 -4
@@ -1151,6 +1151,8 @@ static int migration_cpu_stop(void *datavoid do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) { +lockdep_assert_held (&p->pi_lock); +if (p->sched_class->set_cpus_allowed) p->sched_class->set_cpus_allowed (p, new_mask);
@@ -1167,7 +1169,8 @@ void do_set_cpus_allowed(struct task_str* task must not exit() & deallocate itself prematurely. The * call is not atomic; no spinlocks may be held. */ -int set_cpus_allowed_ptr (struct task_struct *p,const struct cpumask *new_mask) +static int __set_cpus_allowed_ptr (struct task_struct *p, +const struct cpumask *new_mask,bool check) {unsigned long flags;struct rq *rq;
@@ -1176,6 +1179,15 @@ int set_cpus_allowed_ptr(struct task_str rq =task_rq_lock (p, &flags); +/* +* Must re-check here, to close a race against __kthread_bind(), +* sched_setaffinity() is not guaranteed to observe the flag. +*/ +if (check && (p->flags &PF_NO_SETAFFINITY )) { + ret = -EINVAL ; + goto out; + } +if (cpumask_equal (&p->cpus_allowed, new_mask)) goto out;
@@ -1212,6 +1224,11 @@ int set_cpus_allowed_ptr(struct task_strreturn ret; } + +int set_cpus_allowed_ptr (struct task_struct *p,const struct cpumask *new_mask) +{ +return __set_cpus_allowed_ptr (p, new_mask, false); +}EXPORT_SYMBOL_GPL (set_cpus_allowed_ptr);void set_task_cpu (struct task_struct *p,unsigned int new_cpu)
@@ -1593,6 +1610,15 @@ static void update_avg(u64 *avg, u64 sams64 diff = sample - *avg; *avg += diff >>3 ; } + +#else + +static inline int __set_cpus_allowed_ptr (struct task_struct *p, +const struct cpumask *new_mask,bool check) +{ +return set_cpus_allowed_ptr (p, new_mask); +} +#endif /* CONFIG_SMP */ static void
@@ -4338,7 +4364,7 @@ long sched_setaffinity(pid_t pid, const }#endif again: - retval =set_cpus_allowed_ptr (p, new_mask); + retval =__set_cpus_allowed_ptr (p, new_mask, true);if (!retval) {cpuset_cpus_allowed (p, cpus_allowed);
@@ -4863,7 +4889,8 @@ void init_idle(struct task_struct *idle,struct rq *rq =cpu_rq (cpu);unsigned long flags; -raw_spin_lock_irqsave (&rq->lock, flags); +raw_spin_lock_irqsave (&idle->pi_lock, flags); +raw_spin_lock (&rq->lock);__sched_fork (0 , idle); idle->state =TASK_RUNNING ;
@@ -4889,7 +4916,8 @@ void init_idle(struct task_struct *idle,#if defined(CONFIG_SMP ) idle->on_cpu =1 ;#endif -raw_spin_unlock_irqrestore (&rq->lock, flags); +raw_spin_unlock (&rq->lock); +raw_spin_unlock_irqrestore (&idle->pi_lock, flags);/* Set the preempt count _outside_ the spinlocks! */ init_idle_preempt_count (idle, cpu);
kernel/workqueue.c +2 -4
@@ -1714,9 +1714,7 @@ static struct worker *create_worker(stru goto fail;set_user_nice (worker->task, pool->attrs->nice); - -/* prevent userland from meddling with cpumask of workqueue workers */ - worker->task->flags |=PF_NO_SETAFFINITY ; +kthread_bind_mask (worker->task, pool->attrs->cpumask);/* successful, attach the worker to the pool */ worker_attach_to_pool (worker, pool);
@@ -3856,7 +3854,7 @@ struct workqueue_struct *__alloc_workque } wq->rescuer = rescuer; - rescuer->task->flags |=PF_NO_SETAFFINITY ; +kthread_bind_mask (rescuer->task, cpu_possible_mask);wake_up_process (rescuer->task); }
Tejun Heo
maintainerRe: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and sched_setaffinity()
Hello, Peter.
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines
So, the problem there is that
__kthread_bind()doesn't grab the same lock that the syscall side grabs but workqueue usedset_cpus_allowed_ptr()which goes through the rq locking, so as long as the check on syscall side is movied inside rq lock, it should be fine.Currently neither site uses any lock, and that is what the patch fixes (it uses the per-task ->pi_lock instead of the
rq->lock, but that is immaterial).
Yeap, the testing on the syscall side should definitely be moved inside rq->lock.
Quoted 5 lines
What matters though is that you now must hold a scheduler lock while setting
PF_NO_SETAFFINITY. In order to avoid spreading that knowledge around I've taught kthread_bind*() about this and made the workqueue code use that API (rather than having the workqueue code take scheduler locks).
So, as long as PF_NO_SETAFFINITY is set before the task sets its affinity to its target holding the rq lock, it should still be safe.
Quoted 3 lines
Hmm.. a better solution. Have the worker thread creation call
kthread_bind_mask()beforeattach_to_pool()and haveattach_to_pool()keep usingset_cpus_allowed_ptr(). Less ugly.
Yeah, that works too. About the same effect.
Thanks.
Signature · 1 line
tejun
tip-bot for Peter Zijlstra
merge bot[tip:sched/core] sched: Fix a race between __kthread_bind() and sched_setaffinity()
In reply to Peter Zijlstrasched: Fix a race between __kthread_bind() and sched_setaffinity()
Commit-ID: 25834c73f93af7f0712c98ca4593691592e6b360 Gitweb: http://git.kernel.org/tip/25834c73f93af7f0712c98ca4593691592e6b360 Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 15 May 2015 17:43:34 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 12 Aug 2015 12:06:09 +0200
sched: Fix a race between __kthread_bind() and sched_setaffinity()
Because sched_setscheduler() checks p->flags & PF_NO_SETAFFINITY without locks, a caller might observe an old value and race with the set_cpus_allowed_ptr() call from __kthread_bind() and effectively undo it:
__kthread_bind() do_set_cpus_allowed() <SYSCALL> sched_setaffinity() if (p->flags & PF_NO_SETAFFINITIY) set_cpus_allowed_ptr() p->flags |= PF_NO_SETAFFINITY
Fix the bug by putting everything under the regular scheduler locks.
This also closes a hole in the serialization of task_struct::{nr_,}cpus_allowed.
--- include/linux/kthread.h | 1 + include/linux/sched.h | 7 ------- kernel/kthread.c | 20 +++++++++++++++++--- kernel/sched/core.c | 36 ++++++++++++++++++++++++++++++++---- kernel/workqueue.c | 6 ++---- 5 files changed, 52 insertions(+), 18 deletions(-)
include/linux/kthread.h +1 -0
@@ -38,6 +38,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data), })void kthread_bind (struct task_struct *k,unsigned int cpu); +void kthread_bind_mask (struct task_struct *k,const struct cpumask *mask);int kthread_stop (struct task_struct *k);bool kthread_should_stop (void );bool kthread_should_park (void );
include/linux/sched.h +0 -7
@@ -2203,13 +2203,6 @@ static inline void calc_load_enter_idle(void) { }static inline void calc_load_exit_idle (void ) { }#endif /* CONFIG_NO_HZ_COMMON */ -#ifndef CONFIG_CPUMASK_OFFSTACK -static inline int set_cpus_allowed (struct task_struct *p,cpumask_t new_mask) -{ -return set_cpus_allowed_ptr (p, &new_mask); -} -#endif -/* * Do not use outside of architecture code which knows its limitations. *
kernel/kthread.c +17 -3
@@ -325,16 +325,30 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data), }EXPORT_SYMBOL (kthread_create_on_node); -static void __kthread_bind (struct task_struct *p,unsigned int cpu,long state) +static void __kthread_bind_mask (struct task_struct *p,const struct cpumask *mask,long state) { -/* Must have done schedule() in kthread() before we set_task_cpu */ +unsigned long flags; +if (!wait_task_inactive (p, state)) {WARN_ON (1 );return ; } +/* It's safe because the task is inactive. */ -do_set_cpus_allowed (p,cpumask_of (cpu)); +raw_spin_lock_irqsave (&p->pi_lock, flags); +do_set_cpus_allowed (p, mask); p->flags |=PF_NO_SETAFFINITY ; +raw_spin_unlock_irqrestore (&p->pi_lock, flags); +} + +static void __kthread_bind (struct task_struct *p,unsigned int cpu,long state) +{ +__kthread_bind_mask (p,cpumask_of (cpu), state); +} + +void kthread_bind_mask (struct task_struct *p,const struct cpumask *mask) +{ +__kthread_bind_mask (p, mask,TASK_UNINTERRUPTIBLE ); }/**
kernel/sched/core.c +32 -4
@@ -1153,6 +1153,8 @@ static int migration_cpu_stop(void *data)void do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) { +lockdep_assert_held (&p->pi_lock); +if (p->sched_class->set_cpus_allowed) p->sched_class->set_cpus_allowed (p, new_mask);
@@ -1169,7 +1171,8 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)* task must not exit() & deallocate itself prematurely. The * call is not atomic; no spinlocks may be held. */ -int set_cpus_allowed_ptr (struct task_struct *p,const struct cpumask *new_mask) +static int __set_cpus_allowed_ptr (struct task_struct *p, +const struct cpumask *new_mask,bool check) {unsigned long flags;struct rq *rq;
@@ -1178,6 +1181,15 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask) rq =task_rq_lock (p, &flags); +/* +* Must re-check here, to close a race against __kthread_bind(), +* sched_setaffinity() is not guaranteed to observe the flag. +*/ +if (check && (p->flags &PF_NO_SETAFFINITY )) { + ret = -EINVAL ; + goto out; + } +if (cpumask_equal (&p->cpus_allowed, new_mask)) goto out;
@@ -1214,6 +1226,11 @@ out:return ret; } + +int set_cpus_allowed_ptr (struct task_struct *p,const struct cpumask *new_mask) +{ +return __set_cpus_allowed_ptr (p, new_mask, false); +}EXPORT_SYMBOL_GPL (set_cpus_allowed_ptr);void set_task_cpu (struct task_struct *p,unsigned int new_cpu)
@@ -1595,6 +1612,15 @@ static void update_avg(u64 *avg, u64 sample)s64 diff = sample - *avg; *avg += diff >>3 ; } + +#else + +static inline int __set_cpus_allowed_ptr (struct task_struct *p, +const struct cpumask *new_mask,bool check) +{ +return set_cpus_allowed_ptr (p, new_mask); +} +#endif /* CONFIG_SMP */ static void
@@ -4340,7 +4366,7 @@ long sched_setaffinity(pid_t pid, const struct cpumask *in_mask) }#endif again: - retval =set_cpus_allowed_ptr (p, new_mask); + retval =__set_cpus_allowed_ptr (p, new_mask, true);if (!retval) {cpuset_cpus_allowed (p, cpus_allowed);
@@ -4865,7 +4891,8 @@ void init_idle(struct task_struct *idle, int cpu)struct rq *rq =cpu_rq (cpu);unsigned long flags; -raw_spin_lock_irqsave (&rq->lock, flags); +raw_spin_lock_irqsave (&idle->pi_lock, flags); +raw_spin_lock (&rq->lock);__sched_fork (0 , idle); idle->state =TASK_RUNNING ;
@@ -4891,7 +4918,8 @@ void init_idle(struct task_struct *idle, int cpu)#if defined(CONFIG_SMP ) idle->on_cpu =1 ;#endif -raw_spin_unlock_irqrestore (&rq->lock, flags); +raw_spin_unlock (&rq->lock); +raw_spin_unlock_irqrestore (&idle->pi_lock, flags);/* Set the preempt count _outside_ the spinlocks! */ init_idle_preempt_count (idle, cpu);
kernel/workqueue.c +2 -4
@@ -1714,9 +1714,7 @@ static struct worker *create_worker(struct worker_pool *pool) goto fail;set_user_nice (worker->task, pool->attrs->nice); - -/* prevent userland from meddling with cpumask of workqueue workers */ - worker->task->flags |=PF_NO_SETAFFINITY ; +kthread_bind_mask (worker->task, pool->attrs->cpumask);/* successful, attach the worker to the pool */ worker_attach_to_pool (worker, pool);
@@ -3856,7 +3854,7 @@ struct workqueue_struct *__alloc_workqueue_key(const char *fmt, } wq->rescuer = rescuer; - rescuer->task->flags |=PF_NO_SETAFFINITY ; +kthread_bind_mask (rescuer->task, cpu_possible_mask);wake_up_process (rescuer->task); }
Peter Zijlstra
maintainerRFC PATCH 2/4 sched: Make sched_class::set_cpus_allowed() unconditional
In reply to Peter ZijlstraGive every class a set_cpus_allowed() method, this enables some small optimization in the rt,dl implementation by avoiding a double cpumask_weight() call.
--- kernel/sched/core.c | 17 +++++++++++------ kernel/sched/deadline.c | 20 ++++++++++++-------- kernel/sched/fair.c | 1 + kernel/sched/idle_task.c | 1 + kernel/sched/rt.c | 12 ++++++++---- kernel/sched/sched.h | 2 ++ kernel/sched/stop_task.c | 1 + 7 files changed, 36 insertions(+), 18 deletions(-)
kernel/sched/core.c +11 -6
@@ -4791,17 +4791,22 @@ static struct rq *move_queued_task(strucreturn rq; } -void do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) +/* +* sched_class::set_cpus_allowed must do the below, but is not required to +* actually call this function. +*/ +void set_cpus_allowed_common (struct task_struct *p,const struct cpumask *new_mask) { -lockdep_assert_held (&p->pi_lock); - -if (p->sched_class->set_cpus_allowed) - p->sched_class->set_cpus_allowed (p, new_mask); -cpumask_copy (&p->cpus_allowed, new_mask); p->nr_cpus_allowed =cpumask_weight (new_mask); } +void do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) +{ +lockdep_assert_held (&p->pi_lock); + p->sched_class->set_cpus_allowed (p, new_mask); +} +/* * This is how migration works: *
kernel/sched/deadline.c +12 -8
@@ -1597,13 +1597,6 @@ static void set_cpus_allowed_dl(struct traw_spin_unlock (&src_dl_b->lock); } -/* -* Update only if the task is actually running (i.e., -* it is on the rq AND it is not throttled). -*/ -if (!on_dl_rq (&p->dl)) -return ; - weight =cpumask_weight (new_mask);/*
@@ -1611,7 +1604,14 @@ static void set_cpus_allowed_dl(struct t* can migrate or not. */ if ((p->nr_cpus_allowed >1 ) == (weight >1 )) -return ; + goto done; + +/* +* Update only if the task is actually running (i.e., +* it is on the rq AND it is not throttled). +*/ +if (!on_dl_rq (&p->dl)) + goto done;/* * The process used to be able to migrate OR it can now migrate
@@ -1628,6 +1628,10 @@ static void set_cpus_allowed_dl(struct t }update_dl_migration (&rq->dl); + +done: +cpumask_copy (&p->cpus_allowed, new_mask); + p->nr_cpus_allowed = weight; }/* Assumes rq->lock is held */
kernel/sched/fair.c +1 -0
@@ -8406,6 +8406,7 @@ const struct sched_class fair_sched_clas .rq_offline = rq_offline_fair, .task_waking = task_waking_fair, + .set_cpus_allowed = set_cpus_allowed_common, #endif .set_curr_task = set_curr_task_fair,
kernel/sched/idle_task.c +1 -0
@@ -96,6 +96,7 @@ const struct sched_class idle_sched_clas#ifdef CONFIG_SMP .select_task_rq = select_task_rq_idle, + .set_cpus_allowed = set_cpus_allowed_common, #endif .set_curr_task = set_curr_task_idle,
kernel/sched/rt.c +8 -4
@@ -2065,9 +2065,6 @@ static void set_cpus_allowed_rt(struct tBUG_ON (!rt_task (p)); -if (!task_on_rq_queued (p)) -return ; - weight =cpumask_weight (new_mask);/*
@@ -2075,7 +2072,10 @@ static void set_cpus_allowed_rt(struct t* can migrate or not. */ if ((p->nr_cpus_allowed >1 ) == (weight >1 )) -return ; + goto done; + +if (!task_on_rq_queued (p)) + goto done; rq =task_rq (p);
@@ -2094,6 +2094,10 @@ static void set_cpus_allowed_rt(struct t }update_rt_migration (&rq->rt); + +done: +cpumask_copy (&p->cpus_allowed, new_mask); + p->nr_cpus_allowed = weight; }/* Assumes rq->lock is held */
kernel/sched/sched.h +2 -0
@@ -1252,6 +1252,8 @@ extern void trigger_load_balance(structextern void idle_enter_fair (struct rq *this_rq);extern void idle_exit_fair (struct rq *this_rq); +extern void set_cpus_allowed_common (struct task_struct *p,const struct cpumask *new_mask); +#else static inline void idle_enter_fair (struct rq *rq) { }
kernel/sched/stop_task.c +1 -0
@@ -123,6 +123,7 @@ const struct sched_class stop_sched_clas#ifdef CONFIG_SMP .select_task_rq = select_task_rq_stop, + .set_cpus_allowed = set_cpus_allowed_common, #endif .set_curr_task = set_curr_task_stop,
tip-bot for Peter Zijlstra
merge bot[tip:sched/core] sched: Make sched_class::set_cpus_allowed() unconditional
sched: Make sched_class::set_cpus_allowed() unconditional
Commit-ID: c5b2803840817115e9b568d5054e5007ae36176b Gitweb: http://git.kernel.org/tip/c5b2803840817115e9b568d5054e5007ae36176b Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 15 May 2015 17:43:35 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 12 Aug 2015 12:06:09 +0200
sched: Make sched_class::set_cpus_allowed() unconditional
Give every class a set_cpus_allowed() method, this enables some small optimization in the RT,DL implementation by avoiding a double cpumask_weight() call.
--- kernel/sched/core.c | 17 +++++++++++------ kernel/sched/deadline.c | 20 ++++++++++++-------- kernel/sched/fair.c | 1 + kernel/sched/idle_task.c | 1 + kernel/sched/rt.c | 12 ++++++++---- kernel/sched/sched.h | 2 ++ kernel/sched/stop_task.c | 1 + 7 files changed, 36 insertions(+), 18 deletions(-)
kernel/sched/core.c +11 -6
@@ -1151,17 +1151,22 @@ static int migration_cpu_stop(void *data)return 0 ; } -void do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) +/* +* sched_class::set_cpus_allowed must do the below, but is not required to +* actually call this function. +*/ +void set_cpus_allowed_common (struct task_struct *p,const struct cpumask *new_mask) { -lockdep_assert_held (&p->pi_lock); - -if (p->sched_class->set_cpus_allowed) - p->sched_class->set_cpus_allowed (p, new_mask); -cpumask_copy (&p->cpus_allowed, new_mask); p->nr_cpus_allowed =cpumask_weight (new_mask); } +void do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) +{ +lockdep_assert_held (&p->pi_lock); + p->sched_class->set_cpus_allowed (p, new_mask); +} +/* * Change a given task's CPU affinity. Migrate the thread to a * proper CPU and schedule it away if the CPU it's executing on
kernel/sched/deadline.c +12 -8
@@ -1696,13 +1696,6 @@ static void set_cpus_allowed_dl(struct task_struct *p,raw_spin_unlock (&src_dl_b->lock); } -/* -* Update only if the task is actually running (i.e., -* it is on the rq AND it is not throttled). -*/ -if (!on_dl_rq (&p->dl)) -return ; - weight =cpumask_weight (new_mask);/*
@@ -1710,7 +1703,14 @@ static void set_cpus_allowed_dl(struct task_struct *p,* can migrate or not. */ if ((p->nr_cpus_allowed >1 ) == (weight >1 )) -return ; + goto done; + +/* +* Update only if the task is actually running (i.e., +* it is on the rq AND it is not throttled). +*/ +if (!on_dl_rq (&p->dl)) + goto done;/* * The process used to be able to migrate OR it can now migrate
@@ -1727,6 +1727,10 @@ static void set_cpus_allowed_dl(struct task_struct *p, }update_dl_migration (&rq->dl); + +done: +cpumask_copy (&p->cpus_allowed, new_mask); + p->nr_cpus_allowed = weight; }/* Assumes rq->lock is held */
kernel/sched/fair.c +1 -0
@@ -8252,6 +8252,7 @@ const struct sched_class fair_sched_class = { .task_waking = task_waking_fair, .task_dead = task_dead_fair, + .set_cpus_allowed = set_cpus_allowed_common, #endif .set_curr_task = set_curr_task_fair,
kernel/sched/idle_task.c +1 -0
@@ -96,6 +96,7 @@ const struct sched_class idle_sched_class = {#ifdef CONFIG_SMP .select_task_rq = select_task_rq_idle, + .set_cpus_allowed = set_cpus_allowed_common, #endif .set_curr_task = set_curr_task_idle,
kernel/sched/rt.c +8 -4
@@ -2084,9 +2084,6 @@ static void set_cpus_allowed_rt(struct task_struct *p,BUG_ON (!rt_task (p)); -if (!task_on_rq_queued (p)) -return ; - weight =cpumask_weight (new_mask);/*
@@ -2094,7 +2091,10 @@ static void set_cpus_allowed_rt(struct task_struct *p,* can migrate or not. */ if ((p->nr_cpus_allowed >1 ) == (weight >1 )) -return ; + goto done; + +if (!task_on_rq_queued (p)) + goto done; rq =task_rq (p);
@@ -2113,6 +2113,10 @@ static void set_cpus_allowed_rt(struct task_struct *p, }update_rt_migration (&rq->rt); + +done: +cpumask_copy (&p->cpus_allowed, new_mask); + p->nr_cpus_allowed = weight; }/* Assumes rq->lock is held */
kernel/sched/sched.h +2 -0
@@ -1255,6 +1255,8 @@ extern void trigger_load_balance(struct rq *rq);extern void idle_enter_fair (struct rq *this_rq);extern void idle_exit_fair (struct rq *this_rq); +extern void set_cpus_allowed_common (struct task_struct *p,const struct cpumask *new_mask); +#else static inline void idle_enter_fair (struct rq *rq) { }
kernel/sched/stop_task.c +1 -0
@@ -123,6 +123,7 @@ const struct sched_class stop_sched_class = {#ifdef CONFIG_SMP .select_task_rq = select_task_rq_stop, + .set_cpus_allowed = set_cpus_allowed_common, #endif .set_curr_task = set_curr_task_stop,
Re: [RFC][PATCH 2/4] sched: Make sched_class::set_cpus_allowed() unconditional
In reply to Peter ZijlstraQuoted 5 lines · Peter Zijlstra
Give every class a
set_cpus_allowed()method, this enables some small optimization in the rt,dl implementation by avoiding a doublecpumask_weight()call.Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Hey Peter,
Here's the splat I mentioned earlier. I'm not sure if it's caused by this commit or not, it's just that git blame took me here.
[392896.406248] WARNING: CPU: 9 PID: 51 at kernel/sched/core.c:1178 do_set_cpus_allowed+0x1cf/0x480() [392896.407070] Modules linked in: [392896.407408] CPU: 9 PID: 51 Comm: migration/9 Not tainted 4.2.0-rc6-next-20150817-sasha-00041-g9b6b2ab-dirty #2460 [392896.408376] ffffffffab2c2ac0 ffff880477857948 ffffffffaafaf5a9 0000000000000000 [392896.409126] ffff880477857988 ffffffffa1323246 ffffffffa13ab19f ffff880477284000 [392896.409874] ffff8804781e15c0 ffff88006bc03d98 ffffffffae480d20 0000000000000001 [392896.410610] Call Trace: [392896.410947] dump_stack (lib/dump_stack.c:52) [392896.411460] warn_slowpath_common (kernel/panic.c:448) [392896.412037] ? do_set_cpus_allowed (kernel/sched/sched.h:1050 (discriminator 9) kernel/sched/core.c:1180 (discriminator 9)) [392896.412629] warn_slowpath_null (kernel/panic.c:482) [392896.413183] do_set_cpus_allowed (kernel/sched/sched.h:1050 (discriminator 9) kernel/sched/core.c:1180 (discriminator 9)) [392896.413778] cpuset_cpus_allowed_fallback (include/linux/rcupdate.h:911 kernel/cpuset.c:2381) [392896.414415] ? cpuset_cpus_allowed_fallback (kernel/cpuset.c:2378) [392896.415055] select_fallback_rq (kernel/sched/core.c:1589) [392896.415636] ? put_prev_entity (kernel/sched/fair.c:3218) [392896.416234] migration_call (kernel/sched/core.c:5230 kernel/sched/core.c:5492) [392896.416786] notifier_call_chain (kernel/notifier.c:93) [392896.417359] ? cpu_notify (kernel/cpu.c:334) [392896.417863] __raw_notifier_call_chain (kernel/notifier.c:395) [392896.419027] cpu_notify (include/linux/notifier.h:179 kernel/cpu.c:231 kernel/cpu.c:236) [392896.419616] take_cpu_down (kernel/cpu.c:345) [392896.420127] multi_cpu_stop (kernel/stop_machine.c:203) [392896.420671] ? queue_stop_cpus_work (kernel/stop_machine.c:172) [392896.421275] cpu_stopper_thread (kernel/stop_machine.c:440) [392896.421849] ? cpu_stop_create (kernel/stop_machine.c:415) [392896.422404] ? lock_release (kernel/locking/lockdep.c:3643) [392896.422936] ? __raw_callee_save___pv_queued_spin_unlock (??:?) [392896.423710] ? _raw_spin_unlock_irqrestore (./arch/x86/include/asm/paravirt.h:802 include/linux/spinlock_api_smp.h:162 kernel/locking/spinlock.c:191) [392896.424354] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2594 kernel/locking/lockdep.c:2636) [392896.424978] ? _raw_spin_unlock_irqrestore (./arch/x86/include/asm/preempt.h:77 include/linux/spinlock_api_smp.h:163 kernel/locking/spinlock.c:191) [392896.425610] ? cpu_stop_create (kernel/stop_machine.c:415) [392896.426152] smpboot_thread_fn (kernel/smpboot.c:163 (discriminator 1)) [392896.426707] ? sort_range (kernel/smpboot.c:106) [392896.427252] ? __kthread_parkme (kernel/kthread.c:165) [392896.427817] ? sort_range (kernel/smpboot.c:106) [392896.428332] kthread (kernel/kthread.c:209) [392896.428862] ? kthread_stop (kernel/kthread.c:178) [392896.429399] ? lock_release (kernel/locking/lockdep.c:3643) [392896.429977] ? wait_for_completion (kernel/sched/completion.c:77 kernel/sched/completion.c:93 kernel/sched/completion.c:101 kernel/sched/completion.c:122)
Thanks,
Sasha
Peter Zijlstra
maintainerRFC PATCH 3/4 sched: Change sched_class::set_cpus_allowed calling context
In reply to Peter ZijlstraChange the calling context of sched_class::set_cpus_allowed() such that we can assume the task is inactive.
This allows us to easily make changes that affect accounting done by enqueue/dequeue. This does in fact completely remove set_cpus_allowed_rt and greatly reduces set_cpus_allowed_dl.
--- kernel/sched/core.c | 23 +++++++++++++++++++++++ kernel/sched/deadline.c | 39 ++------------------------------------- kernel/sched/rt.c | 45 +-------------------------------------------- 3 files changed, 26 insertions(+), 81 deletions(-)
kernel/sched/core.c +23 -0
@@ -4803,8 +4803,31 @@ void set_cpus_allowed_common(struct taskvoid do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) { +struct rq *rq =task_rq (p); +bool queued, running; +lockdep_assert_held (&p->pi_lock); + + queued =task_on_rq_queued (p); + running =task_current (rq, p); + +if (queued) { +/* +* Because __kthread_bind() calls this on blocked tasks without +* holding rq->lock. +*/ +lockdep_assert_held (&rq->lock); +dequeue_task (rq, p,0 ); + } +if (running) +put_prev_task (rq, p); + p->sched_class->set_cpus_allowed (p, new_mask); + +if (running) + p->sched_class->set_curr_task (rq); +if (queued) +enqueue_task (rq, p,0 ); }/*
kernel/sched/deadline.c +2 -37
@@ -1569,9 +1569,8 @@ static void task_woken_dl(struct rq *rq,static void set_cpus_allowed_dl (struct task_struct *p,const struct cpumask *new_mask) { -struct rq *rq;struct root_domain *src_rd; -int weight; +struct rq *rq;BUG_ON (!dl_task (p));
@@ -1597,41 +1596,7 @@ static void set_cpus_allowed_dl(struct traw_spin_unlock (&src_dl_b->lock); } - weight =cpumask_weight (new_mask); - -/* -* Only update if the process changes its state from whether it -* can migrate or not. -*/ -if ((p->nr_cpus_allowed >1 ) == (weight >1 )) - goto done; - -/* -* Update only if the task is actually running (i.e., -* it is on the rq AND it is not throttled). -*/ -if (!on_dl_rq (&p->dl)) - goto done; - -/* -* The process used to be able to migrate OR it can now migrate -*/ -if (weight <=1 ) { -if (!task_current (rq, p)) -dequeue_pushable_dl_task (rq, p); -BUG_ON (!rq->dl.dl_nr_migratory); - rq->dl.dl_nr_migratory--; - }else { -if (!task_current (rq, p)) -enqueue_pushable_dl_task (rq, p); - rq->dl.dl_nr_migratory++; - } - -update_dl_migration (&rq->dl); - -done: -cpumask_copy (&p->cpus_allowed, new_mask); - p->nr_cpus_allowed = weight; +set_cpus_allowed_common (p, new_mask); }/* Assumes rq->lock is held */
kernel/sched/rt.c +1 -44
57 lines not on this page · raw message
A message that is not in the archive (sent off-list or not yet received): OF66BF3765.2EBFD3B1-ON48257E49.0028DC79-48257E49.0029F058@zte.com.cn
Peter Zijlstra
maintainerRe: [RFC][PATCH 3/4] sched: Change sched_class::set_cpus_allowed calling context
Quoted 5 lines · On Mon, May 18, 2015 at 03:37:43PM +0800, [email protected] wrote:
Hi Peter,
With this modification, I think the pushing action in my previous patch "Check to push the task away after its affinity was changed" will not be able to be implemented inside
sched_class::set_cpus_allowed().
Ah, right, I knew there was a patch I needed to look at.
I'll try and not forget, but there's a few regression reports I need to look at first.
Re: [RFC][PATCH 3/4] sched: Change sched_class::set_cpus_allowed calling context
Hi Peter,
Quoted 12 lines · Peter Zijlstra
Quoted 5 lines · On Mon, May 18, 2015 at 03:37:43PM +0800, [email protected] wrote:
Hi Peter,
With this modification, I think the pushing action in my previous patch "Check to push the task away after its affinity was changed" will not be able to be implemented inside
sched_class::set_cpus_allowed().Ah, right, I knew there was a patch I needed to look at.
I'll try and not forget, but there's a few regression reports I need to look at first.
Apart from this (and the fact that I still have to look at Xunlei's patches too) the changes seem ok with DL. Didn't test them yet though. I'll do it soon.
Best,
- Juri
Peter Zijlstra
maintainerRe: [RFC][PATCH 3/4] sched: Change sched_class::set_cpus_allowed calling context
In reply to Peter ZijlstraQuoted 8 lines · Peter Zijlstra
Quoted 5 lines · On Mon, May 18, 2015 at 03:37:43PM +0800, [email protected] wrote:
Hi Peter,
With this modification, I think the pushing action in my previous patch "Check to push the task away after its affinity was changed" will not be able to be implemented inside
sched_class::set_cpus_allowed().Ah, right, I knew there was a patch I needed to look at.
So basically you want to do:
+check_push:
+ if (weight > 1 &&
+ !task_running(rq, p) &&
+ !test_tsk_need_resched(rq->curr) &&
+ !cpumask_subset(new_mask, &p->cpus_allowed)) {
+ /* Update new affinity and try to push. */
+ cpumask_copy(&p->cpus_allowed, new_mask);
+ p->nr_cpus_allowed = weight;
+ push_rt_tasks(rq);
+ return true;+ }
in set_cpus_allowed_rt(), which would not work because of us calling put_prev_task(), which does enqueue_pushable_task() and would allow pick_next_pushable_task() to select the current task, which would then BUG_ON().
Note however that you already test for !task_running(), which precludes that entire argument, because if @p is not running, we did not call put_prev_task() etc..
So I think the above would still work; albeit it needs a comment on why etc..
tip-bot for Peter Zijlstra
merge bot[tip:sched/core] sched: Change the sched_class::set_cpus_allowed( ) calling context
In reply to Peter Zijlstrasched: Change the sched_class::set_cpus_allowed() calling context
Commit-ID: 6c37067e27867db172b988cc11b9ff921175dee5 Gitweb: http://git.kernel.org/tip/6c37067e27867db172b988cc11b9ff921175dee5 Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 15 May 2015 17:43:36 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 12 Aug 2015 12:06:10 +0200
sched: Change the sched_class::set_cpus_allowed() calling context
Change the calling context of sched_class::set_cpus_allowed() such that we can assume the task is inactive.
This allows us to easily make changes that affect accounting done by enqueue/dequeue. This does in fact completely remove set_cpus_allowed_rt() and greatly reduces set_cpus_allowed_dl().
--- kernel/sched/core.c | 23 +++++++++++++++++++++++ kernel/sched/deadline.c | 39 ++------------------------------------- kernel/sched/rt.c | 45 +-------------------------------------------- 3 files changed, 26 insertions(+), 81 deletions(-)
kernel/sched/core.c +23 -0
@@ -1163,8 +1163,31 @@ void set_cpus_allowed_common(struct task_struct *p, const struct cpumask *new_mavoid do_set_cpus_allowed (struct task_struct *p,const struct cpumask *new_mask) { +struct rq *rq =task_rq (p); +bool queued, running; +lockdep_assert_held (&p->pi_lock); + + queued =task_on_rq_queued (p); + running =task_current (rq, p); + +if (queued) { +/* +* Because __kthread_bind() calls this on blocked tasks without +* holding rq->lock. +*/ +lockdep_assert_held (&rq->lock); +dequeue_task (rq, p,0 ); + } +if (running) +put_prev_task (rq, p); + p->sched_class->set_cpus_allowed (p, new_mask); + +if (running) + p->sched_class->set_curr_task (rq); +if (queued) +enqueue_task (rq, p,0 ); }/*
kernel/sched/deadline.c +2 -37
@@ -1668,9 +1668,8 @@ static void task_woken_dl(struct rq *rq, struct task_struct *p)static void set_cpus_allowed_dl (struct task_struct *p,const struct cpumask *new_mask) { -struct rq *rq;struct root_domain *src_rd; -int weight; +struct rq *rq;BUG_ON (!dl_task (p));
@@ -1696,41 +1695,7 @@ static void set_cpus_allowed_dl(struct task_struct *p,raw_spin_unlock (&src_dl_b->lock); } - weight =cpumask_weight (new_mask); - -/* -* Only update if the process changes its state from whether it -* can migrate or not. -*/ -if ((p->nr_cpus_allowed >1 ) == (weight >1 )) - goto done; - -/* -* Update only if the task is actually running (i.e., -* it is on the rq AND it is not throttled). -*/ -if (!on_dl_rq (&p->dl)) - goto done; - -/* -* The process used to be able to migrate OR it can now migrate -*/ -if (weight <=1 ) { -if (!task_current (rq, p)) -dequeue_pushable_dl_task (rq, p); -BUG_ON (!rq->dl.dl_nr_migratory); - rq->dl.dl_nr_migratory--; - }else { -if (!task_current (rq, p)) -enqueue_pushable_dl_task (rq, p); - rq->dl.dl_nr_migratory++; - } - -update_dl_migration (&rq->dl); - -done: -cpumask_copy (&p->cpus_allowed, new_mask); - p->nr_cpus_allowed = weight; +set_cpus_allowed_common (p, new_mask); }/* Assumes rq->lock is held */
kernel/sched/rt.c +1 -44
57 lines not on this page · raw message
Re: [tip:sched/core] sched: Change the sched_class::set_cpus_allowed( ) calling context
Quoted 16 lines · tip-bot for Peter Zijlstra
Commit-ID: 6c37067e27867db172b988cc11b9ff921175dee5 Gitweb: http://git.kernel.org/tip/6c37067e27867db172b988cc11b9ff921175dee5 Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 15 May 2015 17:43:36 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 12 Aug 2015 12:06:10 +0200sched: Change the
sched_class::set_cpus_allowed()calling contextChange the calling context of
sched_class::set_cpus_allowed()such that we can assume the task is inactive.This allows us to easily make changes that affect accounting done by enqueue/dequeue. This does in fact completely remove
set_cpus_allowed_rt()and greatly reducesset_cpus_allowed_dl().
Quoted 12 lines
Signed-off-by Peter Zijlstra (Intel) <[email protected]>Cc Linus Torvalds <[email protected]>Cc Mike Galbraith <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Signed-off-by Ingo Molnar <[email protected]>
Hey Peter,
This patch breaks boot inside my VM:
[79817.224383] bad: scheduling from the idle thread! [79817.224900] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc6-next-20150813-sasha-00040-g059fd6d #2431 [79817.225709] ffffffffae0c6320 ffff880051b77c00 ffffffffade7f739 ffff8800a69e15c0 [79817.226419] ffff880051b77c18 ffffffffa43c4ed3 ffff880051be8000 ffff880051b77c48 [79817.227122] ffffffffa43a7ffb ffff880051be8000 ffff8800a69e15c0 ffffffffae094d40 [79817.227806] Call Trace: [79817.228039] dump_stack (lib/dump_stack.c:52) [79817.228503] dequeue_task_idle (kernel/sched/idle_task.c:45) [79817.229005] dequeue_task (kernel/sched/core.c:839) [79817.229485] do_set_cpus_allowed (kernel/sched/core.c:1178 (discriminator 9)) [79817.230006] init_idle (kernel/sched/core.c:4937) [79817.230467] idle_thread_get (kernel/smpboot.c:35) [79817.230945] ? cpu_up (kernel/cpu.c:569) [79817.231374] _cpu_up (kernel/cpu.c:513) [79817.231801] ? cpu_down (kernel/cpu.c:500) [79817.232254] ? fork_idle (kernel/fork.c:1688) [79817.232731] ? find_next_bit (lib/find_bit.c:65) [79817.233216] ? idle_threads_init (include/linux/cpumask.h:189 kernel/smpboot.c:71) [79817.233715] cpu_up (kernel/cpu.c:574) [79817.234132] smp_init (kernel/smp.c:578) [79817.234557] kernel_init_freeable (init/main.c:690 init/main.c:884 init/main.c:1009) [79817.235095] ? start_kernel (init/main.c:979) [79817.235575] ? finish_task_switch (kernel/sched/sched.h:1087 kernel/sched/core.c:2526) [79817.236106] ? rest_init (init/main.c:934) [79817.236567] kernel_init (init/main.c:939) [79817.237020] ? rest_init (init/main.c:934) [79817.237496] ret_from_fork (arch/x86/entry/entry_64.S:473) [79817.237994] ? rest_init (init/main.c:934) [79817.238477] BUG: unable to handle kernel NULL pointer dereference at (null) [79817.239170] IP: [< (null)>] null) (??:?) [79817.239608] PGD 0 [79817.239802] Oops: 0010 [#1] SMP DEBUG_PAGEALLOC KASAN [79817.240295] Dumping ftrace buffer: [79817.240593] (ftrace buffer empty) [79817.240899] Modules linked in: [79817.241180] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc6-next-20150813-sasha-00040-g059fd6d #2431 [79817.241957] task: ffff880278228000 ti: ffff880051b70000 task.ti: ffff880051b70000 [79817.242573] RIP: [< (null)>] null) (??:?) [79817.243203] RSP: 0000:ffff880051b77c18 EFLAGS: 00010046 [79817.243646] RAX: dffffc0000000000 RBX: ffff880051be8000 RCX: 0000000000000004 [79817.244229] RDX: 0000000000000000 RSI: ffff880051be8000 RDI: ffff8800a69e15c0 [79817.244815] RBP: ffff880051b77c48 R08: 0000000000000001 R09: 0000000000000004 [79817.245404] R10: ffffed00ff815e03 R11: ffffed00ff815e01 R12: ffff8800a69e15c0 [79817.245987] R13: ffffffffae0c6320 R14: 0000000000000000 R15: 00004897e6254646 [79817.246577] FS: 0000000000000000(0000) GS:ffff880052600000(0000) knlGS:0000000000000000 [79817.247243] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [79817.247723] CR2: 0000000000000000 CR3: 0000000030a2d000 CR4: 00000000000006b0 [79817.248319] Stack: [79817.248496] ffffffffa43a7d9b ffff880051be8000 ffff8800a69e15c0 ffffffffae0c6320 [79817.249176] ffff880051be8058 0000000000000001 ffff880051b77c88 ffffffffa43aab66 [79817.249850] ffff880051be8000 ffff8800a69e15c0 0000000000000001 0000000000000001 [79817.250521] Call Trace: [79817.250735] ? enqueue_task (kernel/sched/core.c:832) [79817.251211] do_set_cpus_allowed (kernel/sched/core.c:1189) [79817.251720] init_idle (kernel/sched/core.c:4937) [79817.252153] idle_thread_get (kernel/smpboot.c:35) [79817.252623] ? cpu_up (kernel/cpu.c:569) [79817.253045] _cpu_up (kernel/cpu.c:513) [79817.253471] ? cpu_down (kernel/cpu.c:500) [79817.253902] ? fork_idle (kernel/fork.c:1688) [79817.254354] ? find_next_bit (lib/find_bit.c:65) [79817.254820] ? idle_threads_init (include/linux/cpumask.h:189 kernel/smpboot.c:71) [79817.255326] cpu_up (kernel/cpu.c:574) [79817.255729] smp_init (kernel/smp.c:578) [79817.256158] kernel_init_freeable (init/main.c:690 init/main.c:884 init/main.c:1009) [79817.256675] ? start_kernel (init/main.c:979) [79817.257158] ? finish_task_switch (kernel/sched/sched.h:1087 kernel/sched/core.c:2526) [79817.257677] ? rest_init (init/main.c:934) [79817.258143] kernel_init (init/main.c:939) [79817.258604] ? rest_init (init/main.c:934) [79817.259094] ret_from_fork (arch/x86/entry/entry_64.S:473) [79817.259558] ? rest_init (init/main.c:934) [79817.260021] Code: Bad RIP value.
Code starting with the faulting instruction
===========================================
[79817.260342] RIP [< (null)>] null) (??:?) [79817.260791] RSP <ffff880051b77c18> [79817.261098] CR2: 0000000000000000 [79817.261415] ---[ end trace eddc979a4104e4f3 ]--- [79817.261818] Kernel panic - not syncing: Fatal exception
Thanks,
Sasha
Peter Zijlstra
maintainerRe: [tip:sched/core] sched: Change the sched_class::set_cpus_allowed( ) calling context
Quoted 4 lines · Sasha Levin
[79817.224383] bad: scheduling from the idle thread! [79817.224900] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc6-next-20150813-sasha-00040-g059fd6d #2431 [79817.225709] ffffffffae0c6320 ffff880051b77c00 ffffffffade7f739 ffff8800a69e15c0 [79817.226419] ffff880051b77c18 ffffffffa43c4ed3 ffff880051be8000 ffff880051b77c48
Quoted 8 lines
[79817.227122] ffffffffa43a7ffb ffff880051be8000 ffff8800a69e15c0 ffffffffae094d40 [79817.227806] Call Trace: [79817.228039] dump_stack (lib/dump_stack.c:52) [79817.228503] dequeue_task_idle (kernel/sched/idle_task.c:45) [79817.229005] dequeue_task (kernel/sched/core.c:839) [79817.229485] do_set_cpus_allowed (kernel/sched/core.c:1178 (discriminator 9)) [79817.230006] init_idle (kernel/sched/core.c:4937) [79817.230467] idle_thread_get (kernel/smpboot.c:35)
Dhurr.. so oddly enough my actual hardware boots without issue.
But I can see how that could go wrong, does the below make your (virtual) machine happy again?
--- kernel/sched/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/sched/core.c +1 -1
@@ -4924,7 +4924,7 @@ void init_idle(struct task_struct *idle, int cpu) idle->state =TASK_RUNNING ; idle->se.exec_start =sched_clock (); -do_set_cpus_allowed (idle,cpumask_of (cpu)); +set_cpus_allowed_common (idle,cpumask_of (cpu));/* * We're having a chicken and egg problem, even though we are * holding rq->lock, the cpu isn't yet set to this cpu so the
Re: [tip:sched/core] sched: Change the sched_class::set_cpus_allowed( ) calling context
Quoted 13 lines · Peter Zijlstra
Quoted 4 lines · Sasha Levin
Quoted 4 lines
[79817.224383] bad: scheduling from the idle thread! [79817.224900] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc6-next-20150813-sasha-00040-g059fd6d #2431 [79817.225709] ffffffffae0c6320 ffff880051b77c00 ffffffffade7f739 ffff8800a69e15c0 [79817.226419] ffff880051b77c18 ffffffffa43c4ed3 ffff880051be8000 ffff880051b77c48Quoted 8 lines
Quoted 8 lines
[79817.227122] ffffffffa43a7ffb ffff880051be8000 ffff8800a69e15c0 ffffffffae094d40 [79817.227806] Call Trace: [79817.228039] dump_stack (lib/dump_stack.c:52) [79817.228503] dequeue_task_idle (kernel/sched/idle_task.c:45) [79817.229005] dequeue_task (kernel/sched/core.c:839) [79817.229485] do_set_cpus_allowed (kernel/sched/core.c:1178 (discriminator 9)) [79817.230006] init_idle (kernel/sched/core.c:4937) [79817.230467] idle_thread_get (kernel/smpboot.c:35)
Quoted 4 lines
Dhurr.. so oddly enough my actual hardware boots without issue.
But I can see how that could go wrong, does the below make your (virtual) machine happy again?
Seems to work fine now, thanks!
Thanks,
Sasha
Re: [tip:sched/core] sched: Change the sched_class::set_cpus_allowed( ) calling context
Quoted 1 line · Sasha Levin
Seems to work fine now, thanks!
I've no clue how this doesn't also explode on actual hardware, that code is convoluted, read and weep.
Ingo, please stick somewhere appropriate :-)
---
Subject: sched: Avoid trying to dequeue/enqueue the idle thread
From: Peter Zijlstra <[email protected]>
Date: Thu Aug 13 23:09:29 CEST 2015
Sasha reports that his virtual machine tries to schedule the idle thread since commit 6c37067e2786 ("sched: Change the sched_class::set_cpus_allowed() calling context").
His trace shows this happening from idle_thread_get()->init_idle(), which is the _second_ init_idle() invocation on that task_struct, the first being done through idle_init()->fork_idle(). (this code is insane...)
Because we call init_idle() twice in a row, its ->sched_class == &idle_sched_class and ->on_rq = TASK_ON_RQ_QUEUED. This means do_set_cpus_allowed() thinks we're queued and will call dequeue_task(), which is implemented with BUG() for the idle class, seeing how dequeueing the idle task is a daft thing.
Aside of the whole insanity of calling init_idle() _twice_, change the code to call set_cpus_allowed_common() instead as this is 'obviously' before the idle task gets ran etc..
--- kernel/sched/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/sched/core.c +1 -1
@@ -4924,7 +4924,7 @@ void init_idle(struct task_struct *idle, idle->state =TASK_RUNNING ; idle->se.exec_start =sched_clock (); -do_set_cpus_allowed (idle,cpumask_of (cpu)); +set_cpus_allowed_common (idle,cpumask_of (cpu));/* * We're having a chicken and egg problem, even though we are * holding rq->lock, the cpu isn't yet set to this cpu so the
Peter Zijlstra
maintainerRFC PATCH 4/4 sched, numa: Ignore pinned tasks
In reply to Peter ZijlstraPer cpu (kernel) threads can currently trick the load balancer in thinking there's something to do and result in moving perfectly placed tasks away.
By virtue of the new do_set_cpus_allowed() we can easily add nr_pinned accounting. Which we'll use to change the fbq classification such that we make it less likely to pick such tasks for migration.
Note that it is still possible we'll migrate these well placed tasks away, further patches could reduce this still.
--- kernel/sched/fair.c | 42 ++++++++++++++++++++++++++++++++++-------- kernel/sched/sched.h | 1 + 2 files changed, 35 insertions(+), 8 deletions(-)
kernel/sched/fair.c +34 -8
@@ -858,12 +858,20 @@ static unsigned int task_scan_max(structstatic void account_numa_enqueue (struct rq *rq,struct task_struct *p) { +if (p->nr_cpus_allowed ==1 ) { + p->numa_preferred_nid =-1 ; + rq->nr_pinned_running++; + } rq->nr_numa_running += (p->numa_preferred_nid !=-1 ); rq->nr_preferred_running += (p->numa_preferred_nid ==task_node (p)); }static void account_numa_dequeue (struct rq *rq,struct task_struct *p) { +if (p->nr_cpus_allowed ==1 ) { + rq->nr_pinned_running--; +WARN_ON_ONCE (p->numa_preferred_nid !=-1 ); + } rq->nr_numa_running -= (p->numa_preferred_nid !=-1 ); rq->nr_preferred_running -= (p->numa_preferred_nid ==task_node (p)); }
@@ -2266,6 +2274,12 @@ void task_tick_numa(struct rq *rq, strucreturn ;/* +* We don't care about NUMA placement if we can't migrate the task. +*/ +if (curr->nr_cpus_allowed ==1 ) +return ; + +/* * Using runtime rather than walltime has the dual advantage that * we (mostly) drive the selection from busy threads and that the * task needs to have done some actual work before we bother with
@@ -5567,7 +5581,7 @@ static bool yield_to_task_fair(struct rqstatic unsigned long __read_mostly max_load_balance_interval =HZ /10 ; -enum fbq_type { regular, remote, all }; +enum fbq_type { regular, remote, movable, all };#define LBF_ALL_PINNED 0x01#define LBF_NEED_BREAK 0x02
@@ -6112,6 +6126,7 @@ struct sg_lb_stats {enum group_type group_type;int group_no_capacity;#ifdef CONFIG_NUMA_BALANCING +unsigned int nr_pinned_running;unsigned int nr_numa_running;unsigned int nr_preferred_running;#endif
@@ -6449,6 +6464,7 @@ static inline void update_sg_lb_stats(st *overload = true;#ifdef CONFIG_NUMA_BALANCING + sgs->nr_pinned_running += rq->nr_pinned_running; sgs->nr_numa_running += rq->nr_numa_running; sgs->nr_preferred_running += rq->nr_preferred_running;#endif
@@ -6522,19 +6538,27 @@ static bool update_sd_pick_busiest(struc#ifdef CONFIG_NUMA_BALANCING static inline enum fbq_type fbq_classify_group (struct sg_lb_stats *sgs) { -if (sgs->sum_nr_running > sgs->nr_numa_running) +unsigned int nr_migratable = sgs->sum_nr_running - sgs->nr_pinned_running; + +if (nr_migratable > sgs->nr_numa_running)return regular; -if (sgs->sum_nr_running > sgs->nr_preferred_running) +if (nr_migratable > sgs->nr_preferred_running)return remote; +if (nr_migratable) +return movable;return all; }static inline enum fbq_type fbq_classify_rq (struct rq *rq) { -if (rq->nr_running > rq->nr_numa_running) +unsigned int nr_migratable = rq->cfs.h_nr_running - rq->nr_pinned_running; + +if (nr_migratable > rq->nr_numa_running)return regular; -if (rq->nr_running > rq->nr_preferred_running) +if (nr_migratable > rq->nr_preferred_running)return remote; +if (nr_migratable) +return movable;return all; }#else
@@ -6938,9 +6962,11 @@ static struct rq *find_busiest_queue(str/* * We classify groups/runqueues into three groups: -* - regular: there are !numa tasks -* - remote: there are numa tasks that run on the 'wrong' node -* - all: there is no distinction +* - regular: there are (migratable) !numa tasks +* - remote: there are (migratable) numa tasks that +* run on the 'wrong' node +* - movable: there are (migratable) tasks +* - all: there are tasks * * In order to avoid migrating ideally placed numa tasks, * ignore those when there's better options.
kernel/sched/sched.h +1 -0
@@ -571,6 +571,7 @@ struct rq {#ifdef CONFIG_NUMA_BALANCING unsigned int nr_numa_running;unsigned int nr_preferred_running; +unsigned int nr_pinned_running;#endif #define CPU_LOAD_IDX_MAX 5unsigned long cpu_load[CPU_LOAD_IDX_MAX ];
Re: [RFC][PATCH 4/4] sched, numa: Ignore pinned tasks
Quoted 10 lines · Peter Zijlstra
Per cpu (kernel) threads can currently trick the load balancer in thinking there's something to do and result in moving perfectly placed tasks away.
By virtue of the new
do_set_cpus_allowed()we can easily addnr_pinnedaccounting. Which we'll use to change the fbq classification such that we make it less likely to pick such tasks for migration.Note that it is still possible we'll migrate these well placed tasks away, further patches could reduce this still.
A few ideas here :)
- Identify whether the current task on a CPU is the one that would like to migrate, and treat the runqueue as "movable" instead of "remote" if none of the runnable but queued tasks want to migrate away.
- Teach can_migrate_task about the fbq types, and have it enforce NUMA placement when the type is "remote", and ignore NUMA placement when we know there are no good tasks to move. This might require another simplification of migrate_improves/degrades_locality.
Quoted 2 lines
Suggested-by Rik van Riel <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signature · 1 line
All rights reversed
Peter Zijlstra
maintainerRe: [RFC][PATCH 4/4] sched, numa: Ignore pinned tasks
In reply to Peter ZijlstraQuoted 4 lines · Peter Zijlstra
static void
account_numa_enqueue(struct rq *rq, struct task_struct *p){kernel/sched/fair.c +2
-0+if (p->nr_cpus_allowed ==1 ) { + p->numa_preferred_nid =-1 ;
kernel/sched/fair.c static unsigned int task_scan_max(struct
Quoted 10 lines
kernel/sched/fair.c +4
-0+ rq->nr_pinned_running++; + } rq->nr_numa_running += (p->numa_preferred_nid !=-1 ); rq->nr_preferred_running += (p->numa_preferred_nid ==task_node (p)); }static inline enum fbq_type fbq_classify_rq (struct rq *rq) { +unsigned int nr_migratable = rq->cfs.h_nr_running - rq->nr_pinned_running; +
FWIW, there's a problem there with CFS bandwidth muck. When we throttle groups we update cfs.h_nr_running properly, but we do not hierarchically account the pinned, preferred and numa counts.
So that above subtraction can end up negative.
I've not yet decided what to do about this; ideally we'd do the hierarchical accounting of the numa stats -- but that's a little bit more expensive than I'd like.
A well. for monday that.
Re: [RFC][PATCH 4/4] sched, numa: Ignore pinned tasks
In reply to Peter Zijlstrakernel/sched/fair.c static unsigned int task_scan_max(struct · line 877
Quoted 10 lines
static void
account_numa_dequeue(struct rq *rq, struct task_struct *p){kernel/sched/fair.c +4
-0+if (p->nr_cpus_allowed ==1 ) { + rq->nr_pinned_running--; +WARN_ON_ONCE (p->numa_preferred_nid !=-1 ); + } rq->nr_numa_running -= (p->numa_preferred_nid !=-1 ); rq->nr_preferred_running -= (p->numa_preferred_nid ==task_node (p)); }
Shouldnt we reset p->numa_preferred_nid when we are setting the allowed cpus in set_cpus_allowed_common()?
Otherwise if an process is set a preferred node based on its numa faults but then is pinned to a different cpu, then we can see this warning.:w!
Signature · 2 lines
Thanks and Regards Srikar Dronamraju
Peter Zijlstra
maintainerRe: [RFC][PATCH 4/4] sched, numa: Ignore pinned tasks
Quoted 15 lines · Srikar Dronamraju
kernel/sched/fair.c static unsigned int task_scan_max(struct · line 877
Quoted 10 lines
static void
account_numa_dequeue(struct rq *rq, struct task_struct *p){kernel/sched/fair.c +4
-0+if (p->nr_cpus_allowed ==1 ) { + rq->nr_pinned_running--; +WARN_ON_ONCE (p->numa_preferred_nid !=-1 ); + } rq->nr_numa_running -= (p->numa_preferred_nid !=-1 ); rq->nr_preferred_running -= (p->numa_preferred_nid ==task_node (p)); }Shouldnt we reset
p->numa_preferred_nidwhen we are setting the allowed cpus inset_cpus_allowed_common()?
Quoted 2 lines
Otherwise if an process is set a preferred node based on its numa faults but then is pinned to a different cpu, then we can see this warning.:w!
We should never get preferred_nid set when nr_cpus_allowed == 1, see the hunk that changes task_tick_numa.
So we set preferred = -1 on pinning, do not partake in numa balancing while this is so, therefore it should still be so when we dequeue, right?
Re: [RFC][PATCH 4/4] sched, numa: Ignore pinned tasks
Quoted 19 lines · Peter Zijlstra
Quoted 15 lines · Srikar Dronamraju
kernel/sched/fair.c static unsigned int task_scan_max(struct · line 877
Quoted 10 lines
static void
account_numa_dequeue(struct rq *rq, struct task_struct *p){kernel/sched/fair.c +4
-0+if (p->nr_cpus_allowed ==1 ) { + rq->nr_pinned_running--; +WARN_ON_ONCE (p->numa_preferred_nid !=-1 ); + } rq->nr_numa_running -= (p->numa_preferred_nid !=-1 ); rq->nr_preferred_running -= (p->numa_preferred_nid ==task_node (p)); }Shouldnt we reset
p->numa_preferred_nidwhen we are setting the allowed cpus inset_cpus_allowed_common()?Quoted 2 lines
Otherwise if an process is set a preferred node based on its numa faults but then is pinned to a different cpu, then we can see this warning.:w!
Quoted 6 lines
We should never get
preferred_nidset whennr_cpus_allowed== 1, see the hunk that changestask_tick_numa.So we set preferred = -1 on pinning, do not partake in numa balancing while this is so, therefore it should still be so when we dequeue, right?
It could be pinned after it has been running for a while, with taskset -c <cpu> -p <pid>
Signature · 1 line
All rights reversed
Re: [RFC][PATCH 4/4] sched, numa: Ignore pinned tasks
In reply to Peter ZijlstraQuoted 19 lines · Peter Zijlstra
Quoted 15 lines · Srikar Dronamraju
kernel/sched/fair.c static unsigned int task_scan_max(struct · line 877
Quoted 10 lines
static void
account_numa_dequeue(struct rq *rq, struct task_struct *p){kernel/sched/fair.c +4
-0+if (p->nr_cpus_allowed ==1 ) { + rq->nr_pinned_running--; +WARN_ON_ONCE (p->numa_preferred_nid !=-1 ); + } rq->nr_numa_running -= (p->numa_preferred_nid !=-1 ); rq->nr_preferred_running -= (p->numa_preferred_nid ==task_node (p)); }Shouldnt we reset
p->numa_preferred_nidwhen we are setting the allowed cpus inset_cpus_allowed_common()?Quoted 2 lines
Otherwise if an process is set a preferred node based on its numa faults but then is pinned to a different cpu, then we can see this warning.:w!
Quoted 6 lines
We should never get
preferred_nidset whennr_cpus_allowed== 1, see the hunk that changestask_tick_numa.So we set preferred = -1 on pinning, do not partake in numa balancing while this is so, therefore it should still be so when we dequeue, right?
lets say if a thread were to do a sched_setaffinity on itself ; would it not call account_numa_dequeue before account_numa_enqueue?
Also setting preferred = -1 in set_cpus_allowed avoids us from setting it at account_numa_enqueue. account_numa_enqueue() would probably be called more times than set_cpus_allowed.
Quoted 7 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 2 lines
Thanks and Regards Srikar Dronamraju
Peter Zijlstra
maintainerRe: [RFC][PATCH 4/4] sched, numa: Ignore pinned tasks
Quoted 9 lines · Srikar Dronamraju
Quoted 6 lines
We should never get
preferred_nidset whennr_cpus_allowed== 1, see the hunk that changestask_tick_numa.So we set preferred = -1 on pinning, do not partake in numa balancing while this is so, therefore it should still be so when we dequeue, right?
lets say if a thread were to do a
sched_setaffinityon itself ; would it not callaccount_numa_dequeuebeforeaccount_numa_enqueue?
Yes, but it would call dequeue while nr_cpus_allowed was still the 'old' value. And it will call enqueue when its the 'new' value.
Quoted 3 lines
Also setting preferred = -1 in
set_cpus_allowedavoids us from setting it ataccount_numa_enqueue.account_numa_enqueue()would probably be called more times thanset_cpus_allowed.
This is true; and that makes sense.
Re: [RFC][PATCH 4/4] sched, numa: Ignore pinned tasks
In reply to Peter Zijlstrakernel/sched/fair.c static struct rq *find_busiest_queue(str · line 6964
Quoted 3 lines
kernel/sched/fair.c +0
-0/* * We classify groups/runqueues into three groups:
Nit: Should we update the above from "three groups" to "four groups"?
Quoted 11 lines
kernel/sched/fair.c +5
-3-* - regular: there are !numa tasks -* - remote: there are numa tasks that run on the 'wrong' node -* - all: there is no distinction +* - regular: there are (migratable) !numa tasks +* - remote: there are (migratable) numa tasks that +* run on the 'wrong' node +* - movable: there are (migratable) tasks +* - all: there are tasks * * In order to avoid migrating ideally placed numa tasks, * ignore those when there's better options.
Signature · 2 lines
Thanks and Regards Srikar Dronamraju
Quoted 8 lines · Peter Zijlstra
Hi,
Here's a first few patches that provide pinned task accounting; they are boot tested only so far.
I don't think this is enough to address Artem's regression, but its a foundation to add some more smarts. In particular we should make it harder still to migrate well placed tasks away due to these pinned tasks.
Hi,
the most useful thing I can do is to test your patches, which I did, and there seem to be no difference: average server response time is still 1.4 seconds.
Artem.