Re: [PATCH 0/3] de_thread() should update ->real_start_time

[Oleg Nesterov <oleg@redhat.com>]

On 06/10, John Stultz wrote:
>
> On Mon, Jun 10, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > 1/3 is the obvious bugfix, 2 and 3 are minor cleanups
>
> Acked-by: John Stultz <john.stultz@linaro.org> for these three patches.

Thanks!

> > simply change copy_process
> >
> >         -       do_posix_clock_monotonic_gettime(&p->start_time);
> >         +       get_monotonic_boottime(&p->start_time);
> >
> > ?
> >
> > Afaics, this will only affect do_acct_process() and bacct_add_tsk(),
> > but do we really want to exclude the suspended time in this case?
>
> So bacct_add_tsk seems easy to change, since its just:
>     do_posix_clock_monotonic_gettime(&uptime);
>     ts = timespec_sub(uptime, tsk->start_time);
>
> So grabbing the monotonic boot time for uptime would provide the same
> relative delta.

Not really, or I misunderstood monotonic/boottime interaction.

IIUC, monotonic doesn't grow during suspend, so the delta can grow if
we use get_monotonic_boottime() in copy_process() and bacct_add_tsk()
and the system was suspended in between. Right?

But perhaps this is fine and even more correct?

> > Another user of ->start_time is cgroup.c and it looks wrong... But
> > the change above should not make any difference.
>
> The cgroup usage I'm unfamiliar with. Though from the comments in the
> code, it seems like using boottime would be ok for this.

Yes, this change can't make any difference. But this code looks wrong
although I'll try to recheck later.  We can't trust started_after()
exactly because ->start_time can be changed by mt-exec. But this is
offtopic.


> An additional thought: task_struct covers quite a bit of stuff, so I
> don't know if this would be too useful, but we could further change
> start_time to be a ktime_t, allowing possible additional space savings
> in the task_struct on 64bit systems.

Agreed.

Oleg.