> -----Original Message-----
> From: Mateusz Guzik [mailto:mgu...@redhat.com]
> Sent: Wednesday, November 05, 2014 7:54 PM
> To: Chen, Hanxiao/陈 晗霄
> Cc: Eric W. Biederman; Serge Hallyn; Oleg Nesterov;
> contain...@lists.linux-foundation.org; linux-kernel@vger.kernel.org; David
> Howells; Richard Weinberger; Pavel Emelyanov; Vasiliy Kulikov
> Subject: Re: [PATCH 1/2v6] procfs: show hierarchy of pid namespace
>
> On Wed, Nov 05, 2014 at 06:41:54PM +0800, Chen Hanxiao wrote:
> > +static void free_pidns_list(struct list_head *head)
> > +{
> > + struct pidns_list *tmp, *pos;
> > +
> > + list_for_each_entry_safe(pos, tmp, head, list) {
> > + list_del(&pos->list);
>
> Any need for this one? stuff is freed anyway...
>
Yes, they're all freed.
But is that ok to leave a list full of freed stuffs..
[snip]
> > +
> > + if (flag == 0) {
> > + rcu_read_lock();
> > + get_pid(pos->pid);
> > + rcu_read_unlock();
>
> At this point you should have a valid reference for pid, so rcu should
> not matter.
>
Right.
>
> > + rc = pidns_list_add(pos->pid, pidns_pid_tree);
> > + if (rc) {
> > + put_pid(pos->pid);
> > + goto out;
> > + }
>
> '}' is misindented. Also 'out' is not a good label if it used solely for
> cleanup on error. 'out_err', 'fail' or something woud be better.
something wrong with my vimrc...
'out' is definitely not a good label.
I think 'cleanup' is a better one.
>
[snip]
> > +static int proc_pidns_list_refresh(struct pid_namespace *curr_ns,
> > + struct list_head *pidns_pid_list,
> > + struct list_head *pidns_pid_tree)
> > +{
> > + struct pid *pid;
> > + int new_nr, nr = 0;
> > + int rc;
> > +
> > + /* collect pids in current namespace */
> > + while (nr < PID_MAX_LIMIT) {
> > + rcu_read_lock();
> > + pid = find_ge_pid(nr, curr_ns);
> > + if (pid) {
> > + new_nr = pid_vnr(pid);
> > + if (!is_child_reaper(pid)) {
> > + nr = new_nr + 1;
> > + rcu_read_unlock();
> > + continue;
> > + }
> > + get_pid(pid);
> > + rcu_read_unlock();
> > + rc = pidns_list_add(pid, pidns_pid_list);
> > + if (rc) {
> > + put_pid(pid);
> > + goto out;
> > + }
> > + } else {
> > + rcu_read_unlock();
> > + break;
> > + }
> > + nr = new_nr + 1;
> > + }
> > +
>
> Would be beneficial to reorganize this loop. Handle shorter case (!pid)
> first.
OK.
>
> I consulted Dr. Grep and it told me about delayed_put_pid, so I guess
> pid itself is not going to be freed in the meantime, but this still
> seems fishy.
I found
call_rcu(&pid->rcu, delayed_put_pid) in free_pid
Thanks,
- Chen
>
> > + /*
> > + * Only one pid found as the child reaper,
> > + * so current pid namespace do not have sub-namespace,
> > + * return 0 directly.
> > + */
> > + if (list_is_singular(pidns_pid_list)) {
> > + rc = 0;
> > + goto out;
> > + }
> > +
> > + /*
> > + * screen duplicate pids from pidns_pid_list
> > + * and form a new list pidns_pid_tree.
> > + */
> > + rc = pidns_list_filter(pidns_pid_list, pidns_pid_tree);
> > + if (rc)
> > + goto out;
> > +
> > + return 0;
> > +
> > +out:
> > + free_pidns_list(pidns_pid_list);
> > + return rc;
> > +}
> > +
> > +static int nslist_proc_show(struct seq_file *m, void *v)
> > +{
> > + struct pidns_list *pos;
> > + struct pid_namespace *ns, *curr_ns;
> > + struct pid *pid;
> > + char pid_buf[16];
> > + int i, rc;
> > +
> > + LIST_HEAD(pidns_pid_list);
> > + LIST_HEAD(pidns_pid_tree);
> > +
> > + curr_ns = task_active_pid_ns(current);
> > +
> > + rc = proc_pidns_list_refresh(curr_ns, &pidns_pid_list, &pidns_pid_tree);
> > + if (rc)
> > + return rc;
> > +
> > + /* print pid namespace's hierarchy */
> > + list_for_each_entry(pos, &pidns_pid_tree, list) {
> > + pid = pos->pid;
> > + for (i = curr_ns->level + 1; i <= pid->level; i++) {
> > + ns = pid->numbers[i].ns;
> > + /* show PID '1' in specific pid ns */
> > + snprintf(pid_buf, 16, "%u",
> > + pid_vnr(find_pid_ns(1, ns)));
> > + seq_printf(m, "%s ", pid_buf);
> > + }
> > +
> > + seq_putc(m, '\n');
> > + }
> > +
> > + free_pidns_list(&pidns_pid_tree);
> > +
> > + return 0;
> > +}
> > +
> > +static int nslist_proc_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, nslist_proc_show, NULL);
> > +}
> > +
> > +static const struct file_operations proc_nspid_nslist_fops = {
> > + .open = nslist_proc_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
> > +
> > +static int __init pidns_hierarchy_init(void)
> > +{
> > + proc_create(NS_HIERARCHY, S_IWUGO,
> > + NULL, &proc_nspid_nslist_fops);
> > +
> > + return 0;
> > +}
> > +fs_initcall(pidns_hierarchy_init);
> > --
> > 1.9.3
> >
>
> --
> Mateusz Guzik
