Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 24 Jun 2011 14:24:43 -0500
From:      Nathan Whitehorn <nwhitehorn@freebsd.org>
To:        Marcel Moolenaar <marcel@xcllnt.net>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r223485 - in head/sys/powerpc: aim booke include ofw powerpc
Message-ID:  <4E04E47B.2030007@freebsd.org>
In-Reply-To: <13E5FED8-30BE-4292-B024-AB692E9E2A89@xcllnt.net>
References:  <201106232221.p5NMLSFj019042@svn.freebsd.org> <1B2D07F9-3716-4581-8426-11BE78CE1D1F@xcllnt.net> <4E04B8DC.1020600@freebsd.org> <13E5FED8-30BE-4292-B024-AB692E9E2A89@xcllnt.net>

next in thread | previous in thread | raw e-mail | index | archive | help
On 06/24/11 11:33, Marcel Moolenaar wrote:
>
> 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...

Ah, I understand now. There's some code in thread switching that 
saves/restores R2/R13 which would need to be turned off, but maybe 
that's something to look into.

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

Thanks!

>>> 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...

Is there a good reason not to just sched_pin() or critical_enter() 
around those? This doesn't work for curthread, of course, but for the 
rest it should.
-Nathan



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E04E47B.2030007>