From owner-svn-src-all@FreeBSD.ORG Fri Jun 24 19:24:45 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (unknown [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 655111065670; Fri, 24 Jun 2011 19:24:45 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from mail.icecube.wisc.edu (trout.icecube.wisc.edu [128.104.255.119]) by mx1.freebsd.org (Postfix) with ESMTP id 24E5E8FC12; Fri, 24 Jun 2011 19:24:44 +0000 (UTC) Received: from localhost (localhost.localdomain [127.0.0.1]) by mail.icecube.wisc.edu (Postfix) with ESMTP id 59269582C0; Fri, 24 Jun 2011 14:24:44 -0500 (CDT) X-Virus-Scanned: amavisd-new at icecube.wisc.edu Received: from mail.icecube.wisc.edu ([127.0.0.1]) by localhost (trout.icecube.wisc.edu [127.0.0.1]) (amavisd-new, port 10030) with ESMTP id 3CbTTET+xgwV; Fri, 24 Jun 2011 14:24:44 -0500 (CDT) Received: from wanderer.tachypleus.net (i3-dhcp-172-16-223-119.icecube.wisc.edu [172.16.223.119]) by mail.icecube.wisc.edu (Postfix) with ESMTP id 1D21B58143; Fri, 24 Jun 2011 14:24:44 -0500 (CDT) Message-ID: <4E04E47B.2030007@freebsd.org> Date: Fri, 24 Jun 2011 14:24:43 -0500 From: Nathan Whitehorn User-Agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.9.2.18) Gecko/20110624 Thunderbird/3.1.11 MIME-Version: 1.0 To: Marcel Moolenaar References: <201106232221.p5NMLSFj019042@svn.freebsd.org> <1B2D07F9-3716-4581-8426-11BE78CE1D1F@xcllnt.net> <4E04B8DC.1020600@freebsd.org> <13E5FED8-30BE-4292-B024-AB692E9E2A89@xcllnt.net> In-Reply-To: <13E5FED8-30BE-4292-B024-AB692E9E2A89@xcllnt.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jun 2011 19:24:45 -0000 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