Add condition variables to pico_sync (fixes #1093)

[pguyot]

This PR provides condition variables as companion to mutexes.

It is implemented without any assumption on the number of cores. Like mutexes, condition variables are protected by a spinlock.

When waiting on a condition variable, a core tries to be the waiter (=owner) and when it is, it waits to be signaled. When signaling a condition variable, the caller verifies that there is a waiter, and if there is, sets a boolean to signal it.

There is a trick to avoid holding two spinlocks which was incompatible with RP2350: the condition variable spinlock protects the cv waiter and the broadcast flag only, while the mutex spinlock is used in cond_wait and cond_signal to for the signal. Signaling means locking two spinlocks (or only one if they are the same). This introduces a potential race condition if more than one core try to signal at the same time, which can be solved by only calling cond_signal while mutex is held.

This busy-loop implementation seems to be immune from spurious wakeup.

[alastairpatrick]

I'm just a contributor so please take this with a "pinch of salt" and I suggest waiting for someone with authority before acting on any of this.

First of all, if this is to be called a condition variable module, I think it would be reasonable for an SDK user to expect it to also include a cond_broadcast() or similarly named function that signals all waiting threads. If an RTOS is in use, there could be >1 waiting threads.

I appreciate that implementing cond_broadcast() is a challenge in the context of the SDK, in part because there is nothing resembling a thread control block that could be used to efficiently implement a linked list of threads waiting for a particular condition variable. I have two suggestions.

My first suggestion is a (hopefully) upcoming thread local variable module for the SDK, which essentially provides a thread control block by another name.

My second suggestion is to add a broadcast_count variable to cond_t, which atomically increments whenever cond_broadcast() is called. I've never implemented condition variables this way and I wonder if it's flawed but I think it might give waiting threads enough information to all wake up on broadcast:

typedef struct __packed_aligned
{
    lock_core_t core;
    lock_owner_id_t priority_waiter;  // arbitrary one of N threads that will wake up on cond_signal().
    uint64_t broadcast_count;  // all N threads will wake up when this changes.
    bool signaled;
} cond_t;

Finally, I think there are some issues with condition variables as implemented.

There are three cases where cond_wait() blocks by calling lock_internal_spin_unlock_with_wait():

1. A thread blocks until the cond_t is signalled.
2. A thread blocks until the cond_t has no current waiter.
3. A thread blocks until the mutex_t is released.

I use the term "blocked" here, and in the operating system sense, because that is exactly what will happen when an RTOS is in use: the calling thread will transition to a blocked state. In order to transition the thread out of the blocked state, another thread must call lock_internal_spin_unlock_with_notify().

Of the three, case 1 is covered by the call to lock_internal_spin_unlock_with_notify() in cond_signal() and I believe this case is fully covered. I think there are issues with cases 2 and 3 though.

In case 2, I there's no notification when cond->waiter becomes LOCK_INVALID_OWNER_ID so a thread waiting to become the current waiter might never wake up.

Case 3 is partially covered by the call to lock_internal_spin_unlock_with_notify() in mutex_exit(). However, code in cond_wait() also releases the mutex by bypassing the public mutex API and setting mtx->owner = LOCK_INVALID_OWNER_ID. So, like case 2, a thread waiting for the mutex to be released might never wake up.

[pguyot]

Thank you.

• I wasn't clear about the notify semantics and obviously got it wrong
• I don't need broadcast for my use case, but thought of an implementation slightly different from your suggestion. I'll give it more thought, I like yours of using a counter that is unlikely to overflow. Maybe 32 bits would be sufficient.

[alastairpatrick]

Perhaps it's slightly less efficient but I think cond_wait() is clearer refactored like this. I haven't attempted to fix any of the issues I mentioned above.

void __time_critical_func(cond_wait)(cond_t *cond, mutex_t *mtx) {
    lock_owner_id_t caller = lock_get_caller_owner_id();
    uint32_t save = save_and_disable_interrupts();
    spin_lock_unsafe_blocking(mtx->core.spin_lock);
    assert(lock_is_owner_id_valid(mtx->owner));

    if (mtx->core.spin_lock != cond->core.spin_lock) {
        spin_lock_unsafe_blocking(cond->core.spin_lock);
    }
    
    // Wait to be the waiter first, then wait for the signal.
    if (lock_is_owner_id_valid(cond->waiter)) {
        mtx->owner = LOCK_INVALID_OWNER_ID;
        spin_unlock_unsafe(mtx->core.spin_lock);
        do {
            if (!lock_is_owner_id_valid(cond->waiter)) {
                cond->waiter = caller;
                break;
            }
            lock_internal_spin_unlock_with_wait(&cond->core, save);
            save = spin_lock_blocking(cond->core.spin_lock);
        } while (true);
    } else {
        cond->waiter = caller;
        mtx->owner = LOCK_INVALID_OWNER_ID;
        spin_unlock_unsafe(mtx->core.spin_lock);
    }
    
    // We are the current waiter, now wait for the signal.
    do {
        if (cond->signaled) {
            cond->waiter = LOCK_INVALID_OWNER_ID;
            cond->signaled = false;
            break;
        }
        lock_internal_spin_unlock_with_wait(&cond->core, save);
        save = spin_lock_blocking(cond->core.spin_lock);
    } while (true);
      
    if (mtx->core.spin_lock != cond->core.spin_lock) {
        spin_unlock_unsafe(cond->core.spin_lock);
    }
    
    do {
        if (!lock_is_owner_id_valid(mtx->owner)) {
            mtx->owner = caller;
            spin_unlock(mtx->core.spin_lock, save);
            break;
        }
        lock_internal_spin_unlock_with_wait(&mtx->core, save);
        save = spin_lock_blocking(mtx->core.spin_lock);
    } while (true);
}

[pguyot]

@alastairpatrick Thank you for your feedback. I eventually implemented broadcast as well as timed waits.

[kilograham]

Thanks for the update; I haven't really had a chance to look at this in detail; I do note however that the latest commit uses "_sev()" in some path which is not valid under a RTOS (it is what lock_internal_spin_unlock_with_notify is there for). So hopefully things can be refactored to use that method in some way - it seems like the @alastairpatrick version might do that... sorry busy with other stuff atm, so not really engaging brain fully!

[pguyot]

@kilograham Thank you for this feedback.

The __sev() call was here to solve an issue raised by @alastairpatrick which wasn't solved by his refactorization proposal, as he wrote. I rewrote the code to use lock_internal_spin_unlock_with_notify instead of spin_unlock_unsafe on the paths where __sev() was called.

[pguyot]

@kilograham this was rebased on top of SDK 2.0 and tested on RP2040, RP2350 ARM and RP2350 RISCV.

[pguyot]

@kilograham this has been rebased on top of SDK 2.1.1. Bazel compilation was fixed. Tests have been extended to increase coverage. They didn't pass on Pico 2W as there was a race condition that has been fixed (mutex spinlock was released before condition waiter was set).

[pguyot]

~Marked as draft as further tests seem to point that this could be faulty.~