Re: broken local_t on i386

[Andi Kleen <ak@suse.de>]

On Monday 12 June 2006 18:37, Christoph Lameter wrote:
> On Mon, 12 Jun 2006, Ingo Molnar wrote:
> 
> > below is an updated patch that includes fixups for i386 - but the real 
> > fix should be to properly reduce the per-arch local.h footprint to the 
> > bare minimum possible, and to do this fix on the asm-generic headers.
> 
> Nak. This is simply evidence of local.t breakage on i386. One cannot 
> calculate the address of a per cpu area and then increment without 
> disabling preemption. The process may have been moved to another processor 
> between those two operations and will then increment the event counter of 
> the former processor (which in turn may at the same time increment the 
> same counter). The inc is not atomic in the sense that it syncs multiple 
> processors. So we will have the race back.

True - i forgot that race.
 
> The increment must occur directly through the atomic-vs-interrupt dec/inc 
> on the local per cpu area *without* any use of *_smp_processor_id().
> 
> As far as I can see x86_64 does the right thing and it increments on the 
> local per cpu area. The definition of __get_cpu_var is:


> 
> #define __get_cpu_var(var) (*RELOC_HIDE(&per_cpu__##var, __my_cpu_offset()))

It is also affected by your race. The inc would only be atomic if the counter
was in the PDA, but standard per cpu data isn't. So it has to follow 
a pointer and then it could already have switched.

[I think Ingo stated this earlier, but I didn't get -- sorry]

Fix would be to disable preemption. I don't think it needs cli/sti
on non preemptible kernels.

> i386 uses the asm-generic/percpu.h but provides its own 
> implementation of local.t. That simply cannot work. i386
> must provide a definition of __get_per_var that increments directly 
> on the variable in the local per cpu area.
> 
> The definition for __get_cpu_var in asm-generic/percpu.h is:
> 
> #define __get_cpu_var(var) per_cpu(var, smp_processor_id())
> 
> This breaks the cpu_* operations for local.t on i386.
> 
> Also we have various forms of __raw_get_cpu_var() around. Is there any 
> reason for their existence. The presence of these shows the assumption 
> that one can determine the current processor id and then index into an 
> array of per cpu areas. That is not possible with preemption enabled.
> 
> In the absence of a race free __get_cpu_var() i386 would need to fall 
> back to atomic ops by using asm-generic/local.t.

Or just disable preemption?

-Andi