OrgApache/incubator-nuttx
OrgApache
/
incubator-nuttx
mirror of https://github.com/apache/incubator-nuttx.git
1
0
Fork 0
Code Issues Releases Wiki Activity

Revert "sched: Remove a redundant critical section" 

There is a potential problem that can lead to deadlock at
condition wait, if the timeout of watchdog is a very small value
(1 tick ?), the timer interrupt will come before the nxsem_wait()

Revert "sched: pthread: Remove a redundant critical section in pthread_condclockwsait.c"
Revert "sched: semaphore: Remove a redundant critical section in nxsem_clockwait()"
Revert "sched: semaphore: Remove a redundant critical section in nxsem_tickwait()"

This reverts commit 7758f3dcb1.
This reverts commit 2976bb212e.
This reverts commit 65dec5d10a.

Signed-off-by: chao.an <anchao@xiaomi.com>
This commit is contained in:
chao.an 2021-12-09 15:08:01 +08:00 committed by Masayuki Ishikawa
parent 0b7b8d274f
commit 102a6357ca
3 changed files with 85 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
sched/pthread/pthread_condclockwait.c
View File

@ -154,6 +154,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
FAR const struct timespec *abstime)
{
FAR struct tcb_s *rtcb = this_task();
irqstate_t flags;
sclock_t ticks;
int mypid = getpid();
int ret = OK;
@ -192,15 +193,14 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
{
sinfo("Give up mutex...\n");
/* NOTE: We do not need a critical section here,
* because nxsem_wait() uses a critical section and
* pthread_condtimedout() is called from wd_timer()
* which uses a critical section for SMP case
/* We must disable pre-emption and interrupts here so that
* the time stays valid until the wait begins. This adds
* complexity because we assure that interrupts and
* pre-emption are re-enabled correctly.
*/
/* REVISIT: Do we need to disable pre-emption? */
sched_lock();
flags = enter_critical_section();
/* Convert the timespec to clock ticks. We must disable pre-
* emption here so that this time stays valid until the wait
@ -208,7 +208,15 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
*/
ret = clock_abstime2ticks(clockid, abstime, &ticks);
if (ret == 0)
if (ret)
{
/* Restore interrupts (pre-emption will be enabled when
* we fall through the if/then/else)
*/
leave_critical_section(flags);
}
else
{
/* Check the absolute time to wait. If it is now or in the
* past, then just return with the timedout condition.
@ -216,6 +224,12 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
if (ticks <= 0)
{
/* Restore interrupts and indicate that we have already
* timed out. (pre-emption will be enabled when we fall
* through the if/then/else
*/
leave_critical_section(flags);
ret = ETIMEDOUT;
}
else
@ -238,7 +252,15 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
nlocks = mutex->nlocks;
#endif
ret = pthread_mutex_give(mutex);
if (ret == 0)
if (ret != 0)
{
/* Restore interrupts (pre-emption will be enabled
* when we fall through the if/then/else)
*/
leave_critical_section(flags);
}
else
{
/* Start the watchdog */
@ -277,6 +299,14 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
ret = status;
}
}
/* The interrupts stay disabled until after we sample
* the errno. This is because when debug is enabled
* and the console is used for debug output, then the
* errno can be altered by interrupt handling! (bad)
*/
leave_critical_section(flags);
}
/* Reacquire the mutex (retaining the ret). */
@ -302,7 +332,9 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
}
}
/* Re-enable pre-emption */
/* Re-enable pre-emption (It is expected that interrupts
* have already been re-enabled in the above logic)
*/
sched_unlock();
}

26
sched/semaphore/sem_clockwait.c
View File

@ -92,6 +92,7 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
FAR const struct timespec *abstime)
{
FAR struct tcb_s *rtcb = this_task();
irqstate_t flags;
sclock_t ticks;
int status;
int ret = ERROR;
@ -109,11 +110,16 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
}
#endif
/* NOTE: We do not need a critical section here, because
* nxsem_wait() and nxsem_timeout() use a critical section
* in the functions.
/* 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
* posting semaphores and to prevent race conditions with watchdog
* timeout. This is not too bad because interrupts will be re-
* enabled while we are blocked waiting for the semaphore.
*/
flags = enter_critical_section();
/* Try to take the semaphore without waiting. */
ret = nxsem_trywait(sem);
@ -121,7 +127,7 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
{
/* We got it! */
goto out;
goto success_with_irqdisabled;
}
/* We will have to wait for the semaphore. Make sure that we were provided
@ -131,7 +137,7 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
{
ret = -EINVAL;
goto out;
goto errout_with_irqdisabled;
}
/* Convert the timespec to clock ticks. We must have interrupts
@ -148,7 +154,7 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
if (status == OK && ticks <= 0)
{
ret = -ETIMEDOUT;
goto out;
goto errout_with_irqdisabled;
}
/* Handle any time-related errors */
@ -156,7 +162,7 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
if (status != OK)
{
ret = -status;
goto out;
goto errout_with_irqdisabled;
}
/* Start the watchdog */
@ -173,7 +179,11 @@ int nxsem_clockwait(FAR sem_t *sem, clockid_t clockid,
wd_cancel(&rtcb->waitdog);
out:
/* We can now restore interrupts and delete the watchdog */
success_with_irqdisabled:
errout_with_irqdisabled:
leave_critical_section(flags);
return ret;
}

33
sched/semaphore/sem_tickwait.c
View File

@ -71,16 +71,22 @@
int nxsem_tickwait(FAR sem_t *sem, clock_t start, uint32_t delay)
{
FAR struct tcb_s *rtcb = this_task();
irqstate_t flags;
clock_t elapsed;
int ret;
DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
/* NOTE: We do not need a critical section here, because
* nxsem_wait() and nxsem_timeout() use a critical section
* in the functions.
/* 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
* posting semaphores and to prevent race conditions with watchdog
* timeout. This is not too bad because interrupts will be re-
* enabled while we are blocked waiting for the semaphore.
*/
flags = enter_critical_section();
/* Try to take the semaphore without waiting. */
ret = nxsem_trywait(sem);
@ -88,7 +94,7 @@ int nxsem_tickwait(FAR sem_t *sem, clock_t start, uint32_t delay)
{
/* We got it! */
goto out;
goto success_with_irqdisabled;
}
/* We will have to wait for the semaphore. Make sure that we were provided
@ -99,7 +105,7 @@ int nxsem_tickwait(FAR sem_t *sem, clock_t start, uint32_t delay)
{
/* Return the errno from nxsem_trywait() */
goto out;
goto errout_with_irqdisabled;
}
/* Adjust the delay for any time since the delay was calculated */
@ -108,7 +114,7 @@ int nxsem_tickwait(FAR sem_t *sem, clock_t start, uint32_t delay)
if (/* elapsed >= (UINT32_MAX / 2) || */ elapsed >= delay)
{
ret = -ETIMEDOUT;
goto out;
goto errout_with_irqdisabled;
}
delay -= elapsed;
@ -125,8 +131,21 @@ int nxsem_tickwait(FAR sem_t *sem, clock_t start, uint32_t delay)
wd_cancel(&rtcb->waitdog);
out:
if (ret < 0)
{
goto errout_with_irqdisabled;
}
/* We can now restore interrupts */
/* Success exits */
success_with_irqdisabled:
/* Error exits */
errout_with_irqdisabled:
leave_critical_section(flags);
return ret;
}