From owner-svn-src-all@FreeBSD.ORG Mon Oct 3 14:23:00 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D05151065742; Mon, 3 Oct 2011 14:23:00 +0000 (UTC) (envelope-from attilio@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id BF5FF8FC14; Mon, 3 Oct 2011 14:23:00 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.4/8.14.4) with ESMTP id p93EN04w013952; Mon, 3 Oct 2011 14:23:00 GMT (envelope-from attilio@svn.freebsd.org) Received: (from attilio@localhost) by svn.freebsd.org (8.14.4/8.14.4/Submit) id p93EN01t013948; Mon, 3 Oct 2011 14:23:00 GMT (envelope-from attilio@svn.freebsd.org) Message-Id: <201110031423.p93EN01t013948@svn.freebsd.org> From: Attilio Rao Date: Mon, 3 Oct 2011 14:23:00 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r225936 - in head/sys: amd64/amd64 i386/i386 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: Mon, 03 Oct 2011 14:23:00 -0000 Author: attilio Date: Mon Oct 3 14:23:00 2011 New Revision: 225936 URL: http://svn.freebsd.org/changeset/base/225936 Log: Add some improvements in the idle table callbacks: - Replace instances of manual assembly instruction "hlt" call with halt() function calling. - In cpu_idle_mwait() avoid races in check to sched_runnable() using the same pattern used in cpu_idle_hlt() with the 'hlt' instruction. - Add comments explaining the logic behind the pattern used in cpu_idle_hlt() and other idle callbacks. In collabouration with: jhb, mav Reviewed by: adri, kib MFC after: 3 weeks Modified: head/sys/amd64/amd64/machdep.c head/sys/i386/i386/machdep.c Modified: head/sys/amd64/amd64/machdep.c ============================================================================== --- head/sys/amd64/amd64/machdep.c Mon Oct 3 14:11:54 2011 (r225935) +++ head/sys/amd64/amd64/machdep.c Mon Oct 3 14:23:00 2011 (r225936) @@ -609,7 +609,7 @@ void cpu_halt(void) { for (;;) - __asm__ ("hlt"); + halt(); } void (*cpu_idle_hook)(void) = NULL; /* ACPI idle hook. */ @@ -630,6 +630,8 @@ cpu_idle_acpi(int busy) state = (int *)PCPU_PTR(monitorbuf); *state = STATE_SLEEPING; + + /* See comments in cpu_idle_hlt(). */ disable_intr(); if (sched_runnable()) enable_intr(); @@ -647,9 +649,22 @@ cpu_idle_hlt(int busy) state = (int *)PCPU_PTR(monitorbuf); *state = STATE_SLEEPING; + /* - * We must absolutely guarentee that hlt is the next instruction - * after sti or we introduce a timing window. + * Since we may be in a critical section from cpu_idle(), if + * an interrupt fires during that critical section we may have + * a pending preemption. If the CPU halts, then that thread + * may not execute until a later interrupt awakens the CPU. + * To handle this race, check for a runnable thread after + * disabling interrupts and immediately return if one is + * found. Also, we must absolutely guarentee that hlt is + * the next instruction after sti. This ensures that any + * interrupt that fires after the call to disable_intr() will + * immediately awaken the CPU from hlt. Finally, please note + * that on x86 this works fine because of interrupts enabled only + * after the instruction following sti takes place, while IF is set + * to 1 immediately, allowing hlt instruction to acknowledge the + * interrupt. */ disable_intr(); if (sched_runnable()) @@ -675,11 +690,19 @@ cpu_idle_mwait(int busy) state = (int *)PCPU_PTR(monitorbuf); *state = STATE_MWAIT; - if (!sched_runnable()) { - cpu_monitor(state, 0, 0); - if (*state == STATE_MWAIT) - cpu_mwait(0, MWAIT_C1); + + /* See comments in cpu_idle_hlt(). */ + disable_intr(); + if (sched_runnable()) { + enable_intr(); + *state = STATE_RUNNING; + return; } + cpu_monitor(state, 0, 0); + if (*state == STATE_MWAIT) + __asm __volatile("sti; mwait" : : "a" (MWAIT_C1), "c" (0)); + else + enable_intr(); *state = STATE_RUNNING; } @@ -691,6 +714,12 @@ cpu_idle_spin(int busy) state = (int *)PCPU_PTR(monitorbuf); *state = STATE_RUNNING; + + /* + * The sched_runnable() call is racy but as long as there is + * a loop missing it one time will have just a little impact if any + * (and it is much better than missing the check at all). + */ for (i = 0; i < 1000; i++) { if (sched_runnable()) return; Modified: head/sys/i386/i386/machdep.c ============================================================================== --- head/sys/i386/i386/machdep.c Mon Oct 3 14:11:54 2011 (r225935) +++ head/sys/i386/i386/machdep.c Mon Oct 3 14:23:00 2011 (r225936) @@ -1222,7 +1222,7 @@ void cpu_halt(void) { for (;;) - __asm__ ("hlt"); + halt(); } #endif @@ -1245,6 +1245,8 @@ cpu_idle_acpi(int busy) state = (int *)PCPU_PTR(monitorbuf); *state = STATE_SLEEPING; + + /* See comments in cpu_idle_hlt(). */ disable_intr(); if (sched_runnable()) enable_intr(); @@ -1263,9 +1265,22 @@ cpu_idle_hlt(int busy) state = (int *)PCPU_PTR(monitorbuf); *state = STATE_SLEEPING; + /* - * We must absolutely guarentee that hlt is the next instruction - * after sti or we introduce a timing window. + * Since we may be in a critical section from cpu_idle(), if + * an interrupt fires during that critical section we may have + * a pending preemption. If the CPU halts, then that thread + * may not execute until a later interrupt awakens the CPU. + * To handle this race, check for a runnable thread after + * disabling interrupts and immediately return if one is + * found. Also, we must absolutely guarentee that hlt is + * the next instruction after sti. This ensures that any + * interrupt that fires after the call to disable_intr() will + * immediately awaken the CPU from hlt. Finally, please note + * that on x86 this works fine because of interrupts enabled only + * after the instruction following sti takes place, while IF is set + * to 1 immediately, allowing hlt instruction to acknowledge the + * interrupt. */ disable_intr(); if (sched_runnable()) @@ -1292,11 +1307,19 @@ cpu_idle_mwait(int busy) state = (int *)PCPU_PTR(monitorbuf); *state = STATE_MWAIT; - if (!sched_runnable()) { - cpu_monitor(state, 0, 0); - if (*state == STATE_MWAIT) - cpu_mwait(0, MWAIT_C1); + + /* See comments in cpu_idle_hlt(). */ + disable_intr(); + if (sched_runnable()) { + enable_intr(); + *state = STATE_RUNNING; + return; } + cpu_monitor(state, 0, 0); + if (*state == STATE_MWAIT) + __asm __volatile("sti; mwait" : : "a" (MWAIT_C1), "c" (0)); + else + enable_intr(); *state = STATE_RUNNING; } @@ -1308,6 +1331,12 @@ cpu_idle_spin(int busy) state = (int *)PCPU_PTR(monitorbuf); *state = STATE_RUNNING; + + /* + * The sched_runnable() call is racy but as long as there is + * a loop missing it one time will have just a little impact if any + * (and it is much better than missing the check at all). + */ for (i = 0; i < 1000; i++) { if (sched_runnable()) return;