Re: [PATCH] fix granularity of task_u/stime(), v2

[Stanislaw Gruszka]

Hi

On Thu, Nov 12, 2009 at 01:33:45PM +0900, Hidetoshi Seto wrote:
> Originally task_s/utime() were designed to return clock_t but later
> changed to return cputime_t by following commit:
> 
>   commit efe567fc8281661524ffa75477a7c4ca9b466c63
>   Author: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>   Date:   Thu Aug 23 15:18:02 2007 +0200
> 
> It only changed the type of return value, but not the implementation.
> As the result the granularity of task_s/utime() is still that of
> clock_t, not that of cputime_t.
> 
> So using task_s/utime() in __exit_signal() makes values accumulated
> to the signal struct to be rounded and coarse grained.
> 
> This patch removes casts to clock_t in task_u/stime(), to keep
> granularity of cputime_t over the calculation.
> 
> v2:
>   Use div_u64() to avoid error "undefined reference to `__udivdi3`"
>   on some 32bit systems.
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@xxxxxxxxxxxxxx>
> ---
>  kernel/sched.c |   22 +++++++++++++---------
>  1 files changed, 13 insertions(+), 9 deletions(-)

Patch not fix the issue on my system. I test it alone, together with 

posix-cpu-timers: avoid do_sys_times() races with __exit_signal(

and (further) together with
 
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -248,8 +248,8 @@ void thread_group_cputime(struct task_struct *tsk,
struct task_cputime *times)

        t = tsk;
        do {
-               times->utime = cputime_add(times->utime, t->utime);
-               times->stime = cputime_add(times->stime, t->stime);
+               times->utime = cputime_add(times->utime, task_utime(t));
+               times->stime = cputime_add(times->stime, task_stime(t));
                times->sum_exec_runtime += t->se.sum_exec_runtime;

                t = next_thread(t);

What only changed was probability to enter the issue. I can not reproduce
the bug with below patch:

diff --git a/kernel/exit.c b/kernel/exit.c
index f7864ac..b85e384 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -110,8 +110,8 @@ static void __exit_signal(struct task_struct *tsk)
 		 * We won't ever get here for the group leader, since it
 		 * will have been the last reference on the signal_struct.
 		 */
-		sig->utime = cputime_add(sig->utime, task_utime(tsk));
-		sig->stime = cputime_add(sig->stime, task_stime(tsk));
+		sig->utime = cputime_add(sig->utime, tsk->utime); //task_utime(tsk));
+		sig->stime = cputime_add(sig->stime, tsk->stime); //task_stime(tsk));
 		sig->gtime = cputime_add(sig->gtime, task_gtime(tsk));
 		sig->min_flt += tsk->min_flt;
 		sig->maj_flt += tsk->maj_flt;
diff --git a/kernel/sys.c b/kernel/sys.c
index ce17760..8be5b75 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -914,8 +914,8 @@ void do_sys_times(struct tms *tms)
 	struct task_cputime cputime;
 	cputime_t cutime, cstime;
 
-	thread_group_cputime(current, &cputime);
 	spin_lock_irq(&current->sighand->siglock);
+	thread_group_cputime(current, &cputime);
 	cutime = current->signal->cutime;
 	cstime = current->signal->cstime;
 	spin_unlock_irq(&current->sighand->siglock);

Perhaps we can remove task_{u,s}time() in some places or maybe at whole ?

Stanislaw
--
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/