Re: [PATCH 2/2] cputime: introduce thread_group_times()

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Mon, Nov 30, 2009 at 06:21:36PM +0900, Hidetoshi Seto wrote:
> This patch fixes the above problem in the following way:
> 
>  - Modify thread's exit (= __exit_signal()) not to use task_times().
>    It means {u,s}time in signal struct accumulates raw values instead
>    of adjusted values.  As the result it makes thread_group_cputime()
>    to return pure sum of "raw" values.
> 
>  - Introduce a new function thread_group_times(*task, *utime, *stime)
>    that converts "raw" values of thread_group_cputime() to "adjusted"
>    values, in same calculation procedure as task_times().
> 
>  - Modify group's exit (= wait_task_zombie()) to use this introduced
>    thread_group_times().  It make c{u,s}time in signal struct to
>    have adjusted values like before this patch.
> 
>  - Replace some thread_group_cputime() by thread_group_times().
>    This replacements are only applied where conveys the "adjusted"
>    cputime to users, and where already uses task_times() near by it.
>    (i.e. sys_times(), getrusage(), and /proc/<PID>/stat.)
> 
> This patch have a positive side effect:
> 
>  - Before this patch, if a group contains many short-life threads
>    (e.g. runs 0.9ms and not interrupted by ticks), the group's
>    cputime could be invisible since thread's cputime was accumulated
>    after adjusted: imagine adjustment function as adj(ticks, runtime),
>      {adj(0, 0.9) + adj(0, 0.9) + ....} = {0 + 0 + ....} = 0.
>    After this patch it will not happen because the adjustment is
>    applied after accumulated.

Idea is very good IMHO.

> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> ---
>  fs/proc/array.c       |    5 +---
>  include/linux/sched.h |    3 +-
>  kernel/exit.c         |   23 +++++++++++----------
>  kernel/fork.c         |    2 +
>  kernel/sched.c        |   52 +++++++++++++++++++++++++++++++++++++++++++++++-
>  kernel/sys.c          |   18 +++++++---------
>  6 files changed, 75 insertions(+), 28 deletions(-)

[snip]

>  #ifndef CONFIG_VIRT_CPU_ACCOUNTING
> -	cputime_t prev_utime, prev_stime;
> +	cputime_t prev_utime, prev_stime, prev_tgutime, prev_tgstime;
>  #endif

I think the new values should be part of struct_signal (see below)

> +static void __thread_group_times(struct task_struct *p,
> +				 struct task_cputime *cputime)
> +{
> +	cputime_t rtime, utime, total;
> +
> +	thread_group_cputime(p, cputime);
> +
> +	total = cputime_add(cputime->utime, cputime->stime);
> +	rtime = nsecs_to_cputime(cputime->sum_exec_runtime);
> +
> +	if (total) {
> +		u64 temp;
> +
> +		temp = (u64)(rtime * cputime->utime);
> +		do_div(temp, total);
> +		utime = (cputime_t)temp;
> +	} else
> +		utime = rtime;
> +
> +	p->prev_tgutime = max(p->prev_tgutime, utime);
> +	p->prev_tgstime = max(p->prev_tgstime,
> +			      cputime_sub(rtime, p->prev_tgutime));

p->sig->prev_utime = max(p->sig->prev_utime, utime);
p->sig->prev_stime = max(p->sig->prev_stime,
   		     	cputime_sub(rtime, p->sig->prev_utime));

>  /*
> + * Must be called with siglock held.
> + */
> +void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *st)
> +{
> +	struct task_cputime cputime;
> +
> +	__thread_group_times(p, &cputime);
> +
> +	if (ut)
> +		*ut = cputime.utime;
> +	if (st)
> +		*st = cputime.stime;

No thread_group_times() nor task_times() is called with NULL arguments, we
can get rid of "if ({u,s}t)" checks. Perhaps thread_group_times() should
have "struct task_cputime" argument as it is wrapper for
thread_group_cputime();

Thanks
Stanislaw