Re: [PATCH 13/16] cgroup_freezer: prepare for removal of TIF_FREEZE

[Paul Menage]

On Fri, Aug 19, 2011 at 7:16 AM, Tejun Heo <tj@xxxxxxxxxx> wrote:
> TIF_FREEZE will be removed soon and freezing() will directly test
> whether any freezing condition is in effect.  Make the following
> changes in preparation.
>
> * Rename cgroup_freezing_or_frozen() to cgroup_freezing() and make it
>  return bool.
>
> * Make cgroup_freezing() access task_freezer() under rcu read lock
>  instead of task_lock().  This makes the state dereferencing racy
>  against task moving to another cgroup; however, it was already racy
>  without this change as ->state dereference wasn't synchronized.
>  This will be later dealt with using attach hooks.
>
> * freezer->state is now set before trying to push tasks into the
>  target state.
>
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> Cc: Paul Menage <menage@xxxxxxxxxx>

Acked-by: Paul Menage <paul@xxxxxxxxxxxxxx>


> Cc: Li Zefan <lizf@xxxxxxxxxxxxxx>
> ---
>  include/linux/freezer.h |    6 +++---
>  kernel/cgroup_freezer.c |   36 ++++++++++++------------------------
>  kernel/power/process.c  |    2 +-
>  3 files changed, 16 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 13559a8..bcc0637 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -63,11 +63,11 @@ extern bool freeze_task(struct task_struct *p, bool sig_only);
>  extern bool __set_freezable(bool with_signal);
>
>  #ifdef CONFIG_CGROUP_FREEZER
> -extern int cgroup_freezing_or_frozen(struct task_struct *task);
> +extern bool cgroup_freezing(struct task_struct *task);
>  #else /* !CONFIG_CGROUP_FREEZER */
> -static inline int cgroup_freezing_or_frozen(struct task_struct *task)
> +static inline bool cgroup_freezing(struct task_struct *task)
>  {
> -       return 0;
> +       return false;
>  }
>  #endif /* !CONFIG_CGROUP_FREEZER */
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index e7fa0ec..0478c35 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -48,19 +48,17 @@ static inline struct freezer *task_freezer(struct task_struct *task)
>                            struct freezer, css);
>  }
>
> -static inline int __cgroup_freezing_or_frozen(struct task_struct *task)
> +bool cgroup_freezing(struct task_struct *task)
>  {
> -       enum freezer_state state = task_freezer(task)->state;
> -       return (state == CGROUP_FREEZING) || (state == CGROUP_FROZEN);
> -}
> +       enum freezer_state state;
> +       bool ret;
>
> -int cgroup_freezing_or_frozen(struct task_struct *task)
> -{
> -       int result;
> -       task_lock(task);
> -       result = __cgroup_freezing_or_frozen(task);
> -       task_unlock(task);
> -       return result;
> +       rcu_read_lock();
> +       state = task_freezer(task)->state;
> +       ret = state == CGROUP_FREEZING || state == CGROUP_FROZEN;
> +       rcu_read_unlock();
> +
> +       return ret;
>  }
>
>  /*
> @@ -102,9 +100,6 @@ struct cgroup_subsys freezer_subsys;
>  * freezer_can_attach():
>  * cgroup_mutex (held by caller of can_attach)
>  *
> - * cgroup_freezing_or_frozen():
> - * task->alloc_lock (to get task's cgroup)
> - *
>  * freezer_fork() (preserving fork() performance means can't take cgroup_mutex):
>  * freezer->lock
>  *  sighand->siglock (if the cgroup is freezing)
> @@ -177,13 +172,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>
>  static int freezer_can_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  {
> -       rcu_read_lock();
> -       if (__cgroup_freezing_or_frozen(tsk)) {
> -               rcu_read_unlock();
> -               return -EBUSY;
> -       }
> -       rcu_read_unlock();
> -       return 0;
> +       return cgroup_freezing(tsk) ? -EBUSY : 0;
>  }
>
>  static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
> @@ -279,7 +268,6 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
>        struct task_struct *task;
>        unsigned int num_cant_freeze_now = 0;
>
> -       freezer->state = CGROUP_FREEZING;
>        cgroup_iter_start(cgroup, &it);
>        while ((task = cgroup_iter_next(cgroup, &it))) {
>                if (!freeze_task(task, true))
> @@ -303,8 +291,6 @@ static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
>        while ((task = cgroup_iter_next(cgroup, &it)))
>                __thaw_task(task);
>        cgroup_iter_end(cgroup, &it);
> -
> -       freezer->state = CGROUP_THAWED;
>  }
>
>  static int freezer_change_state(struct cgroup *cgroup,
> @@ -321,6 +307,8 @@ static int freezer_change_state(struct cgroup *cgroup,
>        if (goal_state == freezer->state)
>                goto out;
>
> +       freezer->state = goal_state;
> +
>        switch (goal_state) {
>        case CGROUP_THAWED:
>                unfreeze_cgroup(cgroup, freezer);
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index f8012f2..1bbc363 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -154,7 +154,7 @@ void thaw_processes(void)
>
>        read_lock(&tasklist_lock);
>        do_each_thread(g, p) {
> -               if (cgroup_freezing_or_frozen(p))
> +               if (cgroup_freezing(p))
>                        continue;
>
>                __thaw_task(p);
> --
> 1.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/