utime/stime decreasing on thread exit

[Spencer Candland]

I am seeing a problem with utime/stime decreasing on thread exit in a
multi-threaded process.  I have been able to track this regression down
to the "process wide cpu clocks/timers" changes introduces in
2.6.29-rc5, specifically when I revert the following commits I know
longer see decreasing utime/stime values:

4da94d49b2ecb0a26e716a8811c3ecc542c2a65d
3fccfd67df79c6351a156eb25a7a514e5f39c4d9
7d8e23df69820e6be42bcc41d441f4860e8c76f7
4cd4c1b40d40447fb5e7ba80746c6d7ba91d7a53
32bd671d6cbeda60dc73be77fa2b9037d9a9bfa0

I poked around a little, but I am afraid I have to admit that I am not
familiar enough with how this works to resolve this or suggest a fix.

I have verified this in happening in kernels 2.6.29-rc5 - 2.6.32-rc6, I
have been testing this on x86 vanilla kernels, but have also verified it
on several x86 2.6.29+ distro kernels (fedora and ubuntu).

I first noticed this on a production environment running Apache with the
worker MPM, however while tracking this down I put together a simple
program that has been reliable in showing me utime decreasing, hopefully
it will be helpful in demonstrating the issue:

#include <stdio.h>
#include <pthread.h>
#include <sys/times.h>

#define NUM_THREADS 500

struct tms start;
int oldutime;

void *pound (void *threadid)
{
  struct tms end;
  int utime;
  int c;
  for (c = 0; c < 10000000; c++);
  times(&end);
  utime = ((int)end.tms_utime - (int)start.tms_utime);
  if (oldutime > utime) {
    printf("utime decreased, was %d, now %d!\n", oldutime, utime);
  }
  oldutime = utime;
  pthread_exit(NULL);
}


int main()
{
  pthread_t th[NUM_THREADS];
  long i;
  times(&start);
  for (i = 0; i < NUM_THREADS; i++) {
    pthread_create (&th[i], NULL, pound, (void *)i);
  }
  pthread_exit(NULL);
  return 0;
}

Output:

# ./decrease_utime
utime decreased, was 1288, now 1287!
utime decreased, was 1294, now 1293!
utime decreased, was 1296, now 1295!
utime decreased, was 1297, now 1296!
utime decreased, was 1298, now 1297!
utime decreased, was 1299, now 1298!

Please let me know if you need any additional information.

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

[Hidetoshi Seto]

Spencer Candland wrote:

> I am seeing a problem with utime/stime decreasing on thread exit in a
> multi-threaded process.  I have been able to track this regression down
> to the "process wide cpu clocks/timers" changes introduces in
> 2.6.29-rc5, specifically when I revert the following commits I know
> longer see decreasing utime/stime values:
>
> 4da94d49b2ecb0a26e716a8811c3ecc542c2a65d
> 3fccfd67df79c6351a156eb25a7a514e5f39c4d9
> 7d8e23df69820e6be42bcc41d441f4860e8c76f7
> 4cd4c1b40d40447fb5e7ba80746c6d7ba91d7a53
> 32bd671d6cbeda60dc73be77fa2b9037d9a9bfa0
>
> I poked around a little, but I am afraid I have to admit that I am not
> familiar enough with how this works to resolve this or suggest a fix.
>
> I have verified this in happening in kernels 2.6.29-rc5 - 2.6.32-rc6, I
> have been testing this on x86 vanilla kernels, but have also verified it
> on several x86 2.6.29+ distro kernels (fedora and ubuntu).
>
> I first noticed this on a production environment running Apache with the
> worker MPM, however while tracking this down I put together a simple
> program that has been reliable in showing me utime decreasing, hopefully
> it will be helpful in demonstrating the issue:
>
> #include <stdio.h>
> #include <pthread.h>
> #include <sys/times.h>
>
> #define NUM_THREADS 500
>
> struct tms start;
> int oldutime;
>
> void *pound (void *threadid)
> {
>   struct tms end;
>   int utime;
>   int c;
>   for (c = 0; c < 10000000; c++);
>   times(&end);
>   utime = ((int)end.tms_utime - (int)start.tms_utime);
>   if (oldutime > utime) {
>     printf("utime decreased, was %d, now %d!\n", oldutime, utime);
>   }
>   oldutime = utime;
>   pthread_exit(NULL);
> }
>
>
> int main()
> {
>   pthread_t th[NUM_THREADS];
>   long i;
>   times(&start);
>   for (i = 0; i < NUM_THREADS; i++) {
>     pthread_create (&th[i], NULL, pound, (void *)i);
>   }
>   pthread_exit(NULL);
>   return 0;
> }

This test program doesn't show the problem correctly, because the
oldutime can be modified by any of threads running simultaneously.

Thread 1: Thread 2:

>   times(&end);

                        >   times(&end);
                        >   utime = ((int)end.tms_utime - (int)start.tms_utime);
                        >   if (oldutime > utime) {
                        >     printf("utime decreased, was %d, now %d!\n", oldutime, utime);
                        >   }
                        >   oldutime = utime;

>   utime = ((int)end.tms_utime - (int)start.tms_utime);
>   if (oldutime > utime) {
>     printf("utime decreased, was %d, now %d!\n", oldutime, utime);
>   }
>   oldutime = utime;

So I thought it is not a bug, but....

>
> Output:
>
> # ./decrease_utime
> utime decreased, was 1288, now 1287!
> utime decreased, was 1294, now 1293!
> utime decreased, was 1296, now 1295!
> utime decreased, was 1297, now 1296!
> utime decreased, was 1298, now 1297!
> utime decreased, was 1299, now 1298!
>
> Please let me know if you need any additional information.
>
> Thank you,
> Spencer Candland

I revised the test program but still I can see the problem.
So I agree that something wrong is actually there.

[seto@localhost work]$ cat test.c
#include <stdio.h>
#include <pthread.h>
#include <sys/times.h>

#define NUM_THREADS 500

