OpenVZ Forum - RDF feed
type: channel
items: [RFC/PATCH 0/8]: CGroup Files: Clean up locking and boilerplate
items: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
items: [RFC/PATCH 2/8]: CGroup Files: Add a cgroup write_string control file method
items: [RFC/PATCH 3/8]: CGroup Files: Move the release_agent file to use typed handlers
items: [RFC/PATCH 4/8]: CGroup Files: Move notify_on_release file to separate write handler
items: [RFC/PATCH 5/8]: CGroup Files: Turn attach_task_by_pid directly into a cgroup write handler
items: [RFC/PATCH 6/8]: CGroup Files: Remove cpuset_common_file_write()
items: [RFC/PATCH 7/8]: CGroup Files: Convert devcgroup_access_write() into a cgroup write_string() handler
items: [RFC/PATCH 8/8]: CGroup Files: Convert res_counter_write() to be a cgroups write_string() handler
items: Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
items: Re: [RFC/PATCH 2/8]: CGroup Files: Add a cgroup write_string control file method
items: Re: [RFC/PATCH 3/8]: CGroup Files: Move the release_agent file to use typed handlers
items: Re: [RFC/PATCH 6/8]: CGroup Files: Remove cpuset_common_file_write()

[RFC/PATCH 0/8]: CGroup Files: Clean up locking and boilerplate
type: item
description: Most of the remaining cgroup control files that implement raw file handlers (those where the userspace pointer/length/ppos are passed through unchanged by cgroups) appear to do so in order to have some control over the kind of locking that their handler uses. In particular, some handlers need to call cgroup_lock() after copying data from userspace and others don't. They also often dynamically allocate memory when it's not strictly needed. This patchset provides three main features: 1) A new "lockmode" field that indicates what kind of stability/locking guarantees the handler needs from cgroups while it is running. This can be used to reduce the dependency on cgroup_lock(), and prevent its use as a BKL from becoming a contention point as the number of cgroups subsystems grow. An alternative to this would be to not have cgroups do the locking internally, but to export similar functionality via functions such as cgroup_lock_rmdir(struct cgroup*) cgroup_lock_hierarchy(struct cgroup*) cgroup_lock_attach(struct cgroup*) that would perform any necessary locking, and recheck !cgroup_removed(). This results in clearer code from the locking point of view, as you can see within the function exactly what locking it relies upon. The downside is that it makes the code more complex in that you can't simply return from the middle of the function, but instead need to save the return code and goto the point in the function that releases the lock. E.g. the difference would be: int some_handler(struct cgroup *cgrp, struct cftype *cft) { if (!do_locked_operation1(cgrp)) return -EINVAL; return do_locked_operation2(cgrp); } (and setting CFT_LOCK_RMDIR in the cftype descriptor) versus int some_handler(struct cgroup *cgrp, struct cftype *cft) { int retval = cgroup_lock_rmdir(cgrp); if (retval) return retval; if (!do_locked_operation1(cgrp)) { retval = -EINVAL; goto out_unlock; } retval = do_locked_operation2(cgrp); out_unlock: cgroup_unlock_rmdir(cgrp); return retval; } The latter style is …
creator: Paul Menage
date: 2008-05-13T06:37:07-00:00

[RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: Different cgroup files have different stability requirements of the cgroups framework while the handler is running; currently most subsystems that don't have their own internal synchronization just call cgroup_lock()/cgroup_unlock(), which takes the global cgroup_mutex. This patch introduces a range of locking modes that can be requested by a control file; currently these are all implemented internally by taking cgroup_mutex, but expressing the intention will make it simpler to move to a finer-grained locking scheme in the future. Signed-off-by: Paul Menage<menage@google.com> --- include/linux/cgroup.h | 76 +++++++++++++++++++ kernel/cgroup.c | 192 ++++++++++++++++++++++++++++++++++--------------- 2 files changed, 212 insertions(+), 56 deletions(-) Index: cgroup-2.6.25-mm1/include/linux/cgroup.h =================================================================== --- cgroup-2.6.25-mm1.orig/include/linux/cgroup.h +++ cgroup-2.6.25-mm1/include/linux/cgroup.h @@ -200,11 +200,87 @@ struct cgroup_map_cb { */ #define MAX_CFTYPE_NAME 64 + +/* locking modes for control files. + * + * These determine what level of guarantee the file handler wishes + * cgroups to provide about the stability of control group entities + * for the duration of the handler callback. + * + * The minimum guarantee is that the subsystem state for this + * subsystem will not be freed (via a call to the subsystem's + * destroy() callback) until after the control file handler + * returns. This guarantee is provided by the fact that the open + * dentry for the control file keeps its parent (cgroup) dentry alive, + * which in turn keeps the cgroup object from being actually freed + * (although it can be moved into the removed state in the + * meantime). This is suitable for subsystems that completely control + * their own synchronization. + * + * Other possible guarantees are given below. + * + * XXX_READ bits are used for a read operation on the control file, + * XXX_WRITE bits are used for a write operat…
creator: Paul Menage
date: 2008-05-13T06:37:08-00:00

