lkml.org 
[lkml]   [2008]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem
    On Mon, 11 Aug 2008 16:53:26 -0700
    Matt Helsley <matthltc@us.ibm.com> wrote:

    > This patch implements a new freezer subsystem in the control groups framework.
    > It provides a way to stop and resume execution of all tasks in a cgroup by
    > writing in the cgroup filesystem.
    >
    > 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 freezer /containers
    > # mkdir /containers/0
    > # echo $some_pid > /containers/0/tasks
    >
    > to get status of the freezer subsystem :
    >
    > # cat /containers/0/freezer.state
    > RUNNING
    >
    > to freeze all tasks in the container :
    >
    > # echo FROZEN > /containers/0/freezer.state
    > # cat /containers/0/freezer.state
    > FREEZING
    > # cat /containers/0/freezer.state
    > FROZEN
    >
    > to unfreeze all tasks in the container :
    >
    > # echo RUNNING > /containers/0/freezer.state
    > # cat /containers/0/freezer.state
    > RUNNING
    >
    > This is the basic mechanism which should do the right thing for user space task
    > in a simple scenario.
    >
    > 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.
    >
    > ...

    Is a Documentation/ update planned? Documentation/cgroups.txt might be
    the place, or not.

    > +
    > +#ifdef CONFIG_CGROUP_FREEZER
    > +SUBSYS(freezer)
    > +#endif
    > +
    > +/* */
    > Index: linux-2.6.27-rc1-mm1/include/linux/freezer.h
    > ===================================================================
    > --- linux-2.6.27-rc1-mm1.orig/include/linux/freezer.h
    > +++ linux-2.6.27-rc1-mm1/include/linux/freezer.h
    > @@ -47,22 +47,30 @@ static inline bool should_send_signal(st
    > /*
    > * Wake up a frozen process
    > *
    > - * task_lock() is taken to prevent the race with refrigerator() which may
    > + * task_lock() is needed to prevent the race with refrigerator() which may
    > * occur if the freezing of tasks fails. Namely, without the lock, if the
    > * freezing of tasks failed, thaw_tasks() might have run before a task in
    > * refrigerator() could call frozen_process(), in which case the task would be
    > * frozen and no one would thaw it.
    > */
    > -static inline int thaw_process(struct task_struct *p)
    > +static inline int __thaw_process(struct task_struct *p)
    > {
    > - task_lock(p);
    > if (frozen(p)) {
    > p->flags &= ~PF_FROZEN;
    > + return 1;
    > + }
    > + clear_freeze_flag(p);
    > + return 0;
    > +}
    > +
    > +static inline int thaw_process(struct task_struct *p)
    > +{
    > + task_lock(p);
    > + if (__thaw_process(p) == 1) {
    > task_unlock(p);
    > wake_up_process(p);
    > return 1;
    > }
    > - clear_freeze_flag(p);
    > task_unlock(p);
    > return 0;
    > }

    I wonder why these are inlined.

    > @@ -83,6 +91,12 @@ static inline int try_to_freeze(void)
    > extern bool freeze_task(struct task_struct *p, bool sig_only);
    > extern void cancel_freezing(struct task_struct *p);
    >
    > +#ifdef CONFIG_CGROUP_FREEZER
    > +extern int cgroup_frozen(struct task_struct *task);
    > +#else /* !CONFIG_CGROUP_FREEZER */
    > +static inline int cgroup_frozen(struct task_struct *task) { return 0; }
    > +#endif /* !CONFIG_CGROUP_FREEZER */
    > +
    > /*
    > * 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
    > Index: linux-2.6.27-rc1-mm1/init/Kconfig
    > ===================================================================
    > --- linux-2.6.27-rc1-mm1.orig/init/Kconfig
    > +++ linux-2.6.27-rc1-mm1/init/Kconfig
    > @@ -299,6 +299,13 @@ config CGROUP_NS
    > for instance virtual servers and checkpoint/restart
    > jobs.
    >
    > +config CGROUP_FREEZER
    > + bool "control group freezer subsystem"
    > + depends on CGROUPS

    Should it depend on FREEZER also?

    oh,

    > --- linux-2.6.27-rc1-mm1.orig/kernel/power/Kconfig
    > +++ linux-2.6.27-rc1-mm1/kernel/power/Kconfig
    > @@ -86,7 +86,7 @@ config PM_SLEEP
    > default y
    >
    > config FREEZER
    > - def_bool PM_SLEEP
    > + def_bool PM_SLEEP || CGROUP_FREEZER
    >

    we did it that way. Spose that makes sense.

    > + help
    > + Provides a way to freeze and unfreeze all tasks in a
    > + cgroup.
    > +
    > config CGROUP_DEVICE
    > bool "Device controller for cgroups"
    > depends on CGROUPS && EXPERIMENTAL
    > Index: linux-2.6.27-rc1-mm1/kernel/Makefile
    > ===================================================================
    > --- linux-2.6.27-rc1-mm1.orig/kernel/Makefile
    > +++ linux-2.6.27-rc1-mm1/kernel/Makefile
    > @@ -54,6 +54,7 @@ obj-$(CONFIG_KEXEC) += kexec.o
    > obj-$(CONFIG_COMPAT) += compat.o
    > obj-$(CONFIG_CGROUPS) += cgroup.o
    > obj-$(CONFIG_CGROUP_DEBUG) += cgroup_debug.o
    > +obj-$(CONFIG_CGROUP_FREEZER) += cgroup_freezer.o
    > obj-$(CONFIG_CPUSETS) += cpuset.o
    > obj-$(CONFIG_CGROUP_NS) += ns_cgroup.o
    > obj-$(CONFIG_UTS_NS) += utsname.o
    > Index: linux-2.6.27-rc1-mm1/kernel/cgroup_freezer.c
    > ===================================================================
    > --- /dev/null
    > +++ linux-2.6.27-rc1-mm1/kernel/cgroup_freezer.c
    > @@ -0,0 +1,366 @@
    > +/*
    > + * cgroup_freezer.c - control group freezer subsystem
    > + *
    > + * Copyright IBM Corporation, 2007
    > + *
    > + * Author : Cedric Le Goater <clg@fr.ibm.com>
    > + *
    > + * This program is free software; you can redistribute it and/or modify it
    > + * under the terms of version 2.1 of the GNU Lesser General Public License
    > + * as published by the Free Software Foundation.
    > + *
    > + * This program is distributed in the hope that it would be useful, but
    > + * WITHOUT ANY WARRANTY; without even the implied warranty of
    > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
    > + */
    > +
    > +#include <linux/module.h>
    > +#include <linux/cgroup.h>
    > +#include <linux/fs.h>
    > +#include <linux/uaccess.h>
    > +#include <linux/freezer.h>
    > +#include <linux/seq_file.h>
    > +
    > +enum freezer_state {
    > + STATE_RUNNING = 0,

    That's a pretty vanilla-sounding identifier. Let's hope this file
    never ends up including drivers/net/sfc/net_driver.h by some means.
    That's rather unlikely, but someone could easily choose to implement a
    new STATE_RUNNING somewhere else.

    > + STATE_FREEZING,
    > + STATE_FROZEN,
    > +};
    > +
    > +struct freezer {
    > + struct cgroup_subsys_state css;
    > + enum freezer_state state;
    > + spinlock_t lock; /* protects _writes_ to state */
    > +};
    > +
    > +static inline struct freezer *cgroup_freezer(
    > + struct cgroup *cgroup)
    > +{
    > + return container_of(
    > + cgroup_subsys_state(cgroup, freezer_subsys_id),
    > + struct freezer, css);
    > +}
    > +
    > +static inline struct freezer *task_freezer(struct task_struct *task)
    > +{
    > + return container_of(task_subsys_state(task, freezer_subsys_id),
    > + struct freezer, css);
    > +}
    > +
    > +int cgroup_frozen(struct task_struct *task)
    > +{
    > + struct freezer *freezer;
    > + enum freezer_state state;
    > +
    > + task_lock(task);
    > + freezer = task_freezer(task);
    > + state = freezer->state;
    > + task_unlock(task);
    > +
    > + return state == STATE_FROZEN;
    > +}
    > +
    > +/*
    > + * Buffer size for freezer state is limited by cgroups write_string()
    > + * interface. See cgroups code for the current size.
    > + */

    Is this comment in the correct place?

    > +static const char *freezer_state_strs[] = {
    > + "RUNNING",
    > + "FREEZING",
    > + "FROZEN",
    > +};
    > +
    >
    > ...
    >
    > +
    > +/*
    > + * caller must hold freezer->lock
    > + */
    > +static void check_if_frozen(struct cgroup *cgroup,
    > + struct freezer *freezer)

    check_if_frozen() is an unfortunate name, I suspect. Normally one
    would expect a check_foo() to return a bool and have no side-effects.

    Perhaps some comments explaining what it does would help.

    > +{
    > + struct cgroup_iter it;
    > + struct task_struct *task;
    > + unsigned int nfrozen = 0, ntotal = 0;
    > +
    > + cgroup_iter_start(cgroup, &it);
    > + while ((task = cgroup_iter_next(cgroup, &it))) {
    > + ntotal++;
    > + /*
    > + * Task is frozen or will freeze immediately when next it gets
    > + * woken
    > + */
    > + if (frozen(task) ||
    > + (task_is_stopped_or_traced(task) && freezing(task)))
    > + nfrozen++;
    > + }
    > +
    > + /*
    > + * Transition to FROZEN when no new tasks can be added ensures
    > + * that we never exist in the FROZEN state while there are unfrozen
    > + * tasks.
    > + */
    > + if (nfrozen == ntotal)
    > + freezer->state = STATE_FROZEN;
    > + cgroup_iter_end(cgroup, &it);
    > +}
    > +
    >
    > ...
    >
    > +static int freezer_write(struct cgroup *cgroup,
    > + struct cftype *cft,
    > + const char *buffer)
    > +{
    > + int retval;
    > + enum freezer_state goal_state;
    > +
    > + if (strcmp(buffer, freezer_state_strs[STATE_RUNNING]) == 0)

    Did some higher-level code take care of removing the trailing \n?

    > + goal_state = STATE_RUNNING;
    > + else if (strcmp(buffer, freezer_state_strs[STATE_FROZEN]) == 0)
    > + goal_state = STATE_FROZEN;
    > + else
    > + return -EIO;
    > +
    > + if (!cgroup_lock_live_group(cgroup))
    > + return -ENODEV;
    > + retval = freezer_change_state(cgroup, goal_state);
    > + cgroup_unlock();
    > + return retval;
    > +}
    > +
    >
    > ...
    >



    \
     
     \ /
      Last update: 2008-08-13 01:01    [W:0.050 / U:60.228 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site