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

Rusty Russell
2008-06-02T03:34:48Z

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.