From owner-freebsd-current@FreeBSD.ORG Mon Feb 25 12:48:11 2013 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 670D2694; Mon, 25 Feb 2013 12:48:11 +0000 (UTC) (envelope-from cognet@ci0.org) Received: from kanar.ci0.org (unknown [IPv6:2a01:e0b:1:150:ca0a:a9ff:fef1:a4c9]) by mx1.freebsd.org (Postfix) with ESMTP id E97F7848; Mon, 25 Feb 2013 12:48:10 +0000 (UTC) Received: from kanar.ci0.org (pluxor@localhost [127.0.0.1]) by kanar.ci0.org (8.14.5/8.14.5) with ESMTP id r1PCllf5021109; Mon, 25 Feb 2013 13:47:47 +0100 (CET) (envelope-from cognet@ci0.org) Received: (from doginou@localhost) by kanar.ci0.org (8.14.5/8.14.5/Submit) id r1PCllAx021108; Mon, 25 Feb 2013 13:47:47 +0100 (CET) (envelope-from cognet@ci0.org) X-Authentication-Warning: kanar.ci0.org: doginou set sender to cognet@ci0.org using -f Date: Mon, 25 Feb 2013 13:47:47 +0100 From: Olivier Houchard To: Andrew Turner Subject: Re: [patch] i386 pmap sysmaps_pcpu[] atomic access Message-ID: <20130225124747.GA21027@ci0.org> References: <201302201022.29294.jhb@freebsd.org> <201302211043.49906.jhb@freebsd.org> <20130225190920.1b7d348d@bender> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130225190920.1b7d348d@bender> User-Agent: Mutt/1.5.21 (2010-09-15) X-Mailman-Approved-At: Mon, 25 Feb 2013 12:50:38 +0000 Cc: Konstantin Belousov , freebsd-current@freebsd.org, Svatopluk Kraus X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Feb 2013 12:48:11 -0000 On Mon, Feb 25, 2013 at 07:09:20PM +1300, Andrew Turner wrote: > On Thu, 21 Feb 2013 10:43:49 -0500 > John Baldwin wrote: > > > On Thursday, February 21, 2013 7:53:52 am Svatopluk Kraus wrote: > > > On Wed, Feb 20, 2013 at 4:22 PM, John Baldwin > > > wrote: > > > > On Wednesday, February 20, 2013 7:31:08 am Svatopluk Kraus wrote: > > > >> On Tue, Feb 19, 2013 at 7:51 PM, Konstantin Belousov > > > >> wrote: > > > >> > On Mon, Feb 18, 2013 at 11:18:16PM +0100, Svatopluk Kraus > > > >> > wrote: > > > >> >> On Mon, Feb 18, 2013 at 9:36 PM, Konstantin Belousov > > > >> >> wrote: > > > >> >> Well, I'm taking a part on porting FreeBSD to ARM11mpcore. UP > > > >> >> case was simple. SMP case is more complex and rather new for > > > >> >> me. Recently, I was solving a problem with PCPU stuff. For > > > >> >> example, PCPU_GET is implemented by one instruction on i386 > > > >> >> arch. So, a need of atomicity with respect to interrupts can > > > >> >> be overlooked. On load-store archs, the implementation which > > > >> >> works in SMP case is not so simple. And what works in UP case > > > >> >> (single PCPU), not works in SMP case. Believe me, mysterious > > > >> >> and sporadic 'mutex not owned' assertions and others ones > > > >> >> caused by curthreads mess, it takes a while ... > > > >> > Note that PCPU_GET() is not needed to be atomic. The reason is > > > >> > that the code which uses its result would not be atomic > > > >> > (single-instruction or whatever, see below). Thus, either the > > > >> > preemption should not matter since the action with the per-cpu > > > >> > data is advisory, as is in the case of i386 pmap you noted. or > > > >> > some external measures should be applied in advance to the > > > >> > containing region (which you proposed, by bracing with > > > >> > sched_pin()). > > > >> > > > >> So, it's advisory in the case of i386 pmap... Well, if you can > > > >> live with that, I can too. > > > >> > > > >> > > > > >> > Also, note that it is not interrupts but preemption which is > > > >> > concern. > > > >> > > > >> Yes and no. In theory, yes, a preemption is a concern. In > > > >> FreeBSD, however, sched_pin() and critical_enter() and their > > > >> counterparts are implemented with help of curthread. And > > > >> curthread definition falls to PCPU_GET(curthread) if not defined > > > >> in other way. So, curthread should be atomic with respect to > > > >> interrupts and in general, PCPU_GET() should be too. Note that > > > >> spinlock_enter() definitions often (always) use curthread too. > > > >> Anyhow, it's defined in MD code and can be defined for each arch > > > >> separately. > > > > > > > > curthread is a bit magic. :) If you perform a context switch > > > > during an interrupt (which will change 'curthread') you also > > > > change your register state. When you resume, the register state > > > > is also restored. This means that while something like > > > > 'PCPU_GET(cpuid)' might be stale after you read it, 'curthread' > > > > never is. However, it is true that actually reading curthread > > > > has to be atomic. If your read of curthread looks like: > > > > > > > > mov , r0 > > > > add , r0 > > > > ld r0, r1 > > > > > > > > Then that will indeed break. Alpha used a fixed register for > > > > 'pcpu_reg' (as does ia64 IIRC). OTOH, you might also be able to > > > > depend on the fact that pc_curthread is the first thing in > > > > 'struct pcpu' (and always will be, you could add a CTASSERT to > > > > future-proof). In that case, you can remove the 'add' > > > > instruction and instead just do: > > > > > > > > ld , r1 > > > > > > > > which is fine. > > > > > > Just for the record. There are three extra (coprocessor) registers > > > per a core in arm11mpcore (armv6k). Unfortunately only one is > > > Privileged. The other ones are respectively User Read only and User > > > Read Write. For now, we are using the Privileged one to store pcpu > > > pointer (curthread is correctly set during each context switch). > > > Thus, using a coprocessor register means that reading of curthread > > > is not a single instruction implementation now. After we figured > > > out the curthread issue in SMP case, using a fixed (processor) > > > register for pcpu is an option. Meanwhile, we disable interrupts > > > before reading of curthread and enable them after. The same is done > > > for other PCPU stuff. For now we have not stable system enough for > > > profiling, however, when it will be, it would be interesting to > > > learn how various implementations of curthread reading impact > > > system performance. > > > > curthread is read _a lot_, so I would expect this to hurt. What we > > did on Alpha was to use a fixed register for pcpu access, but we used > > the equivalent of a coprocessor register to also store the value so > > we could set that fixed register on entry to the kernel (userland was > > free to use the register for its own purposes). > > > > The current code on ARM is not atomic, it loads the base address of the > pcpu data from the coprocessor then loads curthread from this address. > One solution I discussed with Olivier Houchard is to keep the data in > the coprocessor but to then load it into a dedicated register when we > enter the kernel. > > Reading curthread would then be a single load instruction thanks to > ARM's ldr using an immediate offset [1], e.g. to load from r12 into r1 > it would be 'ldr r1, [r12]'. > Wouldn't it be doable to either use the extra coprocessor register for curthread, setting it when entering the kernel as John suggested, or just using the privileged one for curthread, and changing get_pcpu() to be __pcpu[cpuid], and then making sure pcpu accesses are atomic, either by disabling interrupts or using ldrex/strex ? I take it curthread (and curpcb, which we can get from curthread) is by far the most used, and it wouldn't hurt that badly to pessimize the other pcpu variables ? Olivier