From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Menage Subject: Re: [PATCH 4/7] [RFC] Allow cgroup hierarchies to be created with no bound subsystems Date: Tue, 31 Mar 2009 01:45:57 -0700 Message-ID: <6599ad830903310145l7b24ad0vb4462f94390ac264@mail.gmail.com> References: <20090312104507.24154.71691.stgit@menage.corp.google.com> <20090312105137.24154.34890.stgit@menage.corp.google.com> <49BF470D.8080600@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <49BF470D.8080600-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Li Zefan Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org List-Id: containers.vger.kernel.org On Mon, Mar 16, 2009 at 11:45 PM, Li Zefan 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