kernel - Fix serious issue w/ smp_invltlb(), plus other issues.
authorMatthew Dillon <dillon@apollo.backplane.com>
Thu, 28 Oct 2010 06:55:58 +0000 (23:55 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Thu, 28 Oct 2010 21:21:46 +0000 (14:21 -0700)
* smp_invltlb() was running asynchronously when it really needs to run
  synchronously.  Generally speaking the asynchronous ipi did in fact work
  pretty well but it still presents a 1uS window of opportunity which
  bypasses normal write ordering safeties.

  Run smp_invltlb() synchronously.

* Fixing the above lea to the discovery of an ACPI issue.  The ACPI
  cpu idle halt code, at least on the gigabyte phenom x 6 I've been
  testing with, can cause IPIs to be lost.  Not just delayed, straight
  out lost.  Gone.  Poof.  It doesn't matter whether the IPI is a
  broadcast IPI or a directed IPI, it can still get lost.

  This was particularly noticeable when I fixed smp_invltlb() and my
  test box started locking up due to a random cpu sometimes not receiving
  the Xinvltlb IPI, and it is quite possible that this issue was also
  responsible for the random seg-faults we would sometimes get on 64-bit
  boxes.

  For now the acpi halt code has been disabled.  It can be enabled with
  sysctl machdep.cpu_idle_hlt=2 if you want to risk it.

* Use doreti_syscall_ret and doreti_iret in several cases that were
  previously popping the interrupt frame and iret'ing manually.  This
  is operationally equivalent.

* Add a missing "sti" in the idle loop.  Usually the cpu_idle_hook()
  deals with this but there are some alternative paths which might not,
  potentially causing interrupts to be delayed unnecessarily.
  At worst the idle thread has an extra sti in it.

* Add v_smpinvltlb to struct vmmeter, plus some reserved slots for
  future expansion.

* Adjust vmstat -s to report smpinvltlb's.

13 files changed:
sys/cpu/i386/include/cpufunc.h
sys/cpu/x86_64/include/cpufunc.h
sys/platform/pc32/apic/apic_vector.s
sys/platform/pc32/i386/machdep.c
sys/platform/pc32/i386/mp_machdep.c
sys/platform/pc32/include/globaldata.h
sys/platform/pc64/apic/apic_vector.s
sys/platform/pc64/include/globaldata.h
sys/platform/pc64/x86_64/machdep.c
sys/platform/pc64/x86_64/mp_machdep.c
sys/platform/pc64/x86_64/pmap.c
sys/sys/vmmeter.h
usr.bin/vmstat/vmstat.c

index 3f68235..fa0b3ed 100644 (file)
@@ -365,6 +365,7 @@ invd(void)
  */
 #ifdef SMP
 void smp_invltlb(void);
+void smp_invltlb_intr(void);
 #else
 #define smp_invltlb()
 #endif
index 8b1305a..d177316 100644 (file)
@@ -378,6 +378,7 @@ invd(void)
  */
 #ifdef SMP
 void smp_invltlb(void);
+void smp_invltlb_intr(void);
 #else
 #define smp_invltlb()
 #endif
index 99add4d..d7f9d0e 100644 (file)
@@ -195,17 +195,17 @@ Xspuriousint:
        SUPERALIGN_TEXT
        .globl  Xinvltlb
 Xinvltlb:
-       pushl   %eax
-
-       movl    %cr3, %eax              /* invalidate the TLB */
-       movl    %eax, %cr3
-
-       ss                              /* stack segment, avoid %ds load */
+       PUSH_FRAME
        movl    $0, lapic_eoi           /* End Of Interrupt to APIC */
+       FAKE_MCOUNT(15*4(%esp))
 
-       popl    %eax
-       iret
+       subl    $8,%esp                 /* make same as interrupt frame */
+       pushl   %esp                    /* pass frame by reference */
+       call    smp_invltlb_intr
+       addl    $12,%esp
 
+       MEXITCOUNT
+       jmp     doreti_syscall_ret
 
 /*
  * Executed by a CPU when it receives an Xcpustop IPI from another CPU,
@@ -315,8 +315,7 @@ Xipiq:
 1:
        orl     $RQF_IPIQ,PCPU(reqflags)
        MEXITCOUNT
-       POP_FRAME
-       iret
+       jmp     doreti_syscall_ret
 
        .text
        SUPERALIGN_TEXT
@@ -346,8 +345,7 @@ Xtimer:
 1:
        orl     $RQF_TIMER,PCPU(reqflags)
        MEXITCOUNT
-       POP_FRAME
-       iret
+       jmp     doreti_syscall_ret
 
 #ifdef APIC_IO
 
index 5c1394b..5eaed51 100644 (file)
@@ -872,10 +872,21 @@ cpu_halt(void)
  * check for pending interrupts due to entering and exiting its own 
  * critical section.
  *
- * Note on cpu_idle_hlt:  On an SMP system we rely on a scheduler IPI
- * to wake a HLTed cpu up.  However, there are cases where the idlethread
- * will be entered with the possibility that no IPI will occur and in such
- * cases lwkt_switch() sets TDF_IDLE_NOHLT.
+ * NOTE: On an SMP system we rely on a scheduler IPI to wake a HLTed cpu up.
+ *      However, there are cases where the idlethread will be entered with
+ *      the possibility that no IPI will occur and in such cases
+ *      lwkt_switch() sets TDF_IDLE_NOHLT.
+ *
+ * WARNING: On a SMP system the ACPI halt code can sometimes cause IPIs
+ *         to be completely lost.  And I mean completely... vector never
+ *         gets delivered.  Because of this we default to NOT using the
+ *         ACPI halt code.  If you want to use the ACPI halt code anyway
+ *         use sysctl machdep.cpu_idle_hlt=2.
+ *
+ *         This wasn't a huge deal before since our IPIs were queued, but
+ *         now that we have a separate per-cpu Xtimer IPI and now that
+ *         our Xinvltlb IPI is synchronous a missed IPI can cause total
+ *         havoc.
  */
 static int     cpu_idle_hlt = 1;
 static int     cpu_idle_hltcnt;
@@ -922,12 +933,17 @@ cpu_idle(void)
                    (td->td_flags & TDF_IDLE_NOHLT) == 0) {
                        __asm __volatile("cli");
                        splz();
-                       if (!lwkt_runnable())
-                               cpu_idle_hook();
+                       if (!lwkt_runnable()) {
+                               if (cpu_idle_hlt == 1)
+                                       cpu_idle_default_hook();
+                               else
+                                       cpu_idle_hook();
+                       }
 #ifdef SMP
                        else
                                handle_cpu_contention_mask();
 #endif
+                       __asm __volatile("sti");
                        ++cpu_idle_hltcnt;
                } else {
                        td->td_flags &= ~TDF_IDLE_NOHLT;
index c3bdc1a..2fb3f89 100644 (file)
@@ -2602,28 +2602,63 @@ smitest(void)
  * If for some reason we were unable to start all cpus we cannot safely
  * use broadcast IPIs.
  */
+
+static cpumask_t smp_invltlb_req;
+
 void
 smp_invltlb(void)
 {
 #ifdef SMP
-#if 0
-       pmap_inval_info info;
+       struct mdglobaldata *md = mdcpu;
 
-       pmap_inval_init(&info);
-       pmap_inval_interlock(&info, &kernel_pmap, -1);
-       pmap_inval_deinterlock(&info, &kernel_pmap);
-       pmap_inval_done(&info);
-#else
+       crit_enter_gd(&md->mi);
+       md->gd_invltlb_ret = 0;
+       ++md->mi.gd_cnt.v_smpinvltlb;
+       atomic_set_int(&smp_invltlb_req, md->mi.gd_cpumask);
        if (smp_startup_mask == smp_active_mask) {
                all_but_self_ipi(XINVLTLB_OFFSET);
        } else {
-               selected_apic_ipi(smp_active_mask, XINVLTLB_OFFSET,
-                       APIC_DELMODE_FIXED);
+               selected_apic_ipi(smp_active_mask & ~md->mi.gd_cpumask,
+                                 XINVLTLB_OFFSET, APIC_DELMODE_FIXED);
        }
+       while ((md->gd_invltlb_ret & smp_active_mask & ~md->mi.gd_cpumask) !=
+              (smp_active_mask & ~md->mi.gd_cpumask)) {
+               cpu_mfence();
+               cpu_pause();
+       }
+       atomic_clear_int(&smp_invltlb_req, md->mi.gd_cpumask);
+       crit_exit_gd(&md->mi);
 #endif
-#endif
 }
 
+#ifdef SMP
+
+/*
+ * Called from Xinvltlb assembly with interrupts disabled.  We didn't
+ * bother to bump the critical section count or nested interrupt count
+ * so only do very low level operations here.
+ */
+void
+smp_invltlb_intr(void)
+{
+       struct mdglobaldata *md = mdcpu;
+       struct mdglobaldata *omd;
+       cpumask_t mask;
+       int cpu;
+
+       mask = smp_invltlb_req;
+       cpu_mfence();
+       cpu_invltlb();
+       while (mask) {
+               cpu = bsfl(mask);
+               mask &= ~(1 << cpu);
+               omd = (struct mdglobaldata *)globaldata_find(cpu);
+               atomic_set_int(&omd->gd_invltlb_ret, md->mi.gd_cpumask);
+       }
+}
+
+#endif
+
 /*
  * When called the executing CPU will send an IPI to all other CPUs
  *  requesting that they halt execution.
index 3fcf667..2135a1c 100644 (file)
@@ -96,6 +96,7 @@ struct mdglobaldata {
        unsigned        *gd_GDADDR1;    /* per-cpu whole page table map va */
        u_int           gd_acpi_id;
        u_int           gd_apic_id;
+       cpumask_t       gd_invltlb_ret;
 };
 
 #define MDGLOBALDATA_BASEALLOC_SIZE    \
