Re: [RFC/PATCH 0/4] CPUSET driven CPU isolation

David Rientjes
2008-02-29T04:01:36Z

On Thu, 28 Feb 2008, Paul Jackson wrote:

> Apparently, you thought I was trying to reject the patch on the grounds
> that no such problem could ever occur, and you were showing how such a
> problem could occur. I wasn't trying to reject the patch, and I agree
> that the check made sense, and I agree that such a problem could occur,
> as your example shows.
>

Ok, good. We're in agreement that my patch fixes a problem in allowing
kthreads to move to cpusets that either currently or eventually deny them
access to the cpu to which they are bound.

Now do you have any preference without extensive cpusets or cgroups
hacking where we can gracefully handle -EINVAL being returned from
set_cpus_allowed() so the task doesn't end up in a different cpuset? The
return value of set_cpus_allowed() is currently ignored by the cpuset
implementation and, regardless, cpuset_attach() returns void.

cpuset_can_attach() does some sanity checking, but we need a call to
set_cpus_allowed() to check the new logic and will actually change
tsk->cpus_allowed if correctly invoked.

So the cpusets implementation needs a similar check in cpuset_can_attach()
so that it is expressly denied from moving before we ever call
cpuset_attach(). It doesn't make sense that a kthread is going to move
itself through the cpusets interface, so this can simply be done by
checking for tsk->flags & PF_THREAD_BOUND.

sched: prevent bound kthreads from changing cpus_allowed

Kthreads that have called kthread_bind() are bound to specific cpus, so
other tasks should not be able to change their cpus_allowed from under
them. Otherwise, it is possible to move kthreads, such as the migration
or watchdog threads, so they are not allowed access to the cpu they work
on.

Signed-off-by: David Rientjes <rientjes@google.com>
---
include/linux/sched.h | 1 +
kernel/cpuset.c | 2 ++
kernel/kthread.c | 1 +
kernel/sched.c | 6 ++++++
4 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1464,6 +1464,7 @@ static inline void put_task_struct(struct task_struct *t)
#define PF_SWAPWRITE 0x00800000 /* Allowed to write to swap */
#define PF_SPREAD_PAGE 0x01000000 /* Spread page cache over cpuset */
#define PF_SPREAD_SLAB 0x02000000 /* Spread some slab caches over cpuset */
+#define PF_THREAD_BOUND 0x04000000 /* Thread bound to specific cpu */
#define PF_MEMPOLICY 0x10000000 /* Non-default NUMA mempolicy */
#define PF_MUTEX_TESTER 0x20000000 /* Thread belongs to the rt mutex tester */
#define PF_FREEZER_SKIP 0x40000000 /* Freezer should not count it as freezeable */
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1162,6 +1162,8 @@ static int cpuset_can_attach(struct cgroup_subsys *ss,

if (cpus_empty(cs->cpus_allowed) || nodes_empty(cs->mems_allowed))
return -ENOSPC;
+ if (tsk->flags & PF_THREAD_BOUND)
+ return -EINVAL;

return security_task_setscheduler(tsk, 0, NULL);
}
diff --git a/kernel/kthread.c b/kernel/kthread.c
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -180,6 +180,7 @@ void kthread_bind(struct task_struct *k, unsigned int cpu)
wait_task_inactive(k);
set_task_cpu(k, cpu);
k->cpus_allowed = cpumask_of_cpu(cpu);
+ k->flags |= PF_THREAD_BOUND;
}
EXPORT_SYMBOL(kthread_bind);

diff --git a/kernel/sched.c b/kernel/sched.c
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -5345,6 +5345,12 @@ int set_cpus_allowed(struct task_struct *p, cpumask_t new_mask)
goto out;
}

+ if (unlikely((p->flags & PF_THREAD_BOUND) && p != current &&
+ !cpus_equal(p->cpus_allowed, new_mask))) {
+ ret = -EINVAL;
+ goto out;
+ }
+
if (p->sched_class->set_cpus_allowed)
p->sched_class->set_cpus_allowed(p, &new_mask);
else {