void *pound (void *threadid)
{
  struct tms t1, t2;
  int c;
  for (c = 0; c < 10000000; c++);
  times(&t1);
  times(&t2);
  if (t1.tms_utime > t2.tms_utime) {
    printf("utime decreased, was %d, now %d! : (%d %d %d %d) (%d %d %d %d)\n",
                t1.tms_utime, t2.tms_utime,
                t1.tms_stime, t1.tms_utime, t1.tms_cstime, t1.tms_cutime,
                t2.tms_stime, t2.tms_utime, t2.tms_cstime, t2.tms_cutime);
  }
  pthread_exit(NULL);
}


int main()
{
  pthread_t th[NUM_THREADS];
  long i;
  for (i = 0; i < NUM_THREADS; i++) {
    pthread_create (&th[i], NULL, pound, (void *)i);
  }
  pthread_exit(NULL);
  return 0;
}

[seto@localhost work]$ ./a.out
utime decreased, was 1066, now 1063! : (102 1066 0 0) (102 1063 0 0)
utime decreased, was 1101, now 1099! : (114 1101 0 0) (114 1099 0 0)
utime decreased, was 1095, now 1093! : (146 1095 0 0) (146 1093 0 0)
utime decreased, was 1089, now 1086! : (166 1089 0 0) (166 1086 0 0)
utime decreased, was 1071, now 1069! : (212 1071 0 0) (212 1069 0 0)
utime decreased, was 1057, now 1054! : (274 1057 0 0) (274 1054 0 0)
utime decreased, was 1054, now 1049! : (79 1054 0 0) (303 1049 0 0)
utime decreased, was 1050, now 1048! : (313 1050 0 0) (313 1048 0 0)
utime decreased, was 1049, now 1046! : (319 1049 0 0) (319 1046 0 0)
utime decreased, was 1058, now 1038! : (83 1058 0 0) (369 1038 0 0)
utime decreased, was 1038, now 1036! : (378 1038 0 0) (378 1036 0 0)
utime decreased, was 1037, now 1035! : (384 1037 0 0) (384 1035 0 0)
utime decreased, was 1035, now 1034! : (385 1035 0 0) (386 1034 0 0)
utime decreased, was 1037, now 1035! : (385 1037 0 0) (385 1035 0 0)
utime decreased, was 1035, now 1032! : (389 1035 0 0) (390 1032 0 0)
utime decreased, was 1030, now 1028! : (402 1030 0 0) (402 1028 0 0)
utime decreased, was 1029, now 1026! : (405 1029 0 0) (405 1026 0 0)
utime decreased, was 1025, now 1024! : (409 1025 0 0) (410 1024 0 0)
utime decreased, was 1023, now 1021! : (420 1023 0 0) (420 1021 0 0)
utime decreased, was 1022, now 1020! : (423 1022 0 0) (423 1020 0 0)
utime decreased, was 1037, now 1017! : (372 1037 0 0) (432 1017 0 0)
utime decreased, was 1019, now 1017! : (431 1019 0 0) (432 1017 0 0)
utime decreased, was 1053, now 1015! : (79 1053 0 0) (434 1015 0 0)
utime decreased, was 1013, now 1011! : (446 1013 0 0) (446 1011 0 0)
utime decreased, was 1013, now 1010! : (448 1013 0 0) (448 1010 0 0)
utime decreased, was 1053, now 1009! : (79 1053 0 0) (451 1009 0 0)
[seto@localhost work]$ taskset 0x1 ./a.out
utime decreased, was 1025, now 1021! : (59 1025 0 0) (417 1021 0 0)
utime decreased, was 1023, now 1021! : (59 1023 0 0) (419 1021 0 0)
utime decreased, was 1027, now 1020! : (60 1027 0 0) (420 1020 0 0)
utime decreased, was 1068, now 1000! : (173 1068 0 0) (500 1000 0 0)


I'll check commits you pointed/reverted.


Thanks,
H.Seto

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

[Hidetoshi Seto]

Hidetoshi Seto wrote:

> Spencer Candland wrote:
>> I am seeing a problem with utime/stime decreasing on thread exit in a
>> multi-threaded process.  I have been able to track this regression down
>> to the "process wide cpu clocks/timers" changes introduces in
>> 2.6.29-rc5, specifically when I revert the following commits I know
>> longer see decreasing utime/stime values:
>>
>> 4da94d49b2ecb0a26e716a8811c3ecc542c2a65d
>> 3fccfd67df79c6351a156eb25a7a514e5f39c4d9
>> 7d8e23df69820e6be42bcc41d441f4860e8c76f7
>> 4cd4c1b40d40447fb5e7ba80746c6d7ba91d7a53
>> 32bd671d6cbeda60dc73be77fa2b9037d9a9bfa0

(snip)
>
> I'll check commits you pointed/reverted.

These are likely series of:

> commit 4cd4c1b40d40447fb5e7ba80746c6d7ba91d7a53
> Author: Peter Zijlstra <a.p.zijlstra@...>
> Date:   Thu Feb 5 12:24:16 2009 +0100
>
>     timers: split process wide cpu clocks/timers

This results in making sys_times() to use "clocks" instead of
"timers".  Please refer the description of the above commit.

I found 2 problems through my review.


Problem [1]:
  thread_group_cputime() vs exit

+void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
+{
+       struct sighand_struct *sighand;
+       struct signal_struct *sig;
+       struct task_struct *t;
+
+       *times = INIT_CPUTIME;
+
+       rcu_read_lock();
+       sighand = rcu_dereference(tsk->sighand);
+       if (!sighand)
+               goto out;
+
+       sig = tsk->signal;
+
+       t = tsk;
+       do {
+               times->utime = cputime_add(times->utime, t->utime);
+               times->stime = cputime_add(times->stime, t->stime);
+               times->sum_exec_runtime += t->se.sum_exec_runtime;
+
+               t = next_thread(t);
+       } while (t != tsk);
+
+       times->utime = cputime_add(times->utime, sig->utime);
+       times->stime = cputime_add(times->stime, sig->stime);
+       times->sum_exec_runtime += sig->sum_sched_runtime;
+out:
+       rcu_read_unlock();
+}