index 8d01976..201e341 100644 (file)
@@ -168,7 +168,7 @@ Xspuriousint:
 
        /* No EOI cycle used here */
 
-       iretq
+       jmp     doreti_iret
 
 
 /*
@@ -178,17 +178,17 @@ Xspuriousint:
        SUPERALIGN_TEXT
        .globl  Xinvltlb
 Xinvltlb:
-       pushq   %rax
-
-       movq    %cr3, %rax              /* invalidate the TLB */
-       movq    %rax, %cr3
-
+       APIC_PUSH_FRAME
        movq    lapic, %rax
        movl    $0, LA_EOI(%rax)        /* End Of Interrupt to APIC */
-
-       popq    %rax
-       iretq
-
+       FAKE_MCOUNT(TF_RIP(%rsp))
+       subq    $8,%rsp                 /* make same as interrupt frame */
+       movq    %rsp,%rdi               /* pass frame by reference */
+       call    smp_invltlb_intr
+       addq    $8,%rsp                 /* turn into trapframe */
+       MEXITCOUNT
+       APIC_POP_FRAME
+       jmp     doreti_iret
 
 /*
  * Executed by a CPU when it receives an Xcpustop IPI from another CPU,
@@ -249,7 +249,7 @@ Xcpustop:
 2:
        MEXITCOUNT
        APIC_POP_FRAME
-       iretq
+       jmp     doreti_iret
 
        /*
         * For now just have one ipiq IPI, but what we really want is
@@ -283,7 +283,7 @@ Xipiq:
        orl     $RQF_IPIQ,PCPU(reqflags)
        MEXITCOUNT
        APIC_POP_FRAME
-       iretq
+       jmp     doreti_iret
 
        .text
        SUPERALIGN_TEXT
@@ -314,7 +314,7 @@ Xtimer:
        orl     $RQF_TIMER,PCPU(reqflags)
        MEXITCOUNT
        APIC_POP_FRAME
-       iretq
+       jmp     doreti_iret
 
 #ifdef APIC_IO
 
index 6d765ca..75351e1 100644 (file)
@@ -99,6 +99,7 @@ struct mdglobaldata {
        register_t      gd_rsp0;
        register_t      gd_user_fs;     /* current user fs in MSR */
        register_t      gd_user_gs;     /* current user gs in MSR */
