Re: [PATCH 7/7] cgroups: Add a max number of tasks subsystem

Paul Menage
2011-07-26T01:17:38Z

On Mon, Jul 11, 2011 at 7:15 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>  /* */
> +
> +#ifdef CONFIG_CGROUP_MAX_TASKS
> +SUBSYS(max_tasks)
> +#endif

I think I'd be inclined to make the naming slightly (IMO) cleaner:
call the subsystem 'tasks' and the files 'limit' and 'count' (or maybe
'usage' to be more in line with memcg).
>
> +config CGROUP_MAX_TASKS
> +        bool "Control max number of tasks in a cgroup"
> +       depends on RESOURCE_COUNTERS
> +       help
> +         This option let the user to set up an upper bound allowed number
> +         of tasks.
> +

Needs to depend on CGROUPS too?

> +
> +
> +struct task_counter {
> +       struct res_counter              res;
> +       struct cgroup_subsys_state      css;
> +};

All other CSS structures put the "css" field as the first thing in the
structure. Not sure that anything relies on that, but consistency
can't hurt and it probably makes the code fractionally smaller since
the translation between CSS and task_counter becomes a no-op.

> +static void task_counter_exit(struct cgroup_subsys *ss, struct cgroup *cgrp,
> +                             struct cgroup *old_cgrp, struct task_struct *task)
> +{
> +       struct task_counter *cnt = cgroup_task_counter(old_cgrp);
> +
> +       if (cnt != &root_counter)
> +               res_counter_uncharge_until(&cnt->res, &root_counter.res, 1);
> +}

Do we even need the root_counter to be exposed in any way? Why not
make children of the root cgroup just have a NULL parent res_counter?

You'll still need a task_counter object so that the cgroups framework
has a CSS object for cgroups housekeeping, but I don't think it needs
to actually be used for anything.

Paul