Fix a number of mp_lock issues. I had outsmarted myself trying to deal with
authorMatthew Dillon <dillon@dragonflybsd.org>
Thu, 25 Sep 2003 23:49:09 +0000 (23:49 +0000)
committerMatthew Dillon <dillon@dragonflybsd.org>
Thu, 25 Sep 2003 23:49:09 +0000 (23:49 +0000)
td->td_mpcount / mp_lock races.  The new rule is: you first modify
td->td_mpcount, then you deal with mp_lock assuming that an interrupt might
have already dealt with it for you, and various other pieces of code
deal with the race if an interrupt occurs in the middle of the above two
data accesses.

14 files changed:
sys/i386/apic/apic_vector.s
sys/i386/i386/mp_machdep.c
sys/i386/i386/mplock.s
sys/i386/i386/swtch.s
sys/i386/isa/apic_vector.s
sys/i386/isa/ipl.s
sys/kern/kern_intr.c
sys/kern/lwkt_thread.c
sys/platform/pc32/apic/apic_vector.s
sys/platform/pc32/i386/mp_machdep.c
sys/platform/pc32/i386/mplock.s
sys/platform/pc32/i386/swtch.s
sys/platform/pc32/isa/apic_vector.s
sys/platform/pc32/isa/ipl.s

index 8504156..306dbaa 100644 (file)
@@ -1,7 +1,7 @@
 /*
  *     from: vector.s, 386BSD 0.1 unknown origin
  * $FreeBSD: src/sys/i386/isa/apic_vector.s,v 1.47.2.5 2001/09/01 22:33:38 tegge Exp $
- * $DragonFly: src/sys/i386/apic/Attic/apic_vector.s,v 1.13 2003/08/25 19:50:32 dillon Exp $
+ * $DragonFly: src/sys/i386/apic/Attic/apic_vector.s,v 1.14 2003/09/25 23:49:08 dillon Exp $
  */
 
 
  *     - Push the trap frame required by doreti
  *     - Mask the interrupt and reenable its source
  *     - If we cannot take the interrupt set its fpending bit and
- *       doreti.
+ *       doreti.  Note that we cannot mess with mp_lock at all
+ *       if we entered from a critical section!
  *     - If we can take the interrupt clear its fpending bit,
  *       call the handler, then unmask and doreti.
  *
@@ -242,7 +243,8 @@ IDTVEC(vec_name) ;                                                  \
  *     - If we cannot take the interrupt set its ipending bit and
  *       doreti.  In addition to checking for a critical section
  *       and cpl mask we also check to see if the thread is still
- *       running.
+ *       running.  Note that we cannot mess with mp_lock at all
+ *       if we entered from a critical section!
  *     - If we can take the interrupt clear its ipending bit
  *       and schedule the thread.  Leave interrupts masked and doreti.
  *
index 1bb66ae..7749961 100644 (file)
@@ -23,7 +23,7 @@
  * SUCH DAMAGE.
  *
  * $FreeBSD: src/sys/i386/i386/mp_machdep.c,v 1.115.2.15 2003/03/14 21:22:35 jhb Exp $
- * $DragonFly: src/sys/i386/i386/Attic/mp_machdep.c,v 1.16 2003/08/27 01:43:07 dillon Exp $
+ * $DragonFly: src/sys/i386/i386/Attic/mp_machdep.c,v 1.17 2003/09/25 23:49:03 dillon Exp $
  */
 
 #include "opt_cpu.h"
@@ -2410,11 +2410,12 @@ ap_init(void)
 
        /*
         * Get the MP lock so we can finish initializing.  Note: we are
-        * in a critical section.
+        * in a critical section.  td_mpcount must always be bumped prior
+        * to obtaining the actual lock.
         */
+       ++curthread->td_mpcount;
        while (cpu_try_mplock() == 0)
            ;
-       ++curthread->td_mpcount;
 
        /* BSP may have changed PTD while we're waiting for the lock */
        cpu_invltlb();
index c87c588..32cda05 100644 (file)
@@ -1,24 +1,57 @@
 /*
+ * $FreeBSD: src/sys/i386/i386/mplock.s,v 1.29.2.2 2000/05/16 06:58:06 dillon Exp $
+ * $DragonFly: src/sys/i386/i386/Attic/mplock.s,v 1.10 2003/09/25 23:49:03 dillon Exp $
+ *
+ * Copyright (c) 2003 Matthew Dillon <dillon@backplane.com>
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ *
+ *                             DragonFly MPLOCK operation
+ *
+ * Each thread as an MP lock count, td_mpcount, and there is a shared
+ * global called mp_lock.  mp_lock is the physical MP lock and contains either
+ * -1 or the cpuid of the cpu owning the lock.  The count is *NOT* integrated
+ * into mp_lock but instead resides in each thread td_mpcount.
+ *
+ * When obtaining or releasing the MP lock the td_mpcount is PREDISPOSED
+ * to the desired count *PRIOR* to operating on the mp_lock itself.  MP
+ * lock operations can occur outside a critical section with interrupts
+ * enabled with the provisio (which the routines below handle) that an
+ * interrupt may come along and preempt us, racing our cmpxchgl instruction
+ * to perform the operation we have requested by pre-dispoing td_mpcount.
+ *
+ * Additionally, the LWKT threading system manages the MP lock and
+ * lwkt_switch(), in particular, may be called after pre-dispoing td_mpcount
+ * to handle 'blocking' on the MP lock.
+ *
+ *
+ * Recoded from the FreeBSD original:
  * ----------------------------------------------------------------------------
  * "THE BEER-WARE LICENSE" (Revision 42):
  * <phk@FreeBSD.org> wrote this file.  As long as you retain this notice you
  * can do whatever you want with this stuff. If we meet some day, and you think
  * this stuff is worth it, you can buy me a beer in return.   Poul-Henning Kamp
  * ----------------------------------------------------------------------------
- *
- * $FreeBSD: src/sys/i386/i386/mplock.s,v 1.29.2.2 2000/05/16 06:58:06 dillon Exp $
- * $DragonFly: src/sys/i386/i386/Attic/mplock.s,v 1.9 2003/07/20 07:46:19 dillon Exp $
- *
- * Functions for locking between CPUs in a SMP system.
- *
- * This is an "exclusive counting semaphore".  This means that it can be
- * free (0xffffffff) or be owned by a CPU (0xXXYYYYYY where XX is CPU-id
- * and YYYYYY is the count).
- *
- * Contrary to most implementations around, this one is entirely atomic:
- * The attempt to seize/release the semaphore and the increment/decrement
- * is done in one atomic operation.  This way we are safe from all kinds
- * of weird reentrancy situations.
  */
 
 #include <machine/asmacros.h>
