OpenVZ Forum - RDF feed
type: channel
items: [PATCH 03/33] task containersv11 add tasks file interface
items: Re: [PATCH 03/33] task containersv11 add tasks file interface

[PATCH 03/33] task containersv11 add tasks file interface
type: item
description: Add the per-directory "tasks" file for cgroupfs mounts; this allows the user to determine which tasks are members of a cgroup by reading a cgroup's "tasks", and to move a task into a cgroup by writing its pid to its "tasks". Signed-off-by: Paul Menage <menage@google.com> --- include/linux/cgroup.h | 10 + kernel/cgroup.c | 359 +++++++++++++++++++++++++++++++++++- 2 files changed, 368 insertions(+), 1 deletion(-) diff -puN include/linux/cgroup.h~task-cgroupsv11-add-tasks-file-interface include/linux/cgroup.h --- a/include/linux/cgroup.h~task-cgroupsv11-add-tasks-file-interface +++ a/include/linux/cgroup.h @@ -144,6 +144,16 @@ int cgroup_is_removed(const struct co int cgroup_path(const struct cgroup *cont, char *buf, int buflen); +int __cgroup_task_count(const struct cgroup *cont); +static inline int cgroup_task_count(const struct cgroup *cont) +{ + int task_count; + rcu_read_lock(); + task_count = __cgroup_task_count(cont); + rcu_read_unlock(); + return task_count; +} + /* Return true if the cgroup is a descendant of the current cgroup */ int cgroup_is_descendant(const struct cgroup *cont); diff -puN kernel/cgroup.c~task-cgroupsv11-add-tasks-file-interface kernel/cgroup.c --- a/kernel/cgroup.c~task-cgroupsv11-add-tasks-file-interface +++ a/kernel/cgroup.c @@ -40,7 +40,7 @@ #include <linux/magic.h> #include <linux/spinlock.h> #include <linux/string.h> - +#include <linux/sort.h> #include <asm/atomic.h> /* Generate an array of cgroup subsystem pointers */ @@ -713,6 +713,127 @@ int cgroup_path(const struct containe return 0; } +/* + * Return the first subsystem attached to a cgroup's hierarchy, and + * its subsystem id. + */ + +static void get_first_subsys(const struct cgroup *cont, + struct cgroup_subsys_state **css, int *subsys_id) +{ + const struct cgroupfs_root *root = cont->root; + const struct cgroup_subsys *test_ss; + BUG_ON(list_empty(&root->subsys_list)); + test_ss = list_entry(root->subsys_list.next, + struct cgroup_subsys, sibling); + if (css) { + *css = cont->…
creator: Paul Menage
date: 2007-09-17T21:03:10-00:00

Re: [PATCH 03/33] task containersv11 add tasks file interface
type: item
description: Cgroup (aka container) code review: Except for the very last item below, my other comments are minor. And the last item is pretty easy too - just more important. Overall - nice stuff. I like this generalization of the cpuset hierarchy. Thanks. =========== Review comments on Documentation/cgroups.txt - You can drop the cpuset mini-history at the top of cgroups.txt: Modified by Paul Jackson <pj@sgi.com> Modified by Christoph Lameter <clameter@sgi.com> - The "Definitions" section is well done, thanks. - Could you change the present tense in: There are multiple efforts to provide ... These all require ... to past tense: There have been multiple efforts to provide ... These all required ... The present tense suggests to me that there are, as of the time I am reading this (perhaps years in the future) implementations of cpusets, CKRM/ResGroups, UserBeanCounters, and virtual server namespaces all somewhere in the main line kernel. I don't think so. - I guess that the items "CPU", "Memory", Disk", and "Network" in the example are separate hierarchies - but that's not clear. Perhaps "CPU" and "Memory" are in the same hierarchy - cpusets ?? There is a mix of terminology in this example that is confusing. The word "class" is out of context. I guess that "the WWW Network class" refers to the "WWW browsing" thingie -- is that a cgroup -- but not to the "Network:" thingie -- is that a cgroup hierarchy ? I got lost in this "example of a scenario ...". It reads like a cut+paste from some other effort, incompletely reworded. - What are these apparent 'exec notifications' that are provided to user space that the following mentions - I cannot find any other mention of them: With the ability to classify tasks differently for different resources (by putting those resource subsystems in different hierarchies) then the admin can easily set up a script which receives exec notifications and depending on who is launching the browser he can - Try replacing the following with a single period c…
creator: Paul Jackson
date: 2007-10-03T08:09:23-00:00

Re: [PATCH 03/33] task containersv11 add tasks file interface
type: item
description: On 10/3/07, Paul Jackson <pj@sgi.com> wrote: > > - What are these apparent 'exec notifications' that are provided to > user space that the following mentions - I cannot find any other > mention of them: > > With the ability to classify tasks differently for different > resources (by putting those resource subsystems in different > hierarchies) then the admin can easily set up a script which > receives exec notifications and depending on who is launching > the browser he can It's the process connector netlink notifier. It can report fork/exit/exec/setuid events to userspace. See drivers/connector/cn_proc.c > > > - It states in cgroups.txt: > > *** notify_on_release is disabled in the current patch set. It will be > *** reactivated in a future patch in a less-intrusive manner > > This doesn't seem to be true, and had better not be true. > From what I can tell, notify_on_release still works for cpusets, > and it is important that it continue to work when cgroups are > folded into the main line kernel. Correct, it's reactivated in a later patch in the series, but this intermediate comment snuck through. > > Each cgroup object created by the system has an array of pointers, > indexed by subsystem id; this pointer is entirely managed by the > subsystem; the generic cgroup code will never touch this pointer. > > Is plural "pointers", or singular "pointer", the correct wording? Probably plural. > > - Several lines near the end of cgroups.txt start with "LL". > I guess they list what locks are held while taking the call, > but the notation seems cryptic and unfamiliar to me, and its > meaning here undocumented. "Locking Level", describing which locks *are* held, and which are *not* held during a call. I thought it was a more generally widely-used commenting convention, but I don't see any other uses of it in the kernel. I can replace them with "holds cgroup_mutex" or "doesn't hold cgroup_mutex" for clarity. > > - There are many instances of the local variable 'cont', referri…
creator: Paul Menage
date: 2007-10-03T15:16:53-00:00

Re: [PATCH 03/33] task containersv11 add tasks file interface
type: item
description: Paul M wrote: > Are there cases when userspace is > required to try to reattach a task to its current cpuset in order to > get a cpu mask change to stick? Yes, there are such cases. If tasks are running in a cpuset, and someone changes the 'cpus' of that cpuset, the tasks already in that cpuset don't move. They stay with their task structs cpus_allowed pointing to the old value. The only code path that user space can trigger that leads to changing an existing tasks cpus_allowed mask is to write that tasks pid to a cpuset 'tasks' file. So after changing the 'cpus' of a cpuset, you (something in user space) then has to rewrite every pid in that cpusets tasks file back to that same tasks file, in order to get the change to be applied to each of those tasks, and get them to start running on their new CPUs. -- 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-03T17:51:57-00:00

Re: [PATCH 03/33] task containersv11 add tasks file interface
type: item
description: On 10/3/07, Paul Jackson <pj@sgi.com> wrote: > > So after changing the 'cpus' of a cpuset, you (something in > user space) then has to rewrite every pid in that cpusets > tasks file back to that same tasks file, in order to get the > change to be applied to each of those tasks, and get them to > start running on their new CPUs. What happens to the new child that got forked right between userspace reading the 'tasks' file and writing back the pids? Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2007-10-03T18:15:57-00:00

Re: [PATCH 03/33] task containersv11 add tasks file interface
type: item
description: > > - There are many instances of the local variable 'cont', referring > > to a struct cgroup pointer. I presume the spelling 'cont' is a > > holdover from the time when we called these containers. > > Yes, and since cgroup is short for "control group", "cont" still > seemed like a reasonable abbreviation. (And made the automatic > renaming much simpler). The following will change all 'cont' words to your choice (I doubt you want to use 'XXXX' as I did here) in cgroup.c: sed -i -r 's/(\W|^)cont(\W|$)/\1XXXX\2/g' kernel/cgroup.c I can't say for sure, but I suspect that if cgroups had always been cgroups (short for control groups), then these local 'cont' variables would have a different name. One can often, as in this case, find some justification for most any name. The question is which name is most quickly and easily understood. ... yes ... I'm a stickler for names ... sorry. -- 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-04T02:46:52-00:00

Re: [PATCH 03/33] task containersv11 add tasks file interface
type: item
description: On 10/3/07, Paul Jackson <pj@sgi.com> wrote: > > I can't say for sure, but I suspect that if cgroups had always > been cgroups (short for control groups), then these local 'cont' > variables would have a different name. Oh, absolutely. I just refrained from changing them in the rename since the name was sort of still relevant and it made the change simpler. Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2007-10-04T02:53:03-00:00

Re: [PATCH 03/33] task containersv11 add tasks file interface
type: item
description: One more cgroup code review detail ... The following is evidence of some more stale comments in kernel/cpuset.c. Some routines which used to be in that file, but which are now reimplemented in cgroups, are still named in cpuset.c comments: $ grep -E 'cpuset_rmdir|cpuset_exit|cpuset_fork' kernel/cpuset.c * knows that the cpuset won't be removed, as cpuset_rmdir() needs * The fork and exit callbacks cpuset_fork() and cpuset_exit(), don't * critical pieces of code here. The exception occurs on cpuset_exit(), * the_top_cpuset_hack in cpuset_exit(), which sets an exiting tasks The downside of my writing too many comments ... its more of a maintenance burden on those changing the code ;). -- 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-04T02:55:03-00:00