From b3b3f1f4cfc4c86af19ca437c4f43be67bcac629 Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Fri, 15 Nov 2024 14:31:32 +0900 Subject: [PATCH] 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 fa94ef92ad456..f207b339bb3e8 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 1da9f3e2a94de..a51ea9a30ac93 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 500b00f5954ce..33293ad037e69 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 dd0418d4bbdce..1a6e7ea2bfe95 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 d7774ea504291..14418fe7043de 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 c4b512547bd7f..8b0239ec9e9f3 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 a0a8b9d76f177..7ea4d36480857 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 e3cd08db1825d..746c46e05f55d 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 45f745f58bc26..b61085ea3c3e6 100644 --- a/sched/semaphore/semaphore.h +++ b/sched/semaphore/semaphore.h @@ -31,17 +31,10 @@ #include #include #include -#include #include #include -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - -#define NXSEM_COUNT(s) ((FAR atomic_short *)&(s)->semcount) - /**************************************************************************** * Public Function Prototypes ****************************************************************************/