OpenVZ Forum - RDF feed
type: channel
items: [RFC][PATCH 0/5] Container Freezer: Reuse Suspend Freezer
items: [RFC][PATCH 1/5] Container Freezer: Add TIF_FREEZE flag to all architectures
items: [RFC][PATCH 2/5] Container Freezer: Make refrigerator always available
items: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
items: [RFC][PATCH 4/5] Container Freezer: Skip frozen cgroups during power management resume
items: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
items: Re: [RFC][PATCH 1/5] Container Freezer: Add TIF_FREEZE flag to all architectures
items: Re: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
items: Re: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
items: Re: [RFC][PATCH 2/5] Container Freezer: Make refrigerator always available
items: [RFC][PATCH] Freezer: NOSIG flag (was: Re: [RFC][PATCH 2/5] Container Freezer: Make refrigerator alw

[RFC][PATCH 0/5] Container Freezer: Reuse Suspend Freezer
type: item
description: This patchset reuses the container infrastructure and the swsusp freezer to freeze a group of tasks. I've merely taken Cedric's patches, forward-ported them to 2.6.25-mm1 and tested the expected common cases. Changes since v1: v2 (roughly patches 3 and 5): Moved the "kill" file into a separate cgroup subsystem (signal) and it's own patch. Changed the name of the file from freezer.freeze to freezer.state. Switched from taking 1 and 0 as input to the strings "FROZEN" and "RUNNING", respectively. This helps keep the interface human-usable if/when we need to more states. Checked that stopped or interrupted is "frozen enough" Since try_to_freeze() is called upon wakeup of these tasks this should be fine. This idea comes from recent changes to the freezer. Checked that if (task == current) whilst freezing cgroup we're ok Fixed bug where -EBUSY would always be returned when freezing Added code to handle userspace retries for any remaining -EBUSY The freezer subsystem in the container filesystem defines a file named freezer.state. Writing "FROZEN" to the state file will freeze all tasks in the cgroup. Subsequently writing "RUNNING" will unfreeze the tasks in the cgroup. Reading will return the current state. * Examples of usage : # mkdir /containers/freezer # mount -t cgroup -ofreezer,signal freezer /containers/freezer # mkdir /containers/freezer/0 # echo $some_pid > /containers/freezer/0/tasks to get status of the freezer subsystem : # cat /containers/freezer/0/freezer.state RUNNING to freeze all tasks in the container : # echo FROZEN > /containers/freezer/0/freezer.state # cat /containers/freezer/0/freezer.state FREEZING # cat /containers/freezer/0/freezer.state FROZEN to unfreeze all tasks in the container : # echo RUNNING > /containers/freezer/0/freezer.state # cat /containers/freezer/0/freezer.state RUNNING to kill all tasks in the container : # echo 9 > /containers/freezer/0/signal.kill * Caveats: - The cgroup moves into the FROZEN state once all tasks in the cgroup a…
creator: Matt Helsley
date: 2008-04-24T06:47:56-00:00

[RFC][PATCH 1/5] Container Freezer: Add TIF_FREEZE flag to all architectures
type: item
description: This patch is the first step in making the refrigerator() available to all architectures, even for those without power management. The purpose of such a change is to be able to use the refrigerator() in a new control group subsystem which will implement a control group freezer. Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Tested-by: Matt Helsley <matthltc@us.ibm.com> --- include/asm-alpha/thread_info.h | 2 ++ include/asm-avr32/thread_info.h | 2 ++ include/asm-cris/thread_info.h | 2 ++ include/asm-h8300/thread_info.h | 2 ++ include/asm-m68k/thread_info.h | 1 + include/asm-m68knommu/thread_info.h | 2 ++ include/asm-parisc/thread_info.h | 2 ++ include/asm-s390/thread_info.h | 2 ++ include/asm-sparc/thread_info.h | 2 ++ include/asm-sparc64/thread_info.h | 2 ++ include/asm-um/thread_info.h | 2 ++ include/asm-v850/thread_info.h | 2 ++ include/asm-xtensa/thread_info.h | 2 ++ 13 files changed, 25 insertions(+) Index: linux-2.6.25-mm1/include/asm-alpha/thread_info.h =================================================================== --- linux-2.6.25-mm1.orig/include/asm-alpha/thread_info.h +++ linux-2.6.25-mm1/include/asm-alpha/thread_info.h @@ -74,16 +74,18 @@ register struct thread_info *__current_t #define TIF_UAC_NOPRINT 5 /* see sysinfo.h */ #define TIF_UAC_NOFIX 6 #define TIF_UAC_SIGBUS 7 #define TIF_MEMDIE 8 #define TIF_RESTORE_SIGMASK 9 /* restore signal mask in do_signal */ +#define TIF_FREEZE 19 /* is freezing for suspend */ #define _TIF_SYSCALL_TRACE (1<<TIF_SYSCALL_TRACE) #define _TIF_SIGPENDING (1<<TIF_SIGPENDING) #define _TIF_NEED_RESCHED (1<<TIF_NEED_RESCHED) #define _TIF_POLLING_NRFLAG (1<<TIF_POLLING_NRFLAG) #define _TIF_RESTORE_SIGMASK (1<<TIF_RESTORE_SIGMASK) +#define _TIF_FREEZE (1<<TIF_FREEZE) /* Work to do on interrupt/exception return. */ #define _TIF_WORK_MASK (_TIF_SIGPENDING | _TIF_NEED_RESCHED) /* Work to do on any return to userspace. */ Index: linux-2.6.25-mm1/include/asm-avr32/thread_info.h =…
creator: Matt Helsley
date: 2008-04-24T06:47:57-00:00

[RFC][PATCH 2/5] Container Freezer: Make refrigerator always available
type: item
description: Now that the TIF_FREEZE flag is available in all architectures, extract the refrigerator() and freeze_task() from kernel/power/process.c and make it available to all. The refrigerator() can now be used in a control group subsystem implementing a control group freezer. Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Tested-by: Matt Helsley <matthltc@us.ibm.com> --- Changelog: Merged Roland's "STOPPED is frozen enough" changes. For details see: http://lkml.org/lkml/2008/3/3/676 include/linux/freezer.h | 19 ++----- kernel/Makefile | 4 - kernel/freezer.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++ kernel/power/process.c | 113 ------------------------------------------- 4 files changed, 133 insertions(+), 127 deletions(-) Index: linux-2.6.25-mm1/include/linux/freezer.h =================================================================== --- linux-2.6.25-mm1.orig/include/linux/freezer.h +++ linux-2.6.25-mm1/include/linux/freezer.h @@ -4,11 +4,10 @@ #define FREEZER_H_INCLUDED #include <linux/sched.h> #include <linux/wait.h> -#ifdef CONFIG_PM_SLEEP /* * Check if a process has been frozen */ static inline int frozen(struct task_struct *p) { @@ -61,22 +60,27 @@ static inline int thaw_process(struct ta task_unlock(p); return 0; } extern void refrigerator(void); -extern int freeze_processes(void); -extern void thaw_processes(void); static inline int try_to_freeze(void) { if (freezing(current)) { refrigerator(); return 1; } else return 0; } +extern int freeze_task(struct task_struct *p, int with_mm_only); + +#ifdef CONFIG_PM_SLEEP + +extern int freeze_processes(void); +extern void thaw_processes(void); + /* * The PF_FREEZER_SKIP flag should be set by a vfork parent right before it * calls wait_for_completion(&vfork) and reset right after it returns from this * function. Next, the parent should call try_to_freeze() to freeze itself * appropriately in case the child has exited before the freezing of tasks is @@ -156,22 +…
creator: Matt Helsley
date: 2008-04-24T06:47:58-00:00

[RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
type: item
description: This patch implements a new freezer subsystem for Paul Menage's control groups framework. It provides a way to stop and resume execution of all tasks in a cgroup by writing in the cgroup filesystem. This is the basic mechanism which should do the right thing for user space tasks in a simple scenario. This will require more work to get the freezing right (cf. try_to_freeze_tasks()) for ptraced tasks. It's important to note that freezing can be incomplete. In that case we return EBUSY. This means that some tasks in the cgroup are busy doing something that prevents us from completely freezing the cgroup at this time. After EBUSY, the cgroup will remain partially frozen -- reflected by freezer.state reporting "FREEZING" when read. The state will remain "FREEZING" until one of these things happens: 1) Userspace cancels the freezing operation by writing "RUNNING" to the freezer.state file 2) Userspace retries the freezing operation by writing "FROZEN" to the freezer.state file (writing "FREEZING" is not legal and returns EIO) 3) The tasks that blocked the cgroup from entering the "FROZEN" state disappear from the cgroup's set of tasks. Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Tested-by: Matt Helsley <matthltc@us.ibm.com> --- TODO: Check that we handle ptrace'd and vfork-ing tasks correctly. Changelog: v2: Moved the "kill" file into a separate cgroup subsystem (signal) and it's own patch. Changed the name of the file from freezer.freeze to freezer.state. Switched from taking 1 and 0 as input to the strings "FROZEN" and "RUNNING", respectively. This helps keep the interface human-usable if/when we need to more states. Checked that stopped or interrupted is "frozen enough" Since try_to_freeze() is called upon wakeup of these tasks this should be fine. This idea comes from recent changes to the freezer. Checked that if (task == current) whilst freezing cgroup we're ok Fixed bug where -EBUSY would always be returned whe…
creator: Matt Helsley
date: 2008-04-24T06:47:59-00:00

