Skip to content

Commit

Permalink
pthread_cond remove csection
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
hujun260 authored and acassis committed Nov 2, 2024
1 parent 7f84a64 commit 2c0e5e8
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 61 deletions.
1 change: 1 addition & 0 deletions include/pthread.h
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ struct pthread_cond_s
{
sem_t sem;
clockid_t clockid;
volatile int16_t lock_count;
};

#ifndef __PTHREAD_COND_T_DEFINED
Expand Down
1 change: 1 addition & 0 deletions libs/libc/pthread/pthread_condinit.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
22 changes: 1 addition & 21 deletions sched/pthread/pthread_condclockwait.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 */

Expand All @@ -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");
Expand All @@ -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();
Expand Down
34 changes: 4 additions & 30 deletions sched/pthread/pthread_condsignal.c
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
int pthread_cond_signal(FAR pthread_cond_t *cond)
{
int ret = OK;
int sval;

sinfo("cond=%p\n", cond);

Expand All @@ -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);
}
}

Expand Down
11 changes: 1 addition & 10 deletions sched/pthread/pthread_condwait.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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)
{
Expand All @@ -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
Expand Down

0 comments on commit 2c0e5e8

Please sign in to comment.