Re: [patch 04/41] cpu ops: Core piece for generic atomic per cpu operations

[Rusty Russell <rusty@rustcorp.com.au>]

On Saturday 31 May 2008 04:00:40 Christoph Lameter wrote:
> On Fri, 30 May 2008, Rusty Russell wrote:
> > > No its not! In order to increment a per cpu value you need to calculate
> > > the per cpu pointer address in the current per cpu segment.
> >
> > Christoph, you just missed it, that's all.  Look at cpu_local_read et al
> > in include/asm-i386/local.h (ie. before the x86 mergers chose the lowest
> > common denominator one).
>
> There is no doubt that local_t does perform an atomic vs. interrupt inc
> for example. But its not usable. Because you need to determine the address
> of the local_t belonging to the current processor first.

Christoph!

STOP typing, and START reading.

cpu_local_inc() does all this: it takes the name of a local_t var, and is 
expected to increment this cpu's version of that.  You ripped this out and 
called it CPU_INC().

Do not make me explain it a third time.

> As soon as you 
> have loaded a processor specific address you can no longer be preempted
> because that may change the processor and then the wrong address may be
> increment (and then we have a race again since now we are incrementing
> counters belonging to other processors). So local_t at mininum requires
> disabling preempt.

Think for a moment.  What are the chances that I didn't understand this when I 
wrote the multiple implementations of local_t?

You are wasting my time explaining the obvious, and wasting your own.

> Believe me I have tried to use local_t repeatedly for vm statistics etc.
> It always fails on that issue.

Frankly, I am finding it increasingly easy to believe that you failed.  But 
you are blaming the wrong thing.

There are three implementations of local_t which are obvious.  The best is for 
architectures which can locate and increment a per-cpu var in one instruction 
(eg. x86).  Otherwise, using atomic_t/atomic64_t for local_t provides a 
general solution.  The other general solution would involve 
local_irq_disable()/increment/local_irq_enable().

My (fading) hope is that this idiocy is an abberation,
Rusty.