Re: [patch] i386 pmap sysmaps_pcpu[] atomic access

On Wednesday, February 20, 2013 7:31:08 am Svatopluk Kraus wrote:
> On Tue, Feb 19, 2013 at 7:51 PM, Konstantin Belousov
> <kostikbel_at_gmail.com> 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
> >> <kostikbel_at_gmail.com> 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 <pcpu reg>, r0
	add <offset of pc_curthread>, 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 <pcpu reg>, r1

which is fine.

-- 
John Baldwin