[PATCH] ARM: implement optimized percpu variable access

[Will Deacon]

On Mon, Nov 26, 2012 at 11:13:37AM +0000, Will Deacon wrote:
> On Sun, Nov 25, 2012 at 06:46:55PM +0000, Rob Herring wrote:
> > On 11/22/2012 05:34 AM, Will Deacon wrote:
> > > As an aside, you also need to make the asm block volatile in
> > > __my_cpu_offset -- I can see it being re-ordered before the set for
> > > secondary CPUs otherwise.
> > 
> > I don't really see where there would be a re-ordering issue. There's no
> > percpu var access before or near the setting that I can see.
> 
> The issue is on bringing up the secondary core, so I assumed that a lot
> of inlining goes on inside secondary_start_kernel and then the result is
> shuffled around, placing a cpu-offset read before we've done the set.
> 
> Unfortunately, looking at the disassembly I can't see this happening at
> all, so I'll keep digging. The good news is that I've just reproduced the
> problem on the model, so I've got more visibility now (although both cores
> are just stuck in spinlocks...).

That was a fun bit of debugging -- my hunch was right, but I was looking in the
wrong place because I had an unrelated problem with my bootloader.

What happens is that every man and his dog is inlined into __schedule,
including all the runqueue accessors, such as this_rq(), which make use of
per-cpu offsets to get the correct pointer. The compiler then spits out
something like this near the start of the function:

  c02c1d66:       af04            add     r7, sp, #16
  [...]
  c02c1d6c:       ee1d 3f90       mrc     15, 0, r3, cr13, cr0, {4}
  c02c1d70:       199b            adds    r3, r3, r6
  c02c1d72:       f8c7 e008       str.w   lr, [r7, #8]
  c02c1d76:       617b            str     r3, [r7, #20]
  c02c1d78:       613e            str     r6, [r7, #16]
  c02c1d7a:       60fb            str     r3, [r7, #12]

so the address of the current runqueue has been calculated and stored, with
a bunch of other stuff, in a structure on the stack.

We then do our context_switch dance (which is also inlined) and return as
the next task (since we've done switch_{mm,to}) before doing:

	barrier();
	/*
	 * this_rq must be evaluated again because prev may have moved
	 * CPUs since it called schedule(), thus the 'rq' on its stack
	 * frame will be invalid.
	 */
	finish_task_switch(this_rq(), prev);

The problem here is that, because our CPU accessors don't actually make any
memory references, the barrier() has no effect and the old value is just
reloaded off the stack:

  c02c1f22:       f54a fe49       bl      c000cbb8 <__switch_to>
  c02c1f26:       4601            mov     r1, r0
  c02c1f28:       68f8            ldr     r0, [r7, #12]
  c02c1f2a:       f56f ffd5       bl      c0031ed8 <finish_task_switch>

which obviously causes complete chaos if the new task has been pulled from
a different runqueue! (this appears as a double spin unlock on rq->lock).

Fixing this without giving up the performance improvement we gain by *avoiding*
the memory access in the first place is going to be tricky...

Will