> -----Original Message----- > From: Oleg Nesterov [mailto:oleg@xxxxxxxxxx] > Sent: Wednesday, October 08, 2014 11:13 PM > To: Chen, Hanxiao/陈 晗霄 > Cc: containers@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Serge > Hallyn; Eric W. Biederman; David Howells; Richard Weinberger; Pavel Emelyanov; > Vasiliy Kulikov; Mateusz Guzik > Subject: Re: [PATCHv4] procfs: show hierarchy of pid namespace > > Sorry if this was already discussed, I have to admit that I ignored > the previous discussion ;) And it is possible I misread this patch > completely. > > On 10/08, Chen Hanxiao wrote: > > > > This patch will show the hierarchy of pid namespace > > by /proc/pidns_hierarchy like: > > > > [root@localhost ~]#cat /proc/pidns_hierarchy > > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid > > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid > > /proc/1550/ns/pid > > Well, personally I too think that the filenames look a bit strange, > can't it just print the numbers? Yes, let's print PID numbers only. > > And, iiuc what this patch does... perhaps in this case we should > simply add "struct list_head children" into struct pid_namespace? > In this case the patch will be really simple. I dunno. > If we had a children list in pid_namespace, all we had to do is a iteration from pid 1 of current ns. That would be nice. > > +pidns_list_add(struct pid *pid, struct list_head *list_head, > > + struct pid_namespace *curr_ns) > > +{ > > + struct pidns_list *ent; > > + struct pid_namespace *ns; > > + > > + if (is_child_reaper(pid)) { > > + ent = kmalloc(sizeof(*ent), GFP_KERNEL); > > GFP_KERNEL under rcu_read_lock(). This is not safe without > CONFIG_PREEMPT_RCU. It should be GFP_ATOMIC, Matesuz have already pointed out and I'v changed it in v3. Sorry for that mistake. > > > + if (!ent) > > + return -ENOMEM; > > + > > + ent->pid = pid; > > + ns = pid->numbers[pid->level].ns; > > + if (curr_ns) { > > + /* add pids who is the child of curr_ns */ > > + for (; ns != NULL; ns = ns->parent) > > + if (ns == curr_ns) > > + list_add_tail(&ent->list, list_head); > > afaics, it doesn't make sense to continue after list_add() ? Oops, we need a break here. > > > +static int proc_pidns_list_refresh(struct pid_namespace *curr_ns) > > +{ > > + struct pid *pid; > > + struct task_struct *p; > > + int rc; > > + > > + /* collect pid in differet ns */ > > + for_each_process(p) { > > Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid() > makes more sense? Only tasks from our ns is valid. But how could find_ge_pid() do that? nr = 1; while (nr < PID_MAX_LIMIT) { find_ge_pid(nr, curr_ns); list_add(); nr++; } Perhaps that's not a good way. > > > + pid = task_pid(p); > > Well, in theory you need barrier() here. Or perhaps we should add > ACCESS_ONCE() into task_pid()... You mean modify task_pid as: return ACCESS_ONCE(task->pids[PIDTYPE_PID].pid;); > > And imho it would be better to declare pidns_list/pidns_tree locally > in nslist_proc_show() and pass them to the callees. That's a good idea. Will changed in the next version. Thanks, - Chen _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/containers
