From c2cdce970d1af5b826cee4460f71a7fec317a368 Mon Sep 17 00:00:00 2001 From: xiangxistu <52819708+xiangxistu@users.noreply.github.com> Date: Fri, 1 Jul 2022 09:36:18 +0800 Subject: [PATCH] [fix] use atomic operation to protect pthread conditional variable. (#6113) * [fix] use atomic operation to protect pthread conditional variable. --- components/libc/posix/pthreads/pthread_cond.c | 189 +++++++++++++++--- 1 file changed, 163 insertions(+), 26 deletions(-) diff --git a/components/libc/posix/pthreads/pthread_cond.c b/components/libc/posix/pthreads/pthread_cond.c index 9bf49704ed..1fe9ede905 100644 --- a/components/libc/posix/pthreads/pthread_cond.c +++ b/components/libc/posix/pthreads/pthread_cond.c @@ -6,8 +6,10 @@ * Change Logs: * Date Author Notes * 2010-10-26 Bernard the first version + * 2022-06-27 xiangxistu use atomic operation to protect pthread conditional variable */ +#include #include #include "pthread_internal.h" @@ -31,14 +33,14 @@ int pthread_condattr_init(pthread_condattr_t *attr) RTM_EXPORT(pthread_condattr_init); int pthread_condattr_getclock(const pthread_condattr_t *attr, - clockid_t *clock_id) + clockid_t *clock_id) { return 0; } RTM_EXPORT(pthread_condattr_getclock); int pthread_condattr_setclock(pthread_condattr_t *attr, - clockid_t clock_id) + clockid_t clock_id) { return 0; } @@ -55,7 +57,7 @@ int pthread_condattr_getpshared(const pthread_condattr_t *attr, int *pshared) } RTM_EXPORT(pthread_condattr_getpshared); -int pthread_condattr_setpshared(pthread_condattr_t*attr, int pshared) +int pthread_condattr_setpshared(pthread_condattr_t *attr, int pshared) { if ((pshared != PTHREAD_PROCESS_PRIVATE) && (pshared != PTHREAD_PROCESS_SHARED)) @@ -84,14 +86,21 @@ int pthread_cond_init(pthread_cond_t *cond, const pthread_condattr_t *attr) rt_snprintf(cond_name, sizeof(cond_name), "cond%02d", cond_num++); - if (attr == RT_NULL) /* use default value */ + /* use default value */ + if (attr == RT_NULL) + { cond->attr = PTHREAD_PROCESS_PRIVATE; + } else + { cond->attr = *attr; + } result = rt_sem_init(&cond->sem, cond_name, 0, RT_IPC_FLAG_FIFO); if (result != RT_EOK) + { return EINVAL; + } /* detach the object from system object container */ rt_object_detach(&(cond->sem.parent.parent)); @@ -105,13 +114,26 @@ int pthread_cond_destroy(pthread_cond_t *cond) { rt_err_t result; if (cond == RT_NULL) + { return EINVAL; + } + /* which is not initialized */ if (cond->attr == -1) - return 0; /* which is not initialized */ + { + return 0; + } - result = rt_sem_trytake(&(cond->sem)); - if (result != RT_EOK) + if (!rt_list_isempty(&cond->sem.parent.suspend_thread)) + { return EBUSY; + } +__retry: + result = rt_sem_trytake(&(cond->sem)); + if (result == EBUSY) + { + pthread_cond_broadcast(cond); + goto __retry; + } /* clean condition */ rt_memset(cond, 0, sizeof(pthread_cond_t)); @@ -130,7 +152,6 @@ int pthread_cond_broadcast(pthread_cond_t *cond) if (cond->attr == -1) pthread_cond_init(cond, RT_NULL); - rt_enter_critical(); while (1) { /* try to take condition semaphore */ @@ -148,12 +169,9 @@ int pthread_cond_broadcast(pthread_cond_t *cond) } else { - rt_exit_critical(); - return EINVAL; } } - rt_exit_critical(); return 0; } @@ -161,6 +179,7 @@ RTM_EXPORT(pthread_cond_broadcast); int pthread_cond_signal(pthread_cond_t *cond) { + rt_base_t temp; rt_err_t result; if (cond == RT_NULL) @@ -168,36 +187,141 @@ int pthread_cond_signal(pthread_cond_t *cond) if (cond->attr == -1) pthread_cond_init(cond, RT_NULL); - result = rt_sem_release(&(cond->sem)); - if (result == RT_EOK) + /* disable interrupt */ + temp = rt_hw_interrupt_disable(); + if (rt_list_isempty(&cond->sem.parent.suspend_thread)) + { + /* enable interrupt */ + rt_hw_interrupt_enable(temp); return 0; + } + else + { + /* enable interrupt */ + rt_hw_interrupt_enable(temp); + result = rt_sem_release(&(cond->sem)); + if (result == RT_EOK) + { + return 0; + } - return 0; + return 0; + } } RTM_EXPORT(pthread_cond_signal); -rt_err_t _pthread_cond_timedwait(pthread_cond_t *cond, +rt_err_t _pthread_cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex, - rt_int32_t timeout) + rt_int32_t timeout) { - rt_err_t result; + rt_err_t result = RT_EOK; + rt_sem_t sem; + rt_int32_t time; + + sem = &(cond->sem); + if (sem == RT_NULL) + { + return -RT_ERROR; + } + time = timeout; if (!cond || !mutex) + { return -RT_ERROR; + } /* check whether initialized */ if (cond->attr == -1) + { pthread_cond_init(cond, RT_NULL); + } /* The mutex was not owned by the current thread at the time of the call. */ if (mutex->lock.owner != rt_thread_self()) + { return -RT_ERROR; - /* unlock a mutex failed */ - if (pthread_mutex_unlock(mutex) != 0) - return -RT_ERROR; + } - result = rt_sem_take(&(cond->sem), timeout); - /* lock mutex again */ - pthread_mutex_lock(mutex); + { + register rt_base_t temp; + struct rt_thread *thread; + + /* parameter check */ + RT_ASSERT(sem != RT_NULL); + RT_ASSERT(rt_object_get_type(&sem->parent.parent) == RT_Object_Class_Semaphore); + + /* disable interrupt */ + temp = rt_hw_interrupt_disable(); + + if (sem->value > 0) + { + /* semaphore is available */ + sem->value--; + + /* enable interrupt */ + rt_hw_interrupt_enable(temp); + } + else + { + /* no waiting, return with timeout */ + if (time == 0) + { + rt_hw_interrupt_enable(temp); + + return -RT_ETIMEOUT; + } + else + { + /* current context checking */ + RT_DEBUG_IN_THREAD_CONTEXT; + + /* semaphore is unavailable, push to suspend list */ + /* get current thread */ + thread = rt_thread_self(); + + /* reset thread error number */ + thread->error = RT_EOK; + + /* suspend thread */ + rt_thread_suspend(thread); + + /* Only support FIFO */ + rt_list_insert_before(&(sem->parent.suspend_thread), &(thread->tlist)); + + /** + rt_ipc_list_suspend(&(sem->parent.suspend_thread), + thread, + sem->parent.parent.flag); + */ + + /* has waiting time, start thread timer */ + if (time > 0) + { + /* reset the timeout of thread timer and start it */ + rt_timer_control(&(thread->thread_timer), + RT_TIMER_CTRL_SET_TIME, + &time); + rt_timer_start(&(thread->thread_timer)); + } + + /* to avoid the lost of singal< cond->sem > */ + if (pthread_mutex_unlock(mutex) != 0) + { + return -RT_ERROR; + } + + /* enable interrupt */ + rt_hw_interrupt_enable(temp); + + /* do schedule */ + rt_schedule(); + + result = thread->error; + + /* lock mutex again */ + pthread_mutex_lock(mutex); + } + } + } return result; } @@ -207,16 +331,26 @@ int pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex) { rt_err_t result; +__retry: result = _pthread_cond_timedwait(cond, mutex, RT_WAITING_FOREVER); if (result == RT_EOK) + { return 0; + } + else if (result == -RT_EINTR) + { + /* https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cond_wait.html + * These functions shall not return an error code of [EINTR]. + */ + goto __retry; + } return EINVAL; } RTM_EXPORT(pthread_cond_wait); -int pthread_cond_timedwait(pthread_cond_t *cond, - pthread_mutex_t *mutex, +int pthread_cond_timedwait(pthread_cond_t *cond, + pthread_mutex_t *mutex, const struct timespec *abstime) { int timeout; @@ -225,11 +359,14 @@ int pthread_cond_timedwait(pthread_cond_t *cond, timeout = rt_timespec_to_tick(abstime); result = _pthread_cond_timedwait(cond, mutex, timeout); if (result == RT_EOK) + { return 0; + } if (result == -RT_ETIMEOUT) + { return ETIMEDOUT; + } return EINVAL; } RTM_EXPORT(pthread_cond_timedwait); -