> -----Original Message----- > From: Mateusz Guzik [mailto:mguzik@xxxxxxxxxx] > Sent: Wednesday, November 05, 2014 7:54 PM > To: Chen, Hanxiao/陈 晗霄 > Cc: Eric W. Biederman; Serge Hallyn; Oleg Nesterov; > containers@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 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 _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/containers
