svn commit: r223485 - in head/sys/powerpc: aim booke include ofw powerpc

[Marcel Moolenaar]

On Jun 24, 2011, at 9:18 AM, Nathan Whitehorn wrote:

> On 06/24/11 11:11, Marcel Moolenaar wrote:
>> 
>> On Jun 23, 2011, at 3:21 PM, Nathan Whitehorn wrote:
>> 
>>> Author: nwhitehorn
>>> Date: Thu Jun 23 22:21:28 2011
>>> New Revision: 223485
>>> URL: http://svn.freebsd.org/changeset/base/223485
>>> 
>>> Log:
>>>  Use the ABI-mandated thread pointer register (r2 for ppc32, r13 for ppc64)
>>>  instead of a PCPU field for curthread. This averts a race on SMP systems
>>>  with a high interrupt rate where the thread looking up the value of
>>>  curthread could be preempted and migrated between obtaining the PCPU
>>>  pointer and reading the value of pc_curthread, resulting in curthread being
>>>  observed to be the current thread on the thread's original CPU. This played
>>>  merry havoc with the system, in particular with mutexes. Many thanks to
>>>  jhb for helping me work this one out.
>> 
>> Nice catch!
>> 
>> Another approach would be to have r2/r13 hold the address of the PCPU
>> structure and simply do a load from that address to get curthread.
>> The difference between the approaches is the need to to a memory load
>> or not for curthread. But with r2/r13 pointing to the PCPU, you may
>> be faster to get other PCPU fields if reading from the a SPR adds to
>> the overhead. Plus, it's easier to be atomic if you don't have to
>> read the SPR first and then do a load.
> 
> The trouble with this approach is that r2/r13 would need to be updated on every CPU switch with the new PCPU pointer, so I just put curthread in there due to laziness, which is of course constant for a given thread.

Actually, you need to save and assign that register on entry
into the kernel only and restore it when you go back to user
space (due to it being the thread pointer in user space). It
remains a constant within the kernel (from the CPU's point of
view) and does not have to be changed on a context switch.

If r2/r13 hold curthread, then r2/r13 are indeed constant from
the perspective of the thread, but it needs to be changed for
every context switch in that case. From the CPU's perspective
these registers are not constant then.

> Another consideration is that we'd have to additionally maintain SPRG0 as the PCPU pointer anyway, since we need a PCPU area that userland can't change (r2/r13 is set from PCPU data when traps are taken now).

Yes.

On ia64 we use the thread register to hold the address of the
PCPU structure and since curthread is at offset 0 in the PCPU
structure, we can do an atomic load. We use a kernel register
to restore the PCPU address in the thread register on entry
into the kernel. Works well and it doesn't make curthread too
special (other than it needing to be at offset 0 in the PCPU
structure for the dereferencing to be atomic). As such, even
PCPU_GET(curthread) is atomic...

Anyway: I'll see about fixing Book-E...

> 
>> Is curthread the only field that needs to be atomically accessed or
>> are other fields in the PCPU susceptible to race conditions?
> 
> In my discussion with John yesterday, he said he thought it was the only one susceptible to races of this type. The approach I used here (providing a special accessor for curthread that reads a non-PCPU-related register) is borrowed from the one used on amd64, i386, and alpha. Whether it is the only possibility for this kind of race or not, none of these platforms at least override anything else.

Thanks. That's what I figured, but I never really had any firm
confirmation and we still have comments in our code like:

/*
 * XXX The implementation of this operation should be made atomic
 * with respect to preemption.
 */
#define PCPU_ADD(member, value) (pcpup->pc_ ## member += (value))


It's always created uncertainty and doubt in my mind...

-- 
Marcel Moolenaar
marcel at xcllnt.net