From 516b82a0dd346ca7f6f431faf72f3ba2b5a47b36 Mon Sep 17 00:00:00 2001 From: maxdev1 Date: Fri, 28 Feb 2025 21:33:18 +0100 Subject: [PATCH] Avoid nested locking where possible; add scheduler preference to fix g_yield_t Holding locks and calling other code can lead to the situation where processor 0 tries to lock ABBA and processor 1 tries BAAB, which causes a deadlock when the simultaneously reach the second lock. This pattern should avoid this. --- kernel/inc/build_config.hpp | 2 +- kernel/inc/shared/system/spinlock.hpp | 11 +-- kernel/src/kernel/calls/syscall_memory.cpp | 4 +- kernel/src/kernel/calls/syscall_messaging.cpp | 6 +- kernel/src/kernel/calls/syscall_tasking.cpp | 16 +++-- kernel/src/kernel/filesystem/filesystem.cpp | 8 ++- kernel/src/kernel/ipc/message.cpp | 4 -- kernel/src/kernel/ipc/pipes.cpp | 4 -- .../kernel/system/interrupts/interrupts.cpp | 2 +- .../src/kernel/system/processor/processor.cpp | 5 ++ .../src/kernel/system/processor/processor.hpp | 5 ++ kernel/src/kernel/tasking/cleanup.cpp | 4 +- .../scheduler/scheduler_round_robin.cpp | 5 +- kernel/src/kernel/tasking/tasking.cpp | 15 +++- kernel/src/kernel/tasking/tasking.hpp | 70 +++++++++---------- kernel/src/kernel/tasking/user_mutex.cpp | 4 +- 16 files changed, 95 insertions(+), 70 deletions(-) diff --git a/kernel/inc/build_config.hpp b/kernel/inc/build_config.hpp index dbbbf626..6e725af0 100644 --- a/kernel/inc/build_config.hpp +++ b/kernel/inc/build_config.hpp @@ -48,7 +48,7 @@ // version #define G_VERSION_MAJOR 0 #define G_VERSION_MINOR 20 -#define G_VERSION_PATCH 0 +#define G_VERSION_PATCH 1 #define G_LOADER_VERSION_MAJOR 1 #define G_LOADER_VERSION_MINOR 1 diff --git a/kernel/inc/shared/system/spinlock.hpp b/kernel/inc/shared/system/spinlock.hpp index 5573cbb7..049cee2e 100644 --- a/kernel/inc/shared/system/spinlock.hpp +++ b/kernel/inc/shared/system/spinlock.hpp @@ -21,12 +21,13 @@ #ifndef __SYSTEM_SPINLOCK__ #define __SYSTEM_SPINLOCK__ -typedef volatile int g_spinlock; +typedef int g_spinlock; -#define G_SPINLOCK_ACQUIRE(lock) \ - while(!__sync_bool_compare_and_swap(&lock, 0, 1)) \ - asm volatile("pause"); +#define G_SPINLOCK_ACQUIRE(lock) \ + while(!__sync_bool_compare_and_swap(&lock, 0, 1)) \ + asm volatile("pause"); -#define G_SPINLOCK_RELEASE(lock) lock = 0; +#define G_SPINLOCK_RELEASE(lock) \ + do { __sync_synchronize(); lock = 0; } while(0) #endif diff --git a/kernel/src/kernel/calls/syscall_memory.cpp b/kernel/src/kernel/calls/syscall_memory.cpp index 10adc738..65bd8fb0 100644 --- a/kernel/src/kernel/calls/syscall_memory.cpp +++ b/kernel/src/kernel/calls/syscall_memory.cpp @@ -143,16 +143,16 @@ void syscallShareMemory(g_task* task, g_syscall_share_mem* data) g_virtual_address virtualRangeBase = addressRangePoolAllocate(targetProcess->virtualRangePool, pages, G_PROC_VIRTUAL_RANGE_FLAG_NONE); + mutexRelease(&targetProcess->lock); + if(virtualRangeBase == 0) { logInfo( "%! task %i was unable to share memory area %h of size %h with task %i because there was no free virtual range", "syscall", task->id, memory, pages * G_PAGE_SIZE, targetProcess->main->id); - mutexRelease(&targetProcess->lock); return; } - mutexRelease(&targetProcess->lock); for(uint32_t i = 0; i < pages; i++) { diff --git a/kernel/src/kernel/calls/syscall_messaging.cpp b/kernel/src/kernel/calls/syscall_messaging.cpp index 69ce759b..2300892f 100644 --- a/kernel/src/kernel/calls/syscall_messaging.cpp +++ b/kernel/src/kernel/calls/syscall_messaging.cpp @@ -29,12 +29,14 @@ void syscallMessageSend(g_task* task, g_syscall_send_message* data) G_MESSAGE_SEND_STATUS_QUEUE_FULL && data->mode == G_MESSAGE_SEND_MODE_BLOCKING) { + INTERRUPTS_PAUSE; mutexAcquire(&task->lock); task->status = G_TASK_STATUS_WAITING; task->waitsFor = "message-send"; - messageWaitForSend(task->id, data->receiver); mutexRelease(&task->lock); + messageWaitForSend(task->id, data->receiver); taskingYield(); + INTERRUPTS_RESUME; } messageUnwaitForSend(task->id, data->receiver); } @@ -55,11 +57,13 @@ void syscallMessageReceive(g_task* task, g_syscall_receive_message* data) // break; // } + INTERRUPTS_PAUSE; mutexAcquire(&task->lock); task->status = G_TASK_STATUS_WAITING; task->waitsFor = "message-recv"; mutexRelease(&task->lock); taskingYield(); + INTERRUPTS_RESUME; } } diff --git a/kernel/src/kernel/calls/syscall_tasking.cpp b/kernel/src/kernel/calls/syscall_tasking.cpp index eeaffbae..5456d952 100644 --- a/kernel/src/kernel/calls/syscall_tasking.cpp +++ b/kernel/src/kernel/calls/syscall_tasking.cpp @@ -32,12 +32,14 @@ void syscallSleep(g_task* task, g_syscall_sleep* data) { + INTERRUPTS_PAUSE; mutexAcquire(&task->lock); task->status = G_TASK_STATUS_WAITING; task->waitsFor = "sleeps"; - clockWaitForTime(task->id, clockGetLocal()->time + data->milliseconds); mutexRelease(&task->lock); + clockWaitForTime(task->id, clockGetLocal()->time + data->milliseconds); taskingYield(); + INTERRUPTS_RESUME; } void syscallYield(g_task* task, g_syscall_yield* data) @@ -85,12 +87,14 @@ void syscallGetProcessIdForTaskId(g_task* task, g_syscall_get_pid_for_tid* data) void syscallJoin(g_task* task, g_syscall_join* data) { + INTERRUPTS_PAUSE; mutexAcquire(&task->lock); task->status = G_TASK_STATUS_WAITING; task->waitsFor = "sleeps"; - taskingWaitForExit(data->taskId, task->id); mutexRelease(&task->lock); + taskingWaitForExit(data->taskId, task->id); taskingYield(); + INTERRUPTS_RESUME; } void syscallSpawn(g_task* task, g_syscall_spawn* data) @@ -141,12 +145,14 @@ void syscallKill(g_task* task, g_syscall_kill* data) g_task* target = taskingGetById(data->pid); if(target) { + INTERRUPTS_PAUSE; mutexAcquire(&target->lock); data->status = G_KILL_STATUS_SUCCESSFUL; target->process->main->status = G_TASK_STATUS_DEAD; - waitQueueWake(&target->waitersJoin); mutexRelease(&target->lock); + waitQueueWake(&target->waitersJoin); taskingYield(); + INTERRUPTS_RESUME; } else { @@ -156,8 +162,6 @@ void syscallKill(g_task* task, g_syscall_kill* data) void syscallCreateTask(g_task* task, g_syscall_create_task* data) { - mutexAcquire(&task->process->lock); - g_task* newTask = taskingCreateTask((g_virtual_address) data->initialEntry, task->process, task->process->main->securityLevel); if(newTask) @@ -172,8 +176,6 @@ void syscallCreateTask(g_task* task, g_syscall_create_task* data) { data->status = G_CREATE_TASK_STATUS_FAILED; } - - mutexRelease(&task->process->lock); } void syscallGetTaskEntry(g_task* task, g_syscall_get_task_entry* data) diff --git a/kernel/src/kernel/filesystem/filesystem.cpp b/kernel/src/kernel/filesystem/filesystem.cpp index a5ff802b..c72f4c61 100644 --- a/kernel/src/kernel/filesystem/filesystem.cpp +++ b/kernel/src/kernel/filesystem/filesystem.cpp @@ -411,12 +411,14 @@ g_fs_read_status filesystemRead(g_task* task, g_fd fd, uint8_t* buffer, uint64_t panic("%! task %i tried to wait for file %i but delegate didn't provide wait-for-read implementation", "filesytem", task->id, node->id); + INTERRUPTS_PAUSE; mutexAcquire(&task->lock); task->status = G_TASK_STATUS_WAITING; task->waitsFor = "read"; - delegate->waitForRead(task->id, node); mutexRelease(&task->lock); + delegate->waitForRead(task->id, node); taskingYield(); + INTERRUPTS_RESUME; // TODO if(node->nonInterruptible && clockHasTimedOut(task->id)) @@ -503,12 +505,14 @@ g_fs_write_status filesystemWrite(g_task* task, g_fd fd, uint8_t* buffer, uint64 panic("%! task %i tried to wait for file %i but delegate didn't provide wait-for-write implementation", "filesytem", task->id, node->id); + INTERRUPTS_PAUSE; mutexAcquire(&task->lock); task->status = G_TASK_STATUS_WAITING; task->waitsFor = "write"; - delegate->waitForWrite(task->id, node); mutexRelease(&task->lock); + delegate->waitForWrite(task->id, node); taskingYield(); + INTERRUPTS_RESUME; } if(wrote > 0) { diff --git a/kernel/src/kernel/ipc/message.cpp b/kernel/src/kernel/ipc/message.cpp index 83ea6af4..ef28b439 100644 --- a/kernel/src/kernel/ipc/message.cpp +++ b/kernel/src/kernel/ipc/message.cpp @@ -161,17 +161,13 @@ void messageTaskRemoved(g_tid task) void messageWaitForSend(g_tid sender, g_tid receiver) { g_message_queue* queue = _messageGetQueue(receiver); - mutexAcquire(&queue->lock); waitQueueAdd(&queue->waitersSend, sender); - mutexRelease(&queue->lock); } void messageUnwaitForSend(g_tid sender, g_tid receiver) { g_message_queue* queue = _messageGetQueue(receiver); - mutexAcquire(&queue->lock); waitQueueRemove(&queue->waitersSend, sender); - mutexRelease(&queue->lock); } void _messageWakeWaitingReceiver(g_message_queue* queue) diff --git a/kernel/src/kernel/ipc/pipes.cpp b/kernel/src/kernel/ipc/pipes.cpp index e90bdc91..364acd80 100644 --- a/kernel/src/kernel/ipc/pipes.cpp +++ b/kernel/src/kernel/ipc/pipes.cpp @@ -264,9 +264,7 @@ void pipeWaitForRead(g_tid task, g_fs_phys_id pipeId) if(!pipe) return; - mutexAcquire(&pipe->lock); waitQueueAdd(&pipe->waitersRead, task); - mutexRelease(&pipe->lock); } void pipeWaitForWrite(g_tid task, g_fs_phys_id pipeId) @@ -275,7 +273,5 @@ void pipeWaitForWrite(g_tid task, g_fs_phys_id pipeId) if(!pipe) return; - mutexAcquire(&pipe->lock); waitQueueAdd(&pipe->waitersWrite, task); - mutexRelease(&pipe->lock); } diff --git a/kernel/src/kernel/system/interrupts/interrupts.cpp b/kernel/src/kernel/system/interrupts/interrupts.cpp index a9bf07cf..7a3276ec 100644 --- a/kernel/src/kernel/system/interrupts/interrupts.cpp +++ b/kernel/src/kernel/system/interrupts/interrupts.cpp @@ -91,7 +91,7 @@ extern "C" volatile g_processor_state* _interruptHandler(volatile g_processor_st if(irq == 0) // Timer { clockUpdate(); - taskingSchedule(); + taskingSchedule(true); } else { diff --git a/kernel/src/kernel/system/processor/processor.cpp b/kernel/src/kernel/system/processor/processor.cpp index af34456a..553b5d6e 100644 --- a/kernel/src/kernel/system/processor/processor.cpp +++ b/kernel/src/kernel/system/processor/processor.cpp @@ -313,3 +313,8 @@ const uint8_t* processorGetInitialFpuState() { return _processorGetCurrent()->fpu.initialState; } + +bool processorIsBsp() +{ + return processorGetCurrentId() == 0; +} diff --git a/kernel/src/kernel/system/processor/processor.hpp b/kernel/src/kernel/system/processor/processor.hpp index f3258f07..d31385c4 100644 --- a/kernel/src/kernel/system/processor/processor.hpp +++ b/kernel/src/kernel/system/processor/processor.hpp @@ -258,4 +258,9 @@ bool processorHasFeatureReady(g_cpuid_standard_edx_feature feature); */ const uint8_t* processorGetInitialFpuState(); +/** + * @return whether this code is running on the BSP + */ +bool processorIsBsp(); + #endif diff --git a/kernel/src/kernel/tasking/cleanup.cpp b/kernel/src/kernel/tasking/cleanup.cpp index a1d47b2d..b24ac461 100644 --- a/kernel/src/kernel/tasking/cleanup.cpp +++ b/kernel/src/kernel/tasking/cleanup.cpp @@ -73,11 +73,13 @@ void taskingCleanupThread() } // Sleep for some time + INTERRUPTS_PAUSE; mutexAcquire(&self->lock); self->status = G_TASK_STATUS_WAITING; self->waitsFor = "cleanup-sleep"; - clockWaitForTime(self->id, clockGetLocal()->time + 3000); mutexRelease(&self->lock); + clockWaitForTime(self->id, clockGetLocal()->time + 3000); taskingYield(); + INTERRUPTS_RESUME; } } diff --git a/kernel/src/kernel/tasking/scheduler/scheduler_round_robin.cpp b/kernel/src/kernel/tasking/scheduler/scheduler_round_robin.cpp index 66fee15e..0c3af780 100644 --- a/kernel/src/kernel/tasking/scheduler/scheduler_round_robin.cpp +++ b/kernel/src/kernel/tasking/scheduler/scheduler_round_robin.cpp @@ -50,9 +50,8 @@ g_schedule_entry* schedulerGetNextTask(g_tasking_local* local) return entry; } - // // Check if there is a global "preferred task" to do next - // TODO: This causes rare deadlocks... - if(false && preferredTask != G_TID_NONE) + // Check if there is a global "preferred task" to do next + if(preferredTask != G_TID_NONE) { while(entry) { diff --git a/kernel/src/kernel/tasking/tasking.cpp b/kernel/src/kernel/tasking/tasking.cpp index 1f64418b..44d57ca3 100644 --- a/kernel/src/kernel/tasking/tasking.cpp +++ b/kernel/src/kernel/tasking/tasking.cpp @@ -308,7 +308,12 @@ void taskingRestoreState(g_task* task) processorRestoreFpuState(task->fpu.state); } -void taskingSchedule() { schedulerSchedule(taskingGetLocal()); } +void taskingSchedule(bool resetPreference) +{ + if(resetPreference && processorIsBsp()) + schedulerPrefer(G_TID_NONE); + schedulerSchedule(taskingGetLocal()); +} g_process* taskingCreateProcess(g_security_level securityLevel) { @@ -439,7 +444,7 @@ void taskingDestroyTask(g_task* task) void _taskingInitializeTask(g_task* task, g_process* process, g_security_level level) { - if(process->main == 0) + if(process->main == nullptr) task->id = process->id; else task->id = taskingGetNextId(); @@ -510,17 +515,19 @@ g_spawn_result taskingSpawn(g_fd fd, g_security_level securityLevel) mutexAcquire(&caller->lock); caller->status = G_TASK_STATUS_WAITING; caller->waitsFor = "spawn"; + mutexRelease(&caller->lock); waitQueueAdd(&res.process->waitersSpawn, caller->id); taskingAssignBalanced(newTask); - mutexRelease(&caller->lock); while(!newTask->spawnFinished) { + INTERRUPTS_PAUSE; mutexAcquire(&caller->lock); caller->status = G_TASK_STATUS_WAITING; caller->waitsFor = "spawn-rep"; mutexRelease(&caller->lock); taskingYield(); + INTERRUPTS_RESUME; } // Take result @@ -566,12 +573,14 @@ void taskingFinalizeSpawn(g_task* task) taskingStateReset(task, process->spawnArgs->entry, task->securityLevel); waitQueueWake(&process->waitersSpawn); + INTERRUPTS_PAUSE; mutexAcquire(&task->lock); task->status = G_TASK_STATUS_WAITING; task->waitsFor = "wake-after-spawn"; task->spawnFinished = true; mutexRelease(&task->lock); taskingYield(); + INTERRUPTS_RESUME; } void taskingWaitForExit(g_tid joinedTid, g_tid waiter) diff --git a/kernel/src/kernel/tasking/tasking.hpp b/kernel/src/kernel/tasking/tasking.hpp index db6f6ec3..76e2bbe2 100644 --- a/kernel/src/kernel/tasking/tasking.hpp +++ b/kernel/src/kernel/tasking/tasking.hpp @@ -31,8 +31,8 @@ extern g_hashmap* taskGlobalMap; struct g_schedule_entry { - g_task* task; - g_schedule_entry* next; + g_task* task; + g_schedule_entry* next; }; /** @@ -41,42 +41,42 @@ struct g_schedule_entry */ struct g_tasking_local { - g_mutex lock; - uint32_t processor; - - struct - { - /** - * Each first acquire of a global mutex increases the count on this - * processor, each release decreases it. - */ - int globalLockCount; - - /** - * As long as a global mutex is held, interrupts are disabled and this - * flag holds the IF state on the first acquire. On the last release, - * this state is restored. - */ - bool globalLockSetIFAfterRelease; - } locking; - - /** - * Scheduling information for this processor. - */ - struct - { - g_schedule_entry* list; - g_task* current; - - g_task* idleTask; - } scheduling; + g_mutex lock; + uint32_t processor; + + struct + { + /** + * Each first acquire of a global mutex increases the count on this + * processor, each release decreases it. + */ + int globalLockCount; + + /** + * As long as a global mutex is held, interrupts are disabled and this + * flag holds the IF state on the first acquire. On the last release, + * this state is restored. + */ + bool globalLockSetIFAfterRelease; + } locking; + + /** + * Scheduling information for this processor. + */ + struct + { + g_schedule_entry* list; + g_task* current; + + g_task* idleTask; + } scheduling; }; struct g_spawn_result { - g_spawn_status status; - g_process* process; - g_spawn_validation_details validation; + g_spawn_status status; + g_process* process; + g_spawn_validation_details validation; }; /** @@ -183,7 +183,7 @@ void taskingProcessRemoveFromTaskList(g_task* task); /** * Schedules and sets the next task as the current. May only be called during interrupt handling! */ -void taskingSchedule(); +void taskingSchedule(bool resetPreference = false); /** * Saves the state pointer that points to the stored state on the tasks kernel diff --git a/kernel/src/kernel/tasking/user_mutex.cpp b/kernel/src/kernel/tasking/user_mutex.cpp index d2657a54..d76b8b70 100644 --- a/kernel/src/kernel/tasking/user_mutex.cpp +++ b/kernel/src/kernel/tasking/user_mutex.cpp @@ -128,15 +128,17 @@ g_user_mutex_status userMutexAcquire(g_task* task, g_user_mutex mutex, uint64_t break; } + INTERRUPTS_PAUSE; mutexAcquire(&task->lock); task->status = G_TASK_STATUS_WAITING; task->waitsFor = "user-mutex"; - userMutexWaitForAcquire(mutex, task->id); mutexRelease(&task->lock); + userMutexWaitForAcquire(mutex, task->id); mutexRelease(&entry->lock); taskingYield(); + INTERRUPTS_RESUME; } if(useTimeout)