[Devel] Re: [PATCH 4/7] [RFC] Allow cgroup hierarchies to be created with no bound subsystems

[Paul Menage]

On Mon, Mar 16, 2009 at 11:45 PM, Li Zefan <lizf at cn.fujitsu.com> wrote:
>>
>> +static void init_root_id(struct cgroupfs_root *root)
>> +{
>> +     BUG_ON(!ida_pre_get(&hierarchy_ida, GFP_KERNEL));
>
> we should return -errno, BUG_ON() on this is wrong

Hmm. Fair enough in the new root case, since it's possible we'd not
get the memory then - I've changed init_root_id() to return false on
failure. But I think that in cgroup_init(), we need to
BUG_ON(!init_root_id()) - we shouldn't be able to fail that early in
boot and there's no sensible way to handle it if it happens.

>
>> +     spin_lock(&hierarchy_id_lock);
>> +     if (ida_get_new_above(&hierarchy_ida, next_hierarchy_id,
>> +                           &root->hierarchy_id)) {
>> +             BUG_ON(ida_get_new(&hierarchy_ida, &root->hierarchy_id));
>> +     }
>
> next_hierarchy_id is redundant, just use ida_get_new() instead of
> ida_get_new_above()

>From the idr.c code it looks as though ida_get_new() always returns
the smallest unused ID. I want to cycle through increasing IDs until
we hit the end of the range, and then start again at 0.

>
> and I think we should check EAGAIN:

OK, I've changed this to handle EAGAIN. I don't see any value in
handling ENOSPC though - we're not going to fill up a 31-bit IDR.

>>
>> -     if (!opts->subsys_bits)
>> +     if (!opts->subsys_bits && !opts->none)
>>               return ERR_PTR(-EINVAL);
>>
>
> Shouldn't we check this in parse_cgroupfs_options() instead?

No, becase at that point it's valid - we can request the root named X
without specifying what subsystems X is already mounted with. But if
we don't find a root named X, then we're creating a new root, and we
have to have specified a set of subsystems, or else explicitly "none".

>>
>> +static void cgroup_drop_root(struct cgroupfs_root *root)
>> +{
>> +     BUG_ON(!root->hierarchy_id);
>
> I think this BUG_ON() is redundant, if root->hierarchy_id == 0,
> we are freeing the dummy root, and kfree(root) should trigger
> kernel oops.

It's more obvious this way.

>> +             seq_printf(seq, "css_set %p\n", cg);
>> +             list_for_each_entry_safe(task, saved_task, &cg->tasks,
>> +                                      cg_list) {
>> +                     if (count++ > MAX_TASKS_SHOWN_PER_CSS) {
>
> actually this prints at most 26 tasks but not 25

Not really a big deal :-)

>
> and what's the concern to not print out all the tasks? the buffer
> limit of seqfile?

Yes.

>
>> +                             seq_puts(seq, "  ...\n");
>> +                             break;
>> +                     } else {
>> +                             seq_printf(seq, "  task %d\n", task->pid);
>
> I guess we should call task_pid_vnr(tsk) instead of accessing task->pid
> directly.

Changed.

Paul
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers