Enable mutex functionality in nxsem #16194

jlaitine · 2025-04-11T12:52:06Z

Summary

This enables mutex functionality in the nxsem code, enabling wait/post atomic fast paths, without needs for syscall, also when priority inheritance is enabled for mutexes.

The basics of the implementations are as follows:

For mutexes, replace semaphore count with mutex holder, which consits of 1 bit for mutex internal locking and 31 bits for task PID
Instead of manipulating the count with atomic instructions, manipulate the mutex holder
Instead of allocating and storing the holder structure for the task's list for priority inheritance, do it at the time when task blocks on the mutex. Since there can be only one running holder for a mutex, this can be done.

Doing this allows cleaning up the nxmutex interface to be a very thin wrapper on top of nxsem (e.g. removing the extra holder variable inside of nxmutex. Plain nxmutex could be just a typedeffed nxsem). This is left for future PRs.

This also improves performance significantly in SMP and in CONFIG_BUILD_KERNEL / CONFIG_BUILD_PROTECTED targets, since most of the time there is no need for syscall to the kernel when a mutex is taken/posted.

I am putting this as a draft for now and will do some more testing still on qemu targets

Impact

This has no functional impact. For the performance, on my real application the speed improvements on RISC-V are as follows in terms of CPU utilization:

RISCV 64bit (MFPS SMP, 4 harts, CONFIG_BUILD_KERNEL): CPU usage: 36 % -> 25 %
RISCV 64bit (MFPS SMP, 4 harts, CONFIG_BUILD_FLAT): CPU usage: 12% -> 9%

Testing

Real hardware:

Custom HW, mpfs (risc-v 64-bit): non-SMP and SMP 4 harts, both with CONFIG_BUILD_FLAT and CONFIG_BUILD_KERNEL.
Custom HW, i.MX93: Single core CONFIG_BUILD_FLAT and CONFIG_BUILD_KERNEL

nuttxpr · 2025-04-11T13:04:44Z

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it's marked as a draft and needs more testing. The summary clearly explains the "why," "what," and "how" of the changes. The impact section addresses key areas, including performance improvements and build configurations tested. The testing section provides information about the hardware and configurations used, though the logs are missing. The lack of "before" and "after" logs is the primary reason it doesn't fully meet the requirements yet. Adding those logs, completing testing on qemu, and removing the draft status will complete the PR.

jlaitine · 2025-04-12T05:37:59Z

This is an initial push, from which I forgot to squash one patch, so it is still broken when priority inheritance is disabled (all the testing I did was with prio inheritance enabled)

So please don't review yet, I will still fix it and add some cleanups.

This is just a FYI if someone wants to get a preliminary view ;)

include/nuttx/mutex.h

include/semaphore.h

include/nuttx/semaphore.h

jlaitine · 2025-04-15T06:38:54Z

Re-tested this, and I believe it is now ready for review. I disagreed on chaning the mholder into int32, so I didn't do that. It is obviously open for discussion still, but please see my points above first

jlaitine · 2025-04-15T07:22:58Z

Seems that I still need to work on this; citest is failing for some reason

lupyuen · 2025-04-15T09:39:18Z

@jlaitine I restarted the CI Test, let's see if it fails again...

libs/libc/semaphore/sem_trywait.c

libs/libc/semaphore/sem_wait.c

sched/semaphore/sem_holder.c

sched/semaphore/sem_destroy.c

sched/semaphore/sem_holder.c

sched/semaphore/sem_post.c

jlaitine · 2025-04-15T13:08:16Z

Fixed: bugs causing the citest to fail, review comments. Need to run a bunch of tests again.

lupyuen

Tested OK with OSTest on Real Hardware. Thanks!

jlaitine · 2025-04-24T07:23:09Z

Re-tested MPFS with TII's real applications WITH CONFIG_BUILD_FLAT smp, CONFIG_BUILD_KERNEL smp, CONFIG_BUILD_KERNEL single-core. Also executed ostest on rv-virt:nsh and on real HW (MPFS smp 4 cores, CONFIG_BUILD_FLAT).

All look still good after adding the nxmutex patch.

include/nuttx/semaphore.h

libs/libc/semaphore/sem_getvalue.c

libs/libc/semaphore/sem_init.c

sched/misc/deadlock.c

sched/semaphore/sem_destroy.c

sched/semaphore/sem_post.c

pussuw · 2025-04-24T16:52:01Z

include/nuttx/mutex.h