[RFC][PATCH 4/5] Container Freezer: Skip frozen cgroups during power management resume
type: item
description: When a system is resumed after a suspend, it will also unfreeze frozen cgroups. This patchs modifies the resume sequence to skip the tasks which are part of a frozen control group. Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Tested-by: Matt Helsley <matthltc@us.ibm.com> --- kernel/power/process.c | 4 ++++ 1 file changed, 4 insertions(+) Index: linux-2.6.25-mm1/kernel/power/process.c =================================================================== --- linux-2.6.25-mm1.orig/kernel/power/process.c +++ linux-2.6.25-mm1/kernel/power/process.c @@ -11,10 +11,11 @@ #include <linux/interrupt.h> #include <linux/suspend.h> #include <linux/module.h> #include <linux/syscalls.h> #include <linux/freezer.h> +#include <linux/cgroup_freezer.h> /* * Timeout for stopping processes */ #define TIMEOUT (20 * HZ) @@ -150,10 +151,13 @@ static void thaw_tasks(int thaw_user_spa continue; if (!p->mm == thaw_user_space) continue; + if (cgroup_frozen(p)) + continue; + thaw_process(p); } while_each_thread(g, p); read_unlock(&tasklist_lock); } -- _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Matt Helsley
date: 2008-04-24T06:48:00-00:00

