Skip to content

Port "condvar" bugfix to runtime 5 #2076

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions ocaml/otherlibs/systhreads/st_pthreads.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,15 @@ typedef struct {
pthread_mutex_t lock; /* to protect contents */
uintnat busy; /* 0 = free, 1 = taken */
atomic_uintnat waiters; /* number of threads waiting on master lock */
pthread_cond_t is_free; /* signaled when free */
custom_condvar is_free; /* signaled when free */
} st_masterlock;

static void st_masterlock_init(st_masterlock * m)
{
if (!m->init) {
// FIXME: check errors
pthread_mutex_init(&m->lock, NULL);
pthread_cond_init(&m->is_free, NULL);
custom_condvar_init(&m->is_free);
m->init = 1;
}
m->busy = 1;
Expand Down Expand Up @@ -156,7 +156,7 @@ static void st_masterlock_acquire(st_masterlock *m)
pthread_mutex_lock(&m->lock);
while (m->busy) {
atomic_fetch_add(&m->waiters, +1);
pthread_cond_wait(&m->is_free, &m->lock);
custom_condvar_wait(&m->is_free, &m->lock);
atomic_fetch_add(&m->waiters, -1);
}
m->busy = 1;
Expand All @@ -171,7 +171,7 @@ static void st_masterlock_release(st_masterlock * m)
pthread_mutex_lock(&m->lock);
m->busy = 0;
st_bt_lock_release(m);
pthread_cond_signal(&m->is_free);
custom_condvar_signal(&m->is_free);
pthread_mutex_unlock(&m->lock);

return;
Expand Down Expand Up @@ -203,7 +203,7 @@ Caml_inline void st_thread_yield(st_masterlock * m)

m->busy = 0;
atomic_fetch_add(&m->waiters, +1);
pthread_cond_signal(&m->is_free);
custom_condvar_signal(&m->is_free);
/* releasing the domain lock but not triggering bt messaging
messaging the bt should not be required because yield assumes
that a thread will resume execution (be it the yielding thread
Expand All @@ -215,7 +215,7 @@ Caml_inline void st_thread_yield(st_masterlock * m)
wait, which is good: we'll reliably continue waiting until the next
yield() or enter_blocking_section() call (or we see a spurious condvar
wakeup, which are rare at best.) */
pthread_cond_wait(&m->is_free, &m->lock);
custom_condvar_wait(&m->is_free, &m->lock);
} while (m->busy);

m->busy = 1;
Expand All @@ -233,7 +233,7 @@ Caml_inline void st_thread_yield(st_masterlock * m)
typedef struct st_event_struct {
pthread_mutex_t lock; /* to protect contents */
int status; /* 0 = not triggered, 1 = triggered */
pthread_cond_t triggered; /* signaled when triggered */
custom_condvar triggered; /* signaled when triggered */
} * st_event;


Expand All @@ -244,7 +244,7 @@ static int st_event_create(st_event * res)
if (e == NULL) return ENOMEM;
rc = pthread_mutex_init(&e->lock, NULL);
if (rc != 0) { caml_stat_free(e); return rc; }
rc = pthread_cond_init(&e->triggered, NULL);
rc = custom_condvar_init(&e->triggered);
if (rc != 0)
{ pthread_mutex_destroy(&e->lock); caml_stat_free(e); return rc; }
e->status = 0;
Expand All @@ -256,7 +256,7 @@ static int st_event_destroy(st_event e)
{
int rc1, rc2;
rc1 = pthread_mutex_destroy(&e->lock);
rc2 = pthread_cond_destroy(&e->triggered);
rc2 = custom_condvar_destroy(&e->triggered);
caml_stat_free(e);
return rc1 != 0 ? rc1 : rc2;
}
Expand All @@ -269,7 +269,7 @@ static int st_event_trigger(st_event e)
e->status = 1;
rc = pthread_mutex_unlock(&e->lock);
if (rc != 0) return rc;
rc = pthread_cond_broadcast(&e->triggered);
rc = custom_condvar_broadcast(&e->triggered);
return rc;
}