If one of (thousands) threads do exit while a thread is doing do-while
above, the s/utime of exited thread can be accounted twice, at do-while
(before exit) and at cputime_add() at last (after exit).

I suppose this is hard to fix: Taking lock on signal would solve this
problem, but it could block all other threads long and cause serious
performance issue and so on...


Problem [2]:
  use of task_s/utime()

I modified the test program more, to take times() 6 times and print them
if utime decreased between 3rd and 4th.
I noticed that I cannot explain that if the problem [1] was the root cause
then why results show decreased value continuously, instead of an increased
value at a point (like (v)(v)(V)(v)(v)(v)) which is expected.

 :
times decreased : (104 984) (104 984) (104 984) (105 983) (105 983) (105 983)
times decreased : (115 981) (116 980) (116 978) (117 977) (117 977) (119 979)
times decreased : (116 980) (117 980) (117 980) (117 977) (118 979) (118 977)
 :

And it seems that the more thread exits the more utime decreases.

Soon I found:

[kernel/exit.c]
+               sig->utime = cputime_add(sig->utime, task_utime(tsk));
+               sig->stime = cputime_add(sig->stime, task_stime(tsk));

While the thread_group_cputime() accumulates raw s/utime in do-while loop,
the signal struct accumulates adjusted s/utime of exited threads.

I'm not sure how this adjustment works but applying the following patch
makes the result little bit better:

 :
times decreased : (436 741) (436 741) (437 744) (436 742) (436 742) (436 742)
times decreased : (454 792) (454 792) (455 794) (454 792) (454 792) (454 792)
times decreased : (503 941) (503 941) (504 943) (503 941) (503 941) (503 941)
 :

But still decreasing(or increasing) continues, because there is a problem [1]
at least.

I think I couldn't handle this problem any more... Anybody can help?


Thanks,
H.Seto

===

Subject: [PATCH] thread_group_cputime() should use task_s/utime()

The signal struct accumulates adjusted cputime of exited threads,
so thread_group_cputime() should use task_s/utime() instead of raw
task->s/utime, to accumulate adjusted cputime of live threads.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...>
---
 kernel/posix-cpu-timers.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 5c9dc22..e065b8a 100644
--- 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);
--
1.6.5.2

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

[Peter Zijlstra-5]

On Thu, 2009-11-05 at 14:24 +0900, Hidetoshi Seto wrote:

> Problem [1]:
>   thread_group_cputime() vs exit
>
> +void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
> +{
> +       struct sighand_struct *sighand;
> +       struct signal_struct *sig;
> +       struct task_struct *t;
> +
> +       *times = INIT_CPUTIME;
> +
> +       rcu_read_lock();
> +       sighand = rcu_dereference(tsk->sighand);
> +       if (!sighand)
> +               goto out;
> +
> +       sig = tsk->signal;
> +
> +       t = tsk;
> +       do {
> +               times->utime = cputime_add(times->utime, t->utime);
> +               times->stime = cputime_add(times->stime, t->stime);
> +               times->sum_exec_runtime += t->se.sum_exec_runtime;
> +
> +               t = next_thread(t);
> +       } while (t != tsk);
> +
> +       times->utime = cputime_add(times->utime, sig->utime);
> +       times->stime = cputime_add(times->stime, sig->stime);
> +       times->sum_exec_runtime += sig->sum_sched_runtime;
> +out:
> +       rcu_read_unlock();
> +}
>
> If one of (thousands) threads do exit while a thread is doing do-while
> above, the s/utime of exited thread can be accounted twice, at do-while
> (before exit) and at cputime_add() at last (after exit).
>
> I suppose this is hard to fix: Taking lock on signal would solve this
> problem, but it could block all other threads long and cause serious
> performance issue and so on...

I just checked .22 and there we seem to hold p->sighand->siglock over
the full task iteration. So we might as well revert back to that if
people really mind counting things twice :-)

FWIW getrusage() also takes siglock over the task iteration.

Alternatively, we could try reading the sig->[us]time before doing the
loop, but I guess that's still racy in that we can then miss someone
altogether.

> Problem [2]:
>   use of task_s/utime()
>
> I modified the test program more, to take times() 6 times and print them
> if utime decreased between 3rd and 4th.
> I noticed that I cannot explain that if the problem [1] was the root cause
> then why results show decreased value continuously, instead of an increased
> value at a point (like (v)(v)(V)(v)(v)(v)) which is expected.
>
>  :
> times decreased : (104 984) (104 984) (104 984) (105 983) (105 983) (105 983)
> times decreased : (115 981) (116 980) (116 978) (117 977) (117 977) (119 979)
> times decreased : (116 980) (117 980) (117 980) (117 977) (118 979) (118 977)
>  :
>
> And it seems that the more thread exits the more utime decreases.
>
> Soon I found:
>
> [kernel/exit.c]
> +               sig->utime = cputime_add(sig->utime, task_utime(tsk));
> +               sig->stime = cputime_add(sig->stime, task_stime(tsk));
>
> While the thread_group_cputime() accumulates raw s/utime in do-while loop,
> the signal struct accumulates adjusted s/utime of exited threads.
>
> I'm not sure how this adjustment works but applying the following patch
> makes the result little bit better:
>
>  :
> times decreased : (436 741) (436 741) (437 744) (436 742) (436 742) (436 742)
> times decreased : (454 792) (454 792) (455 794) (454 792) (454 792) (454 792)
> times decreased : (503 941) (503 941) (504 943) (503 941) (503 941) (503 941)
>  :
>
> But still decreasing(or increasing) continues, because there is a problem [1]
> at least.
>
> I think I couldn't handle this problem any more... Anybody can help?

Stick in a few trace_printk()s and see what happens?

> Subject: [PATCH] thread_group_cputime() should use task_s/utime()
>
> The signal struct accumulates adjusted cputime of exited threads,
> so thread_group_cputime() should use task_s/utime() instead of raw
> task->s/utime, to accumulate adjusted cputime of live threads.
>
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...>
> ---
>  kernel/posix-cpu-timers.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> index 5c9dc22..e065b8a 100644
> --- 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);

