Re: Linux 2.6.32-rc1

[Eric Dumazet <eric.dumazet@gmail.com>]

Eric Dumazet a écrit :
> Martin Schwidefsky a écrit :
>> On Mon, 28 Sep 2009 08:39:31 -0700 (PDT)
>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>
>>> On Mon, 28 Sep 2009, Eric Dumazet wrote:
>>>> Linus Torvalds a écrit :
>>>>> Go wild, test it out, and let us know about any regressions you find,
>>>> Something seems wrong with process time accounting.
>>>>
>>>> Following program should consume 10*8 seconds of cpu on a 8 cpu machine, but
>>>> with 2.6.32-rc1 numbers are crazy.
>>> Ok, so the top-level process time looks sane _while_ the thing is running 
>>> (sum of all threads), but the per-thread times look broken (as if they had 
>>> randomly had the times of the other threads added into them - looks to me 
>>> like they on average had half the other threads' time accounted into 
>>> them).
>>>
>>> And then at the end, it looks like the time of the threads (which was 
>>> over-accounted) get re-accounted back into the main process, so the final 
>>> time is indeed wildly inflated.
>>>
>>> IOW, looks like we're adding thread times multiple times to other threads 
>>> (and then finally to the parent).
>>>
>>> I'm adding the usual suspects to the cc, and leaving your results and 
>>> test-program quoted here for them.. Thomas, Martin, John - if you're not 
>>> the people I should blame for this, let me know.
>> Uh-oh.. usual suspects, eh?
>>
>> For starters I run the test program on an s390 with
>> VIRT_CPU_ACCOUNTING=y:
>>
>> time ./burn-cpu 
>>   PID TTY      STAT   TIME COMMAND 
>>  2979 pts/0    -      0:08 ./burn-cpu
>>     - -        Sl+    0:00 -         
>>     - -        Rl+    0:01 -         
>>     - -        Rl+    0:01 -         
>>     - -        Rl+    0:01 -         
>>     - -        Rl+    0:01 -         
>>     - -        Rl+    0:01 -         
>>     - -        Rl+    0:01 -         
>>     - -        Rl+    0:01 -         
>>     - -        Rl+    0:01 -         
>>   PID TTY      STAT   TIME COMMAND   
>>  2979 pts/0    -      0:16 ./burn-cpu
>>     - -        Sl+    0:00 -         
>>     - -        Rl+    0:02 -         
>>     - -        Rl+    0:02 -         
>>     - -        Rl+    0:02 -         
>>     - -        Rl+    0:02 -         
>>     - -        Rl+    0:02 -         
>>     - -        Rl+    0:02 -         
>>     - -        Rl+    0:02 -         
>>     - -        Rl+    0:02 -         
>>   PID TTY      STAT   TIME COMMAND   
>>  2979 pts/0    -      0:25 ./burn-cpu
>>     - -        Sl+    0:00 -         
>>     - -        Rl+    0:03 -         
>>     - -        Rl+    0:03 -         
>>     - -        Rl+    0:03 -         
>>     - -        Rl+    0:03 -         
>>     - -        Rl+    0:03 -         
>>     - -        Rl+    0:03 -         
>>     - -        Rl+    0:03 -         
>>     - -        Rl+    0:03 -         
>>   PID TTY      STAT   TIME COMMAND   
>>  2979 pts/0    -      0:33 ./burn-cpu
>>     - -        Sl+    0:00 -         
>>     - -        Rl+    0:04 -         
>>     - -        Rl+    0:04 -         
>>     - -        Rl+    0:04 -         
>>     - -        Rl+    0:04 -         
>>     - -        Rl+    0:04 -         
>>     - -        Rl+    0:04 -         
>>     - -        Rl+    0:04 -         
>>     - -        Rl+    0:04 -         
>>   PID TTY      STAT   TIME COMMAND   
>>  2979 pts/0    -      0:41 ./burn-cpu
>>     - -        Sl+    0:00 -         
>>     - -        Rl+    0:05 -         
>>     - -        Rl+    0:05 -         
>>     - -        Rl+    0:05 -         
>>     - -        Rl+    0:05 -         
>>     - -        Rl+    0:05 -         
>>     - -        Rl+    0:05 -         
>>     - -        Rl+    0:05 -         
>>     - -        Rl+    0:05 -         
>>   PID TTY      STAT   TIME COMMAND   
>>  2979 pts/0    -      0:50 ./burn-cpu
>>     - -        Sl+    0:00 -
>>     - -        Rl+    0:06 -
>>     - -        Rl+    0:06 -
>>     - -        Rl+    0:06 -
>>     - -        Rl+    0:06 -
>>     - -        Rl+    0:06 -
>>     - -        Rl+    0:06 -
>>     - -        Rl+    0:06 -
>>     - -        Rl+    0:06 -
>>   PID TTY      STAT   TIME COMMAND
>>  2979 pts/0    -      0:58 ./burn-cpu
>>     - -        Sl+    0:00 -
>>     - -        Rl+    0:07 -
>>     - -        Rl+    0:07 -
>>     - -        Rl+    0:07 -
>>     - -        Rl+    0:07 -
>>     - -        Rl+    0:07 -
>>     - -        Rl+    0:07 -
>>     - -        Rl+    0:07 -
>>     - -        Rl+    0:07 -
>>   PID TTY      STAT   TIME COMMAND
>>  2979 pts/0    -      1:07 ./burn-cpu
>>     - -        Sl+    0:00 -
>>     - -        Rl+    0:08 -
>>     - -        Rl+    0:08 -
>>     - -        Rl+    0:08 -
>>     - -        Rl+    0:08 -
>>     - -        Rl+    0:08 -
>>     - -        Rl+    0:08 -
>>     - -        Rl+    0:08 -
>>     - -        Rl+    0:08 -
>>   PID TTY      STAT   TIME COMMAND
>>  2979 pts/0    -      1:15 ./burn-cpu
>>     - -        Sl+    0:00 -
>>     - -        Rl+    0:09 -
>>     - -        Rl+    0:09 -
>>     - -        Rl+    0:09 -
>>     - -        Rl+    0:09 -
>>     - -        Rl+    0:09 -
>>     - -        Rl+    0:09 -
>>     - -        Rl+    0:09 -
>>     - -        Rl+    0:09 -
>>   PID TTY      STAT   TIME COMMAND
>>  2979 pts/0    -      1:25 ./burn-cpu
>>     - -        S+     1:25 -
>>
>> real    0m10.708s
>> user    1m25.051s
>> sys     0m0.174s
>>
>> looks better, gives an average of 10.63 seconds per thread and the per
>> thread numbers are fine. I'll see what happens with the test case on my
>> pc@home.
>>
> 
> 
> I did a bisection and found commit def0a9b2573e00ab0b486cb5382625203ab4c4a6
> was the origin of the problem on my x86_32 machine.
> 
> def0a9b2573e00ab0b486cb5382625203ab4c4a6 is first bad commit
> commit def0a9b2573e00ab0b486cb5382625203ab4c4a6
> Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date:   Fri Sep 18 20:14:01 2009 +0200
> 
>     sched_clock: Make it NMI safe
> 
>     Arjan complained about the suckyness of TSC on modern machines, and
>     asked if we could do something about that for PERF_SAMPLE_TIME.
> 
>     Make cpu_clock() NMI safe by removing the spinlock and using
>     cmpxchg. This also makes it smaller and more robust.
> 
>     Affects architectures that use HAVE_UNSTABLE_SCHED_CLOCK, i.e. IA64
>     and x86.
> 
>     Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
>     LKML-Reference: <new-submission>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 