Expand All @@ -279,7 +279,7 @@ static int st_event_wait(st_event e)
rc = pthread_mutex_lock(&e->lock);
if (rc != 0) return rc;
while(e->status == 0) {
rc = pthread_cond_wait(&e->triggered, &e->lock);
rc = custom_condvar_wait(&e->triggered, &e->lock);
if (rc != 0) return rc;
}
rc = pthread_mutex_unlock(&e->lock);
Expand Down
3 changes: 2 additions & 1 deletion ocaml/runtime/caml/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <string.h>
#include "config.h"
#include "mlvalues.h"
#include "sync.h"
#include "sys.h"

#if defined(MAP_ANON) && !defined(MAP_ANONYMOUS)
Expand Down Expand Up @@ -106,7 +107,7 @@ void caml_plat_assert_locked(caml_plat_mutex*);
void caml_plat_assert_all_locks_unlocked(void);
Caml_inline void caml_plat_unlock(caml_plat_mutex*);
void caml_plat_mutex_free(caml_plat_mutex*);
typedef struct { pthread_cond_t cond; caml_plat_mutex* mutex; } caml_plat_cond;
typedef struct { custom_condvar cond; caml_plat_mutex* mutex; } caml_plat_cond;
#define CAML_PLAT_COND_INITIALIZER(m) { PTHREAD_COND_INITIALIZER, m }
void caml_plat_cond_init(caml_plat_cond*, caml_plat_mutex*);
void caml_plat_wait(caml_plat_cond*);
Expand Down
13 changes: 13 additions & 0 deletions ocaml/runtime/caml/sync.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,19 @@ typedef pthread_mutex_t * sync_mutex;
CAMLextern int caml_mutex_lock(sync_mutex mut);
CAMLextern int caml_mutex_unlock(sync_mutex mut);

/* If we're using glibc, use a custom condition variable implementation to
avoid this bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25847

For now we only have this on linux because it directly uses the linux futex
syscalls. */
#if defined(__linux__) && defined(__GNU_LIBRARY__) && defined(__GLIBC__) && defined(__GLIBC_MINOR__)
typedef struct {
volatile unsigned counter;
} custom_condvar;
#else
typedef pthread_cond_t custom_condvar;
#endif

#endif /* CAML_INTERNALS */

#endif /* CAML_SYNC_H */
20 changes: 8 additions & 12 deletions ocaml/runtime/platform.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
#include "caml/domain.h"
#endif

#include "caml/alloc.h"
#include "sync_posix.h"

/* Error reporting */

void caml_plat_fatal_error(const char * action, int err)
Expand Down Expand Up @@ -89,14 +92,7 @@ void caml_plat_mutex_free(caml_plat_mutex* m)

static void caml_plat_cond_init_aux(caml_plat_cond *cond)
{
pthread_condattr_t attr;
pthread_condattr_init(&attr);
#if defined(_POSIX_TIMERS) && \
defined(_POSIX_MONOTONIC_CLOCK) && \
_POSIX_MONOTONIC_CLOCK != (-1)
pthread_condattr_setclock(&attr, CLOCK_MONOTONIC);
#endif
pthread_cond_init(&cond->cond, &attr);
custom_condvar_init(&cond->cond);
}

/* Condition variables */
Expand All @@ -109,24 +105,24 @@ void caml_plat_cond_init(caml_plat_cond* cond, caml_plat_mutex* m)
void caml_plat_wait(caml_plat_cond* cond)
{
caml_plat_assert_locked(cond->mutex);
check_err("wait", pthread_cond_wait(&cond->cond, cond->mutex));
check_err("wait", custom_condvar_wait(&cond->cond, cond->mutex));
}

void caml_plat_broadcast(caml_plat_cond* cond)
{
caml_plat_assert_locked(cond->mutex);
check_err("cond_broadcast", pthread_cond_broadcast(&cond->cond));
check_err("cond_broadcast", custom_condvar_broadcast(&cond->cond));
}

void caml_plat_signal(caml_plat_cond* cond)
{
caml_plat_assert_locked(cond->mutex);
check_err("cond_signal", pthread_cond_signal(&cond->cond));
check_err("cond_signal", custom_condvar_signal(&cond->cond));
}

void caml_plat_cond_free(caml_plat_cond* cond)
{
check_err("cond_free", pthread_cond_destroy(&cond->cond));
check_err("cond_free", custom_condvar_destroy(&cond->cond));
cond->mutex=0;
}

