> -----Original Message----- > From: Oleg Nesterov [mailto:oleg@xxxxxxxxxx] > Sent: Friday, October 10, 2014 5:34 AM > 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 > > On 10/09, Chen, Hanxiao wrote: > > > > > From: Oleg Nesterov [mailto:oleg@xxxxxxxxxx] > > > > > > 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++; > > } > > something like this, except list_add() should obviously depend on > is_child_reaper() check. > > This can be more optimal in sub-namespaces, you do not need to abuse > the global process list. > > And if you change this code to use get_pid/put_pid, then you do not > need to hold rcu_read_lock() throughout, you only need it around > find_ge_pid + get_pid. > > At the same time, for_each_process() in the global namespace can be > faster if there are a lot of sub-threads. > > > Perhaps that's not a good way. > > OK, I won't insist. > > although it would be nice to know why do you think this is bad. > I worried about it may slower in global namespace. But it will provide a great convenient way when query pid hierarchy when not in init_pid_ns. > > > > + 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;); > > Yes. But not now an not in this patch of course. I'd suggest to add > barrier() just in case. > We can get rid of task_pid when we use find_ge_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. > > Good. And I forgot to mention, in this case you do not need pidns_list_lock > at all afaics. Thanks for your comments. I'll post a new patch using find_ge_pid + get_pid Thanks, - Chen _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/containers
