Re: [PATCH] hotplug cpu: move tasks in empty cpusets to parent

David Rientjes
2007-09-21T23:15:53Z

On Fri, 21 Sep 2007, Cliff Wickman wrote:

> This patch corrects a situation that occurs when one disables all the cpus
> in a cpuset.
>
> Currently, the disabled (cpu-less) cpuset inherits the cpus of its parent,
> which may overlap its exclusive sibling.
> (You will get non-removable cpusets -- "Invalid argument")
>
> Tasks of an empty cpuset should be moved to the cpuset which is the parent
> of their current cpuset. Or if the parent cpuset has no cpus, to its
> parent, etc.
>
> And the empty cpuset should be removed (if it is flagged notify_on_release).
>

Again, being flagged notify_on_release does not remove the empty cpuset,
it simply calls a userspace agent to do cleanup, if such a userspace agent
exists and notify_on_release is enabled.

Inline comments below.

> Index: linus.070921/kernel/cpuset.c
> ===================================================================
> --- linus.070921.orig/kernel/cpuset.c
> +++ linus.070921/kernel/cpuset.c
> @@ -52,6 +52,8 @@
> #include <asm/uaccess.h>
> #include <asm/atomic.h>
> #include <linux/mutex.h>
> +#include <linux/kfifo.h>
> +#include <linux/workqueue.h>
>
> #define CPUSET_SUPER_MAGIC 0x27e0eb
>
> @@ -109,6 +111,7 @@ typedef enum {
> CS_NOTIFY_ON_RELEASE,
> CS_SPREAD_PAGE,
> CS_SPREAD_SLAB,
> + CS_RELEASED_RESOURCE,
> } cpuset_flagbits_t;
>
> /* convenient tests for these bits */
> @@ -147,6 +150,11 @@ static inline int is_spread_slab(const s
> return test_bit(CS_SPREAD_SLAB, &cs->flags);
> }
>
> +static inline int has_released_a_resource(const struct cpuset *cs)
> +{
> + return test_bit(CS_RELEASED_RESOURCE, &cs->flags);
> +}
> +
> /*
> * Increment this integer everytime any cpuset changes its
> * mems_allowed value. Users of cpusets can track this generation
> @@ -541,7 +549,7 @@ static void cpuset_release_agent(const c
> static void check_for_release(struct cpuset *cs, char **ppathbuf)
> {
> if (notify_on_release(cs) && atomic_read(&cs->count) == 0 &&
> - list_empty(&cs->children)) {
> + list_empty(&cs->children)) {
> char *buf;
>
> buf = kmalloc(PAGE_SIZE, GFP_KERNEL);

Unnecessary change.

> @@ -1265,6 +1273,7 @@ static int attach_task(struct cpuset *cs
>
> from = oldcs->mems_allowed;
> to = cs->mems_allowed;
> + set_bit(CS_RELEASED_RESOURCE, &oldcs->flags);
>
> mutex_unlock(&callback_mutex);
>
> @@ -1995,6 +2004,7 @@ static int cpuset_rmdir(struct inode *un
> cpuset_d_remove_dir(d);
> dput(d);
> number_of_cpusets--;
> + set_bit(CS_RELEASED_RESOURCE, &parent->flags);
> mutex_unlock(&callback_mutex);
> if (list_empty(&parent->children))
> check_for_release(parent, &pathbuf);
> @@ -2062,50 +2072,180 @@ out:
> }
>
> /*
> + * Move every task that is a member of cpuset "from" to cpuset "to".
> + *
> + * Called with both manage_sem and callback_sem held
> + */
> +static void move_member_tasks_to_cpuset(struct cpuset *from, struct cpuset *to)
> +{
> + int moved=0;
> + struct task_struct *g, *tsk;
> +
> + read_lock(&tasklist_lock);
> + do_each_thread(g, tsk) {
> + if (tsk->cpuset == from) {
> + moved++;
> + task_lock(tsk);
> + tsk->cpuset = to;
> + task_unlock(tsk);
> + }
> + } while_each_thread(g, tsk);
> + read_unlock(&tasklist_lock);
> + atomic_add(moved, &to->count);
> + atomic_set(&from->count, 0);
> +}
> +

This isn't that simple. You're missing mpol_rebind_mm() checks, updating
tsk->mems_allowed, etc. It's much easier to make
remove_tasks_in_empty_cpuset() a client of attach_task() by supplying
pids; this would make it the only function where cpuset assignment
changes.