So what you're trying to say is that because __exit_signal() uses
task_[usg]time() to accumulate sig->[usg]time, we should use it too in
the loop over the live threads?

I'm thinking its the task_[usg]time() usage in __exit_signal() that's
the issue.

I tried running the modified test.c on a current -tip kernel but could
not observe the problem (dual-core opteron).


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

[Oleg Nesterov-3]

On 11/09, Peter Zijlstra wrote:

>
> On Thu, 2009-11-05 at 14:24 +0900, Hidetoshi Seto wrote:
>
> > Problem [1]:
> >   thread_group_cputime() vs exit
> >
> > +void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
> > +{
> > +       struct sighand_struct *sighand;
> > +       struct signal_struct *sig;
> > +       struct task_struct *t;
> > +
> > +       *times = INIT_CPUTIME;
> > +
> > +       rcu_read_lock();
> > +       sighand = rcu_dereference(tsk->sighand);
> > +       if (!sighand)
> > +               goto out;
> > +
> > +       sig = tsk->signal;
> > +
> > +       t = tsk;
> > +       do {
> > +               times->utime = cputime_add(times->utime, t->utime);
> > +               times->stime = cputime_add(times->stime, t->stime);
> > +               times->sum_exec_runtime += t->se.sum_exec_runtime;
> > +
> > +               t = next_thread(t);
> > +       } while (t != tsk);
> > +
> > +       times->utime = cputime_add(times->utime, sig->utime);
> > +       times->stime = cputime_add(times->stime, sig->stime);
> > +       times->sum_exec_runtime += sig->sum_sched_runtime;
> > +out:
> > +       rcu_read_unlock();
> > +}
> >
> > If one of (thousands) threads do exit while a thread is doing do-while
> > above, the s/utime of exited thread can be accounted twice, at do-while
> > (before exit) and at cputime_add() at last (after exit).
> >
> > I suppose this is hard to fix: Taking lock on signal would solve this
> > problem, but it could block all other threads long and cause serious
> > performance issue and so on...
>
> I just checked .22 and there we seem to hold p->sighand->siglock over
> the full task iteration.

Yes. Then thread_group_cputime() was changed so that it didn't itearate
over sub-threads, then this callsite was move outside of ->siglock, now
it does while_each_thread() again.

> So we might as well revert back to that if
> people really mind counting things twice :-)

Stanislaw has already sent the patch, but I don't know what happened
with this patch:

        [PATCH 1/2] posix-cpu-timers: avoid do_sys_times() races with __exit_signal()
        http://marc.info/?l=linux-kernel&m=124505545131145

Oleg.

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

[Oleg Nesterov-3]

Sorry, forgot to cc Stanislaw...

On 11/09, Peter Zijlstra wrote:

>
> On Thu, 2009-11-05 at 14:24 +0900, Hidetoshi Seto wrote:
>
> > Problem [1]:
> >   thread_group_cputime() vs exit
> >
> > +void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
> > +{
> > +       struct sighand_struct *sighand;
> > +       struct signal_struct *sig;
> > +       struct task_struct *t;
> > +
> > +       *times = INIT_CPUTIME;
> > +
> > +       rcu_read_lock();
> > +       sighand = rcu_dereference(tsk->sighand);
> > +       if (!sighand)
> > +               goto out;
> > +
> > +       sig = tsk->signal;
> > +
> > +       t = tsk;
> > +       do {
> > +               times->utime = cputime_add(times->utime, t->utime);
> > +               times->stime = cputime_add(times->stime, t->stime);
> > +               times->sum_exec_runtime += t->se.sum_exec_runtime;
> > +
> > +               t = next_thread(t);
> > +       } while (t != tsk);
> > +
> > +       times->utime = cputime_add(times->utime, sig->utime);
> > +       times->stime = cputime_add(times->stime, sig->stime);
> > +       times->sum_exec_runtime += sig->sum_sched_runtime;
> > +out:
> > +       rcu_read_unlock();
> > +}
> >
> > If one of (thousands) threads do exit while a thread is doing do-while
> > above, the s/utime of exited thread can be accounted twice, at do-while
> > (before exit) and at cputime_add() at last (after exit).
> >
> > I suppose this is hard to fix: Taking lock on signal would solve this
> > problem, but it could block all other threads long and cause serious
> > performance issue and so on...
>
> I just checked .22 and there we seem to hold p->sighand->siglock over
> the full task iteration.

Yes. Then thread_group_cputime() was changed so that it didn't itearate
over sub-threads, then this callsite was move outside of ->siglock, now
it does while_each_thread() again.

> So we might as well revert back to that if
> people really mind counting things twice :-)

Stanislaw has already sent the patch, but I don't know what happened
with this patch:

        [PATCH 1/2] posix-cpu-timers: avoid do_sys_times() races with __exit_signal()
        http://marc.info/?l=linux-kernel&m=124505545131145

Oleg.

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

[Peter Zijlstra-5]

On Mon, 2009-11-09 at 18:20 +0100, Oleg Nesterov wrote:

> > I just checked .22 and there we seem to hold p->sighand->siglock over
> > the full task iteration.
>
> Yes. Then thread_group_cputime() was changed so that it didn't itearate
> over sub-threads, then this callsite was move outside of ->siglock, now
> it does while_each_thread() again.
>
> > So we might as well revert back to that if
> > people really mind counting things twice :-)
>
> Stanislaw has already sent the patch, but I don't know what happened
> with this patch:
>
> [PATCH 1/2] posix-cpu-timers: avoid do_sys_times() races with __exit_signal()
> http://marc.info/?l=linux-kernel&m=124505545131145

That patch has the siglock in the function calling
thread_group_cputime(), the 22 code had it near the loop proper, which
to me seems a more sensible thing, since there could be more callers,
no?

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

[Oleg Nesterov-3]

On 11/09, Peter Zijlstra wrote:

>
> On Mon, 2009-11-09 at 18:20 +0100, Oleg Nesterov wrote:
>
> > Stanislaw has already sent the patch, but I don't know what happened
> > with this patch:
> >
> > [PATCH 1/2] posix-cpu-timers: avoid do_sys_times() races with __exit_signal()
> > http://marc.info/?l=linux-kernel&m=124505545131145
>
> That patch has the siglock in the function calling
> thread_group_cputime(), the 22 code had it near the loop proper, which
> to me seems a more sensible thing, since there could be more callers,
> no?

