Introducing `k_threadlock` A conditional low overhead "mutex"

[bjarki-andreasen]

Problem Description

In zephyr, we have drivers and subsystems which may be used pre kernel, post kernel, with and without multithreading. In all of these scenarios, we need thread safety, however, in some of them, thread safety is a given, since we either only have one thread, in cases of pre kernel and no multithreading

This currently leads to drivers and subsystems needing to implement conditional locking themselves, typically only after its discovered that they needed it, as a driver's init level is changed for example.

The logic looks like this:

#if !CONFIG_NO_MULTITHREADING
if (!k_is_pre_kernel()) {
        (void)k_sem_take(&mysem, K_FOREVER);
}
#endif

...

#if !CONFIG_NO_MULTITHREADING
if (!k_is_pre_kernel()) {
        k_sem_give(&mysem);
}
#endif

We also have the "ISR" thread in any case, which we typically also want to sync with, so an adaptive solution to what follows is proposed as well :)

#if !CONFIG_NO_MULTITHREADING
static K_SEM_DEFINE(mysem, 0, 1);
#else
static atomic_t myatom = ATOM_INIT(0);
#endif

void myisr()
{
#if !CONFIG_NO_MULTITHREADING
        k_sem_give(&mysem);
#else
        atomic_set_bit(&myatom, 0);
#endif
}

void myapi()
{
#if !CONFIG_NO_MULTITHREADING
        k_sem_take(&mysem, K_FOREVER);
#else
        !WAIT_FOR(atomic_test_bit(&myatom, 0), 100000, NULL)
#endif
}

Proposed Change (Summary)

This could preferable be packaged into a new feature: The struct k_threadlock

which would make the code look like:

(void)k_threadlock_lock(&mythreadlock, K_FOREVER);

...

k_treadlock_unlock(&mythreadlock)

which would automatically adapt to the build configurations and kernel stage.

As inspired by @mathieuchopstm we could also consider a primitive which automatically adapts to waiting on something from an ISR using threads or polling, namely a k_threadawait() which would look like this:

#if !CONFIG_NO_MULTITHREADING
static K_SEM_DEFINE(mysem, 0, 1);
#else
static atomic_t myatom = ATOM_INIT(0);
#endif

void myisr()
{
#if !CONFIG_NO_MULTITHREADING
        k_sem_give(&mysem);
#else
        atomic_set_bit(&myatom, 0);
#endif
}

void myapi()
{
#if !CONFIG_NO_MULTITHREADING
        k_sem_take(&mysem, K_FOREVER);
#else
        !WAIT_FOR(atomic_test_bit(&myatom, 0), 100000, NULL)
#endif
}

with

static struct k_threadawait mythreadawait;

void myisr()
{
        k_threadawait_signal(&mythreadawait);
}

void myapi()
{
        k_threadawait_await(&mythreadawait, K_FOREVER);
}

Proposed Change (Detailed)

Simply package a k_sem and use it as a binary sem for locking, if required (!CONFIG_NO_MULTITHREADING). cheap and simple :)

Dependencies

No response

Concerns and Unresolved Questions

No response

Alternatives Considered

What we do now, technically a bit more efficient since we can avoid the check of k_is_pre_kernel() in some cases.

[mathieuchopstm]

From a quick glance at the tree, there are definitely some potential users for this. But wouldn't it be more useful to make k_sem available - in a reduced form - when !CONFIG_MULTITHREADING?

As far as I can tell, there are two main patterns linked with !CONFIG_MULTITHREADING:

1. driver instance mutexes, that ensure only a single thread uses a device at a given time, are compiled out
   1. ==> makes sense, they are not useful/necessary if there's a single thread
2. synchronization with HW event (ISR)
   1. usually done using a k_sem, but some drivers have a fallback when !CONFIG_MULTITHREADING
      1. home-made atomic_t/volatile u32-based semaphores, most of the time

By making k_sem available when !CONFIG_MULTITHREADING:

1. Compiling out sem-as-mutex would no longer be needed (though still wiser to do so)
2. Hand-rolled semaphores would no longer be needed
   1. Avoids duplicated implementation of the same thing all around the tree
   2. Bonus: makes """all""" drivers work with !CONFIG_MULTITHREADING for "free"
      1. (Wishful thinking, but I think a good amount of drivers would work mostly fine, and migrating others would be simpler than it is today)

(I can think of a third usage pattern for k_sem: synchronization with a worker/msgq thread. But if !CONFIG_MULTITHREADING, I'm assuming that creating threads / using the msgq would not work, so I don't think it's something to consider)

Simply package a k_spinlock and use it as a binary sem for locking, if required (!CONFIG_NO_MULTITHREADING). cheap and simple :)

There are two issues with this:

• The spinlock is empty if !CONFIG_SMP, so it has no way to signal to a waiter that the lock has been free'ed. This makes sense because...
• The spinlock suspends interrupts! - attempting to replace homemade-sema with a k_threadlock would instantly deadlock for usecase (2), as the ISR supposed to unlock the object will never run

This brings me back to my previous point: a single-threaded k_sem would not exhibit these issues. (It would be like these hand-rolled semaphores, i.e., a mere atomic_t sem_counter)

[bjarki-andreasen]

From a quick glance at the tree, there are definitely some potential users for this. But wouldn't it be more useful to make k_sem available - in a reduced form - when !CONFIG_MULTITHREADING?

As far as I can tell, there are two main patterns linked with !CONFIG_MULTITHREADING:

