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
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "[email protected]"
