Skip to content

Optimize mutex atomic fast path #16030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xiaoxiang781216 merged 2 commits into from
Apr 1, 2025
Merged

Optimize mutex atomic fast path #16030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6134fd6
sched/semaphore: Optimize fast mutex acquire with CONFIG_PRIORITY_PRO…
jlaitine Mar 21, 2025
2c8f0cf
libc/semaphore: Move fast mutex wait/post paths to libc
jlaitine Mar 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions include/nuttx/semaphore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@
{(c), (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */

/* Macro to retrieve sem count */

#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)

/****************************************************************************
* Public Type Definitions
****************************************************************************/
Expand Down Expand Up @@ -153,7 +157,7 @@ int nxsem_init(FAR sem_t *sem, int pshared, unsigned int value);
int nxsem_destroy(FAR sem_t *sem);

/****************************************************************************
* Name: nxsem_wait
* Name: nxsem_wait / nxsem_wait_slow
*
* Description:
* This function attempts to lock the semaphore referenced by 'sem'. If
Expand Down Expand Up @@ -181,9 +185,10 @@ int nxsem_destroy(FAR sem_t *sem);
****************************************************************************/

int nxsem_wait(FAR sem_t *sem);
int nxsem_wait_slow(FAR sem_t *sem);

/****************************************************************************
* Name: nxsem_trywait
* Name: nxsem_trywait / nxsem_trywait_slow
*
* Description:
* This function locks the specified semaphore only if the semaphore is
Expand All @@ -207,6 +212,7 @@ int nxsem_wait(FAR sem_t *sem);
****************************************************************************/

int nxsem_trywait(FAR sem_t *sem);
int nxsem_trywait_slow(FAR sem_t *sem);

/****************************************************************************
* Name: nxsem_timedwait
Expand Down Expand Up @@ -328,7 +334,7 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
int nxsem_tickwait(FAR sem_t *sem, uint32_t delay);

/****************************************************************************
* Name: nxsem_post
* Name: nxsem_post / nxsem_post_slow
*
* Description:
* When a kernel thread has finished with a semaphore, it will call
Expand Down Expand Up @@ -357,6 +363,7 @@ int nxsem_tickwait(FAR sem_t *sem, uint32_t delay);
****************************************************************************/

int nxsem_post(FAR sem_t *sem);
int nxsem_post_slow(FAR sem_t *sem);

/****************************************************************************
* Name: nxsem_get_value
Expand Down
6 changes: 3 additions & 3 deletions include/sys/syscall_lookup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,11 @@ SYSCALL_LOOKUP(sethostname, 2)
/* Semaphores */

SYSCALL_LOOKUP(nxsem_destroy, 1)
SYSCALL_LOOKUP(nxsem_post, 1)
SYSCALL_LOOKUP(nxsem_post_slow, 1)
SYSCALL_LOOKUP(nxsem_clockwait, 3)
SYSCALL_LOOKUP(nxsem_timedwait, 2)
SYSCALL_LOOKUP(nxsem_trywait, 1)
SYSCALL_LOOKUP(nxsem_wait, 1)
SYSCALL_LOOKUP(nxsem_trywait_slow, 1)
SYSCALL_LOOKUP(nxsem_wait_slow, 1)

#ifdef CONFIG_PRIORITY_INHERITANCE
SYSCALL_LOOKUP(nxsem_set_protocol, 2)
Expand Down
60 changes: 60 additions & 0 deletions libs/libc/semaphore/sem_post.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@
#include <nuttx/config.h>

#include <errno.h>
#include <assert.h>

#include <nuttx/semaphore.h>
#include <nuttx/atomic.h>

/****************************************************************************
* Public Functions
Expand Down Expand Up @@ -84,3 +86,61 @@ int sem_post(FAR sem_t *sem)

return ret;
}

/****************************************************************************
* Name: nxsem_post
*
* Description:
* When a kernel thread has finished with a semaphore, it will call
* nxsem_post(). This function unlocks the semaphore referenced by sem
* by performing the semaphore unlock operation on that semaphore.
*
* If the semaphore value resulting from this operation is positive, then
* no tasks were blocked waiting for the semaphore to become unlocked; the
* semaphore is simply incremented.
*
* If the value of the semaphore resulting from this operation is zero,
* then one of the tasks blocked waiting for the semaphore shall be
* allowed to return successfully from its call to nxsem_wait().
*
* Input Parameters:
* sem - Semaphore descriptor
*
* Returned Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
* returned on success. A negated errno value is returned on failure.
*
* Assumptions:
* This function may be called from an interrupt handler.
*
****************************************************************************/

int nxsem_post(FAR sem_t *sem)
{
DEBUGASSERT(sem != NULL);

/* We don't do atomic fast path in case of LIBC_ARCH_ATOMIC because that
* uses spinlocks, which can't be called from userspace. Also in the kernel
* taking the slow path directly is faster than locking first in here
*/

#ifndef CONFIG_LIBC_ARCH_ATOMIC

if ((sem->flags & SEM_TYPE_MUTEX)
# if defined(CONFIG_PRIORITY_PROTECT) || defined(CONFIG_PRIORITY_INHERITANCE)
&& (sem->flags & SEM_PRIO_MASK) == SEM_PRIO_NONE
# endif
)
{
int32_t old = 0;
if (atomic_try_cmpxchg_release(NXSEM_COUNT(sem), &old, 1))
{
return OK;
}
}

#endif

return nxsem_post_slow(sem);
}
61 changes: 61 additions & 0 deletions libs/libc/semaphore/sem_trywait.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,13 @@
#include <nuttx/config.h>

#include <errno.h>
#include <assert.h>
#include <sched.h>

#include <nuttx/init.h>
#include <nuttx/semaphore.h>
#include <nuttx/atomic.h>
#include <nuttx/irq.h>

/****************************************************************************
* Public Functions
Expand Down Expand Up @@ -76,3 +81,59 @@ int sem_trywait(FAR sem_t *sem)

return ret;
}

/****************************************************************************
* Name: nxsem_trywait
*
* Description:
* This function locks the specified semaphore only if the semaphore is
* currently not locked. In either case, the call returns without
* blocking.
*
* Input Parameters:
* sem - the semaphore descriptor
*
* Returned Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
* returned on success. A negated errno value is returned on failure.
* Possible returned errors:
*
* - EINVAL - Invalid attempt to get the semaphore
* - EAGAIN - The semaphore is not available.
*
****************************************************************************/

int nxsem_trywait(FAR sem_t *sem)
{
DEBUGASSERT(sem != NULL);

/* This API should not be called from the idleloop or interrupt */

#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask() ||
up_interrupt_context());
#endif

/* We don't do atomic fast path in case of LIBC_ARCH_ATOMIC because that
* uses spinlocks, which can't be called from userspace. Also in the kernel
* taking the slow path directly is faster than locking first in here
*/

#ifndef CONFIG_LIBC_ARCH_ATOMIC

if ((sem->flags & SEM_TYPE_MUTEX)
#if defined(CONFIG_PRIORITY_PROTECT) || defined(CONFIG_PRIORITY_INHERITANCE)
&& (sem->flags & SEM_PRIO_MASK) == SEM_PRIO_NONE
#endif
)
{
int32_t old = 1;
return atomic_try_cmpxchg_acquire(NXSEM_COUNT(sem), &old, 0) ?
OK : -EAGAIN;
}

#endif

return nxsem_trywait_slow(sem);
}
69 changes: 69 additions & 0 deletions libs/libc/semaphore/sem_wait.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,14 @@
#include <nuttx/config.h>

#include <errno.h>
#include <assert.h>
#include <sched.h>

#include <nuttx/init.h>
#include <nuttx/cancelpt.h>
#include <nuttx/semaphore.h>
#include <nuttx/atomic.h>
#include <nuttx/irq.h>

/****************************************************************************
* Public Functions
Expand Down Expand Up @@ -98,3 +103,67 @@ int sem_wait(FAR sem_t *sem)
leave_cancellation_point();
return ERROR;
}

/****************************************************************************
* Name: nxsem_wait
*
* Description:
* This function attempts to lock the semaphore referenced by 'sem'. If
* the semaphore value is (<=) zero, then the calling task will not return
* until it successfully acquires the lock.
*
* This is an internal OS interface. It is functionally equivalent to
* sem_wait except that:
*
* - It is not a cancellation point, and
* - It does not modify the errno value.
*
* Input Parameters:
* sem - Semaphore descriptor.
*
* Returned Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
* returned on success. A negated errno value is returned on failure.
* Possible returned errors:
*
* - EINVAL: Invalid attempt to get the semaphore
* - EINTR: The wait was interrupted by the receipt of a signal.
*
****************************************************************************/

int nxsem_wait(FAR sem_t *sem)

@xiaoxiang781216 xiaoxiang781216 Mar 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not remove nxsem_wati and replace line 86 with nxsem_wait_impl directly

@jlaitine jlaitine Mar 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my original idea. However:

  • Using inline for nxsem_wait, nxsem_trywait and nxsem_post would increase code size significantly. There will still be additional code handling the mutex's holder directly in here as well to further optimize the priority inheritance case.
  • libc and kernel neede different "decorations" - kernel requires DEBUGASSERTs and libc should (IMHO) check if sem != NULL. Putting these into inilne functions would make it messy (would require ifdef KERNEL etc..)
  • That would require the inline functions to be included in all places where nxsem_* functions are used; essentially putting the functions into nuttx/include/semaphore.h. This wouldn't work, because atomic.h can't be in cluded from there, it would break several places, especially with cxx.

It is better to have the functions contained in single compilation unit in libc (in memory protected builds) and in kernel. This is better for code size (and cache locality)

@xiaoxiang781216 xiaoxiang781216 Mar 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my original idea. However:

  • Using inline for nxsem_wait, nxsem_trywait and nxsem_post would increase code size significantly. There will still be additional code handling the mutex's holder directly in here as well to further optimize the priority inheritance case.

I don't see any reason that the code size will increase significantly. nxsem_wait is identical to nxsem_trywait_slow except DEBUGASSERT which is no-op in release build.

  • libc and kernel neede different "decorations" - kernel requires DEBUGASSERTs and libc should (IMHO) check if sem != NULL. Putting these into inilne functions would make it messy (would require ifdef KERNEL etc..)

DEBUGASSERT can be called in userspace too, why do you need add __KERNEL__? sem_xxx could check sem != NULL before invoking nxsem_xxx, nxsem_xxx could DEBUGASSERT before invoking nsem_xxx_slow.

  • That would require the inline functions to be included in all places where nxsem_* functions are used; essentially putting the functions into nuttx/include/semaphore.h. This wouldn't work, because atomic.h can't be in cluded from there, it would break several places, especially with cxx.

Since nxsem_xxx_slow become the syscall function but nxsem_xxx as the normal function, you can implment nxsem_xxx as non inline function and put them into libs/libc/semaphore/ and remove the version under sched/semaphore/ directly.

It is better to have the functions contained in single compilation unit in libc (in memory protected builds) and in kernel. This is better for code size (and cache locality)

if you really want you can change nxsem_post_impl to static inline function and move it into libs/libc/semaphore/sem_post.c which alredy contain both sem_post and nxsem_post.
and move these prototype into include/nuttx/semaphore.h:

int nxsem_trywait_slow(FAR sem_t *sem);
int nxsem_wait_slow(FAR sem_t *sem);
int nxsem_post_slow(FAR sem_t *sem);

and remove include/nuttx/sem_fast.h.

@jlaitine jlaitine Mar 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my original idea. However:

  • Using inline for nxsem_wait, nxsem_trywait and nxsem_post would increase code size significantly. There will still be additional code handling the mutex's holder directly in here as well to further optimize the priority inheritance case.

I don't see any reason that the code size will increase significantly. nxsem_wait is identical to nxsem_trywait_slow except DEBUGASSERT which is no-op in release build.

I don't understand what you are talking about. If you inline sem_wait, sem_trywait and sem_post, obviously the code size will increase since all that code will be included in every compilation unit where these funcitions are used (kernel drivers, kernel nxmutex, libc nxmutex, libc sem.... In addition that will be a mess because of atomic.h including in all of those places.

  • libc and kernel neede different "decorations" - kernel requires DEBUGASSERTs and libc should (IMHO) check if sem != NULL. Putting these into inilne functions would make it messy (would require ifdef KERNEL etc..)

DEBUGASSERT can be called in userspace too, why do you need add __KERNEL__? sem_xxx could check sem != NULL before invoking nxsem_xxx, nxsem_xxx could DEBUGASSERT before invoking nsem_xxx_slow.

Yes, but why? You still need two c-files, so why not keep just the common code in the current "inline" and leave the libc specifics in libc c-file and kernel specifics in kernel c-file.

  • That would require the inline functions to be included in all places where nxsem_* functions are used; essentially putting the functions into nuttx/include/semaphore.h. This wouldn't work, because atomic.h can't be in cluded from there, it would break several places, especially with cxx.

Since nxsem_xxx_slow become the syscall function but nxsem_xxx as the normal function, you can implment nxsem_xxx as non inline function and put them into libs/libc/semaphore/ and remove the version under sched/semaphore/ directly.

It is better to have the functions contained in single compilation unit in libc (in memory protected builds) and in kernel. This is better for code size (and cache locality)

if you really want you can change nxsem_post_impl to static inline function and move it into libs/libc/semaphore/sem_post.c which alredy contain both sem_post and nxsem_post. and move these prototype into include/nuttx/semaphore.h:

Of course I can include the _slow prototypes into include/nuttx/semaphore.h. But why? These are now not supposed to be called from anywhere else than the corresponding inline functions. And those inline functions are put into an own header to avoid nasty atomics dependencies. It is easier to include them only in those C files where they are actually used.

And you can't put the code only into libc. That is the whole point why I made them originally macros and later inline functions. You need them both in kernel and in libc. The inline functions are made only for the purpose that the code doesn't need to be duplicated, but it can be included in the corresponding C files, in kernel and in libc.

In protected builds, you can't call libc function from kernel or kernel function from libc. You must have nxsem_wait, nxsem_trywait and nxsem_post in BOTH libc AND kernel.

@xiaoxiang781216 xiaoxiang781216 Mar 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • libc and kernel neede different "decorations" - kernel requires DEBUGASSERTs and libc should (IMHO) check if sem != NULL. Putting these into inilne functions would make it messy (would require ifdef KERNEL etc..)

DEBUGASSERT can be called in userspace too, why do you need add __KERNEL__? sem_xxx could check sem != NULL before invoking nxsem_xxx, nxsem_xxx could DEBUGASSERT before invoking nsem_xxx_slow.

Yes, but why? You still need two c-files, so why not keep just the common code in the current "inline" and leave the libc specifics in libc c-file and kernel specifics in kernel c-file.

You don't need two c-files. nxsem_xxx is a normal function now, but call nxsem_xxx_slow kernel function, so you can put nxsem_xxx and sem_xxx into the same libc source file, and change nxsem_xxx_impl to static function, or let's sem_xxx call nxsem_xxx directly and let compiler optimize them.

@xiaoxiang781216 xiaoxiang781216 Mar 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think defining nxsem_xxx into libc is wrong also. What we should do is:

  • Move nxsem_xxx api back to kernel
  • Implement a version of the sem_xxx APIs into libc that does not use cancelpoints / modify errno, like sem_wait_nocancel() or sem_wait_raw() that does the fast counter handling and system call into kernel if the fast counter cannot be used / the semaphore must block.

but sem_xxx need modify errno from POSIX.

Why do we need sem_wait_raw? Because many places in libc would benefit from the fast semaphore path, and it would be great if we had a set of APIs that implement this (like Jukka did with sem_wait_impl).

Unfortunately this means that we must duplicate a bit of logic in libc/kernel (the fast counter handling needs to be duplicated) but as a benefit we get a much cleaner kernel/user separation.

inline the implementation could avoid the duplication.

@pussuw pussuw Mar 31, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but sem_xxx need modify errno from POSIX.

Yes, obviously. But in libc there is pthread_mutex (maybe others?) that need the fast path as well. So we need a function that implements the fast path without touching errno / cancel points.

inline the implementation could avoid the duplication.

Yes this is one option, the inline code would then be accessed from both user and kernel ?

@jlaitine jlaitine Apr 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change to this, I like it better than having nxsem* in libc as well. A bit of a hassle since we need to change all calls of nxsem within libc.

@jlaitine jlaitine Apr 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pussuw: there are other nxsem functions in libc: nxsem_set_protocol, nxsem_getprioceiling, nxsem_setprioceiling, nxsem_set_protocol.

These are all used both in kernel and in libc. I am not going to re-work this all. And nxsem is not the only interface which is implemented like this.

I believe it is cleaner to change our interpretation of nx* naming that it means "internal to nuttx" instead of "internal to kernel", and keep the current implementation.

@pussuw pussuw Apr 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's not only about naming, we also need to link libc into the kernel, but it seems we need to do it regardless of this change.

{
DEBUGASSERT(sem != NULL);

/* This API should not be called from the idleloop or interrupt */

#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask() ||
Comment thread
pussuw marked this conversation as resolved.
up_interrupt_context());
#endif

/* We don't do atomic fast path in case of LIBC_ARCH_ATOMIC because that
* uses spinlocks, which can't be called from userspace. Also in the kernel
* taking the slow path directly is faster than locking first in here
*/

#ifndef CONFIG_LIBC_ARCH_ATOMIC

if ((sem->flags & SEM_TYPE_MUTEX)
# if defined(CONFIG_PRIORITY_PROTECT) || defined(CONFIG_PRIORITY_INHERITANCE)
&& (sem->flags & SEM_PRIO_MASK) == SEM_PRIO_NONE
# endif
)
{
int32_t old = 1;
if (atomic_try_cmpxchg_acquire(NXSEM_COUNT(sem), &old, 0))
{
return OK;
}
}

#endif

return nxsem_wait_slow(sem);
}
59 changes: 2 additions & 57 deletions sched/semaphore/sem_post.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
#include "semaphore/semaphore.h"