1. driver instance mutexes, that ensure only a single thread uses a device at a given time, are compiled out

   1. ==> makes sense, they are not useful/necessary if there's a single thread

2. synchronization with HW event (ISR)

   1. usually done using a k_sem, but some drivers have a fallback when !CONFIG_MULTITHREADING

      1. home-made atomic_t/volatile u32-based semaphores, most of the time

By making k_sem available when !CONFIG_MULTITHREADING:

1. Compiling out sem-as-mutex would no longer be needed (though still wiser to do so)

2. Hand-rolled semaphores would no longer be needed

   1. Avoids duplicated implementation of the same thing all around the tree

   2. Bonus: makes """all""" drivers work with !CONFIG_MULTITHREADING for "free"

      1. (Wishful thinking, but I think a good amount of drivers would work mostly fine, and migrating others would be simpler than it is today)

(I can think of a third usage pattern for k_sem: synchronization with a worker/msgq thread. But if !CONFIG_MULTITHREADING, I'm assuming that creating threads / using the msgq would not work, so I don't think it's something to consider)

Simply package a k_spinlock and use it as a binary sem for locking, if required (!CONFIG_NO_MULTITHREADING). cheap and simple :)

There are two issues with this:

• The spinlock is empty if !CONFIG_SMP, so it has no way to signal to a waiter that the lock has been free'ed. This makes sense because...
• The spinlock suspends interrupts! - attempting to replace homemade-sema with a k_threadlock would instantly deadlock for usecase (2), as the ISR supposed to unlock the object will never run

This brings me back to my previous point: a single-threaded k_sem would not exhibit these issues. (It would be like these hand-rolled semaphores, i.e., a mere atomic_t sem_counter)

First, sorry, I meant "package k_sem, and use it as a binary semaphore" (max count = 1) :) spinlock is indeed solving a very different issue :)

For synchronization with ISRs, the threadlock would not be of use, its meant only for locking access to a resource and expected to be unlocked by the same thread in the same scope, like a "lockguard" basically.

Sync from ISRs would need to be replaced with spinning and polling a value to preserve the "same" behavior as waiting on a sem to be signaled from ISR, we could consider a new primitive for that as well, k_threadsignal or something, also based on a k_sem for multithreading, atomic and spinning for no multithreading :)

[mathieuchopstm]

First, sorry, I meant "package k_sem, and use it as a binary semaphore" (max count = 1) :) spinlock is indeed solving a very different issue :)

Ok, that makes more sense 🙂

For synchronization with ISRs, the threadlock would not be of use, its meant only for locking access to a resource and expected to be unlocked by the same thread in the same scope, like a "lockguard" basically.

Sync from ISRs would need to be replaced with spinning and polling a value to preserve the "same" behavior as waiting on a sem to be signaled from ISR, we could consider a new primitive for that as well, k_threadsignal or something, also based on a k_sem for multithreading, atomic
and spinning for no multithreading :)

I understand the idea, and I can see good reasons for creating new object types... but in practice, as you say it yourself, these will not be truly new objects, only wrappers around k_sem (with a fallback for !CONFIG_MULTITHREADING)... so why not simply add the fallback to the original k_sem? (similarly, k_threadlock could be a k_mutex, if recursive acquisition is a wanted feature... or a k_sem-with-count=1 otherwise, as done in most places)

As for "removal" (really, hiding) of the !k_is_pre_kernel() check, mini-functions that perform the check could be provided as syntactical sugar. (My assumption is that blocking in pre-kernel is not allowed... but are the primitives already working at that stage otherwise? If we can guarantee that calls to k_sem_take/k_sem_give that do not block work in pre-kernel, the check would not even be necessary)

[bjarki-andreasen]

First, sorry, I meant "package k_sem, and use it as a binary semaphore" (max count = 1) :) spinlock is indeed solving a very different issue :)

Ok, that makes more sense 🙂

For synchronization with ISRs, the threadlock would not be of use, its meant only for locking access to a resource and expected to be unlocked by the same thread in the same scope, like a "lockguard" basically.
Sync from ISRs would need to be replaced with spinning and polling a value to preserve the "same" behavior as waiting on a sem to be signaled from ISR, we could consider a new primitive for that as well, k_threadsignal or something, also based on a k_sem for multithreading, atomic
and spinning for no multithreading :)

I understand the idea, and I can see good reasons for creating new object types... but in practice, as you say it yourself, these will not be truly new objects, only wrappers around k_sem (with a fallback for !CONFIG_MULTITHREADING)... so why not simply add the fallback to the original k_sem? (similarly, k_threadlock could be a k_mutex, if recursive acquisition is a wanted feature... or a k_sem-with-count=1 otherwise, as done in most places)

As for "removal" (really, hiding) of the !k_is_pre_kernel() check, mini-functions that perform the check could be provided as syntactical sugar. (My assumption is that blocking in pre-kernel is not allowed... but are the primitives already working at that stage otherwise? If we can guarantee that calls to k_sem_take/k_sem_give that do not block work in pre-kernel, the check would not even be necessary)

I see, I think a k_sem could be used for both signalling and mutual exclusion, if we replaced its insides with an atom and a while loop if NO_MUTLITHREADING, but, it would take up more size than necessary if used only for mutual exclusion since we will be including an atomic with no purpose, it will also be slower since we will have to check the atom on every k_sem_take if NO_MULTITHREADING, whereas k_threadlock would be completely optimized away, no size, no OP :)

I am okay with both approaches honestly, I like the explicitiy of k_threadlock() and k_threadawait(), but I'm super okay with the "for free" implicit sem behavior as well