+#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+static inline void nxmutex_reset(FAR mutex_t *mutex)
+{
+  nxsem_reset(&mutex->sem, 1);


A mutex does not have a count. This logic and the logic in nxsem_reset is redundant. Setting the holder to NO_HOLDER should suffice ?

Also, I'm having a hard time trying to figure out what is nxmutex_reset supposed to do ?

Yes me too. I implemented it in a way that it does the same thing as before.

I hate the whole "reset" thing for mutex. If you can figure out what nxmutex_reset does in

nuttx/drivers/serial/serial.c

Line 2253 in 8832136

nxmutex_reset(&dev->xmit.lock);

, I am more than happy to remove this abomination.

Otherwise I am not ready to do that yet.

I need to re-read this nxmutex_reset stuff with a clear mind. It's late.. Goes to tomorrow.

pussuw · 2025-04-24T17:07:19Z

include/nuttx/semaphore.h

+
+#define NXSEM_MBLOCKS_BIT    (((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER     ((uint32_t)0x7ffffffe)
+#define NXSEM_MRESET         ((uint32_t)0x7fffffff)


Does NXSEM_MRESET have a meaning any longer ? I don't think (the old function) nxmutex_is_reset semantics are needed anymore.

Basically, it is only used to enable the DEBUGASSERT in sem_post, which checks that the posting task is the actual holder of the mutex. The "reset" does the posting from another thread.

Again, I am more than happy to remove the reset altogether, if you just tell me what to do with the serial driver.

libs/libc/misc/lib_mutex.c

pussuw · 2025-04-24T17:24:19Z

sched/semaphore/sem_post.c

+
+      mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKS_BIT);
+
+      /* Mutex post from another thread is not allowed, unless


Maybe I don't understand the use case well enough, but one option IF a non-owner thread tries to post from nxmutex_reset, it could just take ownership of the mutex there and then post. But what is the use case for this I wonder ? Sounds like asking for trouble tbh.

Going around the reset again.... I DO want to remove it. But I don't understand why it is there in that serial driver, and I implemented it in the way that it works as before. So I don't want to remove it before someone removes that from the serial driver.

Yup, it's the part that has me most baffled. Just because I don't understand what it's supposed to do. I do get what it did before. Keeping the existing functionality as close as possible is a fair choice.

Yes, sticking to that for now... all my attempts to remove it caused havoc, especially with USB CDCACM (in the drone mavlink over USB). Replicating the functionality of nxmutex_reset just does the trick for now. Eventually I got fed up with debugging USB and serial drivers. There has to be some design flaw in the serial if one needs to "reset" mutexes, I just couldn't wrap my head around that yet, I am currently working on a bit too many things at the same time....

pussuw · 2025-04-24T17:26:33Z

sched/semaphore/sem_recover.c

@@ -86,7 +86,8 @@ void nxsem_recover(FAR struct tcb_s *tcb)
  if (tcb->task_state == TSTATE_WAIT_SEM)
    {
      FAR sem_t *sem = tcb->waitobj;
-      DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+
+      DEBUGASSERT(sem != NULL);

      /* Restore the correct priority of all threads that hold references


In case of mutex, only 1 can hold a reference, the holder itself. Should the call to nxsem_canceled be omitted when dealing with mutexes ?

There may be several tasks blocked on a mutex, and one of them can be deleted while holding the mutex. Again, keeping the existing functionality...

pussuw · 2025-04-24T17:37:13Z

sched/semaphore/sem_recover.c

+           * blocking list (ignoring the one that is being cancelled).
+           */
+
+          for (wtcb = dq_peek(SEM_WAITLIST(sem)); wtcb; wtcb = dq_next(wtcb))


In this case tcb is waiting for this mutex. The only thing that needs to be done is to remove it from the SEM_WAITLIST. If the waitlist becomes empty, then clear block mask ?

Yes. But the issue here is, that I can't remove it from the SEM_WAITLIST here. It is done afterwards. So the loop above does exactly that, it checks if there will be other tasks left after this upcoming removal from the waitlist.

pussuw · 2025-04-24T17:37:46Z

sched/semaphore/sem_reset.c

-   */
-
-  while (atomic_read(NXSEM_COUNT(sem)) < 0 && count > 0)
+  if (NXSEM_IS_MUTEX(sem))


A mutex does not have a count, what is the use case for nxmutex_reset ?

nuttx/drivers/serial/serial.c

Line 2253 in 8832136

nxmutex_reset(&dev->xmit.lock);

pussuw · 2025-04-24T17:38:27Z

sched/semaphore/sem_reset.c


-  semcount = atomic_read(NXSEM_COUNT(sem));
-  do
+      atomic_set(NXSEM_MHOLDER(sem), NXSEM_MRESET);


I still think NXSEM_MRESET has no meaning anymore

argh. no comment. I will remove it if you remove the use of nxmutex_reset, deal?

pussuw · 2025-04-24T17:42:29Z

sched/semaphore/sem_reset.c

-   */
+      while (dq_peek(SEM_WAITLIST(sem)) != NULL)
+        {
+          DEBUGVERIFY(nxsem_post(sem));


Won't 1 call to nxsem_post mark the next in SEM_WAITLIST as the owner, and then trying to post from here fails again ?

There is special handling in nxsem_post for posting with value "NXSEM_MRESET | NXSEM_MBLOCKS_BIT", and the nxsem post returns with the same.

Again, removing the reset would be preferred. But now it works as it did before with "nxmutex" code

sched/semaphore/sem_trywait.c

sched/semaphore/sem_wait.c

pussuw · 2025-04-24T17:54:01Z

sched/semaphore/sem_wait.c

@@ -218,6 +271,16 @@ int nxsem_wait_slow(FAR sem_t *sem)
 #endif
    }

