[Devel] Re: [PATCH 4/7] [RFC] Allow cgroup hierarchies to be created with no bound subsystems
Paul Menage
menage at google.com
Tue Mar 31 01:45:57 PDT 2009
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
More information about the Devel
mailing list