[RFC][PATCH 5/5] Add a Signal Control Group Subsystem
type: item
description: Add a signal control group subsystem that allows us to send signals to all tasks in the control group by writing the desired signal(7) number to the kill file. NOTE: We don't really need per-cgroup state, but control groups doesn't support stateless subsystems yet. Signed-off-by: Matt Helsley <matthltc@us.ibm.com> --- include/linux/cgroup_signal.h | 28 +++++++++ include/linux/cgroup_subsys.h | 6 + init/Kconfig | 6 + kernel/Makefile | 1 kernel/cgroup_signal.c | 129 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 170 insertions(+) Index: linux-2.6.25-mm1/include/linux/cgroup_signal.h =================================================================== --- /dev/null +++ linux-2.6.25-mm1/include/linux/cgroup_signal.h @@ -0,0 +1,28 @@ +#ifndef _LINUX_CGROUP_SIGNAL_H +#define _LINUX_CGROUP_SIGNAL_H +/* + * cgroup_signal.h - control group freezer subsystem interface + * + * Copyright IBM Corp. 2007 + * + * Author : Cedric Le Goater <clg@fr.ibm.com> + * Author : Matt Helsley <matthltc@us.ibm.com> + */ + +#include <linux/cgroup.h> + +#ifdef CONFIG_CGROUP_SIGNAL + +struct stateless { + struct cgroup_subsys_state css; +}; + +static inline struct stateless *cgroup_signal(struct cgroup *cgroup) +{ + return container_of(cgroup_subsys_state(cgroup, signal_subsys_id), + struct stateless, css); +} + +#else /* !CONFIG_CGROUP_SIGNAL */ +#endif /* !CONFIG_CGROUP_SIGNAL */ +#endif /* _LINUX_CGROUP_SIGNAL_H */ Index: linux-2.6.25-mm1/kernel/cgroup_signal.c =================================================================== --- /dev/null +++ linux-2.6.25-mm1/kernel/cgroup_signal.c @@ -0,0 +1,129 @@ +/* + * cgroup_signal.c - control group signal subsystem + * + * Copyright IBM Corp. 2007 + * + * Author : Cedric Le Goater <clg@fr.ibm.com> + * Author : Matt Helsley <matthltc@us.ibm.com> + */ + +#include <linux/module.h> +#include <linux/cgroup.h> +#include <linux/fs.h> +#include <linux/uaccess.h> +#include <linux/cgroup_signal.h> + +struct cgroup_subsys signal_subsys; + +static st…
creator: Matt Helsley
date: 2008-04-24T06:48:01-00:00

Re: [RFC][PATCH 1/5] Container Freezer: Add TIF_FREEZE flag to all architectures
type: item
description: On Wed 2008-04-23 23:47:57, Matt Helsley wrote: > This patch is the first step in making the refrigerator() available > to all architectures, even for those without power management. > > The purpose of such a change is to be able to use the refrigerator() > in a new control group subsystem which will implement a control group > freezer. > > Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> > Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > Tested-by: Matt Helsley <matthltc@us.ibm.com> ACK. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pavel Machek
date: 2008-04-24T08:09:18-00:00

Re: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
type: item
description: > +static struct cftype kill_file = { > + .name = "kill", The name "kill" seems ambiguous to me. It suggests that any write will send some default signal (TERM or KILL?) to all tasks in the cgroup, rather like the 'killall' command. I'm guessing that more people, on seeing this file in a cgroup directory, will guess correctly what it does if it were named "signal" or "send_signal" or some such. -- 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-04-24T19:30:32-00:00

Re: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
type: item
description: >+static const char *freezer_state_strs[] = { >+ "RUNNING\n", >+ "FREEZING\n" , >+ "FROZEN\n" >+}; I think it might be cleaner to not include the \n characters in this array. >+static inline int cgroup_frozen(struct task_struct *task) >+{ >+ struct cgroup *cgroup = task_cgroup(task, freezer_subsys_id); >+ struct freezer *freezer = cgroup_freezer(cgroup); >+ enum freezer_state state; >+ >+ spin_lock(&freezer->lock); >+ state = freezer->state; >+ spin_unlock(&freezer->lock); >+ >+ return (state == STATE_FROZEN); >+} You need to be in an RCU critical section or else hold task_lock() in order to dereference the cgroup returned from task_cgroup() I'm not sure that you need to take freezer->lock here - you're just reading a single word. >+ >+ if (!capable(CAP_SYS_ADMIN)) >+ return ERR_PTR(-EPERM); >+ Why does everyone keep throwing calls to check CAP_SYS_ADMIN into their cgroup create callbacks? You have to be root in order to mount a cgroups hierarchy in the first place, and filesystem permissions will control who can create new cgroups. >+static int freezer_can_attach(struct cgroup_subsys *ss, >+ struct cgroup *new_cgroup, >+ struct task_struct *task) >+{ >+ struct freezer *freezer = cgroup_freezer(new_cgroup); >+ int retval = 0; >+ >+ if (freezer->state == STATE_FROZEN) >+ retval = -EBUSY; >+ >+ return retval; >+} You should comment here that the call to cgroup_lock() in the freezer.state write method prevents a write to that file racing against an attach, and hence the can_attach() result will remain valid until the attach completes. >+static ssize_t freezer_write(struct cgroup *cgroup, >+ struct cftype *cft, >+ struct file *file, >+ const char __user *userbuf, >+ size_t nbytes, loff_t *unused_ppos) >+{ >+ char *buffer; >+ int retval = 0; >+ enum freezer_state goal_state; >+ >+ if (nbytes >= PATH_MAX) >+ return -E2BIG; >+ >+ /* +1 for nul-terminator */ >+ buffer = kmalloc(nbytes + 1, GFP_KERNEL); >+ if (buffer == NULL) >+ return -ENOMEM; Given that you're copying a st…
creator: Paul Menage
date: 2008-04-25T05:51:57-00:00

Re: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
type: item
description: I don't think you need cgroup_signal.h. It's only included in cgroup_signal.c, and doesn't really contain any useful definitions anyway. You should just use a cgroup_subsys_state object as your state object, since you'll never need to do anything with it anyway. >+static struct cgroup_subsys_state *signal_create( >+ struct cgroup_subsys *ss, struct cgroup *cgroup) >+{ >+ struct stateless *dummy; >+ >+ if (!capable(CAP_SYS_ADMIN)) >+ return ERR_PTR(-EPERM); This is unnecessary. >+ + dummy = kzalloc(sizeof(struct stateless), GFP_KERNEL); + if (!dummy) + return ERR_PTR(-ENOMEM); + return &dummy->css; +} This function could be simplified to: struct cgroup_subsys_state *css; css = kzalloc(sizeof(*css), GFP_KERNEL); return css ?: ERR_PTR(-ENOMEM); >+static int signal_can_attach(struct cgroup_subsys *ss, >+ struct cgroup *new_cgroup, >+ struct task_struct *task) >+{ >+ return 0; >+} No need for a can_attach() method if it just returns 0 - that's the default. >+static int signal_kill(struct cgroup *cgroup, int signum) >+{ >+ struct cgroup_iter it; >+ struct task_struct *task; >+ int retval = 0; >+ >+ cgroup_iter_start(cgroup, &it); >+ while ((task = cgroup_iter_next(cgroup, &it))) { >+ retval = send_sig(signum, task, 1); >+ if (retval) >+ break; >+ } >+ cgroup_iter_end(cgroup, &it); >+ >+ return retval; >+} cgroup_iter_start() takes a read lock - is send_sig() guaranteed not to sleep? >+static ssize_t signal_write(struct cgroup *cgroup, >+ struct cftype *cft, >+ struct file *file, >+ const char __user *userbuf, >+ size_t nbytes, loff_t *unused_ppos) This should just be a write_u64() method - cgroups will handle the copying/parsing for you. See e.g. kernel/sched.c:cpu_shares_write_u64() >+static struct cftype kill_file = { >+ .name = "kill", >+ .write = signal_write, >+ .private = 0, >+}; I agree with PaulJ that "signal.send" would be a nicer name for this than "signal.kill" _______________________________________________ Containers mailing list Containers@lists.linux-founda…
creator: Paul Menage
date: 2008-04-25T06:01:46-00:00

Re: [RFC][PATCH 2/5] Container Freezer: Make refrigerator always available
type: item
description: Hi! > Now that the TIF_FREEZE flag is available in all architectures, > extract the refrigerator() and freeze_task() from kernel/power/process.c > and make it available to all. > > The refrigerator() can now be used in a control group subsystem > implementing a control group freezer. > > Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> > Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > Tested-by: Matt Helsley <matthltc@us.ibm.com> There's no problem with doing this... but you should get some debate (with Linus?) whether using freezer for cgroups is sane. When that is done, there's no problem with this going in, probably through rafael's patch queue. (The first patch -- add freezer for all archs -- is probably reasonably to go in ASAP, through akpm or something...) Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pavel Machek
date: 2008-04-25T11:04:56-00:00

Re: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
type: item
description: Matt Helsley wrote: > Add a signal control group subsystem that allows us to send signals to all tasks > in the control group by writing the desired signal(7) number to the kill file. > > NOTE: We don't really need per-cgroup state, but control groups doesn't support > stateless subsystems yet. > > Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > --- > include/linux/cgroup_signal.h | 28 +++++++++ > include/linux/cgroup_subsys.h | 6 + > init/Kconfig | 6 + > kernel/Makefile | 1 > kernel/cgroup_signal.c | 129 ++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 170 insertions(+) > > Index: linux-2.6.25-mm1/include/linux/cgroup_signal.h > =================================================================== > --- /dev/null > +++ linux-2.6.25-mm1/include/linux/cgroup_signal.h > @@ -0,0 +1,28 @@ > +#ifndef _LINUX_CGROUP_SIGNAL_H > +#define _LINUX_CGROUP_SIGNAL_H > +/* > + * cgroup_signal.h - control group freezer subsystem interface s/freezer/signal/ > + * > + * Copyright IBM Corp. 2007 > + * > + * Author : Cedric Le Goater <clg@fr.ibm.com> > + * Author : Matt Helsley <matthltc@us.ibm.com> > + */ > + > +#include <linux/cgroup.h> > + > +#ifdef CONFIG_CGROUP_SIGNAL > + > +struct stateless { > + struct cgroup_subsys_state css; > +}; I'm not sure this is correct to say so. Imagine you want to send a SIGKILL to a cgroup, you would expect all tasks to die and the cgroup to become empty. right ? but if a task is doing clone() while it's being killed by this cgroup signal subsystem, we can miss the child. This is because there's a small window in copy_process() where the child is in the cgroup and not visible yet. copy_process() cgroup_fork() do stuff cgroup_fork_callbacks() cgroup_post_fork() put new task in the list. ( I didn't dig too much the code, though. So I might be missing something ) So if we want to send the signal to all tasks in the cgroup, we need to track the new tasks with a fork callback, just like the freezer : static void signal_fork(struct cgroup_sub…
creator: Cedric Le Goater
date: 2008-04-25T11:41:49-00:00

Re: [RFC][PATCH 2/5] Container Freezer: Make refrigerator always available
type: item
description: Pavel Machek wrote: > Hi! > >> Now that the TIF_FREEZE flag is available in all architectures, >> extract the refrigerator() and freeze_task() from kernel/power/process.c >> and make it available to all. >> >> The refrigerator() can now be used in a control group subsystem >> implementing a control group freezer. >> >> Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> >> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> >> Tested-by: Matt Helsley <matthltc@us.ibm.com> > > There's no problem with doing this... but you should get some debate > (with Linus?) whether using freezer for cgroups is sane. Yes that's what we are trying to know, is the fake signal mechanism used by the freezer something we can build upon ? > When that is done, there's no problem with this going in, probably > through rafael's patch queue. OK > (The first patch -- add freezer for all archs -- is probably > reasonably to go in ASAP, through akpm or something...) Thanks, C. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Cedric Le Goater
date: 2008-04-25T12:07:20-00:00

Re: [RFC][PATCH 2/5] Container Freezer: Make refrigerator always available
type: item
description: On Friday, 25 of April 2008, Cedric Le Goater wrote: > Pavel Machek wrote: > > Hi! > > > >> Now that the TIF_FREEZE flag is available in all architectures, > >> extract the refrigerator() and freeze_task() from kernel/power/process.c > >> and make it available to all. > >> > >> The refrigerator() can now be used in a control group subsystem > >> implementing a control group freezer. > >> > >> Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> > >> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > >> Tested-by: Matt Helsley <matthltc@us.ibm.com> > > > > There's no problem with doing this... but you should get some debate > > (with Linus?) whether using freezer for cgroups is sane. > > Yes that's what we are trying to know, is the fake signal mechanism > used by the freezer something we can build upon ? Well, currently, the freezer doesn't send fake signals to kernel threads which turns out to be problematic, becase some kernel threads behave very much like user space processes (basically, the threads related to NFS and CIFS). I have an idea how to fix this issue, but I'm not sure if it's acceptable overall. I'll try to prepare a patch later today or tomorrow. Thanks, Rafael _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: rjw
date: 2008-04-26T13:02:36-00:00

[RFC][PATCH] Freezer: NOSIG flag (was: Re: [RFC][PATCH 2/5] Container Freezer: Make refrigerator alw
type: item
description: On Saturday, 26 of April 2008, Rafael J. Wysocki wrote: > On Friday, 25 of April 2008, Cedric Le Goater wrote: > > Pavel Machek wrote: > > > Hi! > > > > > >> Now that the TIF_FREEZE flag is available in all architectures, > > >> extract the refrigerator() and freeze_task() from kernel/power/process.c > > >> and make it available to all. > > >> > > >> The refrigerator() can now be used in a control group subsystem > > >> implementing a control group freezer. > > >> > > >> Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> > > >> Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > > >> Tested-by: Matt Helsley <matthltc@us.ibm.com> > > > > > > There's no problem with doing this... but you should get some debate > > > (with Linus?) whether using freezer for cgroups is sane. > > > > Yes that's what we are trying to know, is the fake signal mechanism > > used by the freezer something we can build upon ? > > Well, currently, the freezer doesn't send fake signals to kernel threads which > turns out to be problematic, becase some kernel threads behave very much like > user space processes (basically, the threads related to NFS and CIFS). > > I have an idea how to fix this issue, but I'm not sure if it's acceptable > overall. I'll try to prepare a patch later today or tomorrow. Below is the patch I was talking about, the changelog says what it does. I have some candidates for using set_freezable_with_signal(), but I need to check the test cases before modifying them. Comments welcome. Thanks, Rafael --- From: Rafael J. Wysocki <rjw@sisk.pl> The freezer currently attempts to distinguish kernel threads from user space tasks by checking if their mm pointer is unset and it does not send fake signals to kernel threads. However, there are kernel threads, mostly related to networking, that behave like user space tasks and may want to be sent a fake signal to be frozen. Introduce the new process flag PF_FREEZER_NOSIG that will be set by default for all kernel threads and make the freez…
creator: rjw
date: 2008-04-26T23:32:40-00:00

Re: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
type: item
description: Quoting Paul Menage (menage@google.com): > >+static const char *freezer_state_strs[] = { > >+ "RUNNING\n", > >+ "FREEZING\n" , > >+ "FROZEN\n" > >+}; > > I think it might be cleaner to not include the \n characters in this array. > > >+static inline int cgroup_frozen(struct task_struct *task) > >+{ > >+ struct cgroup *cgroup = task_cgroup(task, freezer_subsys_id); > >+ struct freezer *freezer = cgroup_freezer(cgroup); > >+ enum freezer_state state; > >+ > >+ spin_lock(&freezer->lock); > >+ state = freezer->state; > >+ spin_unlock(&freezer->lock); > >+ > >+ return (state == STATE_FROZEN); > >+} > > You need to be in an RCU critical section or else hold task_lock() in > order to dereference the cgroup returned from task_cgroup() > > I'm not sure that you need to take freezer->lock here - you're just > reading a single word. > > >+ > >+ if (!capable(CAP_SYS_ADMIN)) > >+ return ERR_PTR(-EPERM); > >+ > > Why does everyone keep throwing calls to check CAP_SYS_ADMIN into > their cgroup create callbacks? You have to be root in order to mount a > cgroups hierarchy in the first place, and filesystem permissions will > control who can create new cgroups. The scourge of cut-n-paste :) Except I'm thinking that the check should be taken out of even kernel/ns_cgroup.c:ns_create(), which I think is where that all began. The reason why tossing these in is bad is that it requires us to give *away* extra privilege. > >+static int freezer_can_attach(struct cgroup_subsys *ss, > >+ struct cgroup *new_cgroup, > >+ struct task_struct *task) > >+{ > >+ struct freezer *freezer = cgroup_freezer(new_cgroup); > >+ int retval = 0; > >+ > >+ if (freezer->state == STATE_FROZEN) > >+ retval = -EBUSY; > >+ > >+ return retval; > >+} > > You should comment here that the call to cgroup_lock() in the > freezer.state write method prevents a write to that file racing > against an attach, and hence the can_attach() result will remain valid > until the attach completes. > > >+static ssize_t freezer_write(st…
creator: serue
date: 2008-04-28T04:03:58-00:00

Re: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
type: item
description: On Thu, 2008-04-24 at 14:30 -0500, Paul Jackson wrote: > > +static struct cftype kill_file = { > > + .name = "kill", > > The name "kill" seems ambiguous to me. It suggests that any write > will send some default signal (TERM or KILL?) to all tasks in the > cgroup, rather like the 'killall' command. > > I'm guessing that more people, on seeing this file in a cgroup > directory, will guess correctly what it does if it were named > "signal" or "send_signal" or some such. OK, I renamed it signal.send and replaced all uses of "kill" in the code to indicate that we're actually sending a signal -- not "KILL"ing things :). Cheers, -Matt _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Matt Helsley
date: 2008-04-30T07:48:34-00:00

Re: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
type: item
description: Matt wrote: > OK, I renamed it signal.send I'm not familiar with the cgroup subsystem naming conventions, but modulo that, this looks good to me - thanks! -- 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-04-30T08:18:57-00:00

Re: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
type: item
description: On Thu, 2008-04-24 at 23:01 -0700, Paul Menage wrote: > I don't think you need cgroup_signal.h. It's only included in > cgroup_signal.c, and doesn't really contain any useful definitions > anyway. You should just use a cgroup_subsys_state object as your state > object, since you'll never need to do anything with it anyway. > > >+static struct cgroup_subsys_state *signal_create( > >+ struct cgroup_subsys *ss, struct cgroup *cgroup) > >+{ > >+ struct stateless *dummy; > >+ > >+ if (!capable(CAP_SYS_ADMIN)) > >+ return ERR_PTR(-EPERM); > > This is unnecessary. OK, removed. > >+ > + dummy = kzalloc(sizeof(struct stateless), GFP_KERNEL); > + if (!dummy) > + return ERR_PTR(-ENOMEM); > + return &dummy->css; > +} > > This function could be simplified to: > > struct cgroup_subsys_state *css; > css = kzalloc(sizeof(*css), GFP_KERNEL); > return css ?: ERR_PTR(-ENOMEM); I kept the if() syntax but used cgroup_subsys_state as suggested. I kept the name "dummy" too to emphasize that except for following the currently-required form we don't really need the state information. As a side note, I don't think adding state (which signal was/to sent/send) or a fork handling function prevents races. I tend to agree that there are lots of races involving adding/removing tasks to the cgroup where we may not signal "everything". I think that from userspace's perspective we can't solve the races because there will always be a window between releasing whatever lock we use and returning to userspace where new tasks can be added. IMHO the only way to prevent such races would be to allow userspace to "lock" a cgroup so that no new tasks may be added. That would block/fail new forks and prevent writing new pids to the tasks file. Otherwise userspace must always recheck the list to ensure it didn't get any new entries... > >+static int signal_can_attach(struct cgroup_subsys *ss, > >+ struct cgroup *new_cgroup, > >+ struct task_struct *task) > >+{ > >+ return 0; > >+} > > No need for a can_attach() m…
creator: Matt Helsley
date: 2008-04-30T08:29:08-00:00

Re: [RFC][PATCH 2/5] Container Freezer: Make refrigerator always available
type: item
description: On Fri, 2008-04-25 at 13:04 +0200, Pavel Machek wrote: > Hi! > > > Now that the TIF_FREEZE flag is available in all architectures, > > extract the refrigerator() and freeze_task() from kernel/power/process.c > > and make it available to all. > > > > The refrigerator() can now be used in a control group subsystem > > implementing a control group freezer. > > > > Signed-off-by: Cedric Le Goater <clg@fr.ibm.com> > > Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > > Tested-by: Matt Helsley <matthltc@us.ibm.com> > > There's no problem with doing this... but you should get some debate > (with Linus?) whether using freezer for cgroups is sane. When that is OK, I've sent this reply directly to Linus. Hopefully this time he'll let us know... One potential mitigating factor: I don't think we need the full freezer for checkpoint/restart. Right now, because it shares code with "power management" it's convenient to reuse the freezer. I'm hopeful that once the freezer is no longer necessary for power management some code paths can be simplified since I don't think checkpoint/restart requires freezing kernel threads. > done, there's no problem with this going in, probably through rafael's > patch queue. OK, if all goes well then I'll send the next round to Rafael and Cc the rest. If anyone currently on Cc doesn't care to see that feel free to let me know. > (The first patch -- add freezer for all archs -- is probably > reasonably to go in ASAP, through akpm or something...) > Pavel Well, that should only go in if the subsequent patches go in, correct? Also, since to the best of my knowledge this flag hasn't been in every arch, I'm wondering if I should Cc arch maintainers? Cheers, -Matt Helsley _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Matt Helsley
date: 2008-04-30T09:08:29-00:00

Re: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
type: item
description: On Thu, 2008-04-24 at 22:51 -0700, Paul Menage wrote: > >+static const char *freezer_state_strs[] = { > >+ "RUNNING\n", > >+ "FREEZING\n" , > >+ "FROZEN\n" > >+}; > > I think it might be cleaner to not include the \n characters in this array. Sure. Though that might produce weird output from simple_read_from_buffer() -- no newline. I've switched this and the strcmp() code below. > >+static inline int cgroup_frozen(struct task_struct *task) > >+{ > >+ struct cgroup *cgroup = task_cgroup(task, freezer_subsys_id); > >+ struct freezer *freezer = cgroup_freezer(cgroup); > >+ enum freezer_state state; > >+ > >+ spin_lock(&freezer->lock); > >+ state = freezer->state; > >+ spin_unlock(&freezer->lock); > >+ > >+ return (state == STATE_FROZEN); > >+} > > You need to be in an RCU critical section or else hold task_lock() in > order to dereference the cgroup returned from task_cgroup() What are the rules of using subsystem pointers from the cgroup? Suppose I did: rcu_read_lock(); cgroup = task_cgroup(task, freezer_subsys_id); freezer = cgroup_freezer(cgroup); state = freezer->state; rcu_read_unlock(); return (state == STATE_FROZEN); (And guard writes to freezer->state with the freezer->lock) ? > I'm not sure that you need to take freezer->lock here - you're just > reading a single word. Doesn't the safety of that assumption depend on the architecture _and_ compiler? > >+ > >+ if (!capable(CAP_SYS_ADMIN)) > >+ return ERR_PTR(-EPERM); > >+ > > Why does everyone keep throwing calls to check CAP_SYS_ADMIN into > their cgroup create callbacks? You have to be root in order to mount a > cgroups hierarchy in the first place, and filesystem permissions will > control who can create new cgroups. Removed. > >+static int freezer_can_attach(struct cgroup_subsys *ss, > >+ struct cgroup *new_cgroup, > >+ struct task_struct *task) > >+{ > >+ struct freezer *freezer = cgroup_freezer(new_cgroup); > >+ int retval = 0; > >+ > >+ if (freezer->state == STATE_FROZEN) > >+ retval = -EBUSY; > >+ > >+ r…
creator: Matt Helsley
date: 2008-04-30T10:39:45-00:00

Re: [RFC][PATCH 5/5] Add a Signal Control Group Subsystem
type: item
description: On Fri, 2008-04-25 at 13:41 +0200, Cedric Le Goater wrote: > Matt Helsley wrote: > > Add a signal control group subsystem that allows us to send signals to all tasks > > in the control group by writing the desired signal(7) number to the kill file. > > > > NOTE: We don't really need per-cgroup state, but control groups doesn't support > > stateless subsystems yet. > > > > Signed-off-by: Matt Helsley <matthltc@us.ibm.com> > > --- > > include/linux/cgroup_signal.h | 28 +++++++++ > > include/linux/cgroup_subsys.h | 6 + > > init/Kconfig | 6 + > > kernel/Makefile | 1 > > kernel/cgroup_signal.c | 129 ++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 170 insertions(+) > > > > Index: linux-2.6.25-mm1/include/linux/cgroup_signal.h > > =================================================================== > > --- /dev/null > > +++ linux-2.6.25-mm1/include/linux/cgroup_signal.h > > @@ -0,0 +1,28 @@ > > +#ifndef _LINUX_CGROUP_SIGNAL_H > > +#define _LINUX_CGROUP_SIGNAL_H > > +/* > > + * cgroup_signal.h - control group freezer subsystem interface > > s/freezer/signal/ > > > + * > > + * Copyright IBM Corp. 2007 > > + * > > + * Author : Cedric Le Goater <clg@fr.ibm.com> > > + * Author : Matt Helsley <matthltc@us.ibm.com> > > + */ > > + > > +#include <linux/cgroup.h> > > + > > +#ifdef CONFIG_CGROUP_SIGNAL > > + > > +struct stateless { > > + struct cgroup_subsys_state css; > > +}; > > I'm not sure this is correct to say so. Imagine you want to send > a SIGKILL to a cgroup, you would expect all tasks to die and the > cgroup to become empty. right ? > > but if a task is doing clone() while it's being killed by this cgroup > signal subsystem, we can miss the child. This is because there's a > small window in copy_process() where the child is in the cgroup and > not visible yet. > > copy_process() > cgroup_fork() > do stuff > cgroup_fork_callbacks() > > cgroup_post_fork() > put new task in the list. > > ( I didn't dig too much the code, though. So I might be missing > somethin…
creator: Matt Helsley
date: 2008-04-30T18:44:57-00:00

Re: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
type: item
description: On Wed, 2008-04-23 at 23:47 -0700, Matt Helsley wrote: <snip> > +static ssize_t freezer_write(struct cgroup *cgroup, > + struct cftype *cft, > + struct file *file, > + const char __user *userbuf, > + size_t nbytes, loff_t *unused_ppos) > +{ > + char *buffer; > + int retval = 0; > + enum freezer_state goal_state; > + > + if (nbytes >= PATH_MAX) > + return -E2BIG; > + > + /* +1 for nul-terminator */ > + buffer = kmalloc(nbytes + 1, GFP_KERNEL); > + if (buffer == NULL) > + return -ENOMEM; > + > + if (copy_from_user(buffer, userbuf, nbytes)) { > + retval = -EFAULT; > + goto free_buffer; > + } > + buffer[nbytes] = 0; /* nul-terminate */ > + strstrip(buffer); > + if (strcmp(buffer, "RUNNING") == 0) > + goal_state = STATE_RUNNING; > + else if (strcmp(buffer, "FROZEN") == 0) > + goal_state = STATE_FROZEN; > + else { > + retval = -EIO; > + goto free_buffer; > + } > + > + cgroup_lock(); > + > + if (cgroup_is_removed(cgroup)) { > + retval = -ENODEV; > + goto unlock; > + } I think this was copy/paste'd from cgroup_common_file_write() which modifies the cgroup hierarchy. However this function does not modify the cgroup hierarchy and we're not getting the cgroup from the task. So I don't think cgroup_lock()/unlock() are needed here. Paul, do you agree? > + retval = freezer_freeze(cgroup, goal_state); > + if (retval == 0) > + retval = nbytes; > +unlock: > + cgroup_unlock(); > +free_buffer: > + kfree(buffer); > + return retval; > +} Cheers, -Matt Helsley _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Matt Helsley
date: 2008-04-30T21:28:25-00:00

Re: [RFC][PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
type: item
description: On Wed, 2008-04-30 at 14:28 -0700, Matt Helsley wrote: > On Wed, 2008-04-23 at 23:47 -0700, Matt Helsley wrote: > > <snip> > > + cgroup_lock(); > > + > > + if (cgroup_is_removed(cgroup)) { > > + retval = -ENODEV; > > + goto unlock; > > + } > > I think this was copy/paste'd from cgroup_common_file_write() which > modifies the cgroup hierarchy. However this function does not modify the > cgroup hierarchy and we're not getting the cgroup from the task. So I > don't think cgroup_lock()/unlock() are needed here. Paul, do you agree? Oops! I didn't track the call chain correctly. Sorry for the noise... Cheers, -Matt Helsley _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Matt Helsley
date: 2008-04-30T22:30:49-00:00