Re: [RFC][PATCH 09/10] taskstats: Fix exit CPU time accounting

[Oleg Nesterov <oleg@redhat.com>]

Michael, sorry for delay...

On 10/12, Michael Holzheu wrote:
>
> On Mon, 2010-10-11 at 14:37 +0200, Oleg Nesterov wrote:
> >
> > > > Also, the logic behind ->exit_accounting_done looks wrong (and unneeded)
> > > > but I am not sure...
> > >
> > > I think the logic is correct,
> >
> > OK, I misread the patch as if we always account the exited task in
> > parent's cdata_acct,
> >
> > 	+       struct cdata cdata_wait; /* parents have done sys_wait() */
> > 	+       struct cdata cdata_acct; /* complete cumulative data from acct tree */
> >
> > while in fact the "complete" data is cdata_wait + cdata_acct.
>
> No. The complete data is in cdata_acct. It contains both, the task times
> where sys_wait() has been done and the task times, where the tasks have
> reaped themselves.

Hmm. This means my first understanding was correct. But now I am
confused again, see below.

> > Hmm. Let's return to your example above,
> >
> > 	> Snapshot 1: P1 -> P2 -> P3
> > 	> Snapshot 2: P1
> > 	> ...
> > 	> P1 got all the CPU time of P2 and P3
> >
> > Suppose that P2 dies before P3. Then P3 dies, /sbin/init does wait and
> > accounts this task. This means it is not accounted in P1->signal->cdata_acct,
> > no?
>
> No. __account_to_parent() with wait=1 is called when init waits for P3.
> Then both sets are updated cdata_acct and cdata_wait:
>
> +static void __account_to_parent(struct task_struct *p, int wait)
> +{
> +       if (wait)
> +               __account_ctime(p, &p->real_parent->signal->cdata_wait,
> +                               &p->signal->cdata_wait);
> +       __account_ctime(p, &p->acct_parent->signal->cdata_acct,
> +                       &p->signal->cdata_acct);
> +       p->exit_accounting_done = 1;
>
> If a tasks reaps itself, only cdata_acct is updated.

Yes. But __account_to_parent() always sets p->exit_accounting_done = 1.
And __exit_signal() calls __account_to_parent() only if it is not set.

This means that we update either cdata_wait (if the child was reaped
by parent) or cdata_acct (the process auto-reaps itself).

That is why I thought that ->exit_accounting_done should die, and
__exit_signal() should always call __account_to_parent() to update
cdata_acct.

Or I missed something? Confused ;)

 > > Sorry for my ignorance. Probably I have not understood what happens, if
> > > a thread group leader dies. My assumption was that then the whole thread
> > > group dies.
> >
> > No. A thread group dies when the last thread dies. If a leader exits
> > it becomes a zombie until all other sub-threads exit.
>
> That brought me to another question: Does this mean that the thread
> group leader never changes and is always alive (at least as zombie) as
> long as the thread group lives?

Yes. Except de_thread() can change the leader. The new leader is the
thread which calls exec.

> > > Also I assumed that a parent can only be a thread group
> > > leader.
> >
> > No. If a thread T does fork(), the child's ->real_parent is T, not
> > T->group_leader. If T exits, we do not reparent its children to init
> > (unless it is the last thread, of course), we pick another live
> > thread in this thread group for reparenting.
>
> Ok, I hope that I understand now. So either we could set the acct_parent
> to the thread group leader in fork(), or we use the new parent in the
> thread group if there are live threads left, when a thread exits.
>
> Something like the following:
>
>  static void forget_original_parent(struct task_struct *father)
>  {
> +       struct pid_namespace *pid_ns = task_active_pid_ns(father);
>         struct task_struct *p, *n, *reaper;
>         LIST_HEAD(dead_children);
>
>         exit_ptrace(father);
>
>         reaper = find_new_reaper(father);
>
> +       list_for_each_entry_safe(p, n, &father->children_acct, sibling_acct) {
> +               struct task_struct *t = p;
> +               do {
> +                       if (pid_ns->child_reaper == reaper)
> +                               t->acct_parent = t->acct_parent->acct_parent;
> +                       else
> +                               t->acct_parent = reaper;
> +               } while_each_thread(p, t);
> +               list_move_tail(&p->sibling_acct,
> +                              &p->acct_parent->children_acct);
> +       }
> +

I think you can simplify this, but  I am not sure right now.

First of all, ->acct_parent should be moved from task_struct to
signal_struct. No need to initialize t->acct_parent unless t is
the group leader (this means we can avoid do/while_each_thread
loop during re-parenting, but de_thread needs another trivial
change).


No need to change forget_original_parent() at all, instead we
can the single line

	p->signal->acct_parent = father->signal->acct_parent;

to reparent_leader(), after the "if (same_thread_group())" check.

What do you think?

Oleg.