@@ -56,7 +89,15 @@ NON_GPROF_ENTRY(cpu_get_initial_mplock)
 
        /*
         * cpu_try_mplock() returns non-zero on success, 0 on failure.  It
-        * only adjusts mp_lock.  It does not touch td_mpcount.
+        * only adjusts mp_lock, it does not touch td_mpcount.  Callers
+        * should always increment td_mpcount *before* trying to acquire
+        * the actual lock, predisposing td_mpcount to the desired state of
+        * the lock.
+        *
+        * NOTE! Only call cpu_try_mplock() inside a critical section.  If
+        * you don't an interrupt can come along and get and release
+        * the lock before our cmpxchgl instruction, causing us to fail 
+        * but resulting in the lock being held by our cpu.
         */
 NON_GPROF_ENTRY(cpu_try_mplock)
        movl    PCPU(cpuid),%ecx
@@ -74,38 +115,45 @@ NON_GPROF_ENTRY(cpu_try_mplock)
 
        /*
         * get_mplock() Obtains the MP lock and may switch away if it cannot
-        * get it.  Note that td_mpcount may not be synchronized with the
-        * actual state of the MP lock.  This situation occurs when 
-        * get_mplock() or try_mplock() is indirectly called from the
-        * lwkt_switch() code, or from a preemption (though, truthfully,
-        * only try_mplock() should ever be called in this fashion).  If
-        * we cannot get the MP lock we pre-dispose TD_MPCOUNT and call
-        * lwkt_swich().  The MP lock will be held on return.
+        * get it.  This routine may be called WITHOUT a critical section
+        * and with cpu interrupts enabled.
         *
-        * Note that both get_mplock() and try_mplock() must pre-dispose
-        * mpcount before attempting to get the lock, in case we get
-        * preempted.  This allows us to avoid expensive interrupt
-        * disablement instructions and allows us to be called from outside
-        * a critical section.
+        * To handle races in a sane fashion we predispose TD_MPCOUNT,
+        * which prevents us from losing the lock in a race if we already
+        * have it or happen to get it.  It also means that we might get
+        * the lock in an interrupt race before we have a chance to execute
+        * our cmpxchgl instruction, so we have to handle that case.
+        * Fortunately simply calling lwkt_switch() handles the situation
+        * for us and also 'blocks' us until the MP lock can be obtained.
         */
 NON_GPROF_ENTRY(get_mplock)
        movl    PCPU(cpuid),%ecx
        movl    PCPU(curthread),%edx
+       incl    TD_MPCOUNT(%edx)        /* predispose */
        cmpl    %ecx,mp_lock
        jne     1f
-       incl    TD_MPCOUNT(%edx)
-       NON_GPROF_RET
+       NON_GPROF_RET                   /* success! */
+
+       /*
+        * We don't already own the mp_lock, use cmpxchgl to try to get
+        * it.
+        */
 1:
-       incl    TD_MPCOUNT(%edx)
        movl    $-1,%eax
        lock cmpxchgl %ecx,mp_lock
        jnz     2f
 #ifdef PARANOID_INVLTLB
-       movl    %cr3,%eax; movl %eax,%cr3       /* YYY check and remove */
+       movl    %cr3,%eax; movl %eax,%cr3 /* YYY check and remove */
 #endif
-       NON_GPROF_RET
+       NON_GPROF_RET                   /* success */
+
+       /*
+        * Failure, but we could end up owning mp_lock anyway due to
+        * an interrupt race.  lwkt_switch() will clean up the mess
+        * and 'block' until the mp_lock is obtained.
+        */
 2:
-       call    lwkt_switch             /* will be correct on return */
+       call    lwkt_switch
 #ifdef INVARIANTS
        movl    PCPU(cpuid),%eax        /* failure */
        cmpl    %eax,mp_lock
