Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Kevin Hilman
Frederic Weisbecker writes: > On Fri, Feb 22, 2013 at 06:21:31AM -0800, Eric Dumazet wrote: >> On Fri, 2013-02-22 at 15:09 +0100, Peter Zijlstra wrote: >> > On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: >> > > > Argh!! at what cost? 64bit atomics are like expensive. Wouldn't >> >

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Peter Zijlstra
On Fri, 2013-02-22 at 15:16 +0100, Ingo Molnar wrote: > > I checked arch/x86/include/asm/atomic64_32.h and we use > > cmpxchg8b for everything from _set() to _read(), which > > translates into 'horridly stupendifyingly slow' for a number > > of machines, but coherent. > > That's a valid concern

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Frederic Weisbecker
On Fri, Feb 22, 2013 at 06:21:31AM -0800, Eric Dumazet wrote: > On Fri, 2013-02-22 at 15:09 +0100, Peter Zijlstra wrote: > > On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > > > Argh!! at what cost? 64bit atomics are like expensive. Wouldn't > > > adding > > > > a seqlock be saner?

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Eric Dumazet
On Fri, 2013-02-22 at 15:09 +0100, Peter Zijlstra wrote: > On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > > Argh!! at what cost? 64bit atomics are like expensive. Wouldn't > > adding > > > a seqlock be saner? > > > > Not sure. This requires a spinlock in the write side which is

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Ingo Molnar
* Peter Zijlstra wrote: > On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > > Argh!! at what cost? 64bit atomics are like expensive. Wouldn't > > adding > > > a seqlock be saner? > > > > Not sure. This requires a spinlock in the write side which is called > > from > > fast path

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Ingo Molnar
* Peter Zijlstra wrote: > On Fri, 2013-02-22 at 14:54 +0100, Ingo Molnar wrote: > > * Peter Zijlstra wrote: > > > > > On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > > > > > atomic64_read() and atomic64_set() are supposed to take care > > > > of that, without even the need

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Frederic Weisbecker
On Fri, Feb 22, 2013 at 03:05:39PM +0100, Peter Zijlstra wrote: > On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > > Which is a problem how? > > > > So here is a possible scenario, CPU 0 reads a kcpustat value, and CPU > > 1 writes > > it at the same time: > > > > //Initial v

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Peter Zijlstra
On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > Argh!! at what cost? 64bit atomics are like expensive. Wouldn't > adding > > a seqlock be saner? > > Not sure. This requires a spinlock in the write side which is called > from > fast path like the timer interrupt. A single spinloc

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Peter Zijlstra
On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > Which is a problem how? > > So here is a possible scenario, CPU 0 reads a kcpustat value, and CPU > 1 writes > it at the same time: > > //Initial value of "cpustat" is 0x > == CPU 0 == == CPU 1 == >

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Peter Zijlstra
On Fri, 2013-02-22 at 14:54 +0100, Ingo Molnar wrote: > * Peter Zijlstra wrote: > > > On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > > > atomic64_read() and atomic64_set() are supposed to take care > > > of that, without even the need for _inc() or _add() parts > > > that us

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Ingo Molnar
* Peter Zijlstra wrote: > On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > > atomic64_read() and atomic64_set() are supposed to take care > > of that, without even the need for _inc() or _add() parts > > that use LOCK. > > Are you sure? Generally atomic*_set() is not actually

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Peter Zijlstra
On Fri, 2013-02-22 at 13:50 +0100, Frederic Weisbecker wrote: > atomic64_read() and atomic64_set() are supposed to take care of that, > without > even the need for _inc() or _add() parts that use LOCK. Are you sure? Generally atomic*_set() is not actually an atomic operation. -- To unsubscribe fr

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Frederic Weisbecker
On Fri, Feb 22, 2013 at 09:46:07AM +0100, Peter Zijlstra wrote: > On Thu, 2013-02-21 at 21:56 -0800, Kevin Hilman wrote: > > On 64-bit platforms, reads/writes of the various cpustat fields are > > atomic due to native 64-bit loads/stores. However, on non 64-bit > > platforms, reads/writes of the c

Re: [PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-22 Thread Peter Zijlstra
On Thu, 2013-02-21 at 21:56 -0800, Kevin Hilman wrote: > On 64-bit platforms, reads/writes of the various cpustat fields are > atomic due to native 64-bit loads/stores. However, on non 64-bit > platforms, reads/writes of the cpustat fields are not atomic and could > lead to inconsistent statistics

[PATCH 0/2] cpustat: use atomic operations to read/update stats

2013-02-21 Thread Kevin Hilman
On 64-bit platforms, reads/writes of the various cpustat fields are atomic due to native 64-bit loads/stores. However, on non 64-bit platforms, reads/writes of the cpustat fields are not atomic and could lead to inconsistent statistics. This problem was originally reported by Frederic Weisbecker