[RFC/PATCH 2/8]: CGroup Files: Add a cgroup write_string control file method
type: item
description: This patch adds a write_string() method for cgroups control files. The semantics are that a buffer is copied from userspace to kernelspace and the handler function invoked on that buffer. Any control group locking is done after the copy from userspace has occurred. The buffer is guaranteed to be nul-terminated, and no longer than max_write_len (defaulting to 64 bytes if unspecified). Later patches will convert existing raw file write handlers in control group subsystems to use this method. Signed-off-by: Paul Menage <menage@google.com> --- include/linux/cgroup.h | 10 ++++++++++ kernel/cgroup.c | 5 ++++- 2 files changed, 14 insertions(+), 1 deletion(-) Index: cgroup-2.6.25-mm1/include/linux/cgroup.h =================================================================== --- cgroup-2.6.25-mm1.orig/include/linux/cgroup.h +++ cgroup-2.6.25-mm1/include/linux/cgroup.h @@ -281,6 +281,10 @@ struct cftype { */ int lockmode; + /* If non-zero, defines the maximum length of string that can + * be passed to write_string; defaults to 64 */ + int max_write_len; + int (*open) (struct inode *inode, struct file *file); ssize_t (*read) (struct cgroup *cgrp, struct cftype *cft, struct file *file, @@ -323,6 +327,12 @@ struct cftype { * write_s64() is a signed version of write_u64() */ int (*write_s64) (struct cgroup *cgrp, struct cftype *cft, s64 val); + /* + * write_string() is passed a nul-terminated kernelspace + * buffer of maximum length determined by max_write_len + */ + int (*write_string) (struct cgroup *cgrp, struct cftype *cft, + char *buffer); /* * trigger() callback can be used to get some kick from the Index: cgroup-2.6.25-mm1/kernel/cgroup.c =================================================================== --- cgroup-2.6.25-mm1.orig/kernel/cgroup.c +++ cgroup-2.6.25-mm1/kernel/cgroup.c @@ -1461,7 +1461,7 @@ static ssize_t cgroup_file_write(struct ssize_t retval; char static_buffer[64]; char *buffer = static_buffer; - ssize_t max_bytes = sizeof(static_buffer) - 1; + ssize_t m…
creator: Paul Menage
date: 2008-05-13T06:37:09-00:00

[RFC/PATCH 3/8]: CGroup Files: Move the release_agent file to use typed handlers
type: item
description: Adds cgroup_release_agent_write() and cgroup_release_agent_show() methods to handle writing/reading the path to a cgroup hierarchy's release agent. As a result, cgroup_common_file_read() is now unnecessary. As part of the change, a previously-tolerated race in cgroup_release_agent() is avoided by copying the current release_agent_path prior to calling call_usermode_helper(). Signed-off-by: Paul Menage <menage@google.com> --- kernel/cgroup.c | 103 +++++++++++++++++++------------------------------------- 1 file changed, 35 insertions(+), 68 deletions(-) Index: cgroup-2.6.25-mm1/kernel/cgroup.c =================================================================== --- cgroup-2.6.25-mm1.orig/kernel/cgroup.c +++ cgroup-2.6.25-mm1/kernel/cgroup.c @@ -89,11 +89,7 @@ struct cgroupfs_root { /* Hierarchy-specific flags */ unsigned long flags; - /* The path to use for release notifications. No locking - * between setting and use - so if userspace updates this - * while child cgroups exist, you could miss a - * notification. We ensure that it's always a valid - * NUL-terminated string */ + /* The path to use for release notifications. */ char release_agent_path[PATH_MAX]; }; @@ -1371,6 +1367,22 @@ static void cgroup_file_unlock(struct cg mutex_unlock(&cgroup_mutex); } +static int cgroup_release_agent_write(struct cgroup *cgrp, struct cftype *cft, + char *buffer) +{ + BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX); + strcpy(cgrp->root->release_agent_path, buffer); + return 0; +} + +static int cgroup_release_agent_show(struct cgroup *cgrp, struct cftype *cft, + struct seq_file *seq) +{ + seq_puts(seq, cgrp->root->release_agent_path); + seq_putc(seq, '\n'); + return 0; +} + static ssize_t cgroup_write_X64(struct cgroup *cgrp, struct cftype *cft, const char *buffer) { @@ -1435,10 +1447,6 @@ static ssize_t cgroup_common_file_write( else clear_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags); break; - case FILE_RELEASE_AGENT: - BUILD_BUG_ON(sizeof(cgrp->root->release_agent_pat…
creator: Paul Menage
date: 2008-05-13T06:37:10-00:00

[RFC/PATCH 4/8]: CGroup Files: Move notify_on_release file to separate write handler
type: item
description: This patch moves the write handler for the cgroups notify_on_release file into a separate handler. This handler requires no cgroups locking since it relies on atomic bitops for synchronization. Signed-off-by: Paul Menage <menage@google.com> --- kernel/cgroup.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) Index: cgroup-2.6.25-mm1/kernel/cgroup.c =================================================================== --- cgroup-2.6.25-mm1.orig/kernel/cgroup.c +++ cgroup-2.6.25-mm1/kernel/cgroup.c @@ -1440,13 +1440,6 @@ static ssize_t cgroup_common_file_write( case FILE_TASKLIST: retval = attach_task_by_pid(cgrp, buffer); break; - case FILE_NOTIFY_ON_RELEASE: - clear_bit(CGRP_RELEASABLE, &cgrp->flags); - if (simple_strtoul(buffer, NULL, 10) != 0) - set_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags); - else - clear_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags); - break; default: retval = -EINVAL; goto out2; @@ -2275,6 +2268,18 @@ static u64 cgroup_read_notify_on_release return notify_on_release(cgrp); } +static int cgroup_write_notify_on_release(struct cgroup *cgrp, + struct cftype *cft, + u64 val) +{ + clear_bit(CGRP_RELEASABLE, &cgrp->flags); + if (val) + set_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags); + else + clear_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags); + return 0; +} + /* * for the common functions, 'private' gives the type of file */ @@ -2291,7 +2296,7 @@ static struct cftype files[] = { { .name = "notify_on_release", .read_u64 = cgroup_read_notify_on_release, - .write = cgroup_common_file_write, + .write_u64 = cgroup_write_notify_on_release, .private = FILE_NOTIFY_ON_RELEASE, }, }; -- _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-05-13T06:37:11-00:00