@@ -120,59 +168,74 @@ NON_GPROF_ENTRY(get_mplock)
 #endif
 
        /*
-        * try_mplock() attempts to obtain the MP lock and will not switch
-        * away if it cannot get it.  Note that td_mpcoutn may not be 
-        * synchronized with the actual state of the MP lock.
+        * try_mplock() attempts to obtain the MP lock.  1 is returned on
+        * success, 0 on failure.  We do not have to be in a critical section
+        * and interrupts are almost certainly enabled.
+        *
+        * We must pre-dispose TD_MPCOUNT in order to deal with races in
+        * a reasonable way.
+        *
         */
 NON_GPROF_ENTRY(try_mplock)
        movl    PCPU(cpuid),%ecx
        movl    PCPU(curthread),%edx
+       incl    TD_MPCOUNT(%edx)                /* pre-dispose for race */
        cmpl    %ecx,mp_lock
-       jne     1f
-       incl    TD_MPCOUNT(%edx)
-       movl    $1,%eax
-       NON_GPROF_RET
-1:
-       incl    TD_MPCOUNT(%edx)        /* pre-dispose */
+       je      1f                              /* trivial success */
        movl    $-1,%eax
        lock cmpxchgl %ecx,mp_lock
        jnz     2f
+       /*
+        * Success
+        */
 #ifdef PARANOID_INVLTLB
        movl    %cr3,%eax; movl %eax,%cr3       /* YYY check and remove */
 #endif
-       movl    $1,%eax
+1:
+       movl    $1,%eax                         /* success (cmpxchgl good!) */
        NON_GPROF_RET
+
+       /*
+        * The cmpxchgl failed but we might have raced.  Undo the mess by
+        * predispoing TD_MPCOUNT and then checking.  If TD_MPCOUNT is
+        * still non-zero we don't care what state the lock is in (since
+        * we obviously didn't own it above), just return failure even if
+        * we won the lock in an interrupt race.  If TD_MPCOUNT is zero
+        * make sure we don't own the lock in case we did win it in a race.
+        */
 2:
-       decl    TD_MPCOUNT(%edx)        /* un-dispose */
+       decl    TD_MPCOUNT(%edx)
+       cmpl    $0,TD_MPCOUNT(%edx)
+       jne     3f
+       movl    PCPU(cpuid),%eax
+       movl    $-1,%ecx
+       lock cmpxchgl %ecx,mp_lock
+3:
        subl    %eax,%eax
        NON_GPROF_RET
-
+       
        /*
-        * rel_mplock() release the MP lock.  The MP lock MUST be held,
-        * td_mpcount must NOT be out of synch with the lock.  It is allowed
-        * for the physical lock to be released prior to setting the count
-        * to 0, preemptions will deal with the case (see lwkt_thread.c).
+        * rel_mplock() releases a previously obtained MP lock.
+        *
+        * In order to release the MP lock we pre-dispose TD_MPCOUNT for
+        * the release and basically repeat the release portion of try_mplock
+        * above.
         */
 NON_GPROF_ENTRY(rel_mplock)
        movl    PCPU(curthread),%edx
        movl    TD_MPCOUNT(%edx),%eax
-       cmpl    $1,%eax
-       je      1f
 #ifdef INVARIANTS
-       testl   %eax,%eax
-       jz      badmp_rel
+       cmpl    $0,%eax
+       je      badmp_rel
 #endif
        subl    $1,%eax
        movl    %eax,TD_MPCOUNT(%edx)
-       NON_GPROF_RET
-1:
-#ifdef INVARIANTS
-       movl    PCPU(cpuid),%ecx
-       cmpl    %ecx,mp_lock
-       jne     badmp_rel2
-#endif
-       movl    $MP_FREE_LOCK,mp_lock
-       movl    $0,TD_MPCOUNT(%edx)
+       cmpl    $0,%eax
+       jne     3f
+       movl    PCPU(cpuid),%eax
+       movl    $-1,%ecx
+       lock cmpxchgl %ecx,mp_lock
+3:
        NON_GPROF_RET
 
 #ifdef INVARIANTS
@@ -186,9 +249,6 @@ badmp_get2:
 badmp_rel:
        pushl   $bmpsw2
        call    panic
-badmp_rel2:
-       pushl   $bmpsw2a
-       call    panic
 
        .data
 
@@ -201,9 +261,6 @@ bmpsw1a:
 bmpsw2:
        .asciz  "rel_mplock(): mpcount already 0 @ %p %p %p %p %p %p %p %p!"
 
-bmpsw2a:
-       .asciz  "rel_mplock(): Releasing another cpu's MP lock! %p %p"
-
 #endif
 
 #if 0
index 2fee833..d38c8d0 100644 (file)
@@ -35,7 +35,7 @@
  * SUCH DAMAGE.
  *
  * $FreeBSD: src/sys/i386/i386/swtch.s,v 1.89.2.10 2003/01/23 03:36:24 ps Exp $
- * $DragonFly: src/sys/i386/i386/Attic/swtch.s,v 1.27 2003/09/16 20:03:35 dillon Exp $
+ * $DragonFly: src/sys/i386/i386/Attic/swtch.s,v 1.28 2003/09/25 23:49:03 dillon Exp $
  */
 
 #include "use_npx.h"
@@ -162,6 +162,10 @@ ENTRY(cpu_heavy_switch)
         *
         * The switch restore function expects the new thread to be in %eax
         * and the old one to be in %ebx.
+        *
+        * There is a one-instruction window where curthread is the new
+        * thread but %esp still points to the old thread's stack, but
+        * we are protected by a critical section so it is ok.
         */
        movl    12(%esp),%eax           /* EAX = newtd, EBX = oldtd */
        movl    %eax,PCPU(curthread)
