From 2c0e5e872b7c7e31069d9adb77dbfc01080d4824 Mon Sep 17 00:00:00 2001 From: hujun5 Date: Mon, 21 Oct 2024 17:29:19 +0800 Subject: [PATCH] pthread_cond remove csection reason: We decouple semcount from business logic by using an independent counting variable, which allows us to remove critical sections in many cases. Signed-off-by: hujun5 --- include/pthread.h | 1 + libs/libc/pthread/pthread_condinit.c | 1 + sched/pthread/pthread_condclockwait.c | 22 +---------------- sched/pthread/pthread_condsignal.c | 34 ++++----------------------- sched/pthread/pthread_condwait.c | 11 +-------- 5 files changed, 8 insertions(+), 61 deletions(-) diff --git a/include/pthread.h b/include/pthread.h index 0418dfacd0726..75764d96060f4 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -269,6 +269,7 @@ struct pthread_cond_s { sem_t sem; clockid_t clockid; + volatile int16_t lock_count; }; #ifndef __PTHREAD_COND_T_DEFINED diff --git a/libs/libc/pthread/pthread_condinit.c b/libs/libc/pthread/pthread_condinit.c index d4f133b948d34..6c88b19905a34 100644 --- a/libs/libc/pthread/pthread_condinit.c +++ b/libs/libc/pthread/pthread_condinit.c @@ -74,6 +74,7 @@ int pthread_cond_init(FAR pthread_cond_t *cond, else { cond->clockid = attr ? attr->clockid : CLOCK_REALTIME; + cond->lock_count = 0; } sinfo("Returning %d\n", ret); diff --git a/sched/pthread/pthread_condclockwait.c b/sched/pthread/pthread_condclockwait.c index ff557bbe5c1c1..3601991336192 100644 --- a/sched/pthread/pthread_condclockwait.c +++ b/sched/pthread/pthread_condclockwait.c @@ -75,7 +75,6 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond, clockid_t clockid, FAR const struct timespec *abstime) { - irqstate_t flags; int ret = OK; int status; @@ -114,14 +113,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond, sinfo("Give up mutex...\n"); - /* 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. - */ - - sched_lock(); - flags = enter_critical_section(); + cond->lock_count--; /* Give up the mutex */ @@ -136,12 +128,6 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond, } } - /* Restore interrupts (pre-emption will be enabled - * when we fall through the if/then/else) - */ - - leave_critical_section(flags); - /* Reacquire the mutex (retaining the ret). */ sinfo("Re-locking...\n"); @@ -151,12 +137,6 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond, { ret = status; } - - /* Re-enable pre-emption (It is expected that interrupts - * have already been re-enabled in the above logic) - */ - - sched_unlock(); } leave_cancellation_point(); diff --git a/sched/pthread/pthread_condsignal.c b/sched/pthread/pthread_condsignal.c index fb9453b712e09..1cdfa684480ad 100644 --- a/sched/pthread/pthread_condsignal.c +++ b/sched/pthread/pthread_condsignal.c @@ -55,7 +55,6 @@ int pthread_cond_signal(FAR pthread_cond_t *cond) { int ret = OK; - int sval; sinfo("cond=%p\n", cond); @@ -65,36 +64,11 @@ int pthread_cond_signal(FAR pthread_cond_t *cond) } else { - /* Get the current value of the semaphore */ - - if (nxsem_get_value(&cond->sem, &sval) != OK) - { - ret = EINVAL; - } - - /* If the value is less than zero (meaning that one or more - * thread is waiting), then post the condition semaphore. - * Only the highest priority waiting thread will get to execute - */ - - else + if (cond->lock_count < 0) { - /* One of my objectives in this design was to make - * pthread_cond_signal() usable from interrupt handlers. However, - * from interrupt handlers, you cannot take the associated mutex - * before signaling the condition. As a result, I think that - * there could be a race condition with the following logic which - * assumes that the if sval < 0 then the thread is waiting. - * Without the mutex, there is no atomic, protected operation that - * will guarantee this to be so. - */ - - sinfo("sval=%d\n", sval); - if (sval < 0) - { - sinfo("Signalling...\n"); - ret = -nxsem_post(&cond->sem); - } + sinfo("Signalling...\n"); + cond->lock_count++; + ret = -nxsem_post(&cond->sem); } } diff --git a/sched/pthread/pthread_condwait.c b/sched/pthread/pthread_condwait.c index fbc914897c9d3..43942de6fb307 100644 --- a/sched/pthread/pthread_condwait.c +++ b/sched/pthread/pthread_condwait.c @@ -60,7 +60,6 @@ int pthread_cond_wait(FAR pthread_cond_t *cond, FAR pthread_mutex_t *mutex) { int status; int ret; - irqstate_t flags; sinfo("cond=%p mutex=%p\n", cond, mutex); @@ -89,14 +88,9 @@ int pthread_cond_wait(FAR pthread_cond_t *cond, FAR pthread_mutex_t *mutex) sinfo("Give up mutex / take cond\n"); - flags = enter_critical_section(); - sched_lock(); + cond->lock_count--; ret = pthread_mutex_breaklock(mutex, &nlocks); - /* Take the semaphore. This may be awakened only be a signal (EINTR) - * or if the thread is canceled (ECANCELED) - */ - status = -nxsem_wait_uninterruptible(&cond->sem); if (ret == OK) { @@ -105,9 +99,6 @@ int pthread_cond_wait(FAR pthread_cond_t *cond, FAR pthread_mutex_t *mutex) ret = status; } - sched_unlock(); - leave_critical_section(flags); - /* Reacquire the mutex. * * When cancellation points are enabled, we need to hold the mutex