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>