@@ -193,6 +197,10 @@ ENTRY(cpu_exit_switch)
        /*
         * Switch to the next thread.  RET into the restore function, which
         * expects the new thread in EAX and the old in EBX.
+        *
+        * There is a one-instruction window where curthread is the new
+        * thread but %esp still points to the old thread's stack, but
+        * we are protected by a critical section so it is ok.
         */
        movl    4(%esp),%eax
        movl    %eax,PCPU(curthread)
@@ -521,6 +529,10 @@ ENTRY(cpu_kthread_restore)
  *
  *     This function is always called while in a critical section.
  *
+ *     There is a one-instruction window where curthread is the new
+ *     thread but %esp still points to the old thread's stack, but
+ *     we are protected by a critical section so it is ok.
+ *
  *     YYY BGL, SPL
  */
 ENTRY(cpu_lwkt_switch)
index 7e511bb..d5f1108 100644 (file)
@@ -1,7 +1,7 @@
 /*
  *     from: vector.s, 386BSD 0.1 unknown origin
  * $FreeBSD: src/sys/i386/isa/apic_vector.s,v 1.47.2.5 2001/09/01 22:33:38 tegge Exp $
- * $DragonFly: src/sys/i386/isa/Attic/apic_vector.s,v 1.13 2003/08/25 19:50:32 dillon Exp $
+ * $DragonFly: src/sys/i386/isa/Attic/apic_vector.s,v 1.14 2003/09/25 23:49:08 dillon Exp $
  */
 
 
  *     - Push the trap frame required by doreti
  *     - Mask the interrupt and reenable its source
  *     - If we cannot take the interrupt set its fpending bit and
- *       doreti.
+ *       doreti.  Note that we cannot mess with mp_lock at all
+ *       if we entered from a critical section!
  *     - If we can take the interrupt clear its fpending bit,
  *       call the handler, then unmask and doreti.
  *
@@ -242,7 +243,8 @@ IDTVEC(vec_name) ;                                                  \
  *     - If we cannot take the interrupt set its ipending bit and
  *       doreti.  In addition to checking for a critical section
  *       and cpl mask we also check to see if the thread is still
- *       running.
+ *       running.  Note that we cannot mess with mp_lock at all
+ *       if we entered from a critical section!
  *     - If we can take the interrupt clear its ipending bit
  *       and schedule the thread.  Leave interrupts masked and doreti.
  *