/****************************************************************************
* Private Functions
* Public Functions
****************************************************************************/

/****************************************************************************
Expand Down Expand Up @@ -69,7 +69,7 @@
*
****************************************************************************/

static int nxsem_post_slow(FAR sem_t *sem)
int nxsem_post_slow(FAR sem_t *sem)
{
FAR struct tcb_s *stcb = NULL;
irqstate_t flags;
Expand Down Expand Up @@ -217,58 +217,3 @@ static int nxsem_post_slow(FAR sem_t *sem)

return OK;
}

/****************************************************************************
* Public Functions
****************************************************************************/

/****************************************************************************
* Name: nxsem_post
*
* Description:
* When a kernel thread has finished with a semaphore, it will call
* nxsem_post(). This function unlocks the semaphore referenced by sem
* by performing the semaphore unlock operation on that semaphore.
*
* If the semaphore value resulting from this operation is positive, then
* no tasks were blocked waiting for the semaphore to become unlocked; the
* semaphore is simply incremented.
*
* If the value of the semaphore resulting from this operation is zero,
* then one of the tasks blocked waiting for the semaphore shall be
* allowed to return successfully from its call to nxsem_wait().
*
* Input Parameters:
* sem - Semaphore descriptor
*
* Returned Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
* returned on success. A negated errno value is returned on failure.
*
* Assumptions:
* This function may be called from an interrupt handler.
*
****************************************************************************/

int nxsem_post(FAR sem_t *sem)
{
DEBUGASSERT(sem != NULL);

/* If this is a mutex, we can try to unlock the mutex in fast mode,
* else try to get it in slow mode.
*/

#if !defined(CONFIG_PRIORITY_INHERITANCE) && !defined(CONFIG_PRIORITY_PROTECT)
if (sem->flags & SEM_TYPE_MUTEX)
{
int32_t old = 0;
if (atomic_try_cmpxchg_release(NXSEM_COUNT(sem), &old, 1))
{
return OK;
}
}
#endif

return nxsem_post_slow(sem);
}
Loading