Well, we can't take ->siglock in thread_group_cputime(), sometimes it
is called under ->siglock. do_task_stat(), get_cpu_itimer() at least.

IIRC, Stanislaw verified other callers have no problems with this helper.

Oleg.

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

[Peter Zijlstra-5]

On Mon, 2009-11-09 at 20:23 +0100, Oleg Nesterov wrote:

> On 11/09, Peter Zijlstra wrote:
> >
> > On Mon, 2009-11-09 at 18:20 +0100, Oleg Nesterov wrote:
> >
> > > Stanislaw has already sent the patch, but I don't know what happened
> > > with this patch:
> > >
> > > [PATCH 1/2] posix-cpu-timers: avoid do_sys_times() races with __exit_signal()
> > > http://marc.info/?l=linux-kernel&m=124505545131145
> >
> > That patch has the siglock in the function calling
> > thread_group_cputime(), the 22 code had it near the loop proper, which
> > to me seems a more sensible thing, since there could be more callers,
> > no?
>
> Well, we can't take ->siglock in thread_group_cputime(), sometimes it
> is called under ->siglock. do_task_stat(), get_cpu_itimer() at least.
>
> IIRC, Stanislaw verified other callers have no problems with this helper.

Would have made fine changelog material :-)

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

[Hidetoshi Seto]

Peter Zijlstra wrote:
> On Thu, 2009-11-05 at 14:24 +0900, Hidetoshi Seto wrote:
>
>> Problem [1]:
>>   thread_group_cputime() vs exit
(snip)

>
> I just checked .22 and there we seem to hold p->sighand->siglock over
> the full task iteration. So we might as well revert back to that if
> people really mind counting things twice :-)
>
> FWIW getrusage() also takes siglock over the task iteration.
>
> Alternatively, we could try reading the sig->[us]time before doing the
> loop, but I guess that's still racy in that we can then miss someone
> altogether.

Right.  So "before or after" will not be any help.

As you pointed there are some other functions taking siglock over the
task iteration, so making do_sys_times() do same will be acceptable.
In other words using many threads have risks, which might be solved in
future.  

I agree that the following patch is right fix for this.

 [PATCH 1/2] posix-cpu-timers: avoid do_sys_times() races with __exit_signal()
 http://marc.info/?l=linux-kernel&m=124505545131145



>> Problem [2]:
>>   use of task_s/utime()
>>
(snip)

>> [kernel/exit.c]
>> +               sig->utime = cputime_add(sig->utime, task_utime(tsk));
>> +               sig->stime = cputime_add(sig->stime, task_stime(tsk));
>>
>> While the thread_group_cputime() accumulates raw s/utime in do-while loop,
>> the signal struct accumulates adjusted s/utime of exited threads.
>>
>> I'm not sure how this adjustment works but applying the following patch
>> makes the result little bit better:
>>
>>  :
>> times decreased : (436 741) (436 741) (437 744) (436 742) (436 742) (436 742)
>> times decreased : (454 792) (454 792) (455 794) (454 792) (454 792) (454 792)
>> times decreased : (503 941) (503 941) (504 943) (503 941) (503 941) (503 941)
>>  :
>>
>> But still decreasing(or increasing) continues, because there is a problem [1]
>> at least.
>>
>> I think I couldn't handle this problem any more... Anybody can help?
>
> Stick in a few trace_printk()s and see what happens?

Nice idea.
I tried it and show the result in later of this mail.

>> Subject: [PATCH] thread_group_cputime() should use task_s/utime()
>>
>> The signal struct accumulates adjusted cputime of exited threads,
>> so thread_group_cputime() should use task_s/utime() instead of raw
>> task->s/utime, to accumulate adjusted cputime of live threads.
>>
>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...>
>> ---
>>  kernel/posix-cpu-timers.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
>> index 5c9dc22..e065b8a 100644
>> --- 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);
>
> So what you're trying to say is that because __exit_signal() uses
> task_[usg]time() to accumulate sig->[usg]time, we should use it too in
> the loop over the live threads?

Right.  Thank you for trying to understand.

>
> I'm thinking its the task_[usg]time() usage in __exit_signal() that's
> the issue.

It likely means reverting:

  commit 49048622eae698e5c4ae61f7e71200f265ccc529
  Author: Balbir Singh <balbir@...>
  Date:   Fri Sep 5 18:12:23 2008 +0200
    sched: fix process time monotonicity

I'm not sure the reason why the task_[usg]time() have introduced, but
removing them would be one of solutions.

> I tried running the modified test.c on a current -tip kernel but could
> not observe the problem (dual-core opteron).

It might not happen if your box is quite fast (otherwise slow).
Changing loop in test.c (i.e. cycles in user-land) might help the
reproductivity...



Here is the result of additional test:

I put a trace_printk() in the __exit_signal(), to print tsk->s/utime,
task_s/utime() and tsk->se.sum_exec_rumtime.
(And tsk->prev_s/utime before calling task_s/utime())

           <...>-2857  [006]   112.731732: release_task: (37 22)to(40 20), sxr 57480477, (0 0)
           <...>-5077  [009]   526.272338: release_task: (0 27)to(10 20), sxr 27997019, (0 0)
           <...>-4999  [009]   526.272396: release_task: (1 27)to(10 20), sxr 27967513, (0 0)
           <...>-4992  [006]   526.328591: release_task: (2 34)to(10 30), sxr 35823013, (0 0)
           <...>-5022  [012]   526.329183: release_task: (0 27)to(10 20), sxr 27761854, (0 0)
           <...>-4996  [010]   526.329203: release_task: (3 38)to(10 30), sxr 39200207, (0 0)

... It clearly probes that there is the 3rd problem.

Problem [3]:
   granularity of task_s/utime()

According to the git log, 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@...>
  Date:   Thu Aug 23 15:18:02 2007 +0200

It only changes 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.

