Re: [RFC] fs, proc: Introduce /proc/<pid>/task/<tid>/childrenentry v6

[Oleg Nesterov]

On 01/16, Cyrill Gorcunov wrote:
>
> On Mon, Jan 16, 2012 at 05:11:14PM +0100, Oleg Nesterov wrote:
> > On 01/16, Cyrill Gorcunov wrote:
> > >
> > > +get_children_pid(struct proc_pid_children_iter *iter, struct pid *pid_prev, loff_t pos)
> > > +{
> > > +	struct task_struct *start, *task;
> > > +	struct pid *pid = NULL;
> > > +
> > > +	read_lock(&tasklist_lock);
> > > +
> > > +	start = pid_task(iter->pid_start, PIDTYPE_PID);
> > > +	if (!start)
> > > +		goto out;
> > > +
> > > +	/*
> > > +	 * Lets try to continue searching first, this gives
> > > +	 * us significant speedup on children-rich processes.
> > > +	 */
> > > +	if (pid_prev) {
> > > +		task = pid_task(pid_prev, PIDTYPE_PID);
> > > +		if (task && task->real_parent == start &&
> > > +		    !(list_empty(&task->sibling))) {
> >
> > Damn. No, this is wrong.
> >
> > Damn! Yes, it was we who told you to check list_empty(sibling) ;)
> >
> > But this is not enough. exit_ptrace() can do list_move() without
> > changing ->real_parent.
> >
> > I'll try to think. At first glance we can rely on EXIT_DEAD, but
> > I'd like to avoid this, I think EXIT_DEAD should die.
>
> Ouch! Thanks for catching this Oleg. I'll try to come with something
> to show as well.

Do you see another approach? I don't, so I'd suggest to check
"task->exit_state != EXIT_DEAD" instead of !list_empty().

Just in case, we can also check "start->exit_state == 0" instead
of "task->real_parent == start" with the same effect, up to you.

It would be nice to add the comment explaining these checks...

And I forgot to mention, the comment below

	> +			/*
	> +			 * We might miss some freshly created children
	> +			 * here, but it was never promised to be
	> +			 * accurate.
	> +			 */
	> +			if (list_is_last(&task->sibling, &start->children))
	> +				goto out;

looks misleading. Contrary to the slow path, we can't miss the
freshly forked child here, copy_process() does list_add_tail().

But the slow path obviously can skip much more than needed and
miss children (freshly forked or not), probably it would be better
to move the comment down and remove the "freshly created" part.

What do you think?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/