From 8b99e9456241706b3d61fd6b165e57d14618e568 Mon Sep 17 00:00:00 2001 From: Marco Costalba Date: Sat, 20 Feb 2010 18:36:07 +0100 Subject: [PATCH] Revert small state change optimization in idle_loop() Joona says: 1. We should not be afraid of "AllThreadsShouldExit" flag. Because when this is set to true we _must_not_ be searching (= All splits must have been undone). And if we are not searching it's impossible that some other thread could give us work to do. So setting state to THREAD_AVAILABLE doesn't do any harm. If you want to add check for this, you could do it like this: if (threads[threadID].state == THREAD_WORKISWAITING) { + assert(!AllThreadsShouldExit) threads[threadID].state = THREAD_SEARCHING; 2a. If waitSp->cpus == 0, setting state to THREAD_AVAILABLE makes no harm either, because helpful master concept dictates that _only_ our own slave can book us. If we don't have any slaves, noone has the right to book us. 2b. If point (2a) is not correct then your extra check only adds extra race: In smp code checking for waitSp->cpus > 0 is not enough. It's possible that our slave immediately exits and another thread books us as a slave when our state is still THREAD_AVAILABLE. So instead of adding extra level of security we have just introduced extra race. Signed-off-by: Marco Costalba --- src/search.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/search.cpp b/src/search.cpp index 38e2ac14..5539b529 100644 --- a/src/search.cpp +++ b/src/search.cpp @@ -2650,6 +2650,8 @@ namespace { // If this thread has been assigned work, launch a search if (threads[threadID].state == THREAD_WORKISWAITING) { + assert(!AllThreadsShouldExit); + threads[threadID].state = THREAD_SEARCHING; if (threads[threadID].splitPoint->pvNode) @@ -2659,13 +2661,7 @@ namespace { assert(threads[threadID].state == THREAD_SEARCHING); - // If this is a slave thread reset to available, instead - // if it is a master thread and all slaves have finished - // then leave as is to avoid booking by another master, - // we will leave idle loop shortly anyhow. - if ( !AllThreadsShouldExit - && (!waitSp || waitSp->cpus > 0)) - threads[threadID].state = THREAD_AVAILABLE; + threads[threadID].state = THREAD_AVAILABLE; } // If this thread is the master of a split point and all threads have -- 2.39.2