After applying a fix (will follow to this mail), return values are changed
to be fine grained:

           <...>-5438  [006]   135.212289: release_task: (0 28)to(0 26), sxr 26648558, (0 0)
           <...>-5402  [015]   135.213193: release_task: (0 27)to(0 26), sxr 26725886, (0 0)
           <...>-5408  [011]   135.214172: release_task: (0 28)to(0 26), sxr 26607882, (0 0)
           <...>-5419  [005]   135.214410: release_task: (1 27)to(1 25), sxr 26612615, (0 0)
           <...>-5350  [009]   135.214937: release_task: (0 28)to(0 27), sxr 27028388, (0 0)
           <...>-5443  [008]   135.216748: release_task: (0 28)to(0 26), sxr 26372691, (0 0)

But it cannot stop adjusted values become smaller than tsk->s/utime.
So some approach to solve problem [2] is required.


Thanks,
H.Seto


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

[Hidetoshi Seto]

Remove casting to clock_t from task_u/stime(), and keep granularity of
cputime_t over the calculation.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...>
---
 kernel/sched.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 28dd4f4..1632e82 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -5171,41 +5171,44 @@ cputime_t task_stime(struct task_struct *p)
  return p->stime;
 }
 #else
+
+#ifndef nsecs_to_cputime
+# define nsecs_to_cputime(__nsecs) msecs_to_cputime((__nsecs) / NSEC_PER_MSEC)
+#endif
+
 cputime_t task_utime(struct task_struct *p)
 {
- clock_t utime = cputime_to_clock_t(p->utime),
- total = utime + cputime_to_clock_t(p->stime);
+ cputime_t utime = p->utime, total = utime + p->stime;
  u64 temp;
 
  /*
  * Use CFS's precise accounting:
  */
- temp = (u64)nsec_to_clock_t(p->se.sum_exec_runtime);
+ temp = (u64)nsecs_to_cputime(p->se.sum_exec_runtime);
 
  if (total) {
  temp *= utime;
  do_div(temp, total);
  }
- utime = (clock_t)temp;
+ utime = (cputime_t)temp;
 
- p->prev_utime = max(p->prev_utime, clock_t_to_cputime(utime));
+ p->prev_utime = max(p->prev_utime, utime);
  return p->prev_utime;
 }
 
 cputime_t task_stime(struct task_struct *p)
 {
- clock_t stime;
+ cputime_t stime;
 
  /*
  * Use CFS's precise accounting. (we subtract utime from
  * the total, to make sure the total observed by userspace
  * grows monotonically - apps rely on that):
  */
- stime = nsec_to_clock_t(p->se.sum_exec_runtime) -
- cputime_to_clock_t(task_utime(p));
+ stime = nsecs_to_cputime(p->se.sum_exec_runtime) - task_utime(p);
 
  if (stime >= 0)
- p->prev_stime = max(p->prev_stime, clock_t_to_cputime(stime));
+ p->prev_stime = max(p->prev_stime, stime);
 
  return p->prev_stime;
 }
--
1.6.5.2



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

[Stanislaw Gruszka-2]

On Mon, Nov 09, 2009 at 08:23:55PM +0100, Oleg Nesterov wrote:
> On 11/09, Peter Zijlstra wrote:
> >
> > On Mon, 2009-11-09 at 18:20 +0100, Oleg Nesterov wrote:
> >
> > > Stanislaw has already sent the patch, but I don't know what happened
> > > with this patch:
> > >
> > > [PATCH 1/2] posix-cpu-timers: avoid do_sys_times() races with __exit_signal()
> > > http://marc.info/?l=linux-kernel&m=124505545131145

I did not resubmit it enough times :) But I didn't thought it worth to,
since performance can be degraded.

> > That patch has the siglock in the function calling
> > thread_group_cputime(), the 22 code had it near the loop proper, which
> > to me seems a more sensible thing, since there could be more callers,
> > no?
>
> Well, we can't take ->siglock in thread_group_cputime(), sometimes it
> is called under ->siglock. do_task_stat(), get_cpu_itimer() at least.
>
> IIRC, Stanislaw verified other callers have no problems with this helper.

Actually I didn't. Try to do now.

Most calls are done with sighand or tasklist lock taken, except

Cscope tag: thread_group_cputime
   #   line  filename / context / line
   1   1353  fs/binfmt_elf.c <<fill_prstatus>>
             thread_group_cputime(p, &cputime);
   2   1403  fs/binfmt_elf_fdpic.c <<fill_prstatus>>
             thread_group_cputime(p, &cputime);
  10    129  mm/oom_kill.c <<badness>>
             thread_group_cputime(p, &task_time);

I do not think in case 1 and 2 lock is needed since it seems to be core dump
with all threads dead. Not sure about 10 - oom killer.

One other exception is:
fastpath_timer_check() -> thread_group_cputimer() -> thread_group_cputime()

We can solve this like that:

--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -1375,13 +1375,8 @@ static inline int fastpath_timer_check(struct task_struct *tsk)
  }
 
  sig = tsk->signal;
- if (!task_cputime_zero(&sig->cputime_expires)) {
- struct task_cputime group_sample;
-
- thread_group_cputimer(tsk, &group_sample);
- if (task_cputime_expired(&group_sample, &sig->cputime_expires))
- return 1;
- }
+ if (!task_cputime_zero(&sig->cputime_expires))
+ return 1;
 
  return sig->rlim[RLIMIT_CPU].rlim_cur != RLIM_INFINITY;
 }

Or stay with task_cputime_expired() but only if cputimer is currently running.

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

[Oleg Nesterov-3]

On 11/10, Stanislaw Gruszka wrote:
>
>   10    129  mm/oom_kill.c <<badness>>
>              thread_group_cputime(p, &task_time);
>
> Not sure about 10 - oom killer.

Not sure too, but I don't think we should worry about badness().