+       cpumask_t       gd_invltlb_ret;
 };
 
 #define MDGLOBALDATA_BASEALLOC_SIZE    \
index d6c5fae..045c28d 100644 (file)
@@ -897,10 +897,21 @@ cpu_halt(void)
  * check for pending interrupts due to entering and exiting its own 
  * critical section.
  *
- * Note on cpu_idle_hlt:  On an SMP system we rely on a scheduler IPI
- * to wake a HLTed cpu up.  However, there are cases where the idlethread
- * will be entered with the possibility that no IPI will occur and in such
- * cases lwkt_switch() sets TDF_IDLE_NOHLT.
+ * NOTE: On an SMP system we rely on a scheduler IPI to wake a HLTed cpu up.
+ *      However, there are cases where the idlethread will be entered with
+ *      the possibility that no IPI will occur and in such cases
+ *      lwkt_switch() sets TDF_IDLE_NOHLT.
+ *
+ * WARNING: On a SMP system the ACPI halt code can sometimes cause IPIs
+ *         to be completely lost.  And I mean completely... vector never
+ *         gets delivered.  Because of this we default to NOT using the
+ *         ACPI halt code.  If you want to use the ACPI halt code anyway
+ *         use sysctl machdep.cpu_idle_hlt=2.
+ *
+ *         This wasn't a huge deal before since our IPIs were queued, but
+ *         now that we have a separate per-cpu Xtimer IPI and now that
+ *         our Xinvltlb IPI is synchronous a missed IPI can cause total
+ *         havoc.
  */
 static int     cpu_idle_hlt = 1;
 static int     cpu_idle_hltcnt;
