This is an automated email from the ASF dual-hosted git repository.
xiaoxiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
The following commit(s) were added to refs/heads/master by this push:
new 4c3ae2ed4f Revert "sem: change sem wait to atomic operation"
4c3ae2ed4f is described below
commit 4c3ae2ed4f878d195261359cc3eacefda031a01b
Author: YAMAMOTO Takashi <yamam...@midokura.com>
AuthorDate: Fri Nov 15 14:31:32 2024 +0900
Revert "sem: change sem wait to atomic operation"
This reverts commit befe29801ff50494eada535906369f89eb4f8424.
Because a few regressions have been reported and
it likely will take some time to fix them:
* for some configurations, semaphore can be used on the special
memory region, where atomic access is not available.
cf. https://github.com/apache/nuttx/pull/14625
* include/nuttx/lib/stdatomic.h is not compatible with
the C11 semantics, which the change in question relies on.
cf. https://github.com/apache/nuttx/pull/14755
---
sched/semaphore/sem_destroy.c | 14 +----
sched/semaphore/sem_holder.c | 4 +-
sched/semaphore/sem_post.c | 72 ++++----------------------
sched/semaphore/sem_recover.c | 4 +-
sched/semaphore/sem_reset.c | 15 ++----
sched/semaphore/sem_trywait.c | 115 ++++++++++++------------------------------
sched/semaphore/sem_wait.c | 82 +++++++-----------------------
sched/semaphore/sem_waitirq.c | 4 +-
sched/semaphore/semaphore.h | 7 ---
9 files changed, 68 insertions(+), 249 deletions(-)
diff --git a/sched/semaphore/sem_destroy.c b/sched/semaphore/sem_destroy.c
index fa94ef92ad..f207b339bb 100644
--- a/sched/semaphore/sem_destroy.c
+++ b/sched/semaphore/sem_destroy.c
@@ -60,8 +60,6 @@
int nxsem_destroy(FAR sem_t *sem)
{
- short old;
-
DEBUGASSERT(sem != NULL);
/* There is really no particular action that we need
@@ -74,18 +72,10 @@ int nxsem_destroy(FAR sem_t *sem)
* leave the count unchanged but still return OK.
*/
- do
+ if (sem->semcount >= 0)
{
- old = atomic_load(NXSEM_COUNT(sem));
- if (old < 0)
- {
- break;
- }
+ sem->semcount = 1;
}
- while (!atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem),
- &old, 1,
- memory_order_release,
- memory_order_relaxed));
/* Release holders of the semaphore */
diff --git a/sched/semaphore/sem_holder.c b/sched/semaphore/sem_holder.c
index 1da9f3e2a9..a51ea9a30a 100644
--- a/sched/semaphore/sem_holder.c
+++ b/sched/semaphore/sem_holder.c
@@ -880,7 +880,7 @@ void nxsem_canceled(FAR struct tcb_s *stcb, FAR sem_t *sem)
{
/* Check our assumptions */
- DEBUGASSERT(atomic_load(NXSEM_COUNT(sem)) <= 0);
+ DEBUGASSERT(sem->semcount <= 0);
/* Adjust the priority of every holder as necessary */
@@ -978,7 +978,7 @@ void nxsem_release_all(FAR struct tcb_s *htcb)
* that was taken by sem_wait() or sem_post().
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ sem->semcount++;
}
}
diff --git a/sched/semaphore/sem_post.c b/sched/semaphore/sem_post.c
index 500b00f595..33293ad037 100644
--- a/sched/semaphore/sem_post.c
+++ b/sched/semaphore/sem_post.c
@@ -37,11 +37,11 @@
#include "semaphore/semaphore.h"
/****************************************************************************
- * Private Functions
+ * Public Functions
****************************************************************************/
/****************************************************************************
- * Name: nxsem_post_slow
+ * Name: nxsem_post
*
* Description:
* When a kernel thread has finished with a semaphore, it will call
@@ -69,7 +69,7 @@
*
****************************************************************************/
-static int nxsem_post_slow(FAR sem_t *sem)
+int nxsem_post(FAR sem_t *sem)
{
FAR struct tcb_s *stcb = NULL;
irqstate_t flags;
@@ -78,6 +78,8 @@ static int nxsem_post_slow(FAR sem_t *sem)
uint8_t proto;
#endif
+ DEBUGASSERT(sem != NULL);
+
/* The following operations must be performed with interrupts
* disabled because sem_post() may be called from an interrupt
* handler.
@@ -85,13 +87,12 @@ static int nxsem_post_slow(FAR sem_t *sem)
flags = enter_critical_section();
- sem_count = atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ sem_count = sem->semcount;
/* Check the maximum allowable value */
if (sem_count >= SEM_VALUE_MAX)
{
- atomic_fetch_sub(NXSEM_COUNT(sem), 1);
leave_critical_section(flags);
return -EOVERFLOW;
}
@@ -114,6 +115,8 @@ static int nxsem_post_slow(FAR sem_t *sem)
*/
nxsem_release_holder(sem);
+ sem_count++;
+ sem->semcount = sem_count;
#if defined(CONFIG_PRIORITY_INHERITANCE) || defined(CONFIG_PRIORITY_PROTECT)
/* Don't let any unblocked tasks run until we complete any priority
@@ -135,7 +138,7 @@ static int nxsem_post_slow(FAR sem_t *sem)
* there must be some task waiting for the semaphore.
*/
- if (sem_count < 0)
+ if (sem_count <= 0)
{
/* Check if there are any tasks in the waiting for semaphore
* task list that are waiting for this semaphore. This is a
@@ -214,60 +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)
- {
- short old = 0;
- if (atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem), &old, 1,
- memory_order_release,
- memory_order_relaxed))
- {
- return OK;
- }
- }
-#endif
-
- return nxsem_post_slow(sem);
-}
diff --git a/sched/semaphore/sem_recover.c b/sched/semaphore/sem_recover.c
index dd0418d4bb..1a6e7ea2bf 100644
--- a/sched/semaphore/sem_recover.c
+++ b/sched/semaphore/sem_recover.c
@@ -85,7 +85,7 @@ 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_load(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL && sem->semcount < 0);
/* Restore the correct priority of all threads that hold references
* to this semaphore.
@@ -99,7 +99,7 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ sem->semcount++;
}
/* Release all semphore holders for the task */
diff --git a/sched/semaphore/sem_reset.c b/sched/semaphore/sem_reset.c
index d7774ea504..14418fe704 100644
--- a/sched/semaphore/sem_reset.c
+++ b/sched/semaphore/sem_reset.c
@@ -60,7 +60,6 @@
int nxsem_reset(FAR sem_t *sem, int16_t count)
{
irqstate_t flags;
- short semcount;
DEBUGASSERT(sem != NULL && count >= 0);
@@ -81,7 +80,7 @@ int nxsem_reset(FAR sem_t *sem, int16_t count)
* out counts to any waiting threads.
*/
- while (atomic_load(NXSEM_COUNT(sem)) < 0 && count > 0)
+ while (sem->semcount < 0 && count > 0)
{
/* Give out one counting, waking up one of the waiting threads
* and, perhaps, kicking off a lot of priority inheritance
@@ -99,18 +98,10 @@ int nxsem_reset(FAR sem_t *sem, int16_t count)
* value of sem->semcount is already correct in this case.
*/
- do
+ if (sem->semcount >= 0)
{
- semcount = atomic_load(NXSEM_COUNT(sem));
- if (semcount < 0)
- {
- break;
- }
+ sem->semcount = count;
}
- while (!atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem),
- &semcount, count,
- memory_order_release,
- memory_order_relaxed));
/* Allow any pending context switches to occur now */
diff --git a/sched/semaphore/sem_trywait.c b/sched/semaphore/sem_trywait.c
index c4b512547b..8b0239ec9e 100644
--- a/sched/semaphore/sem_trywait.c
+++ b/sched/semaphore/sem_trywait.c
@@ -38,78 +38,6 @@
#include "sched/sched.h"
#include "semaphore/semaphore.h"
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
-
-/****************************************************************************
- * Name: nxsem_trywait_slow
- *
- * Description:
- * This function locks the specified semaphore in slow mode.
- *
- * Input Parameters:
- * sem - the semaphore descriptor
- *
- * Returned Value:
- *
- * EINVAL - Invalid attempt to get the semaphore
- * EAGAIN - The semaphore is not available.
- *
- * Assumptions:
- *
- ****************************************************************************/
-
-static int nxsem_trywait_slow(FAR sem_t *sem)
-{
- FAR struct tcb_s *rtcb;
- irqstate_t flags;
- short semcount;
- int ret;
-
- /* The following operations must be performed with interrupts disabled
- * because sem_post() may be called from an interrupt handler.
- */
-
- flags = enter_critical_section();
- rtcb = this_task();
-
- /* If the semaphore is available, give it to the requesting task */
-
- do
- {
- semcount = atomic_load(NXSEM_COUNT(sem));
- if (semcount <= 0)
- {
- leave_critical_section(flags);
- return -EAGAIN;
- }
- }
- while (!atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem),
- &semcount, semcount - 1,
- memory_order_acquire,
- memory_order_relaxed));
-
- /* It is, let the task take the semaphore */
-
- ret = nxsem_protect_wait(sem);
- if (ret < 0)
- {
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
- leave_critical_section(flags);
- return ret;
- }
-
- nxsem_add_holder(sem);
- rtcb->waitobj = NULL;
- ret = OK;
-
- /* Interrupts may now be enabled. */
-
- leave_critical_section(flags);
- return ret;
-}
-
/****************************************************************************
* Public Functions
****************************************************************************/
@@ -140,30 +68,49 @@ static int nxsem_trywait_slow(FAR sem_t *sem)
int nxsem_trywait(FAR sem_t *sem)
{
+ FAR struct tcb_s *rtcb = this_task();
+ irqstate_t flags;
+ int ret;
+
/* This API should not be called from the idleloop */
DEBUGASSERT(sem != NULL);
DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask() ||
up_interrupt_context());
- /* If this is a mutex, we can try to get the mutex in fast mode,
- * else try to get it in slow mode.
+ /* The following operations must be performed with interrupts disabled
+ * because sem_post() may be called from an interrupt handler.
*/
-#if !defined(CONFIG_PRIORITY_INHERITANCE) && !defined(CONFIG_PRIORITY_PROTECT)
- if (sem->flags & SEM_TYPE_MUTEX)
+ flags = enter_critical_section();
+
+ /* If the semaphore is available, give it to the requesting task */
+
+ if (sem->semcount > 0)
{
- short old = 1;
- if (atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem), &old, 0,
- memory_order_acquire,
- memory_order_relaxed))
+ /* It is, let the task take the semaphore */
+
+ ret = nxsem_protect_wait(sem);
+ if (ret < 0)
{
- return OK;
+ leave_critical_section(flags);
+ return ret;
}
- return -EAGAIN;
+ sem->semcount--;
+ nxsem_add_holder(sem);
+ rtcb->waitobj = NULL;
+ ret = OK;
+ }
+ else
+ {
+ /* Semaphore is not available */
+
+ ret = -EAGAIN;
}
-#endif
- return nxsem_trywait_slow(sem);
+ /* Interrupts may now be enabled. */
+
+ leave_critical_section(flags);
+ return ret;
}
diff --git a/sched/semaphore/sem_wait.c b/sched/semaphore/sem_wait.c
index a0a8b9d76f..7ea4d36480 100644
--- a/sched/semaphore/sem_wait.c
+++ b/sched/semaphore/sem_wait.c
@@ -38,15 +38,16 @@
#include "semaphore/semaphore.h"
/****************************************************************************
- * Private Functions
+ * Public Functions
****************************************************************************/
/****************************************************************************
- * Name: nxsem_wait_slow
+ * Name: nxsem_wait
*
* Description:
- * This function attempts to lock the semaphore referenced by 'sem' in
- * slow mode.
+ * 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:
@@ -68,12 +69,17 @@
*
****************************************************************************/
-static int nxsem_wait_slow(FAR sem_t *sem)
+int nxsem_wait(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
irqstate_t flags;
int ret;
+ /* This API should not be called from interrupt handlers & idleloop */
+
+ DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
+ DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask());
+
/* The following operations must be performed with interrupts
* disabled because nxsem_post() may be called from an interrupt
* handler.
@@ -85,7 +91,7 @@ static int nxsem_wait_slow(FAR sem_t *sem)
/* Check if the lock is available */
- if (atomic_fetch_sub(NXSEM_COUNT(sem), 1) > 0)
+ if (sem->semcount > 0)
{
/* It is, let the task take the semaphore. */
@@ -96,6 +102,7 @@ static int nxsem_wait_slow(FAR sem_t *sem)
return ret;
}
+ sem->semcount--;
nxsem_add_holder(sem);
rtcb->waitobj = NULL;
ret = OK;
@@ -117,6 +124,10 @@ static int nxsem_wait_slow(FAR sem_t *sem)
DEBUGASSERT(rtcb->waitobj == NULL);
+ /* Handle the POSIX semaphore (but don't set the owner yet) */
+
+ sem->semcount--;
+
/* Save the waited on semaphore in the TCB */
rtcb->waitobj = sem;
@@ -213,65 +224,6 @@ static int nxsem_wait_slow(FAR sem_t *sem)
return ret;
}
-/****************************************************************************
- * Public Functions
- ****************************************************************************/
-
-/****************************************************************************
- * 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)
-{
- /* This API should not be called from interrupt handlers & idleloop */
-
- DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
- DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask());
-
- /* If this is a mutex, we can try to get 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)
- {
- short old = 1;
- if (atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem), &old, 0,
- memory_order_acquire,
- memory_order_relaxed))
- {
- return OK;
- }
- }
-#endif
-
- return nxsem_wait_slow(sem);
-}
-
/****************************************************************************
* Name: nxsem_wait_uninterruptible
*
diff --git a/sched/semaphore/sem_waitirq.c b/sched/semaphore/sem_waitirq.c
index e3cd08db18..746c46e05f 100644
--- a/sched/semaphore/sem_waitirq.c
+++ b/sched/semaphore/sem_waitirq.c
@@ -87,7 +87,7 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
* and already changed the task's state.
*/
- DEBUGASSERT(sem != NULL && atomic_load(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL && sem->semcount < 0);
/* Restore the correct priority of all threads that hold references
* to this semaphore.
@@ -101,7 +101,7 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ sem->semcount++;
/* Remove task from waiting list */
diff --git a/sched/semaphore/semaphore.h b/sched/semaphore/semaphore.h
index 45f745f58b..b61085ea3c 100644
--- a/sched/semaphore/semaphore.h
+++ b/sched/semaphore/semaphore.h
@@ -31,17 +31,10 @@
#include <nuttx/compiler.h>
#include <nuttx/semaphore.h>
#include <nuttx/sched.h>
-#include <nuttx/atomic.h>
#include <stdint.h>
#include <stdbool.h>
-/****************************************************************************
- * Pre-processor Definitions
- ****************************************************************************/
-
-#define NXSEM_COUNT(s) ((FAR atomic_short *)&(s)->semcount)
-
/****************************************************************************
* Public Function Prototypes
****************************************************************************/