> One other exception is:
> fastpath_timer_check() -> thread_group_cputimer() -> thread_group_cputime()
>
> We can solve this like that:
>
> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -1375,13 +1375,8 @@ static inline int fastpath_timer_check(struct task_struct *tsk)
>   }
>
>   sig = tsk->signal;
> - if (!task_cputime_zero(&sig->cputime_expires)) {
> - struct task_cputime group_sample;
> -
> - thread_group_cputimer(tsk, &group_sample);
> - if (task_cputime_expired(&group_sample, &sig->cputime_expires))
> - return 1;
> - }
> + if (!task_cputime_zero(&sig->cputime_expires))
> + return 1;
>
>   return sig->rlim[RLIMIT_CPU].rlim_cur != RLIM_INFINITY;
>  }
>
> Or stay with task_cputime_expired() but only if cputimer is currently running.

Oh. I forgot this code completely, can't comment.

Can't we ensure that fastpath_timer_check() never do while_each_thread() ?
IOW, if sig->cputime_expires != 0 then ->running must be true.

At least, shouldn't stop_process_timers() clear sig->cputime_expires ?

Oleg.

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

[Stanislaw Gruszka-2]

On Tue, Nov 10, 2009 at 06:40:08PM +0100, Oleg Nesterov wrote:

> > One other exception is:
> > fastpath_timer_check() -> thread_group_cputimer() -> thread_group_cputime()
> >
> > We can solve this like that:
> >
> > --- a/kernel/posix-cpu-timers.c
> > +++ b/kernel/posix-cpu-timers.c
> > @@ -1375,13 +1375,8 @@ static inline int fastpath_timer_check(struct task_struct *tsk)
> >   }
> >
> >   sig = tsk->signal;
> > - if (!task_cputime_zero(&sig->cputime_expires)) {
> > - struct task_cputime group_sample;
> > -
> > - thread_group_cputimer(tsk, &group_sample);
> > - if (task_cputime_expired(&group_sample, &sig->cputime_expires))
> > - return 1;
> > - }
> > + if (!task_cputime_zero(&sig->cputime_expires))
> > + return 1;
> >
> >   return sig->rlim[RLIMIT_CPU].rlim_cur != RLIM_INFINITY;
> >  }
> >
> > Or stay with task_cputime_expired() but only if cputimer is currently running.
>
> Oh. I forgot this code completely, can't comment.
>
> Can't we ensure that fastpath_timer_check() never do while_each_thread() ?

Removing possibility to call while_each_tread() from fastpath_timer_check()
was exactly my intension here, perhaps I was not clear.

> IOW, if sig->cputime_expires != 0 then ->running must be true.
> At least, shouldn't stop_process_timers() clear sig->cputime_expires ?

I'm going to think about that. However as far seems, checking ->running
explicitly and goto slow patch when is not true is safer solution.

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

[Oleg Nesterov-3]

On 11/10, Stanislaw Gruszka wrote:

>
> On Tue, Nov 10, 2009 at 06:40:08PM +0100, Oleg Nesterov wrote:
> > >
> > > Or stay with task_cputime_expired() but only if cputimer is currently running.
> >
> > Oh. I forgot this code completely, can't comment.
> >
> > Can't we ensure that fastpath_timer_check() never do while_each_thread() ?
>
> Removing possibility to call while_each_tread() from fastpath_timer_check()
> was exactly my intension here, perhaps I was not clear.

Yes, yes, I understand.

I meant, perhaps we can ensure this shouldn't happen "by design", instead
of checking ->running in fastpath_timer_check().

> > IOW, if sig->cputime_expires != 0 then ->running must be true.
> > At least, shouldn't stop_process_timers() clear sig->cputime_expires ?
>
> I'm going to think about that. However as far seems, checking ->running
> explicitly and goto slow patch when is not true is safer solution.

Yes, agreed.


Still. check_process_timers() updates sig->cputime_expires at the end,
but it never clears it. For example,

        if (sched_expires != 0 &&
            (sig->cputime_expires.sched_exp == 0 ||
             sig->cputime_expires.sched_exp > sched_expires))
                sig->cputime_expires.sched_exp = sched_expires;

Why?

Now suppose that (say) sig->cputime_expires.sched_exp != 0, there are
no cpu timers, ->running == F.

In this case fastpath_timer_check() always returns T and triggers the
slow path which does nothing, not good.

But yes, I agree, probably we should start with the simple/safe change
as you suggested.

Oleg.

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

[Stanislaw Gruszka-2]

Hi Hidetoshi

On Tue, Nov 10, 2009 at 02:47:34PM +0900, Hidetoshi Seto wrote:

> Remove casting to clock_t from task_u/stime(), and keep granularity of
> cputime_t over the calculation.
>
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...>
> ---
>  kernel/sched.c |   21 ++++++++++++---------
>  1 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 28dd4f4..1632e82 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -5171,41 +5171,44 @@ cputime_t task_stime(struct task_struct *p)
>   return p->stime;
>  }
>  #else
> +
> +#ifndef nsecs_to_cputime
> +# define nsecs_to_cputime(__nsecs) msecs_to_cputime((__nsecs) / NSEC_PER_MSEC)
> +#endif
> +

I would like to test, but unfortunately with the patch kernel not build
on my system:

kernel/built-in.o: In function `task_utime':
/usr/src/rhel5/linux-2.6/kernel/sched.c:5187: undefined reference to
`__udivdi3'
kernel/built-in.o: In function `task_stime':
/usr/src/rhel5/linux-2.6/kernel/sched.c:5208: undefined reference to
`__udivdi3'
make: *** [.tmp_vmlinux1] Error 1

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

[Hidetoshi Seto]

Stanislaw Gruszka wrote:

