From b167307526897db52743daa0aeffe2a0041abaa8 Mon Sep 17 00:00:00 2001 From: Pawel Dziepak Date: Mon, 17 Mar 2014 02:11:22 +0100 Subject: [PATCH] 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 | 43 +++++++++++++++++++++++++++ headers/private/kernel/thread_types.h | 1 + src/system/kernel/image.cpp | 16 ++++------ src/system/kernel/signal.cpp | 23 +++++++++----- src/system/kernel/team.cpp | 19 ++++-------- 5 files changed, 70 insertions(+), 32 deletions(-) diff --git a/headers/private/kernel/thread.h b/headers/private/kernel/thread.h index f502c94251..b4899ed945 100644 --- a/headers/private/kernel/thread.h +++ b/headers/private/kernel/thread.h @@ -1,4 +1,5 @@ /* + * Copyright 2014, Paweł Dziepak, pdziepak@quarnos.org. * Copyright 2008-2011, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2007, Axel Dörfler, axeld@pinc-software.de. * 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 */ diff --git a/headers/private/kernel/thread_types.h b/headers/private/kernel/thread_types.h index eb7f1ad5e1..bfa1ad1004 100644 --- a/headers/private/kernel/thread_types.h +++ b/headers/private/kernel/thread_types.h @@ -422,6 +422,7 @@ struct Thread : TeamThreadIteratorEntry, 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 diff --git a/src/system/kernel/image.cpp b/src/system/kernel/image.cpp index c948b69f86..bb89b208b3 100644 --- a/src/system/kernel/image.cpp +++ b/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; - InterruptsSpinLocker schedulerLocker(thread->scheduler_lock); + thread_prepare_suspend(); + // wake up the waiting thread - if (thread->state == B_THREAD_SUSPENDED) - scheduler_enqueue_in_run_queue(thread); - schedulerLocker.Unlock(); + thread_continue(loadingInfo->thread); // suspend ourselves, if desired - if (suspend) { - Thread* thread = thread_get_current_thread(); - InterruptsSpinLocker schedulerLocker(thread->scheduler_lock); - - scheduler_reschedule(B_THREAD_SUSPENDED); - } + if (suspend) + thread_suspend(true); } } diff --git a/src/system/kernel/signal.cpp b/src/system/kernel/signal.cpp index 66564601d5..84a51658d1 100644 --- a/src/system/kernel/signal.cpp +++ b/src/system/kernel/signal.cpp @@ -1,4 +1,5 @@ /* + * Copyright 2014, Paweł Dziepak, pdziepak@quarnos.org. * Copyright 2011, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2009, Axel Dörfler, axeld@pinc-software.de. * Copyright 2002, Angelo Mottola, a.mottola@libero.it. @@ -1128,10 +1129,8 @@ handle_signals(Thread* thread) & (CONTINUE_SIGNALS | KILL_SIGNALS)) != 0; locker.Unlock(); - if (!resume) { - InterruptsSpinLocker _(thread->scheduler_lock); - scheduler_reschedule(B_THREAD_SUSPENDED); - } + if (!resume) + thread_suspend(); 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); - if (thread->state == B_THREAD_SUSPENDED) - scheduler_enqueue_in_run_queue(thread); + thread_continue(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); diff --git a/src/system/kernel/team.cpp b/src/system/kernel/team.cpp index b715635bbf..a775d1bb18 100644 --- a/src/system/kernel/team.cpp +++ b/src/system/kernel/team.cpp @@ -1,4 +1,5 @@ /* + * Copyright 2014, Paweł Dziepak, pdziepak@quarnos.org. * Copyright 2008-2011, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2010, Axel Dörfler, axeld@pinc-software.de. * 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) - scheduler_enqueue_in_run_queue(mainThread); + thread_continue(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) { - Thread* thread = thread_get_current_thread(); - - InterruptsSpinLocker schedulerLocker(thread->scheduler_lock); - scheduler_reschedule(B_THREAD_SUSPENDED); - } + while (!loadingInfo.done) + thread_suspend(); 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) - scheduler_enqueue_in_run_queue(loadingInfo->thread); + thread_continue(loadingInfo->thread); } // notify team watchers