kernel: Properly synchronize suspending new thread

After load_image() the child thread is suspended and the parent is
expected to resume it later. However, it is possible that the parent
attempts to resume its child after it has been notified that the image
had been loaded but before the child managed to suspend itself. In such
case the child would suspends itself after that wake up attempt and,
consequently will not be ever resumed.

To mitigate that problem flag Thread::going_to_suspend has been added
which helps synchronizing thread suspension and continuation in a similar
way that "traditional" thread blocking is performed. This means that
the child should behave in a following manner: set its going_to_suspend flag,
notify the parent (i.e. any thread that may want to resume it), acquire
its scheduler_lock and suspend itself if the going_to_suspend flag is set.
The parent should follow pattern: clear going_to_suspend flag of the thread
that is about to be resumed, acquire that thread scheduler_lock and enqueue
it in a run queue if it is suspended.

Thanks Oliver for reporting the bug and identifying what causes it.

headers/private/kernel/thread.h

@@ -1,4 +1,5 @@
 /*
+ * Copyright 2014, Paweł Dziepak, [email protected].
  * Copyright 2008-2011, Ingo Weinhold, [email protected].
  * Copyright 2002-2007, Axel Dörfler, [email protected].
  * Distributed under the terms of the MIT License.
@@ -410,4 +411,46 @@ thread_unpin_from_current_cpu(Thread* thread)
 }
 
 
+static inline void
+thread_prepare_suspend()
+{
+	Thread* thread = thread_get_current_thread();
+	thread->going_to_suspend = true;
+}
+
+
+static inline void
+thread_suspend(bool alreadyPrepared = false)
+{
+	Thread* thread = thread_get_current_thread();
+	if (!alreadyPrepared)
+		thread_prepare_suspend();
+
+	cpu_status state = disable_interrupts();
+	acquire_spinlock(&thread->scheduler_lock);
+
+	if (thread->going_to_suspend)
+		scheduler_reschedule(B_THREAD_SUSPENDED);
+
+	release_spinlock(&thread->scheduler_lock);
+	restore_interrupts(state);
+}
+
+
+static inline void
+thread_continue(Thread* thread)
+{
+	thread->going_to_suspend = false;
+
+	cpu_status state = disable_interrupts();
+	acquire_spinlock(&thread->scheduler_lock);
+
+	if (thread->state == B_THREAD_SUSPENDED)
+		scheduler_enqueue_in_run_queue(thread);
+
+	release_spinlock(&thread->scheduler_lock);
+	restore_interrupts(state);
+}
+
+
 #endif /* _THREAD_H */

headers/private/kernel/thread_types.h

@@ -422,6 +422,7 @@ struct Thread : TeamThreadIteratorEntry<thread_id>, KernelReferenceable {
 	Thread			*hash_next;		// protected by thread hash lock
 	Thread			*team_next;		// protected by team lock and fLock
 	char			name[B_OS_NAME_LENGTH];	// protected by fLock
+	bool			going_to_suspend;	// protected by scheduler lock
 	int32			priority;		// protected by scheduler lock
 	int32			io_priority;	// protected by fLock
 	int32			state;			// protected by scheduler lock

src/system/kernel/image.cpp

@@ -371,20 +371,14 @@ notify_loading_app(status_t result, bool suspend)
 		// we're done with the team stuff, get the scheduler lock instead
 		teamLocker.Unlock();
 
-		Thread* thread = loadingInfo->thread;
+		thread_prepare_suspend();
-		InterruptsSpinLocker schedulerLocker(thread->scheduler_lock);
+
 		// wake up the waiting thread
-		if (thread->state == B_THREAD_SUSPENDED)
+		thread_continue(loadingInfo->thread);
-			scheduler_enqueue_in_run_queue(thread);
-		schedulerLocker.Unlock();
 
 		// suspend ourselves, if desired
-		if (suspend) {
+		if (suspend)
-			Thread* thread = thread_get_current_thread();
+			thread_suspend(true);
-			InterruptsSpinLocker schedulerLocker(thread->scheduler_lock);
-
-			scheduler_reschedule(B_THREAD_SUSPENDED);
-		}
 	}
 }
 

src/system/kernel/signal.cpp

