RE: [PATCHv4] procfs: show hierarchy of pid namespace

[Chen, Hanxiao]

> -----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

N?叉??y??b??千v??藓{.n???{?赙zXФ?塄}?财??j:+v???赙zZ+€?zf＂?????i????ア??璀??撷f?^j谦y??@A?囤?0鹅h??i