[RFC/PATCH 5/8]: CGroup Files: Turn attach_task_by_pid directly into a cgroup write handler
type: item
description: This patch changes attach_task_by_pid() to take a u64 rather than a string; as a result it can be called directly as a control groups write_u64 handler, and cgroup_common_file_write() can be removed. Signed-off-by: Paul Menage <menage@google.com> --- kernel/cgroup.c | 67 ++------------------------------------------------------ 1 file changed, 3 insertions(+), 64 deletions(-) Index: cgroup-2.6.25-mm1/kernel/cgroup.c =================================================================== --- cgroup-2.6.25-mm1.orig/kernel/cgroup.c +++ cgroup-2.6.25-mm1/kernel/cgroup.c @@ -503,10 +503,6 @@ static struct css_set *find_css_set( * knows that the cgroup won't be removed, as cgroup_rmdir() * needs that mutex. * - * The cgroup_common_file_write handler for operations that modify - * the cgroup hierarchy holds cgroup_mutex across the entire operation, - * single threading all such cgroup modifications across the system. - * * The fork and exit callbacks cgroup_fork() and cgroup_exit(), don't * (usually) take cgroup_mutex. These are the two most performance * critical pieces of code here. The exception occurs on cgroup_exit(), @@ -1280,15 +1276,11 @@ int cgroup_attach_task(struct cgroup *cg * Attach task with pid 'pid' to cgroup 'cgrp'. Call with * cgroup_mutex, may take task_lock of task */ -static int attach_task_by_pid(struct cgroup *cgrp, char *pidbuf) +static int attach_task_by_pid(struct cgroup *cgrp, struct cftype *cft, u64 pid) { - pid_t pid; struct task_struct *tsk; int ret; - if (sscanf(pidbuf, "%d", &pid) != 1) - return -EIO; - if (pid) { rcu_read_lock(); tsk = find_task_by_vpid(pid); @@ -1400,60 +1392,6 @@ static ssize_t cgroup_write_X64(struct c } } -static ssize_t cgroup_common_file_write(struct cgroup *cgrp, - struct cftype *cft, - struct file *file, - const char __user *userbuf, - size_t nbytes, loff_t *unused_ppos) -{ - enum cgroup_filetype type = cft->private; - char *buffer; - int retval = 0; - - if (nbytes >= PATH_MAX) - return -E2BIG; - - /* +1 for nul-terminat…
creator: Paul Menage
date: 2008-05-13T06:37:12-00:00

[RFC/PATCH 6/8]: CGroup Files: Remove cpuset_common_file_write()
type: item
description: This patch tweaks the signatures of the update_cpumask() and update_nodemask() functions so that they can be called directly as handlers for the new cgroups write_string() method. This allows cpuset_common_file_write() to be removed. Signed-off-by: Paul Menage <menage@google.com> --- kernel/cpuset.c | 73 +++++++------------------------------------------------- 1 file changed, 10 insertions(+), 63 deletions(-) Index: cgroup-2.6.25-mm1/kernel/cpuset.c =================================================================== --- cgroup-2.6.25-mm1.orig/kernel/cpuset.c +++ cgroup-2.6.25-mm1/kernel/cpuset.c @@ -224,10 +224,6 @@ static struct cpuset top_cpuset = { * The task_struct fields mems_allowed and mems_generation may only * be accessed in the context of that task, so require no locks. * - * The cpuset_common_file_write handler for operations that modify - * the cpuset hierarchy holds cgroup_mutex across the entire operation, - * single threading all such cpuset modifications across the system. - * * The cpuset_common_file_read() handlers only hold callback_mutex across * small pieces of code, such as when reading out possibly multi-word * cpumasks and nodemasks. @@ -747,8 +743,9 @@ static void cpuset_change_cpumask(struct * @cs: the cpuset to consider * @buf: buffer of cpu numbers written to this cpuset */ -static int update_cpumask(struct cpuset *cs, char *buf) +static int update_cpumask(struct cgroup *cgrp, struct cftype *cft, char *buf) { + struct cpuset *cs = cgroup_cs(cgrp); struct cpuset trialcs; struct cgroup_scanner scan; struct ptr_heap heap; @@ -767,7 +764,6 @@ static int update_cpumask(struct cpuset * that parsing. The validate_change() call ensures that cpusets * with tasks have cpus. */ - buf = strstrip(buf); if (!*buf) { cpus_clear(trialcs.cpus_allowed); } else { @@ -875,8 +871,9 @@ static void cpuset_migrate_mm(struct mm_ static void *cpuset_being_rebound; -static int update_nodemask(struct cpuset *cs, char *buf) +static int update_nodemask(struct cgroup *…
creator: Paul Menage
date: 2008-05-13T06:37:13-00:00

[RFC/PATCH 7/8]: CGroup Files: Convert devcgroup_access_write() into a cgroup write_string() handler
type: item
description: This patch converts devcgroup_access_write() from a raw file handler into a handler for the cgroup write_string() method. This allows some boilerplate copying/locking/checking to be removed and simplifies the cleanup path, since these functions are performed by the cgroups framework before calling the handler. Signed-off-by: Paul Menage <menage@google.com> --- security/device_cgroup.c | 79 +++++++++++++---------------------------------- 1 file changed, 22 insertions(+), 57 deletions(-) Index: cgroup-2.6.25-mm1/security/device_cgroup.c =================================================================== --- cgroup-2.6.25-mm1.orig/security/device_cgroup.c +++ cgroup-2.6.25-mm1/security/device_cgroup.c @@ -323,14 +323,14 @@ static int parent_has_perm(struct cgroup * new access is only allowed if you're in the top-level cgroup, or your * parent cgroup has the access you're asking for. */ -static ssize_t devcgroup_access_write(struct cgroup *cgroup, struct cftype *cft, - struct file *file, const char __user *userbuf, - size_t nbytes, loff_t *ppos) +static int devcgroup_access_write(struct cgroup *cgroup, + struct cftype *cft, + char *buffer) { struct cgroup *cur_cgroup; struct dev_cgroup *devcgroup, *cur_devcgroup; int filetype = cft->private; - char *buffer, *b; + char *b; int retval = 0, count; struct dev_whitelist_item wh; @@ -341,22 +341,6 @@ static ssize_t devcgroup_access_write(st cur_cgroup = task_cgroup(current, devices_subsys.subsys_id); cur_devcgroup = cgroup_to_devcgroup(cur_cgroup); - buffer = kmalloc(nbytes+1, GFP_KERNEL); - if (!buffer) - return -ENOMEM; - - if (copy_from_user(buffer, userbuf, nbytes)) { - retval = -EFAULT; - goto out1; - } - buffer[nbytes] = 0; /* nul-terminate */ - - cgroup_lock(); - if (cgroup_is_removed(cgroup)) { - retval = -ENODEV; - goto out2; - } - memset(&wh, 0, sizeof(wh)); b = buffer; @@ -372,14 +356,11 @@ static ssize_t devcgroup_access_write(st wh.type = DEV_CHAR; break; default: - retval = -EINVAL; - goto out2; + return -EINVAL…
creator: Paul Menage
date: 2008-05-13T06:37:14-00:00

[RFC/PATCH 8/8]: CGroup Files: Convert res_counter_write() to be a cgroups write_string() handler
type: item
description: Currently read_counter_write() is a raw file handler even though it's ultimately taking a number, since in some cases it wants to pre-process the string when converting it to a number. This patch converts res_counter_write() from a raw file handler to a write_string() handler; this allows some of the boilerplate copying/locking/checking to be removed, and simplies the cleanup path, since these functions are now performed by the cgroups framework. Signed-off-by: Paul Menage <menage@google.com> --- include/linux/res_counter.h | 4 ++-- kernel/res_counter.c | 36 +++++++++--------------------------- mm/memcontrol.c | 11 +++++------ 3 files changed, 16 insertions(+), 35 deletions(-) Index: cgroup-2.6.25-mm1/include/linux/res_counter.h =================================================================== --- cgroup-2.6.25-mm1.orig/include/linux/res_counter.h +++ cgroup-2.6.25-mm1/include/linux/res_counter.h @@ -63,8 +63,8 @@ u64 res_counter_read_u64(struct res_coun ssize_t res_counter_read(struct res_counter *counter, int member, const char __user *buf, size_t nbytes, loff_t *pos, int (*read_strategy)(unsigned long long val, char *s)); -ssize_t res_counter_write(struct res_counter *counter, int member, - const char __user *buf, size_t nbytes, loff_t *pos, +int res_counter_write(struct res_counter *counter, int member, + char *buffer, int (*write_strategy)(char *buf, unsigned long long *val)); /* Index: cgroup-2.6.25-mm1/kernel/res_counter.c =================================================================== --- cgroup-2.6.25-mm1.orig/kernel/res_counter.c +++ cgroup-2.6.25-mm1/kernel/res_counter.c @@ -102,44 +102,26 @@ u64 res_counter_read_u64(struct res_coun return *res_counter_member(counter, member); } -ssize_t res_counter_write(struct res_counter *counter, int member, - const char __user *userbuf, size_t nbytes, loff_t *pos, - int (*write_strategy)(char *st_buf, unsigned long long *val)) +int res_counter_write( + struct res_counter *counter, int member, + char *buf, + int…
creator: Paul Menage
date: 2008-05-13T06:37:15-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: menage@google.com wrote: > Different cgroup files have different stability requirements of the > cgroups framework while the handler is running; currently most > subsystems that don't have their own internal synchronization just > call cgroup_lock()/cgroup_unlock(), which takes the global cgroup_mutex. > > This patch introduces a range of locking modes that can be requested > by a control file; currently these are all implemented internally by > taking cgroup_mutex, but expressing the intention will make it simpler > to move to a finer-grained locking scheme in the future. > > Signed-off-by: Paul Menage<menage@google.com> > This patch series looks good to me. I've reviewed those patches and didn't see anything wrong, except a little comments below. [..snap..] > -static ssize_t cgroup_write_X64(struct cgroup *cgrp, struct cftype *cft, > - struct file *file, > - const char __user *userbuf, > - size_t nbytes, loff_t *unused_ppos) > + > + > +/** > + * cgroup_file_lock(). Helper for cgroup read/write methods. > + * @cgrp: the cgroup being acted on > + * @cft: the control file being written to or read from > + * *write: true if the access is a write access. s/*write/@write [..snip..] > > static ssize_t cgroup_common_file_read(struct cgroup *cgrp, > @@ -1518,16 +1580,21 @@ static ssize_t cgroup_file_read(struct f > struct cftype *cft = __d_cft(file->f_dentry); > struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent); > > - if (!cft || cgroup_is_removed(cgrp)) > + if (cgroup_is_removed(cgrp)) > return -ENODEV; > This check seems redundant now. > - if (cft->read) > - return cft->read(cgrp, cft, file, buf, nbytes, ppos); > - if (cft->read_u64) > - return cgroup_read_u64(cgrp, cft, file, buf, nbytes, ppos); > - if (cft->read_s64) > - return cgroup_read_s64(cgrp, cft, file, buf, nbytes, ppos); > - return -EINVAL; > + if (cft->read) { > + /* Raw read function - no extra processing by cgroups */ > + ssize_t retval = cgroup_file_lock(cgrp, cft, 0); > + if (!retval) > + retval = …
creator: Li Zefan
date: 2008-05-13T09:23:29-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: Fear, doubt and resistance! On Mon, 12 May 2008 23:37:08 -0700 menage@google.com wrote: > Different cgroup files have different stability requirements of the > cgroups framework while the handler is running; currently most > subsystems that don't have their own internal synchronization just > call cgroup_lock()/cgroup_unlock(), which takes the global cgroup_mutex. > > This patch introduces a range of locking modes that can be requested > by a control file; currently these are all implemented internally by > taking cgroup_mutex, but expressing the intention will make it simpler > to move to a finer-grained locking scheme in the future. > This, umm, doesn't seem to do much to make the kernel a simpler place. Do we expect to gain much from this? Hope so... What? > Index: cgroup-2.6.25-mm1/include/linux/cgroup.h > =================================================================== > --- cgroup-2.6.25-mm1.orig/include/linux/cgroup.h > +++ cgroup-2.6.25-mm1/include/linux/cgroup.h > @@ -200,11 +200,87 @@ struct cgroup_map_cb { > */ > > #define MAX_CFTYPE_NAME 64 > + > +/* locking modes for control files. > + * > + * These determine what level of guarantee the file handler wishes > + * cgroups to provide about the stability of control group entities > + * for the duration of the handler callback. > + * > + * The minimum guarantee is that the subsystem state for this > + * subsystem will not be freed (via a call to the subsystem's > + * destroy() callback) until after the control file handler > + * returns. This guarantee is provided by the fact that the open > + * dentry for the control file keeps its parent (cgroup) dentry alive, > + * which in turn keeps the cgroup object from being actually freed > + * (although it can be moved into the removed state in the > + * meantime). This is suitable for subsystems that completely control > + * their own synchronization. > + * > + * Other possible guarantees are given below. > + * > + * XXX_READ bits are used for a read operation …
creator: akpm
date: 2008-05-13T20:01:27-00:00

Re: [RFC/PATCH 2/8]: CGroup Files: Add a cgroup write_string control file method
type: item
description: On Mon, 12 May 2008 23:37:09 -0700 menage@google.com wrote: > This patch adds a write_string() method for cgroups control files. The > semantics are that a buffer is copied from userspace to kernelspace > and the handler function invoked on that buffer. Any control group > locking is done after the copy from userspace has occurred. The buffer > is guaranteed to be nul-terminated, and no longer than max_write_len > (defaulting to 64 bytes if unspecified). Later patches will convert > existing raw file write handlers in control group subsystems to use > this method. > nits: > > --- > include/linux/cgroup.h | 10 ++++++++++ > kernel/cgroup.c | 5 ++++- > 2 files changed, 14 insertions(+), 1 deletion(-) > > Index: cgroup-2.6.25-mm1/include/linux/cgroup.h > =================================================================== > --- cgroup-2.6.25-mm1.orig/include/linux/cgroup.h > +++ cgroup-2.6.25-mm1/include/linux/cgroup.h > @@ -281,6 +281,10 @@ struct cftype { > */ > int lockmode; > > + /* If non-zero, defines the maximum length of string that can > + * be passed to write_string; defaults to 64 */ > + int max_write_len; would size_t be a more appropriate type? > int (*open) (struct inode *inode, struct file *file); > ssize_t (*read) (struct cgroup *cgrp, struct cftype *cft, > struct file *file, > @@ -323,6 +327,12 @@ struct cftype { > * write_s64() is a signed version of write_u64() > */ > int (*write_s64) (struct cgroup *cgrp, struct cftype *cft, s64 val); s/) (/)(/ would be more conventional. > + /* > + * write_string() is passed a nul-terminated kernelspace > + * buffer of maximum length determined by max_write_len > + */ > + int (*write_string) (struct cgroup *cgrp, struct cftype *cft, > + char *buffer); Should these return size_t? > /* > * trigger() callback can be used to get some kick from the > Index: cgroup-2.6.25-mm1/kernel/cgroup.c > =================================================================== > --- cgroup-2.6.25-mm1.orig/kernel/cgroup.c > +++ cgroup-2.6.2…
creator: akpm
date: 2008-05-13T20:07:10-00:00

Re: [RFC/PATCH 3/8]: CGroup Files: Move the release_agent file to use typed handlers
type: item
description: On Mon, 12 May 2008 23:37:10 -0700 menage@google.com wrote: > + agentbuf = kmalloc(PATH_MAX, GFP_KERNEL); > + if (!agentbuf) > + goto continue_free; > + strcpy(agentbuf, cgrp->root->release_agent_path); Did we need to allocate all that memory, or would kstrdup() suffice? _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: akpm
date: 2008-05-13T20:08:33-00:00

Re: [RFC/PATCH 6/8]: CGroup Files: Remove cpuset_common_file_write()
type: item
description: On Mon, 12 May 2008 23:37:13 -0700 menage@google.com wrote: > @@ -1412,14 +1355,18 @@ static struct cftype files[] = { > { > .name = "cpus", > .read = cpuset_common_file_read, > - .write = cpuset_common_file_write, > + .write_string = update_cpumask, > + .max_write_len = (100U + 6 * NR_CPUS), hm, magic handwavy surely-enough-for-anyone constants. What's going on here? > + .lockmode = CFT_LOCK_CGL_WRITE, > .private = FILE_CPULIST, > }, _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: akpm
date: 2008-05-13T20:11:34-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: On Tue, 2008-05-13 at 13:01 -0700, Andrew Morton wrote: > Fear, doubt and resistance! > > On Mon, 12 May 2008 23:37:08 -0700 > menage@google.com wrote: > > > Different cgroup files have different stability requirements of the > > cgroups framework while the handler is running; currently most > > subsystems that don't have their own internal synchronization just > > call cgroup_lock()/cgroup_unlock(), which takes the global cgroup_mutex. > > > > This patch introduces a range of locking modes that can be requested > > by a control file; currently these are all implemented internally by > > taking cgroup_mutex, but expressing the intention will make it simpler > > to move to a finer-grained locking scheme in the future. > > > > This, umm, doesn't seem to do much to make the kernel a simpler place. > > Do we expect to gain much from this? Hope so... What? > > > Index: cgroup-2.6.25-mm1/include/linux/cgroup.h > > =================================================================== > > --- cgroup-2.6.25-mm1.orig/include/linux/cgroup.h > > +++ cgroup-2.6.25-mm1/include/linux/cgroup.h > > @@ -200,11 +200,87 @@ struct cgroup_map_cb { > > */ > > > > #define MAX_CFTYPE_NAME 64 > > + > > +/* locking modes for control files. > > + * > > + * These determine what level of guarantee the file handler wishes > > + * cgroups to provide about the stability of control group entities > > + * for the duration of the handler callback. > > + * > > + * The minimum guarantee is that the subsystem state for this > > + * subsystem will not be freed (via a call to the subsystem's > > + * destroy() callback) until after the control file handler > > + * returns. This guarantee is provided by the fact that the open > > + * dentry for the control file keeps its parent (cgroup) dentry alive, > > + * which in turn keeps the cgroup object from being actually freed > > + * (although it can be moved into the removed state in the > > + * meantime). This is suitable for subsystems that completely control > …
creator: Matt Helsley
date: 2008-05-13T20:38:58-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: On Tue, 13 May 2008 13:38:58 -0700 Matthew Helsley <matthltc@us.ibm.com> wrote: > > > + char static_buffer[64]; > > > I'm trying to work out what protects static_buffer? > > One of us must be having a brain cramp because it looks to me like the > buffer doesn't need protection -- it's on the stack. It's probably me > but I'm just not seeing how this use is unsafe.. doh. Well it had me going... > Uh, it is on stack. It doesn't use the C keyword "static". It's just > poorly-named. That depends upon one's objectives in choosing a name ;) _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: akpm
date: 2008-05-13T20:43:54-00:00

Re: [RFC/PATCH 2/8]: CGroup Files: Add a cgroup write_string control file method
type: item
description: On Mon, 2008-05-12 at 23:37 -0700, menage@google.com wrote: > plain text document attachment (cgroup_write_string.patch) > This patch adds a write_string() method for cgroups control files. The > semantics are that a buffer is copied from userspace to kernelspace > and the handler function invoked on that buffer. Any control group > locking is done after the copy from userspace has occurred. The buffer > is guaranteed to be nul-terminated, and no longer than max_write_len > (defaulting to 64 bytes if unspecified). Later patches will convert > existing raw file write handlers in control group subsystems to use > this method. > > Signed-off-by: Paul Menage <menage@google.com> I haven't looked at this very thoroughly but I think the goal of this patch is good. Can't this patch and the conversions precede your locking changes? Cheers, -Matt Helsley _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Matt Helsley
date: 2008-05-13T20:44:11-00:00

Re: [RFC/PATCH 2/8]: CGroup Files: Add a cgroup write_string control file method
type: item
description: On Tue, May 13, 2008 at 1:07 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > > > + /* If non-zero, defines the maximum length of string that can > > + * be passed to write_string; defaults to 64 */ > > + int max_write_len; > > would size_t be a more appropriate type? > Probably overkill, but I guess it's technically more correct. Updated for the next version of these patches. > > s/) (/)(/ would be more conventional. > OK, I've updated this and the other extraneous spaces in a separate patch. > > > + /* > > + * write_string() is passed a nul-terminated kernelspace > > + * buffer of maximum length determined by max_write_len > > + */ > > + int (*write_string) (struct cgroup *cgrp, struct cftype *cft, > > + char *buffer); > > Should these return size_t? No, it returns 0 or a -ve error code. I've added a comment to this effect. > > char *buffer = static_buffer; > > - ssize_t max_bytes = sizeof(static_buffer) - 1; > > + ssize_t max_bytes = cft->max_write_len ?: sizeof(static_buffer) - 1; > > A blank line between end-of-locals and start-of-code is conventional > and, IMO, easier on the eye. > > Does gcc actually generate better code with that x?:y thing? I doubt it - but I felt that it made the code a bit clearer since it reduces repetition. I can change it to size_t max_bytes = cft->max_write_len; if (!max_bytes) max_bytes = sizeof(static_buffer) - 1; Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-05-13T21:01:01-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: On Tue, May 13, 2008 at 2:23 AM, Li Zefan <lizf@cn.fujitsu.com> wrote: > > + > > +/** > > + * cgroup_file_lock(). Helper for cgroup read/write methods. > > + * @cgrp: the cgroup being acted on > > + * @cft: the control file being written to or read from > > + * *write: true if the access is a write access. > > s/*write/@write > Fixed. > > @@ -1518,16 +1580,21 @@ static ssize_t cgroup_file_read(struct f > > struct cftype *cft = __d_cft(file->f_dentry); > > struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent); > > > > - if (!cft || cgroup_is_removed(cgrp)) > > + if (cgroup_is_removed(cgrp)) > > return -ENODEV; > > > > This check seems redundant now. > It's not needed for safety, but it doesn't seem to hurt to check cgroup_is_removed() prior to doing any copying, since we'll fail after copying anyway if cgroup_is_removed() returns true (once we've taken any relevant locks). Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-05-13T21:07:40-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: On Tue, May 13, 2008 at 1:01 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > This, umm, doesn't seem to do much to make the kernel a simpler place. > > Do we expect to gain much from this? Hope so... What? > The goal is to prevent cgroup_mutex becoming a BKL for cgroups, to make it easier for subsystems to lock just the bits that they need to remain stable rather than everything. > > Vague handwaving: lockdep doesn't know anything about any of this. > Whereas if we were more conventional in using separate locks and > suitable lock types for each application, we would retain full lockdep > coverage. That's a good point - I'd not thought about lockdep. That's a good argument in favour of not having the locking done in the framework. Stepping back a bit, the idea is definitely that where appropriate subsystems will use their own fine-grained locking. E.g. the res_counter abstraction does this already with a spinlock in each res_counter, and cpusets has the global callback_mutex that just synchronizes cpuset operations. But there are some cases where they need a bit of help from cgroups, such as when doing operations that require stable hierarchies, task membership of cgroups, etc. Right now the default behaviour is to call cgroup_lock(), which I'd like to get away from. Having the framework do the locking results in less need for cleanup code in the subsystem handlers themselves, but that's not an unassailable argument for doing it that way. This is one of those times that I really long for C++ scope-based destructors, to automatically release locks when you exit a scope. > > + ssize_t max_bytes = sizeof(static_buffer) - 1; > > + if (!cft->write && !cft->trigger) { > > + if (!nbytes) > > + return -EINVAL; > > + if (nbytes >= max_bytes) > > + return -E2BIG; > > + if (nbytes >= sizeof(static_buffer)) { > > afaict this can't happen - we would have already returned -E2BIG? You're right, with this patch it can't. The code that makes it necessary to allocate a dynam…
creator: Paul Menage
date: 2008-05-13T21:17:29-00:00

Re: [RFC/PATCH 6/8]: CGroup Files: Remove cpuset_common_file_write()
type: item
description: On Tue, May 13, 2008 at 1:11 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Mon, 12 May 2008 23:37:13 -0700 > menage@google.com wrote: > > > @@ -1412,14 +1355,18 @@ static struct cftype files[] = { > > { > > .name = "cpus", > > .read = cpuset_common_file_read, > > - .write = cpuset_common_file_write, > > + .write_string = update_cpumask, > > + .max_write_len = (100U + 6 * NR_CPUS), > > hm, magic handwavy surely-enough-for-anyone constants. This is just a movement of the check previously in cpuset_common_file_write(): /* Crude upper limit on largest legitimate cpulist user might write. */ if (nbytes > 100U + 6 * max(NR_CPUS, MAX_NUMNODES)) return -E2BIG; I'll add more comments. But I think it's a reasonable limit - assuming that you wastefully enumerate each cpu individually, separated by commas, 6 * NR_CPUS is enough for 100K CPUs. Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-05-13T21:27:24-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: On Tue, 13 May 2008 14:17:29 -0700 "Paul Menage" <menage@google.com> wrote: > On Tue, May 13, 2008 at 1:01 PM, Andrew Morton > <akpm@linux-foundation.org> wrote: > > > > This, umm, doesn't seem to do much to make the kernel a simpler place. > > > > Do we expect to gain much from this? Hope so... What? > > > > The goal is to prevent cgroup_mutex becoming a BKL for cgroups, to > make it easier for subsystems to lock just the bits that they need to > remain stable rather than everything. OK. But do we ever expect that cgroup_mutex will be taken with much frequency, or held for much time? If it's only taken during, say, configuration of a group or during a query of that configuration then perhaps we'll be OK. otoh a per-cgroup lock would seem more appropriate than a global. > > > > Vague handwaving: lockdep doesn't know anything about any of this. > > Whereas if we were more conventional in using separate locks and > > suitable lock types for each application, we would retain full lockdep > > coverage. > > That's a good point - I'd not thought about lockdep. That's a good > argument in favour of not having the locking done in the framework. > > Stepping back a bit, the idea is definitely that where appropriate > subsystems will use their own fine-grained locking. E.g. the > res_counter abstraction does this already with a spinlock in each > res_counter, and cpusets has the global callback_mutex that just > synchronizes cpuset operations. But there are some cases where they > need a bit of help from cgroups, such as when doing operations that > require stable hierarchies, task membership of cgroups, etc. > > Right now the default behaviour is to call cgroup_lock(), which I'd > like to get away from. Having the framework do the locking results in > less need for cleanup code in the subsystem handlers themselves, but > that's not an unassailable argument for doing it that way. Yes, caller-provided locking is the usual pattern in-kernel. Based on painful experience :( > > I…
creator: akpm
date: 2008-05-13T21:32:06-00:00

Re: [RFC/PATCH 3/8]: CGroup Files: Move the release_agent file to use typed handlers
type: item
description: On Tue, May 13, 2008 at 1:08 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Mon, 12 May 2008 23:37:10 -0700 > menage@google.com wrote: > > > + agentbuf = kmalloc(PATH_MAX, GFP_KERNEL); > > + if (!agentbuf) > > + goto continue_free; > > + strcpy(agentbuf, cgrp->root->release_agent_path); > > Did we need to allocate all that memory, or would kstrdup() suffice? > kstrdup() would be fine. Fixed. Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-05-13T21:32:57-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: On Tue, May 13, 2008 at 2:32 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > But do we ever expect that cgroup_mutex will be taken with much > frequency, or held for much time? If it's only taken during, say, > configuration of a group or during a query of that configuration then > perhaps we'll be OK. I'm not so worried about contention on the userspace configuration side - more the case of configuration operations stalling subsystem-initiated operations. An example of that would be the case of hierarchical reclaim in the memory controller, where it needs to be able to traverse up and down the hierarchy without the hierarchy changing under its feet. I'll dump the implicit lock_mode support and make it more explicit in the subsystem handlers. Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-05-13T21:46:05-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: >> > @@ -1518,16 +1580,21 @@ static ssize_t cgroup_file_read(struct f >> > struct cftype *cft = __d_cft(file->f_dentry); >> > struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent); >> > >> > - if (!cft || cgroup_is_removed(cgrp)) >> > + if (cgroup_is_removed(cgrp)) >> > return -ENODEV; >> > >> >> This check seems redundant now. >> > > It's not needed for safety, but it doesn't seem to hurt to check > cgroup_is_removed() prior to doing any copying, since we'll fail after > copying anyway if cgroup_is_removed() returns true (once we've taken > any relevant locks). > It's a bit odd to me that cgroup_is_removed() is removed in cgroup_file_write(), but it's ressered here. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Li Zefan
date: 2008-05-14T01:30:40-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: On Tue, May 13, 2008 at 6:30 PM, Li Zefan <lizf@cn.fujitsu.com> wrote: > > It's a bit odd to me that cgroup_is_removed() is removed in cgroup_file_write(), > but it's ressered here. > Fair point. It's probably better for me to be consistent. Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-05-14T01:40:12-00:00

Re: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
type: item
description: Andrew wrote: > As Matt observed, this is just a poorly-named variable. How about the following code for cgroup_file_write(): char buf[64]; /* avoid kmalloc() in small cases */ char *p; /* buf[] or kmalloc'd buffer */ ... if (nbytes < sizeof(buf)) { p = buf; } else { p = kmalloc(nbytes + 1, GFP_KERNEL); if (p == NULL) return -ENOMEM; } Possible advantages of above code: * Uses short names for local variables of limited scope. * Doesn't set p until needed, so as: 1) to highlight the symmetry of its setting, to either buf[] (small cases) or to a kmalloc'd buffer (large cases), and 2) to avoid implying that p needs to be set in the intervening "..." code above. * Comments variable declarations. -- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.940.382.4214 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Jackson
date: 2008-05-14T01:59:52-00:00