@@ -947,12 +958,17 @@ cpu_idle(void)
                    (td->td_flags & TDF_IDLE_NOHLT) == 0) {
                        __asm __volatile("cli");
                        splz();
-                       if (!lwkt_runnable())
-                               cpu_idle_hook();
+                       if (!lwkt_runnable()) {
+                               if (cpu_idle_hlt == 1)
+                                       cpu_idle_default_hook();
+                               else
+                                       cpu_idle_hook();
+                       }
 #ifdef SMP
                        else
                                handle_cpu_contention_mask();
 #endif
+                       __asm __volatile("sti");
                        ++cpu_idle_hltcnt;
                } else {
                        td->td_flags &= ~TDF_IDLE_NOHLT;
index 32cb86d..dd396c5 100644 (file)
@@ -2780,33 +2780,81 @@ smitest(void)
 }
 
 /*
- * Lazy flush the TLB on all other CPU's.  DEPRECATED.
+ * Synchronously flush the TLB on all other CPU's.  The current cpu's
+ * TLB is not flushed.  If the caller wishes to flush the current cpu's
+ * TLB the caller must call cpu_invltlb() in addition to smp_invltlb().
  *
- * If for some reason we were unable to start all cpus we cannot safely
- * use broadcast IPIs.
+ * NOTE: If for some reason we were unable to start all cpus we cannot
+ *      safely use broadcast IPIs.
  */