> On Tue, Nov 10, 2009 at 02:47:34PM +0900, Hidetoshi Seto wrote:
>> Remove casting to clock_t from task_u/stime(), and keep granularity of
>> cputime_t over the calculation.
>>
>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...>
>> ---
>>  kernel/sched.c |   21 ++++++++++++---------
>>  1 files changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/sched.c b/kernel/sched.c
>> index 28dd4f4..1632e82 100644
>> --- a/kernel/sched.c
>> +++ b/kernel/sched.c
>> @@ -5171,41 +5171,44 @@ cputime_t task_stime(struct task_struct *p)
>>   return p->stime;
>>  }
>>  #else
>> +
>> +#ifndef nsecs_to_cputime
>> +# define nsecs_to_cputime(__nsecs) msecs_to_cputime((__nsecs) / NSEC_PER_MSEC)
>> +#endif
>> +
>
> I would like to test, but unfortunately with the patch kernel not build
> on my system:
>
> kernel/built-in.o: In function `task_utime':
> /usr/src/rhel5/linux-2.6/kernel/sched.c:5187: undefined reference to
> `__udivdi3'
> kernel/built-in.o: In function `task_stime':
> /usr/src/rhel5/linux-2.6/kernel/sched.c:5208: undefined reference to
> `__udivdi3'
> make: *** [.tmp_vmlinux1] Error 1

Wow, what arch?
Could you provide your .config?


Thanks,
H.Seto

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

[Hidetoshi Seto]

Hidetoshi Seto wrote:

> Stanislaw Gruszka wrote:
>> On Tue, Nov 10, 2009 at 02:47:34PM +0900, Hidetoshi Seto wrote:
>>> Remove casting to clock_t from task_u/stime(), and keep granularity of
>>> cputime_t over the calculation.
>>>
>>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...>
>>> ---
>>>  kernel/sched.c |   21 ++++++++++++---------
>>>  1 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/kernel/sched.c b/kernel/sched.c
>>> index 28dd4f4..1632e82 100644
>>> --- a/kernel/sched.c
>>> +++ b/kernel/sched.c
>>> @@ -5171,41 +5171,44 @@ cputime_t task_stime(struct task_struct *p)
>>>   return p->stime;
>>>  }
>>>  #else
>>> +
>>> +#ifndef nsecs_to_cputime
>>> +# define nsecs_to_cputime(__nsecs) msecs_to_cputime((__nsecs) / NSEC_PER_MSEC)
>>> +#endif
>>> +
>> I would like to test, but unfortunately with the patch kernel not build
>> on my system:
>>
>> kernel/built-in.o: In function `task_utime':
>> /usr/src/rhel5/linux-2.6/kernel/sched.c:5187: undefined reference to
>> `__udivdi3'
>> kernel/built-in.o: In function `task_stime':
>> /usr/src/rhel5/linux-2.6/kernel/sched.c:5208: undefined reference to
>> `__udivdi3'
>> make: *** [.tmp_vmlinux1] Error 1
>
> Wow, what arch?
> Could you provide your .config?

Oh, I found I can hit this error on x86_32 (but not on x86_64).
Maybe something on dividing u64...

I'll try to find fix/workaround for this, but would appreciate if someone
could tell me what is happening here.


Thanks,
H.Seto

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

[Américo Wang]

On Thu, Nov 12, 2009 at 10:49 AM, Hidetoshi Seto
<seto.hidetoshi@...> wrote:

> Hidetoshi Seto wrote:
>> Stanislaw Gruszka wrote:
>>> On Tue, Nov 10, 2009 at 02:47:34PM +0900, Hidetoshi Seto wrote:
>>>> Remove casting to clock_t from task_u/stime(), and keep granularity of
>>>> cputime_t over the calculation.
>>>>
>>>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...>
>>>> ---
>>>>  kernel/sched.c |   21 ++++++++++++---------
>>>>  1 files changed, 12 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/kernel/sched.c b/kernel/sched.c
>>>> index 28dd4f4..1632e82 100644
>>>> --- a/kernel/sched.c
>>>> +++ b/kernel/sched.c
>>>> @@ -5171,41 +5171,44 @@ cputime_t task_stime(struct task_struct *p)
>>>>     return p->stime;
>>>>  }
>>>>  #else
>>>> +
>>>> +#ifndef nsecs_to_cputime
>>>> +# define nsecs_to_cputime(__nsecs) msecs_to_cputime((__nsecs) / NSEC_PER_MSEC)
>>>> +#endif
>>>> +
>>> I would like to test, but unfortunately with the patch kernel not build
>>> on my system:
>>>
>>> kernel/built-in.o: In function `task_utime':
>>> /usr/src/rhel5/linux-2.6/kernel/sched.c:5187: undefined reference to
>>> `__udivdi3'
>>> kernel/built-in.o: In function `task_stime':
>>> /usr/src/rhel5/linux-2.6/kernel/sched.c:5208: undefined reference to
>>> `__udivdi3'
>>> make: *** [.tmp_vmlinux1] Error 1
>>
>> Wow, what arch?
>> Could you provide your .config?
>
> Oh, I found I can hit this error on x86_32 (but not on x86_64).
> Maybe something on dividing u64...
>
> I'll try to find fix/workaround for this, but would appreciate if someone
> could tell me what is happening here.

Yeah, this is because on 32-bit gcc tries to use libgcc to do 64-bit
integer computing. You can insert inline asm to force gcc not to
optimize this code. ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Hidetoshi Seto]

Américo Wang wrote:
>>>> kernel/built-in.o: In function `task_utime':
>>>> /usr/src/rhel5/linux-2.6/kernel/sched.c:5187: undefined reference to
>>>> `__udivdi3'
>>>> kernel/built-in.o: In function `task_stime':
>>>> /usr/src/rhel5/linux-2.6/kernel/sched.c:5208: undefined reference to
>>>> `__udivdi3'
>>>> make: *** [.tmp_vmlinux1] Error 1

>> Oh, I found I can hit this error on x86_32 (but not on x86_64).
>> Maybe something on dividing u64...
>>
>> I'll try to find fix/workaround for this, but would appreciate if someone
>> could tell me what is happening here.
>
> Yeah, this is because on 32-bit gcc tries to use libgcc to do 64-bit
> integer computing. You can insert inline asm to force gcc not to
> optimize this code. ;)

Thank you for the information.

According to the following commit which I found that is against "__udivdi3"
error, use of div_u64() will fix this problem, right?

 commit 956dba3caaf66b84fe5f6180e0e4dd03902c7980
 Author: Andy Whitcroft <apw@...>
 Date:   Wed Jul 1 15:20:59 2009 +0100

I'm now testing modified patch...


Thanks,
H.Seto

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