Expand Down
96 changes: 89 additions & 7 deletions ocaml/runtime/sync_posix.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@
#include <pthread.h>
#include <string.h>

#ifdef __linux__
#include <features.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <linux/futex.h>
#include <limits.h>
#endif

typedef int sync_retcode;

/* Mutexes */
Expand Down Expand Up @@ -82,18 +90,92 @@ Caml_inline int sync_mutex_unlock(sync_mutex m)
return pthread_mutex_unlock(m);
}

/* If we're using glibc, use a custom condition variable implementation to
avoid this bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25847

For now we only have this on linux because it directly uses the linux futex
syscalls. */
#if defined(__linux__) && defined(__GNU_LIBRARY__) && defined(__GLIBC__) && defined(__GLIBC_MINOR__)
static int custom_condvar_init(custom_condvar * cv)
{
cv->counter = 0;
return 0;
}

static int custom_condvar_destroy(custom_condvar * cv)
{
return 0;
}

static int custom_condvar_wait(custom_condvar * cv, pthread_mutex_t * mutex)
{
unsigned old_count = cv->counter;
pthread_mutex_unlock(mutex);
syscall(SYS_futex, &cv->counter, FUTEX_WAIT_PRIVATE, old_count, NULL, NULL, 0);
pthread_mutex_lock(mutex);
return 0;
}

static int custom_condvar_signal(custom_condvar * cv)
{
__sync_add_and_fetch(&cv->counter, 1);
syscall(SYS_futex, &cv->counter, FUTEX_WAKE_PRIVATE, 1, NULL, NULL, 0);
return 0;
}

static int custom_condvar_broadcast(custom_condvar * cv)
{
__sync_add_and_fetch(&cv->counter, 1);
syscall(SYS_futex, &cv->counter, FUTEX_WAKE_PRIVATE, INT_MAX, NULL, NULL, 0);
return 0;
}
#else
static int custom_condvar_init(custom_condvar * cv)
{
pthread_condattr_t attr;
pthread_condattr_init(&attr);
#if defined(_POSIX_TIMERS) && \
defined(_POSIX_MONOTONIC_CLOCK) && \
_POSIX_MONOTONIC_CLOCK != (-1)
pthread_condattr_setclock(&attr, CLOCK_MONOTONIC);
#endif
return pthread_cond_init(cv, &attr);
}

static int custom_condvar_destroy(custom_condvar * cv)
{
return pthread_cond_destroy(cv);
}

static int custom_condvar_wait(custom_condvar * cv, pthread_mutex_t * mutex)
{
return pthread_cond_wait(cv, mutex);
}

static int custom_condvar_signal(custom_condvar * cv)
{
return pthread_cond_signal(cv);
}

static int custom_condvar_broadcast(custom_condvar * cv)
{
return pthread_cond_broadcast(cv);
}
#endif


/* Condition variables */

typedef pthread_cond_t * sync_condvar;
typedef custom_condvar * sync_condvar;

#define Condition_val(v) (* (sync_condvar *) Data_custom_val(v))

Caml_inline int sync_condvar_create(sync_condvar * res)
{
int rc;
sync_condvar c = caml_stat_alloc_noexc(sizeof(pthread_cond_t));
sync_condvar c = caml_stat_alloc_noexc(sizeof(custom_condvar));
if (c == NULL) return ENOMEM;
rc = pthread_cond_init(c, NULL);
rc = custom_condvar_init(c);
if (rc != 0) { caml_stat_free(c); return rc; }
*res = c;
return 0;
Expand All @@ -102,24 +184,24 @@ Caml_inline int sync_condvar_create(sync_condvar * res)
Caml_inline int sync_condvar_destroy(sync_condvar c)
{
int rc;
rc = pthread_cond_destroy(c);
rc = custom_condvar_destroy(c);
caml_stat_free(c);
return rc;
}

Caml_inline int sync_condvar_signal(sync_condvar c)
{
return pthread_cond_signal(c);
return custom_condvar_signal(c);
}

Caml_inline int sync_condvar_broadcast(sync_condvar c)
{
return pthread_cond_broadcast(c);
return custom_condvar_broadcast(c);
}

Caml_inline int sync_condvar_wait(sync_condvar c, sync_mutex m)
{
return pthread_cond_wait(c, m);
return custom_condvar_wait(c, m);
}

/* Reporting errors */
Expand Down