Pretty calm lkml these days... must be some kind of event in the states ? :)

Checking this commit, I believe problem comes from cmpxchg(), which doesnt 
handle 64 bit on X86_32 (no compilation error, and  null operation :( )

We have two (or three choices) :

1) Use cmpxchg64()

2) Fix cmpxchg() to handle 64bit as well (or issue a compilation error)

3) Revert Peter patch :(

Here is the patch I used to get proper operation.

[PATCH] sched_clock: Use cmpxchg64()

Commit def0a9b2573e00ab0b486cb5382625203ab4c4a6 
(sched_clock: Make it NMI safe) assumed cmpxchg() of 64bit values was available on X86_32

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---

diff --git a/kernel/sched_clock.c b/kernel/sched_clock.c
index ac2e1dc..479ce56 100644
--- a/kernel/sched_clock.c
+++ b/kernel/sched_clock.c
@@ -127,7 +127,7 @@ again:
 	clock = wrap_max(clock, min_clock);
 	clock = wrap_min(clock, max_clock);
 
-	if (cmpxchg(&scd->clock, old_clock, clock) != old_clock)
+	if (cmpxchg64(&scd->clock, old_clock, clock) != old_clock)
 	goto again;
 
 	return clock;
@@ -163,7 +163,7 @@ again:
 	val = remote_clock;
 	}
 
-	if (cmpxchg(ptr, old_val, val) != old_val)
+	if (cmpxchg64(ptr, old_val, val) != old_val)
 		goto again;
 
 	return val;