From 959d9cd069d3b5e064b51c49dae5b9987b3ecfe5 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Thu, 21 Sep 2023 19:18:48 -0400 Subject: [PATCH] kernel: Cleanups to the XSI message queue & semaphore implementations. * Remove unused/unneeded parameters to Dequeue. * Make use of StackOrHeapArray. * Reorder syscall-entry checks for efficiency. * Inline the unlock-block method and unset variables in the process. * Reorder code for clarity and to reduce indentation. --- src/system/kernel/posix/xsi_message_queue.cpp | 6 +- src/system/kernel/posix/xsi_semaphore.cpp | 135 ++++++++---------- 2 files changed, 66 insertions(+), 75 deletions(-) diff --git a/src/system/kernel/posix/xsi_message_queue.cpp b/src/system/kernel/posix/xsi_message_queue.cpp index cffae6dd9c..299ad382cf 100644 --- a/src/system/kernel/posix/xsi_message_queue.cpp +++ b/src/system/kernel/posix/xsi_message_queue.cpp @@ -123,7 +123,7 @@ public: fMessageQueue.msg_ctime = (time_t)real_time_clock(); } - void Dequeue(ConditionVariableEntry *queueEntry, bool waitForMessage) + void Dequeue(ConditionVariableEntry *queueEntry) { queueEntry->Wait(B_RELATIVE_TIMEOUT, 0); } @@ -733,7 +733,7 @@ _user_xsi_msgrcv(int messageQueueID, void *messagePointer, TRACE(("xsi_msgrcv: thread %d got interrupted while " "waiting on message queue %d\n", (int)thread_get_current_thread_id(), messageQueueID)); - messageQueue->Dequeue(&queueEntry, /* waitForMessage */ true); + messageQueue->Dequeue(&queueEntry); return EINTR; } else { messageQueueLocker.Lock(); @@ -841,7 +841,7 @@ _user_xsi_msgsnd(int messageQueueID, const void *messagePointer, TRACE(("xsi_msgsnd: thread %d got interrupted while " "waiting on message queue %d\n", (int)thread_get_current_thread_id(), messageQueueID)); - messageQueue->Dequeue(&queueEntry, /* waitForMessage */ false); + messageQueue->Dequeue(&queueEntry); delete message; notSent = false; result = EINTR; diff --git a/src/system/kernel/posix/xsi_semaphore.cpp b/src/system/kernel/posix/xsi_semaphore.cpp index c32d9382fa..b9c69d4163 100644 --- a/src/system/kernel/posix/xsi_semaphore.cpp +++ b/src/system/kernel/posix/xsi_semaphore.cpp @@ -23,6 +23,7 @@ #include #include #include +#include //#define TRACE_XSI_SEM @@ -125,14 +126,7 @@ public: } } - status_t BlockAndUnlock(ConditionVariableEntry *queueEntry, MutexLocker *setLocker) - { - // Unlock the set before blocking - setLocker->Unlock(); - return queueEntry->Wait(B_CAN_INTERRUPT); - } - - void Dequeue(ConditionVariableEntry *queueEntry, bool waitForZero) + static void Dequeue(ConditionVariableEntry *queueEntry) { queueEntry->Wait(B_RELATIVE_TIMEOUT, 0); } @@ -696,20 +690,7 @@ _user_xsi_semget(key_t key, int numberOfSemaphores, int flags) // Check if key already exist, if it does it already has a semaphore // set associated with it ipcKey = sIpcHashTable.Lookup(key); - if (ipcKey == NULL) { - // The ipc key does not exist. Create it and add it to the system - if (!(flags & IPC_CREAT)) { - TRACE(("xsi_semget: key %d does not exist, but the " - "caller did not ask for creation\n",(int)key)); - return ENOENT; - } - ipcKey = new(std::nothrow) Ipc(key); - if (ipcKey == NULL) { - TRACE_ERROR(("xsi_semget: failed to create new Ipc object " - "for key %d\n", (int)key)); - return ENOMEM; - } - } else { + if (ipcKey != NULL) { // The IPC key exist and it already has a semaphore if ((flags & IPC_CREAT) && (flags & IPC_EXCL)) { TRACE(("xsi_semget: key %d already exist\n", (int)key)); @@ -740,9 +721,22 @@ _user_xsi_semget(key_t key, int numberOfSemaphores, int flags) return semaphoreSet->ID(); } + + // The ipc key does not exist. Create it and add it to the system + if (!(flags & IPC_CREAT)) { + TRACE(("xsi_semget: key %d does not exist, but the " + "caller did not ask for creation\n",(int)key)); + return ENOENT; + } + ipcKey = new(std::nothrow) Ipc(key); + if (ipcKey == NULL) { + TRACE_ERROR(("xsi_semget: failed to create new Ipc object " + "for key %d\n", (int)key)); + return ENOMEM; + } } - // Create a new sempahore set for this key + // Create a new semaphore set for this key if (numberOfSemaphores <= 0 || numberOfSemaphores >= MAX_XSI_SEMS_PER_TEAM) { TRACE_ERROR(("xsi_semget: numberOfSemaphores out of range\n")); @@ -820,16 +814,10 @@ _user_xsi_semctl(int semaphoreID, int semaphoreNumber, int command, // the command it's not IPC_RMID, this prevents undesidered // situation from happening while (hopefully) improving the // concurrency. - MutexLocker setLocker; + MutexLocker setLocker(semaphoreSet->Lock()); if (command != IPC_RMID) { - setLocker.SetTo(&semaphoreSet->Lock(), false); setHashLocker.Unlock(); ipcHashLocker.Unlock(); - } else { - // We are about to delete the set along with its mutex, so - // we can't use the MutexLocker class, as the mutex itself - // won't exist on function exit - mutex_lock(&semaphoreSet->Lock()); } int result = 0; @@ -1015,6 +1003,7 @@ _user_xsi_semctl(int semaphoreID, int semaphoreNumber, int command, delete entry; } + setLocker.Detach(); delete semaphoreSet; return 0; } @@ -1033,6 +1022,16 @@ _user_xsi_semop(int semaphoreID, struct sembuf *ops, size_t numOps) { TRACE(("xsi_semop: semaphoreID = %d, ops = %p, numOps = %ld\n", semaphoreID, ops, numOps)); + + if (!IS_USER_ADDRESS(ops)) { + TRACE(("xsi_semop: sembuf address is not valid\n")); + return B_BAD_ADDRESS; + } + if (numOps < 0 || numOps >= MAX_XSI_SEMS_PER_TEAM) { + TRACE(("xsi_semop: numOps out of range\n")); + return EINVAL; + } + MutexLocker setHashLocker(sXsiSemaphoreSetLock); XsiSemaphoreSet *semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID); if (semaphoreSet == NULL) { @@ -1043,23 +1042,11 @@ _user_xsi_semop(int semaphoreID, struct sembuf *ops, size_t numOps) MutexLocker setLocker(semaphoreSet->Lock()); setHashLocker.Unlock(); - if (!IS_USER_ADDRESS(ops)) { - TRACE(("xsi_semop: sembuf address is not valid\n")); - return B_BAD_ADDRESS; - } - - if (numOps < 0 || numOps >= MAX_XSI_SEMS_PER_TEAM) { - TRACE(("xsi_semop: numOps out of range\n")); - return EINVAL; - } - - struct sembuf *operations - = (struct sembuf *)malloc(sizeof(struct sembuf) * numOps); - if (operations == NULL) { + BStackOrHeapArray operations(numOps); + if (!operations.IsValid()) { TRACE_ERROR(("xsi_semop: failed to allocate sembuf struct\n")); return B_NO_MEMORY; } - MemoryDeleter operationsDeleter(operations); if (user_memcpy(operations, ops, (sizeof(struct sembuf) * numOps)) != B_OK) { @@ -1077,7 +1064,7 @@ _user_xsi_semop(int semaphoreID, struct sembuf *ops, size_t numOps) status_t result = 0; while (notDone) { XsiSemaphore *semaphore = NULL; - short numberOfSemaphores = semaphoreSet->NumberOfSemaphores(); + const short numberOfSemaphores = semaphoreSet->NumberOfSemaphores(); bool goToSleep = false; uint32 i = 0; @@ -1142,10 +1129,13 @@ _user_xsi_semop(int semaphoreID, struct sembuf *ops, size_t numOps) ConditionVariableEntry queueEntry; semaphore->Enqueue(&queueEntry, waitOnZero); - uint32 sequenceNumber = semaphoreSet->SequenceNumber(); + const uint32 sequenceNumber = semaphoreSet->SequenceNumber(); TRACE(("xsi_semop: thread %d going to sleep\n", (int)thread->id)); - result = semaphore->BlockAndUnlock(&queueEntry, &setLocker); + setLocker.Unlock(); + semaphoreSet = NULL; + semaphore = NULL; + result = queueEntry.Wait(B_CAN_INTERRUPT); TRACE(("xsi_semop: thread %d back to life\n", (int)thread->id)); // We are back to life. Find out why! @@ -1163,7 +1153,7 @@ _user_xsi_semop(int semaphoreID, struct sembuf *ops, size_t numOps) TRACE(("xsi_semop: thread %d got interrupted while " "waiting on semaphore set id %d\n", (int)thread_get_current_thread_id(), semaphoreID)); - semaphore->Dequeue(&queueEntry, waitOnZero); + XsiSemaphore::Dequeue(&queueEntry); result = EINTR; notDone = false; } else { @@ -1176,33 +1166,34 @@ _user_xsi_semop(int semaphoreID, struct sembuf *ops, size_t numOps) TRACE(("xsi_semop: semaphore acquired succesfully\n")); // We acquired the semaphore, now records the sem_undo // requests - XsiSemaphore *semaphore = NULL; - uint32 i = 0; - for (; i < numOps; i++) { + for (uint32 i = 0; i < numOps; i++) { + if ((operations[i].sem_flg & SEM_UNDO) == 0) + continue; + short semaphoreNumber = operations[i].sem_num; - semaphore = semaphoreSet->Semaphore(semaphoreNumber); + XsiSemaphore *semaphore = semaphoreSet->Semaphore(semaphoreNumber); short operation = operations[i].sem_op; - if (operations[i].sem_flg & SEM_UNDO) - if (semaphoreSet->RecordUndo(semaphoreNumber, operation) - != B_OK) { - // Unlikely scenario, but we might get here. - // Undo everything! - // Start with semaphore operations - for (uint32 j = 0; j < numOps; j++) { - short semaphoreNumber = operations[j].sem_num; - semaphore = semaphoreSet->Semaphore(semaphoreNumber); - short operation = operations[j].sem_op; - if (operation != 0) - semaphore->Revert(operation); - } - // Remove all previously registered sem_undo request - for (uint32 j = 0; j < i; j++) { - if (operations[j].sem_flg & SEM_UNDO) - semaphoreSet->RevertUndo(operations[j].sem_num, - operations[j].sem_op); - } - result = ENOSPC; + + if (semaphoreSet->RecordUndo(semaphoreNumber, operation) != B_OK) { + // Unlikely scenario, but we might get here. + // Undo everything! + // Start with semaphore operations + for (uint32 j = 0; j < numOps; j++) { + short semaphoreNumber = operations[j].sem_num; + semaphore = semaphoreSet->Semaphore(semaphoreNumber); + short operation = operations[j].sem_op; + if (operation != 0) + semaphore->Revert(operation); } + // Remove all previously registered sem_undo request + for (uint32 j = 0; j < i; j++) { + if (operations[j].sem_flg & SEM_UNDO) { + semaphoreSet->RevertUndo(operations[j].sem_num, + operations[j].sem_op); + } + } + result = ENOSPC; + } } } }