OpenVZ Forum - RDF feed
type: channel
items: [RFC] cpuset update_cgroup_cpus_allowed
items: Re: [RFC] cpuset update_cgroup_cpus_allowed

[RFC] cpuset update_cgroup_cpus_allowed
type: item
description: Paul M, David R, others -- how does this look? From: Paul Jackson <pj@sgi.com> Update the per-task cpus_allowed of each task in a cgroup whenever it has a cpuset whose 'cpus' mask changes. The change to basing cpusets on the cgroup (aka container) infrastructure broke an essential cpuset hack. The old cpuset code had used the act of reattaching a task to its own cpuset (writing its pid back into the same 'tasks' file it was already in) to trigger the code that updates the cpus_allowed cpumask in the task struct to the cpus_allowed cpumask dictated by that tasks cpuset. This was a hack to avoid having code in the main scheduler code path that checked for changes in the cpus_allowed by each tasks cpuset, which would have unacceptable performance impact on the scheduler. The cgroup code avoids calling the update callout if a task is reattached to the cgroup it is already attached to do. This turned reattaching a task to its own cpuset into a no-op, making it impossible to change a tasks CPU placement by changing the cpus_allowed of the cpuset containing that task. The right thing to do would be to have the code that updates a cpusets cpus_allowed walk through each task currently in that cpuset and update the cpus_allowed in that tasks task_struct. This change does that, adding code called from cpuset update_cpumask() that updates the task_struct cpus_allowed of each task in a cgroup whenever it has a cpuset whose 'cpus' is changed. Signed-off-by: Paul Jackson <pj@sgi.com> --- This patch applies anywhere after: cpusets-decrustify-cpuset-mask-update-code.patch Documentation/cpusets.txt | 23 +++++---------- kernel/cpuset.c | 68 +++++++++++++++++++++++++++++++++++++++++----- kernel/sched.c | 3 ++ mm/pdflush.c | 3 ++ 4 files changed, 76 insertions(+), 21 deletions(-) --- 2.6.23-mm1.orig/kernel/cpuset.c 2007-10-14 22:24:56.268309633 -0700 +++ 2.6.23-mm1/kernel/cpuset.c 2007-10-14 22:34:52.645364388 -0700 @@ -677,6 +677,64 @@ done: } /* + * update_cgroup_cpus_allowed(cont, cp…
creator: Paul Jackson
date: 2007-10-15T07:11:15-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: On Mon, 15 Oct 2007, Paul Jackson wrote: > --- 2.6.23-mm1.orig/kernel/cpuset.c 2007-10-14 22:24:56.268309633 -0700 > +++ 2.6.23-mm1/kernel/cpuset.c 2007-10-14 22:34:52.645364388 -0700 > @@ -677,6 +677,64 @@ done: > } > > /* > + * update_cgroup_cpus_allowed(cont, cpus) > + * > + * Keep looping over the tasks in cgroup 'cont', up to 'ntasks' > + * tasks at a time, setting each task->cpus_allowed to 'cpus', > + * until all tasks in the cgroup have that cpus_allowed setting. > + * > + * The 'set_cpus_allowed()' call cannot be made while holding the > + * css_set_lock lock embedded in the cgroup_iter_* calls, so we stash > + * some task pointers, in the tasks[] array on the stack, then drop > + * that lock (cgroup_iter_end) before looping over the stashed tasks > + * to update their cpus_allowed fields. > + * > + * Making the const 'ntasks' larger would use more stack space (bad), > + * and reduce the number of cgroup_iter_start/cgroup_iter_end calls > + * (good). But perhaps more importantly, it could allow any bugs > + * lurking in the 'need_repeat' looping logic to remain hidden longer. > + * So keep ntasks rather small, to ensure any bugs in this loop logic > + * are exposed quickly. > + */ > +static void update_cgroup_cpus_allowed(struct cgroup *cont, cpumask_t *cpus) > +{ > + int need_repeat = true; > + > + while (need_repeat) { > + struct cgroup_iter it; > + const int ntasks = 10; > + struct task_struct *tasks[ntasks]; > + struct task_struct **p, **q; > + > + need_repeat = false; > + p = tasks; > + > + cgroup_iter_start(cont, &it); > + while (1) { > + struct task_struct *t; > + > + t = cgroup_iter_next(cont, &it); > + if (!t) > + break; > + if (cpus_equal(*cpus, t->cpus_allowed)) > + continue; By making this cpus_equal() and not cpus_intersects(), you're trying to make sure that t->cpus_allowed is always equal to *cpus for each task in the iterator. > + if (p == tasks + ntasks) { > + need_repeat = true; > + break; > + } > + get_task_struct(t); > + *p++ = t; > + } …
creator: David Rientjes
date: 2007-10-15T18:49:02-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: Paul Jackson wrote: > Paul M, David R, others -- how does this look? > Looks plausible, although as David comments I don't think it handles a concurrent CPU hotplug/unplug. Also I don't like the idea of doing a cgroup_lock() across sched_setaffinity() - cgroup_lock() can be held for relatively long periods of time. Here's an alternative for consideration, below. The main differences are: - currently against an older kernel with pre-cgroup cpusets, so it uses tasklist_lock and do_each_thread(); a cgroup version would use cgroup iterators as yours does - solves the race between sched_setaffinity() and update_cpumask() by having sched_setaffinity() check for changes to cpuset_cpus_allowed() after doing set_cpus_allowed() - guarantees to only act on each process once (so guarantees forward progress, in the absence of fork bombs. (And could be adapted to handle fork bombs too) - uses a priority heap to pick the processes to act on, based on start time - uses lock_cpu_hotplug() to avoid races with CPU hotplug; sadly I think this is gone in more recent kernels, so some other synchronization would be needed Paul > From: Paul Jackson <pj@sgi.com> > > Update the per-task cpus_allowed of each task in a cgroup > whenever it has a cpuset whose 'cpus' mask changes. > > The change to basing cpusets on the cgroup (aka container) > infrastructure broke an essential cpuset hack. The old cpuset > code had used the act of reattaching a task to its own cpuset > (writing its pid back into the same 'tasks' file it was already > in) to trigger the code that updates the cpus_allowed cpumask > in the task struct to the cpus_allowed cpumask dictated by that > tasks cpuset. > > This was a hack to avoid having code in the main scheduler > code path that checked for changes in the cpus_allowed by each > tasks cpuset, which would have unacceptable performance impact > on the scheduler. > > The cgroup code avoids calling the update callout if a task > is reattached to the cgroup it is already atta…
creator: Paul Menage
date: 2007-10-15T21:24:21-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: Paul M wrote: > Here's an alternative for consideration, below. I don't see the alternative -- I just see my patch, with the added blurbage: #12 - /usr/local/google/home/menage/kernel9/linux/kernel/cpuset.c ==== # action=edit type=text Should I be increasing my caffeine intake? -- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.925.600.0401 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Jackson
date: 2007-10-16T00:16:36-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: Paul Jackson wrote: > Paul M wrote: >> Here's an alternative for consideration, below. > > I don't see the alternative -- I just see my patch, with the added > blurbage: > > #12 - /usr/local/google/home/menage/kernel9/linux/kernel/cpuset.c ==== > # action=edit type=text > > Should I be increasing my caffeine intake? > Bah. Trying again: Here's an alternative for consideration, below. The main differences are: - currently against an older kernel with pre-cgroup cpusets, so it uses tasklist_lock and do_each_thread(); a cgroup version would use cgroup iterators as yours does - solves the race between sched_setaffinity() and update_cpumask() by having sched_setaffinity() check for changes to cpuset_cpus_allowed() after doing set_cpus_allowed() - guarantees to only act on each process once (so guarantees forward progress, in the absence of fork bombs. (And could be adapted to handle fork bombs too) - uses a priority heap to pick the processes to act on, based on start time - uses lock_cpu_hotplug() to avoid races with CPU hotplug; sadly I think this is gone in more recent kernels, so some other synchronization would be needed Cause writes to cpuset "cpus" file to update cpus_allowed for member tasks: - collect batches of tasks under tasklist_lock and then call set_cpus_allowed() on them outside the lock (since this can sleep). - add a simple generic priority heap type to allow efficient collection of batches of tasks to be processed without duplicating or missing any tasks in subsequent batches. - avoid races with hotplug events via lock_cpu_hotplug() - make "cpus" file update a no-op if the mask hasn't changed - fix race between update_cpumask() and sched_setaffinity() by making sched_setaffinity() to post-check that it's not running on any cpus outside cpuset_cpus_allowed(). include/linux/prio_heap.h | 56 +++++++++++++++++++++++++ kernel/cpuset.c | 103 ++++++++++++++++++++++++++++++++++++++++++++-- kernel/sched.c | 13 +++++ lib/Makefile | 2 lib/prio_heap.c | 68 +++++++…
creator: Paul Menage
date: 2007-10-16T00:20:15-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: > Yet by not doing any locking here to prevent a cpu from being > hot-unplugged, you can race and allow the hot-unplug event to happen > before calling set_cpus_allowed(). That makes this entire function a > no-op with set_cpus_allowed() returning -EINVAL for every call, which > isn't caught, and no error is reported to userspace. Good point ... hmmm ... -- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.925.600.0401 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Jackson
date: 2007-10-16T02:32:59-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: > currently against an older kernel ah .. which older kernel? I tried it against the broken out 2.6.23-rc8-mm2 patch set, inserting it before the task-containersv11-* patches, but that blew up on me - three rejected hunks. Any chance of getting this against a current cgroup (aka container) kernel? Could you use the diff --show-c-function option when composing patches - they're easier to read that way - thanks. + if (!retval) { + cpus_allowed = cpuset_cpus_allowed(p); + if (!cpus_subset(new_mask, cpus_allowed)) { + /* + * We must have raced with a concurrent cpuset + * update. Just reset the cpus_allowed to the + * cpuset's cpus_allowed + */ + new_mask = cpus_allowed; This narrows the race, perhaps sufficiently, but I don't see that it guarantees closure. Memory accesses to two different locations are not guaranteed to be ordered across nodes, as best I recall. The second line above, that rereads the cpuset cpus_allowed, could get an old value, in essence. cpuset update task sched_setaffinity task ------------------ ---------------------- A. write cpuset [Q] V. read cpuset [Q] B. read task [P] W. check ok C. write task [P] X. write task [P] Y. reread cpuset [Q] Z. check ok again Two memory locations: [P] the cpus_allowed mask in the task_struct of the task doing the sched_setaffinity call. [Q] the cpus_allowed mask in the cpuset of the cpuset to which the sched_setaffinity task is attached. Even though, from the perspective of location [P], both B. and C. happened before X., still from the perspective of location [Q] the rereading in Y. could return the value the cpuset cpus_allowed had before the write in A. This could result in a task running with a cpus_allowed that was totally outside its cpusets cpus_allowed. I will grant that this is a narrow window. I won't loose much sleep over it. > - uses a priority heap to pick the processes to act on, based on start time This adds a fair bit of code and complexity, relative to my patch. This I do loose more sleep over. Th…
creator: Paul Jackson
date: 2007-10-16T02:34:39-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: On 10/15/07, Paul Jackson <pj@sgi.com> wrote: > > currently against an older kernel > > ah .. which older kernel? 2.6.18, but I can do a version against 2.6.23-mm1. > + if (!retval) { > + cpus_allowed = cpuset_cpus_allowed(p); > + if (!cpus_subset(new_mask, cpus_allowed)) { > + /* > + * We must have raced with a concurrent cpuset > + * update. Just reset the cpus_allowed to the > + * cpuset's cpus_allowed > + */ > + new_mask = cpus_allowed; > > This narrows the race, perhaps sufficiently, but I don't see that it > guarantees closure. Memory accesses to two different locations are not > guaranteed to be ordered across nodes, as best I recall. The second > line above, that rereads the cpuset cpus_allowed, could get an old > value, in essence. > > cpuset update task sched_setaffinity task > ------------------ ---------------------- > > A. write cpuset [Q] V. read cpuset [Q] > B. read task [P] W. check ok > C. write task [P] X. write task [P] > Y. reread cpuset [Q] > Z. check ok again > > Two memory locations: > [P] the cpus_allowed mask in the task_struct of the > task doing the sched_setaffinity call. > [Q] the cpus_allowed mask in the cpuset of the cpuset > to which the sched_setaffinity task is attached. > > Even though, from the perspective of location [P], both B. and C. > happened before X., still from the perspective of location [Q] the > rereading in Y. could return the value the cpuset cpus_allowed had > before the write in A. This could result in a task running with > a cpus_allowed that was totally outside its cpusets cpus_allowed. But cpuset_cpus_allowed() synchronizes on callback_mutex. So I assert this race isn't an issue. > > I will grant that this is a narrow window. I won't loose much sleep > over it. > > > - uses a priority heap to pick the processes to act on, based on start time > > This adds a fair bit of code and complexity, relative to my patch. > This I do loose more sleep over. There has to be a compelling > reason for doing this. My plan was t…
creator: Paul Menage
date: 2007-10-16T05:12:32-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: > Will do - I justed wanted to get this quickly out to show the idea > that I was working on. Ok - good. In the final analysis, I'll take whatever works ;). I'll lobby for keeping the code "simple" (a subjective metric) and poke what holes I can in things, and propose what alternatives I can muster. But so long as setting a cpusets 'cpus' in 2.6.24 leads, whether by my historical "rewrite the pid to its own 'tasks' file" hack, or by a proper solution such as you have advocated, or by some other scheme or hack, to updating the cpus_allowed of each task in that cpuset, then I'm ok. Right now, that goal is not met, with the cgroup patches lined up in *-mm for what will become 2.6.24. We're getting short of time to fix this. -- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.925.600.0401 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Jackson
date: 2007-10-16T05:20:17-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: > > + if (cpus_equal(*cpus, t->cpus_allowed)) > > + continue; > > ... > > + for (q = tasks; q < p; q++) { > > + set_cpus_allowed(*q, *cpus); > > + put_task_struct(*q); > > + } > > + } > > +} > > Yet by not doing any locking here to prevent a cpu from being > hot-unplugged, you can race and allow the hot-unplug event to happen > before calling set_cpus_allowed(). That makes this entire function a > no-op with set_cpus_allowed() returning -EINVAL for every call, which > isn't caught, and no error is reported to userspace. > > Now all the tasks in the cpuset have an inconsistent state with respect to > their p->cpuset->cpus_allowed, because that was already updated in > update_cpumask(). My solution may be worse than that. Because set_cpus_allowed() will fail if asked to set a non-overlapping cpumask, my solution could never terminate. If asked to set a cpusets cpus to something that went off line right then, this I'd guess this code could keep looping forever, looking for cpumasks that didn't match, and then not noticing that it was failing to set them so as they would match. ... it needs work ... or the alternative solution from Paul M. -- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.925.600.0401 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Jackson
date: 2007-10-16T06:07:29-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: On Mon, 15 Oct 2007, Paul Jackson wrote: > My solution may be worse than that. Because set_cpus_allowed() will > fail if asked to set a non-overlapping cpumask, my solution could never > terminate. If asked to set a cpusets cpus to something that went off > line right then, this I'd guess this code could keep looping forever, > looking for cpumasks that didn't match, and then not noticing that it > was failing to set them so as they would match. > Why can't you just add a helper function to sched.c: void set_hotcpus_allowed(struct task_struct *task, cpumask_t cpumask) { mutex_lock(&sched_hotcpu_mutex); set_cpus_allowed(task, cpumask); mutex_unlock(&sched_hotcpu_mutex); } And then change each task's cpus_allowed via that function instead of set_cpus_allowed() directly? You don't need to worry about making the task->cpuset->cpus_allowed assignment a critical section because common_cpu_mem_hotplug_unplug() will remove any hot-unplugged cpus from each cpuset's cpus_allowed in the hierarchy. Your loop will still need to be reworked so that cgroup_iter_{start,end}() are not reinvoked unnecessarily and you rely only on cgroup_iter_next() returning NULL to determine when you've gone through the entire list. There's no need to go back and check the cpus_allowed of tasks you've already called set_cpus_allowed() on either directly or indirectly via my helper function above. David _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: David Rientjes
date: 2007-10-16T06:21:08-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: David wrote: > Why can't you just add a helper function to sched.c: > > void set_hotcpus_allowed(struct task_struct *task, > cpumask_t cpumask) > { > mutex_lock(&sched_hotcpu_mutex); > set_cpus_allowed(task, cpumask); > mutex_unlock(&sched_hotcpu_mutex); > } > > And then change each task's cpus_allowed via that function instead of > set_cpus_allowed() directly? I guess this would avoid race conditions within the set_cpus_allowed() routine, between its code to read the cpu_online_map and set the tasks cpus_allowed ... though if that's useful, don't we really need to add locking/unlocking on sched_hotcpu_mutex right inside the set_cpus_allowed() routine, for all users of set_cpus_allowed ?? But I don't see where the above code helps at all deal with the races I considered in my previous message: > My solution may be worse than that. Because set_cpus_allowed() will > fail if asked to set a non-overlapping cpumask, my solution could never > terminate. If asked to set a cpusets cpus to something that went off > line right then, this I'd guess this code could keep looping forever, > looking for cpumasks that didn't match, and then not noticing that it > was failing to set them so as they would match. These races involve reading the tasks cpuset cpus_allowed mask, reading the online map, and both reading and writing the tasks task_struct cpus_allowed. Unless one holds the relevant lock for the entire interval surrounding the critical accesses to these values, it won't do any good that I can see. Just briefly holding a lock around each separate access is useless. -- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.925.600.0401 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Jackson
date: 2007-10-16T09:16:26-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: Paul Jackson wrote: > > Any chance you could provide a patch that works against cgroups? > Fix cpusets update_cpumask Cause writes to cpuset "cpus" file to update cpus_allowed for member tasks: - collect batches of tasks under tasklist_lock and then call set_cpus_allowed() on them outside the lock (since this can sleep). - add a simple generic priority heap type to allow efficient collection of batches of tasks to be processed without duplicating or missing any tasks in subsequent batches. - make "cpus" file update a no-op if the mask hasn't changed - fix race between update_cpumask() and sched_setaffinity() by making sched_setaffinity() post-check that it's not running on any cpus outside cpuset_cpus_allowed(). include/linux/prio_heap.h | 56 +++++++++++++++++++++++++ kernel/cpuset.c | 103 ++++++++++++++++++++++++++++++++++++++++++++-- kernel/sched.c | 13 +++++ lib/Makefile | 2 include/linux/prio_heap.h | 58 +++++++++++++++++++++++++ kernel/cpuset.c | 105 ++++++++++++++++++++++++++++++++++++++++++++-- kernel/sched.c | 13 +++++ lib/Makefile | 2 lib/prio_heap.c | 70 ++++++++++++++++++++++++++++++ 5 files changed, 243 insertions(+), 5 deletions(-) Index: container-2.6.23-mm1/include/linux/prio_heap.h =================================================================== --- /dev/null +++ container-2.6.23-mm1/include/linux/prio_heap.h @@ -0,0 +1,58 @@ +#ifndef _LINUX_PRIO_HEAP_H +#define _LINUX_PRIO_HEAP_H + +/* + * Simple insertion-only static-sized priority heap containing + * pointers, based on CLR, chapter 7 + */ + +#include <linux/gfp.h> + +/** + * struct ptr_heap - simple static-sized priority heap + * @ptrs - pointer to data area + * @max - max number of elements that can be stored in @ptrs + * @size - current number of valid elements in @ptrs (in the range 0..@size-1 + * @gt: comparison operator, which should implement "greater than" + */ +struct ptr_heap { + void **ptrs; + int max; + int size; + int (*gt)(void *, void *); +}; + +/** + * heap_init - initialize an e…
creator: Paul Menage
date: 2007-10-16T10:07:59-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: On Tue, 16 Oct 2007, Paul Jackson wrote: > David wrote: > > Why can't you just add a helper function to sched.c: > > > > void set_hotcpus_allowed(struct task_struct *task, > > cpumask_t cpumask) > > { > > mutex_lock(&sched_hotcpu_mutex); > > set_cpus_allowed(task, cpumask); > > mutex_unlock(&sched_hotcpu_mutex); > > } > > > > And then change each task's cpus_allowed via that function instead of > > set_cpus_allowed() directly? > > I guess this would avoid race conditions within the set_cpus_allowed() > routine, between its code to read the cpu_online_map and set the tasks > cpus_allowed ... though if that's useful, don't we really need to add > locking/unlocking on sched_hotcpu_mutex right inside the > set_cpus_allowed() routine, for all users of set_cpus_allowed ?? > Not necessarily because migration only occurs to any online cpu in the mask, it won't attempt to migrate it to some cpu that has been downed. > > My solution may be worse than that. Because set_cpus_allowed() will > > fail if asked to set a non-overlapping cpumask, my solution could never > > terminate. If asked to set a cpusets cpus to something that went off > > line right then, this I'd guess this code could keep looping forever, > > looking for cpumasks that didn't match, and then not noticing that it > > was failing to set them so as they would match. > > These races involve reading the tasks cpuset cpus_allowed mask, reading > the online map, and both reading and writing the tasks task_struct > cpus_allowed. Unless one holds the relevant lock for the entire > interval surrounding the critical accesses to these values, it won't do > any good that I can see. Just briefly holding a lock around each > separate access is useless. > It's not useless since a cpu hot-unplug event automatically updates the cpus_allowed for all cpusets in the hierarchy by removing it. You could protect the entire reassignment with the same mutex since it's so lightly contended, but it's unnecessary. All it would guarantee …
creator: David Rientjes
date: 2007-10-16T18:27:15-00:00

Re: [RFC] cpuset update_cgroup_cpus_allowed
type: item
description: David wrote: > Not necessarily because migration only occurs to any online cpu in the > mask, it won't attempt to migrate it to some cpu that has been downed. > ... ... one of David or I is insane ... I can't tell which one yet, perhaps both of us ;). I'm going to reply to David without all the CC list above. -- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.925.600.0401 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Jackson
date: 2007-10-16T23:14:45-00:00