+
+static cpumask_t smp_invltlb_req;
+
 void
 smp_invltlb(void)
 {
 #ifdef SMP
+       struct mdglobaldata *md = mdcpu;
 #if 0
-       struct pmap_inval_info info;
+       long count = 0;
+#endif
 
-       pmap_inval_init(&info);
-       pmap_inval_interlock(&info, &kernel_pmap, -1);
-       pmap_inval_deinterlock(&info, &kernel_pmap);
-       pmap_inval_done(&info);
-#else
+       crit_enter_gd(&md->mi);
+       md->gd_invltlb_ret = 0;
+       ++md->mi.gd_cnt.v_smpinvltlb;
+       atomic_set_int(&smp_invltlb_req, md->mi.gd_cpumask);
        if (smp_startup_mask == smp_active_mask) {
                all_but_self_ipi(XINVLTLB_OFFSET);
        } else {
-               selected_apic_ipi(smp_active_mask, XINVLTLB_OFFSET,
-                       APIC_DELMODE_FIXED);
+               selected_apic_ipi(smp_active_mask & ~md->mi.gd_cpumask,
+                                 XINVLTLB_OFFSET, APIC_DELMODE_FIXED);
        }
+       while ((md->gd_invltlb_ret & smp_active_mask & ~md->mi.gd_cpumask) !=
+              (smp_active_mask & ~md->mi.gd_cpumask)) {
+               cpu_mfence();
+               cpu_pause();
+#if 0
+               /* DEBUGGING */
+               if (++count == 400000000) {
+                       panic("smp_invltlb: endless loop %08lx %08lx",
+                             (long)md->gd_invltlb_ret,
+                             (long)smp_invltlb_req);
+               }
 #endif
+       }
+       atomic_clear_int(&smp_invltlb_req, md->mi.gd_cpumask);
+       crit_exit_gd(&md->mi);
 #endif
 }
 
+#ifdef SMP
+
+/*
+ * Called from Xinvltlb assembly with interrupts disabled.  We didn't
+ * bother to bump the critical section count or nested interrupt count
+ * so only do very low level operations here.
+ */
+void
+smp_invltlb_intr(void)
+{
+       struct mdglobaldata *md = mdcpu;
+       struct mdglobaldata *omd;
+       cpumask_t mask;
+       int cpu;
+
+       mask = smp_invltlb_req;
+       cpu_mfence();
+       cpu_invltlb();
+       while (mask) {
+               cpu = bsfl(mask);
+               mask &= ~(1 << cpu);
+               omd = (struct mdglobaldata *)globaldata_find(cpu);
+               atomic_set_int(&omd->gd_invltlb_ret, md->mi.gd_cpumask);
+       }
+}
+
+#endif
+
 /*
  * When called the executing CPU will send an IPI to all other CPUs
  *  requesting that they halt execution.
index 6bb1e25..b74422d 100644 (file)
@@ -1076,9 +1076,7 @@ pmap_qenter(vm_offset_t va, vm_page_t *m, int count)
                va += PAGE_SIZE;
                m++;
        }
-#ifdef SMP
-       smp_invltlb();  /* XXX */
-#endif
+       smp_invltlb();
 }
 
 /*
@@ -1102,9 +1100,7 @@ pmap_qremove(vm_offset_t va, int count)
                cpu_invlpg((void *)va);
                va += PAGE_SIZE;
        }
-#ifdef SMP
        smp_invltlb();
-#endif
 }
 
 /*
index 006810f..1baf8a6 100644 (file)
@@ -100,7 +100,16 @@ struct vmmeter {
        u_int v_forwarded_misses;
        u_int v_sendsys;        /* calls to sendsys() */
        u_int v_waitsys;        /* calls to waitsys() */
-#define vmmeter_uint_end       v_waitsys
+       u_int v_smpinvltlb;     /* nasty global invltlbs */
+       u_int v_reserved0;
+       u_int v_reserved1;
+       u_int v_reserved2;
+       u_int v_reserved3;
+       u_int v_reserved4;
+       u_int v_reserved5;
+       u_int v_reserved6;
+       u_int v_reserved7;
+#define vmmeter_uint_end       v_reserved7
 };
 
 struct vmstats {
index 3a6e1e4..7755821 100644 (file)
@@ -604,6 +604,7 @@ dosum(void)
        printf("%9u pages wired down\n", vms.v_wire_count);
        printf("%9u pages free\n", vms.v_free_count);
        printf("%9u bytes per page\n", vms.v_page_size);
+       printf("%9u global smp invltlbs\n", vmm.v_smpinvltlb);
        
        if ((nch_tmp = malloc(nch_size)) == NULL) {
                perror("malloc");