@@ -1,4 +1,5 @@
 /*
+ * Copyright 2014, Paweł Dziepak, [email protected].
  * Copyright 2011, Ingo Weinhold, [email protected].
  * Copyright 2002-2009, Axel Dörfler, [email protected].
  * Copyright 2002, Angelo Mottola, [email protected].
@@ -1128,10 +1129,8 @@ handle_signals(Thread* thread)
 								& (CONTINUE_SIGNALS | KILL_SIGNALS)) != 0;
 					locker.Unlock();
 
-					if (!resume) {
+					if (!resume)
-						InterruptsSpinLocker _(thread->scheduler_lock);
+						thread_suspend();
-						scheduler_reschedule(B_THREAD_SUSPENDED);
-					}
 
 					continue;
 				}
@@ -1377,9 +1376,7 @@ send_signal_to_thread_locked(Thread* thread, uint32 signalNumber,
 
 	if (thread->team == team_get_kernel_team()) {
 		// Signals to kernel threads will only wake them up
-		SpinLocker _(thread->scheduler_lock);
+		thread_continue(thread);
-		if (thread->state == B_THREAD_SUSPENDED)
-			scheduler_enqueue_in_run_queue(thread);
 		return B_OK;
 	}
 
@@ -1401,6 +1398,8 @@ send_signal_to_thread_locked(Thread* thread, uint32 signalNumber,
 				mainThread->AddPendingSignal(SIGKILLTHR);
 
 				// wake up main thread
+				thread->going_to_suspend = false;
+
 				SpinLocker locker(mainThread->scheduler_lock);
 				if (mainThread->state == B_THREAD_SUSPENDED)
 					scheduler_enqueue_in_run_queue(mainThread);
@@ -1416,6 +1415,8 @@ send_signal_to_thread_locked(Thread* thread, uint32 signalNumber,
 		case SIGKILLTHR:
 		{
 			// Wake up suspended threads and interrupt waiting ones
+			thread->going_to_suspend = false;
+
 			SpinLocker locker(thread->scheduler_lock);
 			if (thread->state == B_THREAD_SUSPENDED)
 				scheduler_enqueue_in_run_queue(thread);
@@ -1427,6 +1428,8 @@ send_signal_to_thread_locked(Thread* thread, uint32 signalNumber,
 		case SIGNAL_CONTINUE_THREAD:
 		{
 			// wake up thread, and interrupt its current syscall
+			thread->going_to_suspend = false;
+
 			SpinLocker locker(thread->scheduler_lock);
 			if (thread->state == B_THREAD_SUSPENDED)
 				scheduler_enqueue_in_run_queue(thread);
@@ -1438,6 +1441,8 @@ send_signal_to_thread_locked(Thread* thread, uint32 signalNumber,
 		{
 			// Wake up thread if it was suspended, otherwise interrupt it, if
 			// the signal isn't blocked.
+			thread->going_to_suspend = false;
+
 			SpinLocker locker(thread->scheduler_lock);
 			if (thread->state == B_THREAD_SUSPENDED)
 				scheduler_enqueue_in_run_queue(thread);
@@ -1605,6 +1610,8 @@ send_signal_to_team_locked(Team* team, uint32 signalNumber, Signal* signal,
 				mainThread->AddPendingSignal(SIGKILLTHR);
 
 				// wake up main thread
+				mainThread->going_to_suspend = false;
+
 				SpinLocker _(mainThread->scheduler_lock);
 				if (mainThread->state == B_THREAD_SUSPENDED)
 					scheduler_enqueue_in_run_queue(mainThread);
@@ -1619,6 +1626,8 @@ send_signal_to_team_locked(Team* team, uint32 signalNumber, Signal* signal,
 			// don't block the signal.
 			for (Thread* thread = team->thread_list; thread != NULL;
 					thread = thread->team_next) {
+				thread->going_to_suspend = false;
+
 				SpinLocker _(thread->scheduler_lock);
 				if (thread->state == B_THREAD_SUSPENDED) {
 					scheduler_enqueue_in_run_queue(thread);

src/system/kernel/team.cpp

@@ -1,4 +1,5 @@
 /*
+ * Copyright 2014, Paweł Dziepak, [email protected].
  * Copyright 2008-2011, Ingo Weinhold, [email protected].
  * Copyright 2002-2010, Axel Dörfler, [email protected].
  * Distributed under the terms of the MIT License.
@@ -1811,11 +1812,8 @@ load_image_internal(char**& _flatArgs, size_t flatArgsSize, int32 argCount,
 	// wait for the loader of the new team to finish its work
 	if ((flags & B_WAIT_TILL_LOADED) != 0) {
 		if (mainThread != NULL) {
-			InterruptsSpinLocker schedulerLocker(mainThread->scheduler_lock);
-
 			// resume the team's main thread
-	 		if (mainThread->state == B_THREAD_SUSPENDED)
+			thread_continue(mainThread);
-				scheduler_enqueue_in_run_queue(mainThread);
 		}
 
 		// Now suspend ourselves until loading is finished. We will be woken
@@ -1823,12 +1821,8 @@ load_image_internal(char**& _flatArgs, size_t flatArgsSize, int32 argCount,
 		// the team is going to die (e.g. is killed). In either case the one
 		// setting `loadingInfo.done' is responsible for removing the info from
 		// the team structure.
-		while (!loadingInfo.done) {
+		while (!loadingInfo.done)
-			Thread* thread = thread_get_current_thread();
+			thread_suspend();
-
-			InterruptsSpinLocker schedulerLocker(thread->scheduler_lock);
-			scheduler_reschedule(B_THREAD_SUSPENDED);
-		}
 
 		if (loadingInfo.result < B_OK)
 			return loadingInfo.result;
@@ -3193,11 +3187,8 @@ team_delete_team(Team* team, port_id debuggerPort)
 		loadingInfo->result = B_ERROR;
 		loadingInfo->done = true;
 
-		InterruptsSpinLocker _(loadingInfo->thread->scheduler_lock);
-
 		// wake up the waiting thread
-		if (loadingInfo->thread->state == B_THREAD_SUSPENDED)
+		thread_continue(loadingInfo->thread);
-			scheduler_enqueue_in_run_queue(loadingInfo->thread);
 	}
 
 	// notify team watchers