+  /* If this now holds the mutex, set the holder TID and the lock bit */
+
+  if (mutex && ret == OK)


Is there a race condition here, should this logic be in nxsem_post ?

Can you clarify where do you see the race?

This is the correct place where you know that this task is the holder of the mutex (unless the wait failed of course, hence checking the error - timed wait functions still need to work for mutexes even though signals can't interrupt them). It has just acquired it and is running - and still inside the critical section with the blocking bit set.

The only reason why sem_post needs to set the holder (apart from releasing the last holder of the mutex when no other is blocked) is, that the task who is being posted may be blocked by something else. If some higher priority task again acquires (and blocks on) the mutex, the sem_wait needs to allocate and store the holder structure.

But it always needs to be set here still.

I don't see a race here, I'm asking if there can be one since I know that for counting semaphores the holder is set in nxsem_wait_irq and in sem_post, presumably for reasons. This is different from that.

You wrote this code, I don't know how it works. I cant tell by looking at it for ~30 minutes either. This is why I ask questions, since you know the implementation better.

Yes, thanks for asking, the discussion clarifies things for others as well.

I believe the only "sane" place to set the mutex holder TID is really when it gets out of sem_wait and really holding the mutex. This is after all the priority adjustments and what not.

Setting it in sem_post is more dubious, but it must be set to something else than the TID which just posted. One could arque that the most proper value to set in sem post would be "NXSEM_NO_MHOLDER | NXSEM_MBLOCKS_BIT", since at that point no other threads who have acquired the mutex are running. However, this would require special handling again in sem_wait; if while everyone, who acquired the mutex before, are blocked for some reason and some other thread blocks on the mutex again, one would need special handling for that value. So basically, setting any TID from waiting list is fine - you don't really need to know which one of those will wake up. Picking the one from the top of the waitlist is basically the same thing as what is done with counting semaphores.

sched/semaphore/sem_wait.c

This puts the mutex support fully inside nxsem, allowing locking the mutex and setting the holder with single atomic operation. This enables fast mutex locking from userspace, avoiding taking critical_sections, which may be heavy in SMP and cleanup of nxmutex library in the future. Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>

… inheritance is enabled This enables the mutex fast path for nxsem_wait, nxsem_trywait and nxsem_post also when the priority inheritance is enabled. Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>

- Remove the redundant holder, as nxsem now manages hoder TID - Remove DEBUGASSERTIONS which are managed in nxsem - Remove the "reset" handling logic, as it is now managed in nxsem - Inline the simplest functions Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>

github-actions bot added Area: OS Components Size: L labels Apr 11, 2025

acassis requested review from xiaoxiang781216, adamfeuer, anchao, raiden00pl, jerpelea, pkarashchenko and masayuki2009 April 11, 2025 14:14

acassis approved these changes Apr 11, 2025

View reviewed changes

jlaitine force-pushed the enable_mutex_functionality_in_nxsem branch 2 times, most recently from e6dbdbe to 05ef6a8 Compare April 12, 2025 14:11

xiaoxiang781216 reviewed Apr 13, 2025

View reviewed changes

jlaitine force-pushed the enable_mutex_functionality_in_nxsem branch from 05ef6a8 to a9e50bd Compare April 15, 2025 06:33

jlaitine marked this pull request as ready for review April 15, 2025 06:38

jlaitine force-pushed the enable_mutex_functionality_in_nxsem branch from a9e50bd to 4b07113 Compare April 15, 2025 06:54

pussuw reviewed Apr 15, 2025

View reviewed changes

libs/libc/semaphore/sem_trywait.c Outdated Show resolved Hide resolved

pussuw reviewed Apr 15, 2025

View reviewed changes

libs/libc/semaphore/sem_wait.c Outdated Show resolved Hide resolved

pussuw reviewed Apr 15, 2025

View reviewed changes

sched/semaphore/sem_holder.c Outdated Show resolved Hide resolved

pussuw reviewed Apr 15, 2025

View reviewed changes

sched/semaphore/sem_destroy.c Outdated Show resolved Hide resolved

pussuw reviewed Apr 15, 2025

View reviewed changes

sched/semaphore/sem_holder.c Outdated Show resolved Hide resolved

pussuw reviewed Apr 15, 2025

View reviewed changes

sched/semaphore/sem_post.c Show resolved Hide resolved

jlaitine force-pushed the enable_mutex_functionality_in_nxsem branch 2 times, most recently from 9829870 to 6deed38 Compare April 15, 2025 12:59

lupyuen approved these changes Apr 24, 2025

View reviewed changes