Re: [PATCH 3/5] Container Freezer: Implement freezer cgroup subsystem

[Paul Menage <menage@google.com>]

On Mon, Aug 11, 2008 at 3:53 PM, Matt Helsley <matthltc@us.ibm.com> wrote:
> +}
> +
> +static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
> +{
> +       struct freezer *freezer;
> +
> +       task_lock(task);
> +       freezer = task_freezer(task);
> +       task_unlock(task);
> +
> +       BUG_ON(freezer->state == STATE_FROZEN);
> +       spin_lock_irq(&freezer->lock);
> +       /* Locking avoids race with FREEZING -> RUNNING transitions. */
> +       if (freezer->state == STATE_FREEZING)
> +               freeze_task(task, true);
> +       spin_unlock_irq(&freezer->lock);
> +}

Sorry for such a delayed response to this patch, but I just noticed
(in mainline now)
the change to move the task_lock() to only encompass the
task_freezer() call.

That results in absolutely zero protection from the task_lock() - as
soon as you drop it, then in theory the current task could move cgroup
and the old freezer structure be freed.

Having said that, I think that in this case any locking my be
unnecessary since task isn't on the tasklist yet, so can't be selected
to move cgroups. (Although this does make me wonder whether
cpuset.c:move_member_tasks_to_cpuset() can fail silently if it races
with a fork).

On top of that, for a system that configures in the cgroup freezer
system but doesn't ever use it, every task is in the same freezer
cgroup (the root cgroup) and task_freezer(task)->lock becomes a global
spinlock. I think this would certainly show up on some benchmarks
although I don't know how bad it would be in a practical sense. But it
might be worth considering making using of the cgroup bind() callback
to track whether or not the freezer subsystem is in use, and
short-circuiting this in freezer_fork() without any locking if you're
not active.

Paul