[PATCH] ARM: implement optimized percpu variable access

[Nicolas Pitre]

On Tue, 27 Nov 2012, Nicolas Pitre wrote:

> On Mon, 26 Nov 2012, Will Deacon wrote:
> 
> > 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...
> 
> What about adding a memory constraint in the offset accessor to create a 
> dependency upon which the barrier will have an effect, but without 
> actually making any memory access?

OK, whatever Jamie said.  I see that he pushed the analysis much further 
already.


Nicolas