index 581d58e..2a60b39 100644 (file)
@@ -37,7 +37,7 @@
  *     @(#)ipl.s
  *
  * $FreeBSD: src/sys/i386/isa/ipl.s,v 1.32.2.3 2002/05/16 16:03:56 bde Exp $
- * $DragonFly: src/sys/i386/isa/Attic/ipl.s,v 1.12 2003/08/25 19:50:32 dillon Exp $
+ * $DragonFly: src/sys/i386/isa/Attic/ipl.s,v 1.13 2003/09/25 23:49:08 dillon Exp $
  */
 
 
@@ -77,6 +77,9 @@ softtty_imask:        .long   SWI_TTY_MASK
         * Handle return from interrupts, traps and syscalls.  This function
         * checks the cpl for unmasked pending interrupts (fast, normal, or
         * soft) and schedules them if appropriate, then irets.
+        *
+        * If we are in a critical section we cannot run any pending ints
+        * nor can be play with mp_lock.
         */
        SUPERALIGN_TEXT
        .type   doreti,@function
index 46e630c..fb376ef 100644 (file)
@@ -24,7 +24,7 @@
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
  * $FreeBSD: src/sys/kern/kern_intr.c,v 1.24.2.1 2001/10/14 20:05:50 luigi Exp $
- * $DragonFly: src/sys/kern/kern_intr.c,v 1.11 2003/08/25 19:50:32 dillon Exp $
+ * $DragonFly: src/sys/kern/kern_intr.c,v 1.12 2003/09/25 23:49:09 dillon Exp $
  *
  */
 
@@ -184,7 +184,7 @@ unregister_randintr(int intr)
  * with the interrupt thread's critical section).
  *
  * We are NOT in a critical section, which will allow the scheduled
- * interrupt to preempt us.
+ * interrupt to preempt us.  The MP lock might *NOT* be held here.
  */
 static void
 sched_ithd_remote(void *arg)
index 0a80813..9780d3c 100644 (file)
@@ -28,7 +28,7 @@
  *     to use a critical section to avoid problems.  Foreign thread 
  *     scheduling is queued via (async) IPIs.
  *
- * $DragonFly: src/sys/kern/lwkt_thread.c,v 1.30 2003/09/24 18:37:48 dillon Exp $
+ * $DragonFly: src/sys/kern/lwkt_thread.c,v 1.31 2003/09/25 23:49:09 dillon Exp $
  */
 
 #include <sys/param.h>
@@ -287,7 +287,9 @@ lwkt_free_thread(thread_t td)
  * The MP lock may be out of sync with the thread's td_mpcount.  lwkt_switch()
  * cleans it up.  Note that the td_switch() function cannot do anything that
  * requires the MP lock since the MP lock will have already been setup for
- * the target thread (not the current thread).
+ * the target thread (not the current thread).  It's nice to have a scheduler
+ * that does not need the MP lock to work because it allows us to do some
+ * really cool high-performance MP lock optimizations.
  */
 
 void
@@ -325,7 +327,10 @@ lwkt_switch(void)
     /*
      * td_mpcount cannot be used to determine if we currently hold the
      * MP lock because get_mplock() will increment it prior to attempting
-     * to get the lock, and switch out if it can't.  Look at the actual lock.
+     * to get the lock, and switch out if it can't.  Our ownership of 
+     * the actual lock will remain stable while we are in a critical section
+     * (but, of course, another cpu may own or release the lock so the
+     * actual value of mp_lock is not stable).
      */
     mpheld = MP_LOCK_HELD();
 #endif
@@ -482,8 +487,9 @@ lwkt_maybe_switch()
  * duration of the preemption in order to preserve the atomicy of the
  * MP lock during the preemption.  Therefore, any preempting targets must be
  * careful in regards to MP assertions.  Note that the MP count may be
- * out of sync with the physical mp_lock.  If we preempt we have to preserve
- * the expected situation.
+ * out of sync with the physical mp_lock, but we do not have to preserve
+ * the original ownership of the lock if it was out of synch (that is, we
+ * can leave it synchronized on return).
  */
 void
 lwkt_preempt(thread_t ntd, int critpri)
@@ -531,15 +537,14 @@ lwkt_preempt(thread_t ntd, int critpri)
 #ifdef SMP
     /*
      * note: an interrupt might have occured just as we were transitioning
-     * to the MP lock.  In this case td_mpcount will be pre-disposed but
-     * not actually synchronized with the actual state of the lock.  We
-     * can use it to imply an MP lock requirement for the preemption but
-     * we cannot use it to test whether we hold the MP lock or not.
+     * to or from the MP lock.  In this case td_mpcount will be pre-disposed
+     * (non-zero) but not actually synchronized with the actual state of the
+     * lock.  We can use it to imply an MP lock requirement for the
+     * preemption but we cannot use it to test whether we hold the MP lock
+     * or not.
      */
-    mpheld = MP_LOCK_HELD();
-    if (mpheld && td->td_mpcount == 0)
-       panic("lwkt_preempt(): held and no count");
     savecnt = td->td_mpcount;
+    mpheld = MP_LOCK_HELD();
     ntd->td_mpcount += td->td_mpcount;
     if (mpheld == 0 && ntd->td_mpcount && !cpu_try_mplock()) {
        ntd->td_mpcount -= td->td_mpcount;
@@ -555,9 +560,10 @@ lwkt_preempt(thread_t ntd, int critpri)
     KKASSERT(ntd->td_preempted && (td->td_flags & TDF_PREEMPT_DONE));
 #ifdef SMP
     KKASSERT(savecnt == td->td_mpcount);
-    if (mpheld == 0 && MP_LOCK_HELD())
+    mpheld = MP_LOCK_HELD();
+    if (mpheld && td->td_mpcount == 0)
        cpu_rel_mplock();
-    else if (mpheld && !MP_LOCK_HELD())
+    else if (mpheld == 0 && td->td_mpcount)
        panic("lwkt_preempt(): MP lock was not held through");
 #endif
     ntd->td_preempted = NULL;
index 7e1c48e..5360943 100644 (file)
@@ -1,7 +1,7 @@
 /*
  *     from: vector.s, 386BSD 0.1 unknown origin
  * $FreeBSD: src/sys/i386/isa/apic_vector.s,v 1.47.2.5 2001/09/01 22:33:38 tegge Exp $
- * $DragonFly: src/sys/platform/pc32/apic/apic_vector.s,v 1.13 2003/08/25 19:50:32 dillon Exp $
+ * $DragonFly: src/sys/platform/pc32/apic/apic_vector.s,v 1.14 2003/09/25 23:49:08 dillon Exp $
  */
 
 
  *     - Push the trap frame required by doreti
  *     - Mask the interrupt and reenable its source
  *     - If we cannot take the interrupt set its fpending bit and
- *       doreti.
+ *       doreti.  Note that we cannot mess with mp_lock at all
+ *       if we entered from a critical section!
  *     - If we can take the interrupt clear its fpending bit,
  *       call the handler, then unmask and doreti.
  *
@@ -242,7 +243,8 @@ IDTVEC(vec_name) ;                                                  \
  *     - If we cannot take the interrupt set its ipending bit and
  *       doreti.  In addition to checking for a critical section
  *       and cpl mask we also check to see if the thread is still
- *       running.
+ *       running.  Note that we cannot mess with mp_lock at all
+ *       if we entered from a critical section!
  *     - If we can take the interrupt clear its ipending bit
  *       and schedule the thread.  Leave interrupts masked and doreti.
  *
index d1dc553..efcf7b2 100644 (file)
@@ -23,7 +23,7 @@
  * SUCH DAMAGE.
  *
  * $FreeBSD: src/sys/i386/i386/mp_machdep.c,v 1.115.2.15 2003/03/14 21:22:35 jhb Exp $
- * $DragonFly: src/sys/platform/pc32/i386/mp_machdep.c,v 1.16 2003/08/27 01:43:07 dillon Exp $
+ * $DragonFly: src/sys/platform/pc32/i386/mp_machdep.c,v 1.17 2003/09/25 23:49:03 dillon Exp $
  */
 
 #include "opt_cpu.h"
@@ -2410,11 +2410,12 @@ ap_init(void)
 
        /*
         * Get the MP lock so we can finish initializing.  Note: we are
-        * in a critical section.
+        * in a critical section.  td_mpcount must always be bumped prior
+        * to obtaining the actual lock.
         */
+       ++curthread->td_mpcount;
        while (cpu_try_mplock() == 0)
            ;
-       ++curthread->td_mpcount;
 
        /* BSP may have changed PTD while we're waiting for the lock */
        cpu_invltlb();
index ac0db48..fed9380 100644 (file)
@@ -1,24 +1,57 @@
 /*
+ * $FreeBSD: src/sys/i386/i386/mplock.s,v 1.29.2.2 2000/05/16 06:58:06 dillon Exp $
+ * $DragonFly: src/sys/platform/pc32/i386/mplock.s,v 1.10 2003/09/25 23:49:03 dillon Exp $
+ *
+ * Copyright (c) 2003 Matthew Dillon <dillon@backplane.com>
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ *
+ *                             DragonFly MPLOCK operation
+ *
+ * Each thread as an MP lock count, td_mpcount, and there is a shared
+ * global called mp_lock.  mp_lock is the physical MP lock and contains either
+ * -1 or the cpuid of the cpu owning the lock.  The count is *NOT* integrated
+ * into mp_lock but instead resides in each thread td_mpcount.
+ *
+ * When obtaining or releasing the MP lock the td_mpcount is PREDISPOSED
+ * to the desired count *PRIOR* to operating on the mp_lock itself.  MP
+ * lock operations can occur outside a critical section with interrupts
+ * enabled with the provisio (which the routines below handle) that an
+ * interrupt may come along and preempt us, racing our cmpxchgl instruction
+ * to perform the operation we have requested by pre-dispoing td_mpcount.
+ *
+ * Additionally, the LWKT threading system manages the MP lock and
+ * lwkt_switch(), in particular, may be called after pre-dispoing td_mpcount
+ * to handle 'blocking' on the MP lock.
+ *
+ *
+ * Recoded from the FreeBSD original:
  * ----------------------------------------------------------------------------
  * "THE BEER-WARE LICENSE" (Revision 42):
  * <phk@FreeBSD.org> wrote this file.  As long as you retain this notice you
  * can do whatever you want with this stuff. If we meet some day, and you think
  * this stuff is worth it, you can buy me a beer in return.   Poul-Henning Kamp
  * ----------------------------------------------------------------------------
- *
- * $FreeBSD: src/sys/i386/i386/mplock.s,v 1.29.2.2 2000/05/16 06:58:06 dillon Exp $
- * $DragonFly: src/sys/platform/pc32/i386/mplock.s,v 1.9 2003/07/20 07:46:19 dillon Exp $
- *
- * Functions for locking between CPUs in a SMP system.
- *
- * This is an "exclusive counting semaphore".  This means that it can be
- * free (0xffffffff) or be owned by a CPU (0xXXYYYYYY where XX is CPU-id
- * and YYYYYY is the count).
- *
- * Contrary to most implementations around, this one is entirely atomic:
- * The attempt to seize/release the semaphore and the increment/decrement
- * is done in one atomic operation.  This way we are safe from all kinds
- * of weird reentrancy situations.
  */
 
 #include <machine/asmacros.h>
@@ -56,7 +89,15 @@ NON_GPROF_ENTRY(cpu_get_initial_mplock)
 
        /*
         * cpu_try_mplock() returns non-zero on success, 0 on failure.  It
-        * only adjusts mp_lock.  It does not touch td_mpcount.
+        * only adjusts mp_lock, it does not touch td_mpcount.  Callers
+        * should always increment td_mpcount *before* trying to acquire
+        * the actual lock, predisposing td_mpcount to the desired state of
+        * the lock.
+        *
+        * NOTE! Only call cpu_try_mplock() inside a critical section.  If
+        * you don't an interrupt can come along and get and release
+        * the lock before our cmpxchgl instruction, causing us to fail 
+        * but resulting in the lock being held by our cpu.
         */
 NON_GPROF_ENTRY(cpu_try_mplock)
        movl    PCPU(cpuid),%ecx
@@ -74,38 +115,45 @@ NON_GPROF_ENTRY(cpu_try_mplock)
 
        /*
         * get_mplock() Obtains the MP lock and may switch away if it cannot
-        * get it.  Note that td_mpcount may not be synchronized with the
-        * actual state of the MP lock.  This situation occurs when 
-        * get_mplock() or try_mplock() is indirectly called from the
-        * lwkt_switch() code, or from a preemption (though, truthfully,
-        * only try_mplock() should ever be called in this fashion).  If
-        * we cannot get the MP lock we pre-dispose TD_MPCOUNT and call
-        * lwkt_swich().  The MP lock will be held on return.
+        * get it.  This routine may be called WITHOUT a critical section
+        * and with cpu interrupts enabled.
         *
-        * Note that both get_mplock() and try_mplock() must pre-dispose
-        * mpcount before attempting to get the lock, in case we get
-        * preempted.  This allows us to avoid expensive interrupt
-        * disablement instructions and allows us to be called from outside
-        * a critical section.
+        * To handle races in a sane fashion we predispose TD_MPCOUNT,
+        * which prevents us from losing the lock in a race if we already
+        * have it or happen to get it.  It also means that we might get
+        * the lock in an interrupt race before we have a chance to execute
+        * our cmpxchgl instruction, so we have to handle that case.
+        * Fortunately simply calling lwkt_switch() handles the situation
+        * for us and also 'blocks' us until the MP lock can be obtained.
         */
 NON_GPROF_ENTRY(get_mplock)
        movl    PCPU(cpuid),%ecx
        movl    PCPU(curthread),%edx
+       incl    TD_MPCOUNT(%edx)        /* predispose */
        cmpl    %ecx,mp_lock
        jne     1f
-       incl    TD_MPCOUNT(%edx)
-       NON_GPROF_RET
+       NON_GPROF_RET                   /* success! */
+
+       /*
+        * We don't already own the mp_lock, use cmpxchgl to try to get
+        * it.
+        */
 1:
-       incl    TD_MPCOUNT(%edx)
        movl    $-1,%eax
        lock cmpxchgl %ecx,mp_lock
        jnz     2f
 #ifdef PARANOID_INVLTLB
-       movl    %cr3,%eax; movl %eax,%cr3       /* YYY check and remove */
+       movl    %cr3,%eax; movl %eax,%cr3 /* YYY check and remove */
 #endif
-       NON_GPROF_RET
+       NON_GPROF_RET                   /* success */
+
+       /*
+        * Failure, but we could end up owning mp_lock anyway due to
+        * an interrupt race.  lwkt_switch() will clean up the mess
+        * and 'block' until the mp_lock is obtained.
+        */
 2:
-       call    lwkt_switch             /* will be correct on return */
+       call    lwkt_switch
 #ifdef INVARIANTS
        movl    PCPU(cpuid),%eax        /* failure */
        cmpl    %eax,mp_lock
@@ -120,59 +168,74 @@ NON_GPROF_ENTRY(get_mplock)
 #endif
 
        /*
-        * try_mplock() attempts to obtain the MP lock and will not switch
-        * away if it cannot get it.  Note that td_mpcoutn may not be 
-        * synchronized with the actual state of the MP lock.
+        * try_mplock() attempts to obtain the MP lock.  1 is returned on
+        * success, 0 on failure.  We do not have to be in a critical section
+        * and interrupts are almost certainly enabled.
+        *
+        * We must pre-dispose TD_MPCOUNT in order to deal with races in
+        * a reasonable way.
+        *
         */
 NON_GPROF_ENTRY(try_mplock)
        movl    PCPU(cpuid),%ecx
        movl    PCPU(curthread),%edx
+       incl    TD_MPCOUNT(%edx)                /* pre-dispose for race */
        cmpl    %ecx,mp_lock
-       jne     1f
-       incl    TD_MPCOUNT(%edx)
-       movl    $1,%eax
-       NON_GPROF_RET
-1:
-       incl    TD_MPCOUNT(%edx)        /* pre-dispose */
+       je      1f                              /* trivial success */
        movl    $-1,%eax
        lock cmpxchgl %ecx,mp_lock
        jnz     2f
+       /*
+        * Success
+        */
 #ifdef PARANOID_INVLTLB
        movl    %cr3,%eax; movl %eax,%cr3       /* YYY check and remove */
 #endif
-       movl    $1,%eax
+1:
+       movl    $1,%eax                         /* success (cmpxchgl good!) */
        NON_GPROF_RET
+
+       /*
+        * The cmpxchgl failed but we might have raced.  Undo the mess by
+        * predispoing TD_MPCOUNT and then checking.  If TD_MPCOUNT is
+        * still non-zero we don't care what state the lock is in (since
+        * we obviously didn't own it above), just return failure even if
+        * we won the lock in an interrupt race.  If TD_MPCOUNT is zero
+        * make sure we don't own the lock in case we did win it in a race.
+        */
 2:
-       decl    TD_MPCOUNT(%edx)        /* un-dispose */
+       decl    TD_MPCOUNT(%edx)
+       cmpl    $0,TD_MPCOUNT(%edx)
+       jne     3f
+       movl    PCPU(cpuid),%eax
+       movl    $-1,%ecx
+       lock cmpxchgl %ecx,mp_lock
+3:
        subl    %eax,%eax
        NON_GPROF_RET
-
+       
        /*
-        * rel_mplock() release the MP lock.  The MP lock MUST be held,
-        * td_mpcount must NOT be out of synch with the lock.  It is allowed
-        * for the physical lock to be released prior to setting the count
-        * to 0, preemptions will deal with the case (see lwkt_thread.c).
+        * rel_mplock() releases a previously obtained MP lock.
+        *
+        * In order to release the MP lock we pre-dispose TD_MPCOUNT for
+        * the release and basically repeat the release portion of try_mplock
+        * above.
         */
 NON_GPROF_ENTRY(rel_mplock)
        movl    PCPU(curthread),%edx
        movl    TD_MPCOUNT(%edx),%eax
-       cmpl    $1,%eax
-       je      1f
 #ifdef INVARIANTS
-       testl   %eax,%eax
-       jz      badmp_rel
+       cmpl    $0,%eax
+       je      badmp_rel
 #endif
        subl    $1,%eax
        movl    %eax,TD_MPCOUNT(%edx)
-       NON_GPROF_RET
-1:
-#ifdef INVARIANTS
-       movl    PCPU(cpuid),%ecx
-       cmpl    %ecx,mp_lock
-       jne     badmp_rel2
-#endif
-       movl    $MP_FREE_LOCK,mp_lock
-       movl    $0,TD_MPCOUNT(%edx)
+       cmpl    $0,%eax
+       jne     3f
+       movl    PCPU(cpuid),%eax
+       movl    $-1,%ecx
+       lock cmpxchgl %ecx,mp_lock
+3:
        NON_GPROF_RET
 
 #ifdef INVARIANTS
@@ -186,9 +249,6 @@ badmp_get2:
 badmp_rel:
        pushl   $bmpsw2
        call    panic
-badmp_rel2:
-       pushl   $bmpsw2a
-       call    panic
 
        .data
 
@@ -201,9 +261,6 @@ bmpsw1a:
 bmpsw2:
        .asciz  "rel_mplock(): mpcount already 0 @ %p %p %p %p %p %p %p %p!"
 
-bmpsw2a:
-       .asciz  "rel_mplock(): Releasing another cpu's MP lock! %p %p"
-
 #endif
 
 #if 0
index 97823e3..49167b4 100644 (file)
@@ -35,7 +35,7 @@
  * SUCH DAMAGE.
  *
  * $FreeBSD: src/sys/i386/i386/swtch.s,v 1.89.2.10 2003/01/23 03:36:24 ps Exp $
- * $DragonFly: src/sys/platform/pc32/i386/swtch.s,v 1.27 2003/09/16 20:03:35 dillon Exp $
+ * $DragonFly: src/sys/platform/pc32/i386/swtch.s,v 1.28 2003/09/25 23:49:03 dillon Exp $
  */
 
 #include "use_npx.h"
@@ -162,6 +162,10 @@ ENTRY(cpu_heavy_switch)
         *
         * The switch restore function expects the new thread to be in %eax
         * and the old one to be in %ebx.
+        *
+        * There is a one-instruction window where curthread is the new
+        * thread but %esp still points to the old thread's stack, but
+        * we are protected by a critical section so it is ok.
         */
        movl    12(%esp),%eax           /* EAX = newtd, EBX = oldtd */
        movl    %eax,PCPU(curthread)
@@ -193,6 +197,10 @@ ENTRY(cpu_exit_switch)
        /*
         * Switch to the next thread.  RET into the restore function, which
         * expects the new thread in EAX and the old in EBX.
+        *
+        * There is a one-instruction window where curthread is the new
+        * thread but %esp still points to the old thread's stack, but
+        * we are protected by a critical section so it is ok.
         */
        movl    4(%esp),%eax
        movl    %eax,PCPU(curthread)
@@ -521,6 +529,10 @@ ENTRY(cpu_kthread_restore)
  *
  *     This function is always called while in a critical section.
  *
+ *     There is a one-instruction window where curthread is the new
+ *     thread but %esp still points to the old thread's stack, but
+ *     we are protected by a critical section so it is ok.
+ *
  *     YYY BGL, SPL
  */
 ENTRY(cpu_lwkt_switch)
index a3cd828..edd9c64 100644 (file)
@@ -1,7 +1,7 @@
 /*
  *     from: vector.s, 386BSD 0.1 unknown origin
  * $FreeBSD: src/sys/i386/isa/apic_vector.s,v 1.47.2.5 2001/09/01 22:33:38 tegge Exp $
- * $DragonFly: src/sys/platform/pc32/isa/Attic/apic_vector.s,v 1.13 2003/08/25 19:50:32 dillon Exp $
+ * $DragonFly: src/sys/platform/pc32/isa/Attic/apic_vector.s,v 1.14 2003/09/25 23:49:08 dillon Exp $
  */
 
 
  *     - Push the trap frame required by doreti
  *     - Mask the interrupt and reenable its source
  *     - If we cannot take the interrupt set its fpending bit and
- *       doreti.
+ *       doreti.  Note that we cannot mess with mp_lock at all
+ *       if we entered from a critical section!
  *     - If we can take the interrupt clear its fpending bit,
  *       call the handler, then unmask and doreti.
  *
@@ -242,7 +243,8 @@ IDTVEC(vec_name) ;                                                  \
  *     - If we cannot take the interrupt set its ipending bit and
  *       doreti.  In addition to checking for a critical section
  *       and cpl mask we also check to see if the thread is still
- *       running.
+ *       running.  Note that we cannot mess with mp_lock at all
+ *       if we entered from a critical section!
  *     - If we can take the interrupt clear its ipending bit
  *       and schedule the thread.  Leave interrupts masked and doreti.
  *
index ca802ec..76fd90d 100644 (file)
@@ -37,7 +37,7 @@
  *     @(#)ipl.s
  *
  * $FreeBSD: src/sys/i386/isa/ipl.s,v 1.32.2.3 2002/05/16 16:03:56 bde Exp $
- * $DragonFly: src/sys/platform/pc32/isa/ipl.s,v 1.12 2003/08/25 19:50:32 dillon Exp $
+ * $DragonFly: src/sys/platform/pc32/isa/ipl.s,v 1.13 2003/09/25 23:49:08 dillon Exp $
  */
 
 
@@ -77,6 +77,9 @@ softtty_imask:        .long   SWI_TTY_MASK
         * Handle return from interrupts, traps and syscalls.  This function
         * checks the cpl for unmasked pending interrupts (fast, normal, or
         * soft) and schedules them if appropriate, then irets.
+        *
+        * If we are in a critical section we cannot run any pending ints
+        * nor can be play with mp_lock.
         */
        SUPERALIGN_TEXT
        .type   doreti,@function