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

[Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>]

Paul Menage wrote:
> On Mon, Mar 16, 2009 at 11:45 PM, Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> 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.
> 

agreed

>>> +     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.
> 

Makes sense, but it would be better to add a comment.

>> 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".
> 

I see your poing. Again, better to have a comment in the code.

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

Hmm, I just had a try, and I found we don't need to worry about this,
seqfile can handle output that larger than PAGE_SIZE well.