Add condition variables to pico_sync (fixes #1093) #1101

2022-11-16T17:15:18-06:00

pguyot commented

2022-11-16 17:15:18 -06:00

(Migrated from github.com)

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.

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 commented

2022-11-17 13:14:55 -06:00

(Migrated from github.com)

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():

A thread blocks until the cond_t is signalled.
A thread blocks until the cond_t has no current waiter.
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.

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](https://github.com/raspberrypi/pico-sdk/pull/1019) 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.

👍 1

pguyot commented

2022-11-17 13:27:10 -06:00

(Migrated from github.com)

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.

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 commented

2022-11-17 14:00:11 -06:00

(Migrated from github.com)

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);
}

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 commented

2023-04-09 10:20:39 -05:00

(Migrated from github.com)

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

kilograham commented

2023-04-09 11:03:19 -05:00

(Migrated from github.com)

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!

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 commented

2023-04-09 11:59:32 -05:00

(Migrated from github.com)

@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.

@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 commented

2024-09-21 01:08:15 -05:00

(Migrated from github.com)

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

👍 1

pguyot commented

2025-05-10 23:34:36 -05:00

(Migrated from github.com)

@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 commented

2025-05-11 04:15:21 -05:00

(Migrated from github.com)

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

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

lurch (Migrated from github.com) reviewed

2025-05-11 04:35:21 -05:00

src/common/pico_sync/include/pico/cond.h

					
				@ -0,0 +1,128 @@

				/*

"In this implementation, it is not mandatory." - huh, what is not mandatory? 😕

"In this implementation, it is not mandatory." - huh, **what** is not mandatory? :confused:

pguyot (Migrated from github.com) reviewed

2025-05-11 04:37:21 -05:00

src/common/pico_sync/include/pico/cond.h

					
				@ -0,0 +1,128 @@

				/*

Thanks. I will rephrase. What is not mandatory is that the mutex is held when the condition variable is signaled.

pguyot (Migrated from github.com) reviewed

2025-05-12 02:24:44 -05:00

src/common/pico_sync/cond.c

					
				@ -0,0 +1,163 @@

				/*

broadcast logic was lost somehow.

This pull request has changes conflicting with the target branch.

test/CMakeLists.txt
tools/bazel_build.py

View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Merge commit title

Merge pull request 'Add condition variables to pico_sync (fixes #1093)' (#1101) from pguyot/w46/condition-variables into develop

Merge commit body

Reviewed-on: https://git.richey.dev/gh-raspberrypi/pico-sdk/pulls/1101

Merge pull request 'Add condition variables to pico_sync (fixes #1093)' (#1101) from pguyot/w46/condition-variables into develop

Reviewed-on: https://git.richey.dev/gh-raspberrypi/pico-sdk/pulls/1101

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'pguyot/w46/condition-variables:pguyot/w46/condition-variables'

git switch 'pguyot/w46/condition-variables'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'

git merge --no-ff 'pguyot/w46/condition-variables'

git switch 'pguyot/w46/condition-variables'

git rebase 'develop'

git switch 'develop'

git merge --ff-only 'pguyot/w46/condition-variables'

git switch 'pguyot/w46/condition-variables'

git rebase 'develop'

git switch 'develop'

git merge --no-ff 'pguyot/w46/condition-variables'

git switch 'develop'

git merge --squash 'pguyot/w46/condition-variables'