Messages in this thread | | | Date | Tue, 24 Jan 2012 11:07:30 +0900 | From | KAMEZAWA Hiroyuki <> | Subject | Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8 |
| |
On Mon, 23 Jan 2012 18:20:37 +0400 Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> When we do checkpoint of a task we need to know the list of children > the task, has but there is no easy and fast way to generate reverse > parent->children chain from arbitrary <pid> (while a parent pid is > provided in "PPid" field of /proc/<pid>/status). > > So instead of walking over all pids in the system (creating one big process > tree in memory, just to figure out which children a task has) -- we add > explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has > this kind of information but it is not yet exported. > > This is a first level children, not the whole process tree. > > v2: > - Kame suggested to use a separated /proc/<pid>/children entry > instead of poking /proc/<pid>/status > - Andew suggested to use rcu facility instead of locking > tasklist_lock > - Tejun pointed that non-seekable seq file might not be > enough for tasks with large number of children > > v3: > - To be on a safe side use %lu format for pid_t printing > > v4: > - New line get printed when sequence ends not at seq->stop, > a nit pointed by Tejun > - Documentation update > - tasklist_lock is back, Oleg pointed that ->children list > is actually not rcu-safe > > v5: > - Oleg suggested to make /proc/<pid>/task/<tid>/children > instead of global /proc/<pid>/children, which eliminates > hardness related to threads and children migration, and > allows patch to be a way simplier. > > v6: > - Drop ptrace_may_access tests, pids are can be found anyway > so nothing to protect here. > - Update comments and docs, pointed by Oleg. > > v7: > - Use get_pid over proc-pid directly, to simplify > code, pointed by Oleg. > > v8: > - Obtain a starting pid from the proc's inode directly. > > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org> > Reviewed-by: Oleg Nesterov <oleg@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Pavel Emelyanov <xemul@parallels.com> > Cc: Serge Hallyn <serge.hallyn@canonical.com> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
From viewpoint I played with seq_file, yesterday.
> +static void *children_seq_start(struct seq_file *seq, loff_t *pos) > +{ > + return get_children_pid(seq->private, NULL, *pos); > +} > + > +static void *children_seq_next(struct seq_file *seq, void *v, loff_t *pos) > +{ > + struct pid *pid = NULL; > + > + pid = get_children_pid(seq->private, v, *pos + 1); > + if (!pid) > + seq_printf(seq, "\n"); > + put_pid(v);
Because seq_printf() may fail. This seems dangeorus.
If seq_printf() fails and returns NULL, "\n" will not be printed out and user land parser will go wrong.
I think all seq_printf() should be handled in ->show(). (And you can use seq_putc() for "\n".)
Thanks, -Kame
| |