Paul Menage wrote:
> On Mon, Mar 16, 2009 at 11:45 PM, Li Zefan <lizf@xxxxxxxxxxxxxx> 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.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
