You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by xi...@apache.org on 2022/11/10 14:36:30 UTC
[incubator-nuttx] 02/02: semaphore: move param check to sem_xx level
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/incubator-nuttx.git
commit a9c647d418abceb744f42a7a3600bc256b871474
Author: ligd <li...@xiaomi.com>
AuthorDate: Fri Oct 21 00:00:08 2022 +0800
semaphore: move param check to sem_xx level
for the speed improve
Signed-off-by: ligd <li...@xiaomi.com>
---
libs/libc/semaphore/sem_init.c | 40 +++++-----
sched/semaphore/sem_clockwait.c | 24 +++---
sched/semaphore/sem_destroy.c | 51 ++++++------
sched/semaphore/sem_post.c | 169 ++++++++++++++++++++--------------------
sched/semaphore/sem_trywait.c | 50 ++++++------
sched/semaphore/sem_wait.c | 169 ++++++++++++++++++++--------------------
6 files changed, 256 insertions(+), 247 deletions(-)
diff --git a/libs/libc/semaphore/sem_init.c b/libs/libc/semaphore/sem_init.c
index f4f276b6fa..fdcd8d9f5e 100644
--- a/libs/libc/semaphore/sem_init.c
+++ b/libs/libc/semaphore/sem_init.c
@@ -25,6 +25,7 @@
#include <nuttx/config.h>
#include <sys/types.h>
+#include <assert.h>
#include <limits.h>
#include <errno.h>
@@ -63,35 +64,28 @@ int nxsem_init(FAR sem_t *sem, int pshared, unsigned int value)
{
UNUSED(pshared);
- /* Verify that a semaphore was provided and the count is within the valid
- * range.
- */
+ DEBUGASSERT(sem != NULL && value <= SEM_VALUE_MAX);
- if (sem != NULL && value <= SEM_VALUE_MAX)
- {
- /* Initialize the semaphore count */
+ /* Initialize the semaphore count */
- sem->semcount = (int16_t)value;
+ sem->semcount = (int16_t)value;
- /* Initialize semaphore wait list */
+ /* Initialize semaphore wait list */
- dq_init(&sem->waitlist);
+ dq_init(&sem->waitlist);
- /* Initialize to support priority inheritance */
+ /* Initialize to support priority inheritance */
#ifdef CONFIG_PRIORITY_INHERITANCE
- sem->flags = 0;
+ sem->flags = 0;
# if CONFIG_SEM_PREALLOCHOLDERS > 0
- sem->hhead = NULL;
+ sem->hhead = NULL;
# else
- INITIALIZE_SEMHOLDER(&sem->holder[0]);
- INITIALIZE_SEMHOLDER(&sem->holder[1]);
+ INITIALIZE_SEMHOLDER(&sem->holder[0]);
+ INITIALIZE_SEMHOLDER(&sem->holder[1]);
# endif
#endif
- return OK;
- }
-
- return -EINVAL;
+ return OK;
}
/****************************************************************************
@@ -122,6 +116,16 @@ int sem_init(FAR sem_t *sem, int pshared, unsigned int value)
{
int ret;
+ /* Verify that a semaphore was provided and the count is within the valid
+ * range.
+ */
+
+ if (sem == NULL || value > SEM_VALUE_MAX)
+ {
+ set_errno(EINVAL);
+ return ERROR;
+ }
+
ret = nxsem_init(sem, pshared, value);
if (ret < 0)
{
diff --git a/sched/semaphore/sem_clockwait.c b/sched/semaphore/sem_clockwait.c
index 1ea1d4fc86..77f8b53d0e 100644
--- a/sched/semaphore/sem_clockwait.c
+++ b/sched/semaphore/sem_clockwait.c
@@ -97,19 +97,9 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
int status;
int ret = ERROR;
+ DEBUGASSERT(sem != NULL && abstime != NULL);
DEBUGASSERT(up_interrupt_context() == false);
- /* Verify the input parameters and, in case of an error, set
- * errno appropriately.
- */
-
-#ifdef CONFIG_DEBUG_FEATURES
- if (!abstime || !sem)
- {
- return -EINVAL;
- }
-#endif
-
/* We will disable interrupts until we have completed the semaphore
* wait. We need to do this (as opposed to just disabling pre-emption)
* because there could be interrupt handlers that are asynchronously
@@ -134,11 +124,13 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
* with a valid timeout.
*/
+#ifdef CONFIG_DEBUG_FEATURES
if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
{
ret = -EINVAL;
goto out;
}
+#endif
/* Convert the timespec to clock ticks. We must have interrupts
* disabled here so that this time stays valid until the wait begins.
@@ -268,6 +260,16 @@ int sem_clockwait(FAR sem_t *sem, clockid_t clockid,
{
int ret;
+ /* Verify the input parameters and, in case of an error, set
+ * errno appropriately.
+ */
+
+ if (sem == NULL || abstime == NULL)
+ {
+ set_errno(EINVAL);
+ return ERROR;
+ }
+
/* sem_timedwait() is a cancellation point */
enter_cancellation_point();
diff --git a/sched/semaphore/sem_destroy.c b/sched/semaphore/sem_destroy.c
index a5a33f7c18..883a659a52 100644
--- a/sched/semaphore/sem_destroy.c
+++ b/sched/semaphore/sem_destroy.c
@@ -56,34 +56,29 @@
*
****************************************************************************/
-int nxsem_destroy (FAR sem_t *sem)
+int nxsem_destroy(FAR sem_t *sem)
{
- /* Assure a valid semaphore is specified */
-
- if (sem != NULL)
+ DEBUGASSERT(sem != NULL);
+
+ /* There is really no particular action that we need
+ * take to destroy a semaphore. We will just reset
+ * the count to some reasonable value (0) and release
+ * ownership.
+ *
+ * Check if other threads are waiting on the semaphore.
+ * In this case, the behavior is undefined. We will:
+ * leave the count unchanged but still return OK.
+ */
+
+ if (sem->semcount >= 0)
{
- /* There is really no particular action that we need
- * take to destroy a semaphore. We will just reset
- * the count to some reasonable value (1) and release
- * ownership.
- *
- * Check if other threads are waiting on the semaphore.
- * In this case, the behavior is undefined. We will:
- * leave the count unchanged but still return OK.
- */
-
- if (sem->semcount >= 0)
- {
- sem->semcount = 1;
- }
-
- /* Release holders of the semaphore */
-
- nxsem_destroyholder(sem);
- return OK;
+ sem->semcount = 1;
}
- return -EINVAL;
+ /* Release holders of the semaphore */
+
+ nxsem_destroyholder(sem);
+ return OK;
}
/****************************************************************************
@@ -114,6 +109,14 @@ int sem_destroy (FAR sem_t *sem)
{
int ret;
+ /* Assure a valid semaphore is specified */
+
+ if (sem == NULL)
+ {
+ set_errno(EINVAL);
+ return ERROR;
+ }
+
ret = nxsem_destroy(sem);
if (ret < 0)
{
diff --git a/sched/semaphore/sem_post.c b/sched/semaphore/sem_post.c
index 851c5f2f7f..475bc9d121 100644
--- a/sched/semaphore/sem_post.c
+++ b/sched/semaphore/sem_post.c
@@ -72,121 +72,112 @@ int nxsem_post(FAR sem_t *sem)
FAR struct tcb_s *stcb = NULL;
irqstate_t flags;
int16_t sem_count;
- int ret = -EINVAL;
- /* Make sure we were supplied with a valid semaphore. */
+ DEBUGASSERT(sem != NULL);
- if (sem != NULL)
- {
- /* The following operations must be performed with interrupts
- * disabled because sem_post() may be called from an interrupt
- * handler.
- */
+ /* The following operations must be performed with interrupts
+ * disabled because sem_post() may be called from an interrupt
+ * handler.
+ */
- flags = enter_critical_section();
+ flags = enter_critical_section();
- sem_count = sem->semcount;
+ sem_count = sem->semcount;
- /* Check the maximum allowable value */
+ /* Check the maximum allowable value */
- if (sem_count >= SEM_VALUE_MAX)
- {
- leave_critical_section(flags);
- return -EOVERFLOW;
- }
+ DEBUGASSERT(sem_count < SEM_VALUE_MAX);
- /* Perform the semaphore unlock operation, releasing this task as a
- * holder then also incrementing the count on the semaphore.
- *
- * NOTE: When semaphores are used for signaling purposes, the holder
- * of the semaphore may not be this thread! In this case,
- * nxsem_release_holder() will do nothing.
- *
- * In the case of a mutex this could be simply resolved since there is
- * only one holder but for the case of counting semaphores, there may
- * be many holders and if the holder is not this thread, then it is
- * not possible to know which thread/holder should be released.
- *
- * For this reason, it is recommended that priority inheritance be
- * disabled via nxsem_set_protocol(SEM_PRIO_NONE) when the semaphore is
- * initialized if the semaphore is to used for signaling purposes.
- */
+ /* Perform the semaphore unlock operation, releasing this task as a
+ * holder then also incrementing the count on the semaphore.
+ *
+ * NOTE: When semaphores are used for signaling purposes, the holder
+ * of the semaphore may not be this thread! In this case,
+ * nxsem_release_holder() will do nothing.
+ *
+ * In the case of a mutex this could be simply resolved since there is
+ * only one holder but for the case of counting semaphores, there may
+ * be many holders and if the holder is not this thread, then it is
+ * not possible to know which thread/holder should be released.
+ *
+ * For this reason, it is recommended that priority inheritance be
+ * disabled via nxsem_set_protocol(SEM_PRIO_NONE) when the semaphore is
+ * initialized if the semaphore is to used for signaling purposes.
+ */
- nxsem_release_holder(sem);
- sem_count++;
- sem->semcount = sem_count;
+ nxsem_release_holder(sem);
+ sem_count++;
+ sem->semcount = sem_count;
#ifdef CONFIG_PRIORITY_INHERITANCE
- /* Don't let any unblocked tasks run until we complete any priority
- * restoration steps. Interrupts are disabled, but we do not want
- * the head of the ready-to-run list to be modified yet.
- *
- * NOTE: If this sched_lock is called from an interrupt handler, it
- * will do nothing.
- */
-
- sched_lock();
+ /* Don't let any unblocked tasks run until we complete any priority
+ * restoration steps. Interrupts are disabled, but we do not want
+ * the head of the ready-to-run list to be modified yet.
+ *
+ * NOTE: If this sched_lock is called from an interrupt handler, it
+ * will do nothing.
+ */
+
+ sched_lock();
#endif
- /* If the result of semaphore unlock is non-positive, then
- * there must be some task waiting for the semaphore.
+ /* If the result of semaphore unlock is non-positive, then
+ * there must be some task waiting for the semaphore.
+ */
+
+ 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
+ * prioritized list so the first one we encounter is the one
+ * that we want.
*/
- if (sem_count <= 0)
+ stcb = (FAR struct tcb_s *)dq_peek(SEM_WAITLIST(sem));
+
+ if (stcb != NULL)
{
- /* Check if there are any tasks in the waiting for semaphore
- * task list that are waiting for this semaphore. This is a
- * prioritized list so the first one we encounter is the one
- * that we want.
+ /* The task will be the new holder of the semaphore when
+ * it is awakened.
*/
- stcb = (FAR struct tcb_s *)dq_peek(SEM_WAITLIST(sem));
-
- if (stcb != NULL)
- {
- /* The task will be the new holder of the semaphore when
- * it is awakened.
- */
-
- nxsem_add_holder_tcb(stcb, sem);
+ nxsem_add_holder_tcb(stcb, sem);
- /* Stop the watchdog timer */
+ /* Stop the watchdog timer */
- if (WDOG_ISACTIVE(&stcb->waitdog))
- {
- wd_cancel(&stcb->waitdog);
- }
+ if (WDOG_ISACTIVE(&stcb->waitdog))
+ {
+ wd_cancel(&stcb->waitdog);
+ }
- /* Restart the waiting task. */
+ /* Restart the waiting task. */
- up_unblock_task(stcb);
- }
+ up_unblock_task(stcb);
+ }
#if 0 /* REVISIT: This can fire on IOB throttle semaphore */
- else
- {
- /* This should not happen. */
+ else
+ {
+ /* This should not happen. */
- DEBUGPANIC();
- }
-#endif
+ DEBUGPANIC();
}
+#endif
+ }
- /* Check if we need to drop the priority of any threads holding
- * this semaphore. The priority could have been boosted while they
- * held the semaphore.
- */
+ /* Check if we need to drop the priority of any threads holding
+ * this semaphore. The priority could have been boosted while they
+ * held the semaphore.
+ */
#ifdef CONFIG_PRIORITY_INHERITANCE
- nxsem_restore_baseprio(stcb, sem);
- sched_unlock();
+ nxsem_restore_baseprio(stcb, sem);
+ sched_unlock();
#endif
- ret = OK;
- /* Interrupts may now be enabled. */
+ /* Interrupts may now be enabled. */
- leave_critical_section(flags);
- }
+ leave_critical_section(flags);
- return ret;
+ return OK;
}
/****************************************************************************
@@ -222,6 +213,14 @@ int sem_post(FAR sem_t *sem)
{
int ret;
+ /* Make sure we were supplied with a valid semaphore. */
+
+ if (sem == NULL)
+ {
+ set_errno(EINVAL);
+ return ERROR;
+ }
+
ret = nxsem_post(sem);
if (ret < 0)
{
diff --git a/sched/semaphore/sem_trywait.c b/sched/semaphore/sem_trywait.c
index 6fe483c51d..e67038a0c2 100644
--- a/sched/semaphore/sem_trywait.c
+++ b/sched/semaphore/sem_trywait.c
@@ -75,41 +75,33 @@ int nxsem_trywait(FAR sem_t *sem)
DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask());
- if (sem != NULL)
- {
- /* The following operations must be performed with interrupts disabled
- * because sem_post() may be called from an interrupt handler.
- */
-
- flags = enter_critical_section();
-
- /* If the semaphore is available, give it to the requesting task */
+ /* The following operations must be performed with interrupts disabled
+ * because sem_post() may be called from an interrupt handler.
+ */
- if (sem->semcount > 0)
- {
- /* It is, let the task take the semaphore */
+ flags = enter_critical_section();
- sem->semcount--;
- nxsem_add_holder(sem);
- rtcb->waitobj = NULL;
- ret = OK;
- }
- else
- {
- /* Semaphore is not available */
+ /* If the semaphore is available, give it to the requesting task */
- ret = -EAGAIN;
- }
-
- /* Interrupts may now be enabled. */
+ if (sem->semcount > 0)
+ {
+ /* It is, let the task take the semaphore */
- leave_critical_section(flags);
+ sem->semcount--;
+ nxsem_add_holder(sem);
+ rtcb->waitobj = NULL;
+ ret = OK;
}
else
{
- ret = -EINVAL;
+ /* Semaphore is not available */
+
+ ret = -EAGAIN;
}
+ /* Interrupts may now be enabled. */
+
+ leave_critical_section(flags);
return ret;
}
@@ -138,6 +130,12 @@ int sem_trywait(FAR sem_t *sem)
{
int ret;
+ if (sem == NULL)
+ {
+ set_errno(EINVAL);
+ return ERROR;
+ }
+
/* Let nxsem_trywait do the real work */
ret = nxsem_trywait(sem);
diff --git a/sched/semaphore/sem_wait.c b/sched/semaphore/sem_wait.c
index 33c8b3d4b6..67e1722469 100644
--- a/sched/semaphore/sem_wait.c
+++ b/sched/semaphore/sem_wait.c
@@ -72,7 +72,7 @@ int nxsem_wait(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
irqstate_t flags;
- int ret = -EINVAL;
+ int ret;
/* This API should not be called from interrupt handlers & idleloop */
@@ -88,108 +88,105 @@ int nxsem_wait(FAR sem_t *sem)
/* Make sure we were supplied with a valid semaphore. */
- if (sem != NULL)
+ /* Check if the lock is available */
+
+ if (sem->semcount > 0)
{
- /* Check if the lock is available */
+ /* It is, let the task take the semaphore. */
- if (sem->semcount > 0)
- {
- /* It is, let the task take the semaphore. */
+ sem->semcount--;
+ nxsem_add_holder(sem);
+ rtcb->waitobj = NULL;
+ ret = OK;
+ }
- sem->semcount--;
- nxsem_add_holder(sem);
- rtcb->waitobj = NULL;
- ret = OK;
- }
+ /* The semaphore is NOT available, We will have to block the
+ * current thread of execution.
+ */
- /* The semaphore is NOT available, We will have to block the
- * current thread of execution.
+ else
+ {
+ /* First, verify that the task is not already waiting on a
+ * semaphore
*/
- else
- {
- /* First, verify that the task is not already waiting on a
- * semaphore
- */
-
- DEBUGASSERT(rtcb->waitobj == NULL);
+ DEBUGASSERT(rtcb->waitobj == NULL);
- /* Handle the POSIX semaphore (but don't set the owner yet) */
+ /* Handle the POSIX semaphore (but don't set the owner yet) */
- sem->semcount--;
+ sem->semcount--;
- /* Save the waited on semaphore in the TCB */
+ /* Save the waited on semaphore in the TCB */
- rtcb->waitobj = sem;
+ rtcb->waitobj = sem;
- /* If priority inheritance is enabled, then check the priority of
- * the holder of the semaphore.
- */
+ /* If priority inheritance is enabled, then check the priority of
+ * the holder of the semaphore.
+ */
#ifdef CONFIG_PRIORITY_INHERITANCE
- /* Disable context switching. The following operations must be
- * atomic with regard to the scheduler.
- */
+ /* Disable context switching. The following operations must be
+ * atomic with regard to the scheduler.
+ */
- sched_lock();
+ sched_lock();
- /* Boost the priority of any threads holding a count on the
- * semaphore.
- */
+ /* Boost the priority of any threads holding a count on the
+ * semaphore.
+ */
- nxsem_boost_priority(sem);
+ nxsem_boost_priority(sem);
#endif
- /* Set the errno value to zero (preserving the original errno)
- * value). We reuse the per-thread errno to pass information
- * between sem_waitirq() and this functions.
- */
-
- rtcb->errcode = OK;
-
- /* Add the TCB to the prioritized semaphore wait queue, after
- * checking this is not the idle task - descheduling that
- * isn't going to end well.
- */
-
- DEBUGASSERT(!is_idle_task(rtcb));
- up_block_task(rtcb, TSTATE_WAIT_SEM);
-
- /* When we resume at this point, either (1) the semaphore has been
- * assigned to this thread of execution, or (2) the semaphore wait
- * has been interrupted by a signal or a timeout. We can detect
- * these latter cases be examining the per-thread errno value.
- *
- * In the event that the semaphore wait was interrupted by a
- * signal or a timeout, certain semaphore clean-up operations have
- * already been performed (see sem_waitirq.c). Specifically:
- *
- * - nxsem_canceled() was called to restore the priority of all
- * threads that hold a reference to the semaphore,
- * - The semaphore count was decremented, and
- * - tcb->waitobj was nullifed.
- *
- * It is necessary to do these things in sem_waitirq.c because a
- * long time may elapse between the time that the signal was issued
- * and this thread is awakened and this leaves a door open to
- * several race conditions.
- */
-
- /* Check if an error occurred while we were sleeping. Expected
- * errors include EINTR meaning that we were awakened by a signal
- * or ETIMEDOUT meaning that the timer expired for the case of
- * sem_timedwait().
- *
- * If we were not awakened by a signal or a timeout, then
- * nxsem_add_holder() was called by logic in sem_wait() fore this
- * thread was restarted.
- */
-
- ret = rtcb->errcode != OK ? -rtcb->errcode : OK;
+ /* Set the errno value to zero (preserving the original errno)
+ * value). We reuse the per-thread errno to pass information
+ * between sem_waitirq() and this functions.
+ */
+
+ rtcb->errcode = OK;
+
+ /* Add the TCB to the prioritized semaphore wait queue, after
+ * checking this is not the idle task - descheduling that
+ * isn't going to end well.
+ */
+
+ DEBUGASSERT(!is_idle_task(rtcb));
+ up_block_task(rtcb, TSTATE_WAIT_SEM);
+
+ /* When we resume at this point, either (1) the semaphore has been
+ * assigned to this thread of execution, or (2) the semaphore wait
+ * has been interrupted by a signal or a timeout. We can detect
+ * these latter cases be examining the per-thread errno value.
+ *
+ * In the event that the semaphore wait was interrupted by a
+ * signal or a timeout, certain semaphore clean-up operations have
+ * already been performed (see sem_waitirq.c). Specifically:
+ *
+ * - nxsem_canceled() was called to restore the priority of all
+ * threads that hold a reference to the semaphore,
+ * - The semaphore count was decremented, and
+ * - tcb->waitobj was nullifed.
+ *
+ * It is necessary to do these things in sem_waitirq.c because a
+ * long time may elapse between the time that the signal was issued
+ * and this thread is awakened and this leaves a door open to
+ * several race conditions.
+ */
+
+ /* Check if an error occurred while we were sleeping. Expected
+ * errors include EINTR meaning that we were awakened by a signal
+ * or ETIMEDOUT meaning that the timer expired for the case of
+ * sem_timedwait().
+ *
+ * If we were not awakened by a signal or a timeout, then
+ * nxsem_add_holder() was called by logic in sem_wait() fore this
+ * thread was restarted.
+ */
+
+ ret = rtcb->errcode != OK ? -rtcb->errcode : OK;
#ifdef CONFIG_PRIORITY_INHERITANCE
- sched_unlock();
+ sched_unlock();
#endif
- }
}
leave_critical_section(flags);
@@ -254,6 +251,12 @@ int sem_wait(FAR sem_t *sem)
int errcode;
int ret;
+ if (sem == NULL)
+ {
+ set_errno(EINVAL);
+ return ERROR;
+ }
+
/* sem_wait() is a cancellation point */
if (enter_cancellation_point())