Make it possible to upgrade a lock for destruction without holding it. The ability to upgrade for destruction without holding the lock is useful for cleaning up from the error handler. Add various tests to detect regressions in the `unlock` function. Simplify some tests to reduce code size. Increase the timers in `test_destruct_lock_wake_up` as the test is inherently a bit racy. Signed-off-by: Hugo Lefeuvre <hugo.lefeuvre@scisemi.com>
diff --git a/sdk/include/locks.h b/sdk/include/locks.h index 676e2d2..1dbab8c 100644 --- a/sdk/include/locks.h +++ b/sdk/include/locks.h
@@ -135,6 +135,10 @@ * currently attempting to acquire the lock will wake and fail to acquire the * lock. This should be called before deallocating an object that contains a * lock. + * + * Note that callers do not need to hold the lock; the ability to upgrade for + * destruction without holding the lock is useful for cleaning up from the + * error handler. */ void __cheri_libcall flaglock_upgrade_for_destruction(struct FlagLockState *lock);
diff --git a/sdk/lib/locks/locks.cc b/sdk/lib/locks/locks.cc index f1081d8..5cd407c 100644 --- a/sdk/lib/locks/locks.cc +++ b/sdk/lib/locks/locks.cc
@@ -128,7 +128,9 @@ */ void unlock() { - auto old = lockWord.exchange(Flag::Unlocked); + // Atomically empty all bits of the lockword except the + // destruct mode bit which we want to preserve. + auto old = lockWord.fetch_and(Flag::LockedInDestructMode); // Assert that the locked is not already locked, and // that the caller holds the lock. This is only @@ -152,38 +154,28 @@ /** * Set the destruction bit in the flag lock word and wake - * waiters. Assumes that the lock is held by the caller. - * - * Note: This does not check that the lock is owned by the - * calling thread. + * waiters. Callers do not need to hold the lock. */ void upgrade_for_destruction() { Debug::log("Setting {} for destruction", &lockWord); - // Assert that the caller holds the lock. This is only - // compiled-in with debugging enabled for locking. - Debug::Assert( - (lockWord & 0x0000ffff) == thread_id_get(), - "Calling thread {} does not hold the lock on {} (owner: {})", - thread_id_get(), - &lockWord, - lockWord & 0x0000ffff); - - // Set the destruction bit. + // Atomically set the destruction bit. lockWord |= Flag::LockedInDestructMode; // Wake up waiters. // There should not be any 'missed wake' because any // thread calling lock() concurrently will either: - // - successfully CAS, pass the the -ENOENT check, reach + // - successfully CAS and take the lock (i.e., not got + // to sleep) + // - fail the CAS, and fail the -ENOENT check; + // - fail the CAS, pass the the -ENOENT check, reach // the futex `wait` before we set the // `Flag::LockedInDestructMode` bit, and be woken up // by our `notify_all`; - // - fail the -ENOENT check; - // - miss the `Flag::LockedInDestructMode` bit, pass the - // -ENOENT check but fail their CAS to add the waiters - // bit, retry, and fail the -ENOENT check; + // - miss the `Flag::LockedInDestructMode` bit, fail + // the CAS, pass the -ENOENT check, fail the CAS to add + // the waiters bit, retry, and fail the -ENOENT check; // - or miss the `Flag::LockedInDestructMode` bit, pass // the -ENOENT check but fail their CAS in the futex // `wait` call, retry (because the futex returns 0 in
diff --git a/tests/locks-test.cc b/tests/locks-test.cc index 8401b0d..ffd8da2 100644 --- a/tests/locks-test.cc +++ b/tests/locks-test.cc
@@ -23,14 +23,12 @@ cheriot::atomic<int> counter; /** - * Test that a lock meets the minimum requirements: it actually provides - * mutual exclusion. + * Test that a lock actually provides mutual exclusion. */ template<typename Lock> void test_lock(Lock &lock) { modified = false; - debug_log("Acquiring lock in {}", __PRETTY_FUNCTION__); { LockGuard g{lock}; async([&]() { @@ -83,7 +81,7 @@ // making progress if this test fails. // The generous timer makes sure that we reach the // modified == true assert before the timeout. - Timeout t2{20}; + Timeout t2{25}; TEST(lock.try_lock(&t2) == false, "Lock acquisition should not succeed!"); @@ -99,7 +97,7 @@ lock.upgrade_for_destruction(); // Give the waiter a chance to wake up - sleep(1); + sleep(5); // Check that the destruction mode woke up the waiters TEST(modified == true, "Destruction mode did not wake up waiters!"); @@ -113,6 +111,39 @@ } /** + * Test that unlocking a flag lock works: once a lock is released, it + * can be re-acquired. This will fail if the unlock function does not + * properly clear bits. + */ + void test_flaglock_unlock() + { + // Check for the case where a lock was released with the + // waiters bit set. Do not check the simple case without + // waiters as this is covered by other tests (at least + // `test_lock`). + Timeout t{5}; + TEST(flagLock.try_lock(&t), "Failed to acquire uncontended lock"); + counter = 0; + async([&]() { + Timeout t2{1}; + TEST(flagLock.try_lock(&t2) == false, + "Lock acquisition should not succeed!"); + counter++; + }); + do + { + debug_log("Other thread not finished, yielding"); + sleep(1); + } while (counter.load() == 0); + flagLock.unlock(); + + TEST(flagLock.try_lock(&t), + "Failed to acquire uncontended lock after unlock when the waiters " + "bit was previously set"); + flagLock.unlock(); + } + + /** * Test that a lock with the destruction bit set cannot be acquired * anymore. * @@ -123,6 +154,36 @@ static FlagLockState flagLockState; static FlagLockState priorityFlagLockState; + // Upgrade the lock to destruction mode. No need to acquire the + // lock for that. + flaglock_upgrade_for_destruction(&flagLockState); + + // Check that we now fail to grab the lock with the right error + Timeout t{5}; + TEST(flaglock_trylock(&t, &flagLockState) == -ENOENT, + "Acquiring the lock did not fail with -ENOENT although it is in " + "destruction mode"); + + // Now, do the same tests with the priority inheriting flag lock + flaglock_upgrade_for_destruction(&priorityFlagLockState); + + TEST(flaglock_priority_inheriting_trylock(&t, &priorityFlagLockState) == + -ENOENT, + "Acquiring the lock did not fail with -ENOENT although it is in " + "destruction mode"); + } + + /** + * Test that the destruction bit is preserved when unlocking. + * + * Note: here, plug at the C API to be able to check C error codes. + */ + void test_destruct_flag_lock_unlock() + { + // Only test without priority inheriting for code size reasons, + // but both should behave identically + static FlagLockState flagLockState; + Timeout t{5}; int ret = flaglock_trylock(&t, &flagLockState); TEST(ret == 0, "Flag lock trylock failed with error {}", ret); @@ -130,23 +191,13 @@ // Upgrade the lock to destruction mode flaglock_upgrade_for_destruction(&flagLockState); - // Check that we now fail to grab the lock with the right error + // Now unlock the lock + flaglock_unlock(&flagLockState); + + // Check that the destruction bit is still set (by trying to + // grab the lock again - it should fail with -ENOENT) TEST(flaglock_trylock(&t, &flagLockState) == -ENOENT, - "Acquiring the lock did not fail with -ENOENT although it is in " - "destruction mode"); - - // Now, do the same tests with the priority inheriting flag lock - ret = flaglock_priority_inheriting_trylock(&t, &priorityFlagLockState); - TEST(ret == 0, - "Priority inheriting flag lock trylock failed with error {}", - ret); - - flaglock_upgrade_for_destruction(&priorityFlagLockState); - - TEST(flaglock_priority_inheriting_trylock(&t, &priorityFlagLockState) == - -ENOENT, - "Acquiring the lock did not fail with -ENOENT although it is in " - "destruction mode"); + "Unlocking unsets the destruction bit of flag lock"); } void test_recursive_mutex() @@ -211,6 +262,7 @@ */ void test_ticket_lock_ordering() { + counter = 0; debug_log("Starting ticket-lock ordering tests"); { LockGuard g{ticketLock}; @@ -311,11 +363,13 @@ test_lock(flagLock); test_lock(flagLockPriorityInherited); test_lock(ticketLock); + test_flaglock_unlock(); test_trylock(flagLock); test_trylock(flagLockPriorityInherited); test_destruct_lock_wake_up(flagLock); test_destruct_lock_wake_up(flagLockPriorityInherited); test_destruct_flag_lock_acquire(); + test_destruct_flag_lock_unlock(); test_ticket_lock_ordering(); test_ticket_lock_overflow(); test_recursive_mutex();