From 3fe1910509e32d611b3a7b8503502103bc53b5e4 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Thomas=20K=C3=B6nig?= Date: Thu, 13 Feb 2020 22:22:04 +0100 Subject: [PATCH] Use au->lock exclusively for locking in async I/O. 2020-02-18 Thomas Koenig PR fortran/93599 * io/async.c (destroy_adv_cond): Do not destroy lock. (async_io): Make sure au->lock is locked for finishing of thread. Do not lock/unlock around signalling emptysignal. Unlock au->lock before return. (init_adv_cond): Do not initialize lock. (enqueue_transfer): Unlock after signal. (enqueue_done_id): Likewise. (enqueue_done): Likewise. (enqueue_close): Likewise. (enqueue_data_transfer): Likewise. (async_wait_id): Do not lock/unlock around signalling au->work. (async_wait): Unlock after signal. * io/async.h (SIGNAL): Add comment about needed au->lock. Remove locking/unlocking of advcond->lock. (WAIT_SIGNAL_MUTEX): Add comment. Remove locking/unlocking of advcond->lock. Unlock mutex only at the end. Loop on __ghread_cond_wait returning zero. (REVOKE_SIGNAL): Add comment. Remove locking/unlocking of advcond->lock. (struct adv_cond): Remove mutex from struct. asdf --- libgfortran/ChangeLog | 24 ++++++++++++++++++++++ libgfortran/io/async.c | 23 +++++++++------------ libgfortran/io/async.h | 45 +++++++++++++++++++++--------------------- 3 files changed, 55 insertions(+), 37 deletions(-) diff --git a/libgfortran/ChangeLog b/libgfortran/ChangeLog index 5632fbc8272..bef6306fbc6 100644 --- a/libgfortran/ChangeLog +++ b/libgfortran/ChangeLog @@ -1,3 +1,27 @@ +2020-02-18 Thomas Koenig + + PR fortran/93599 + * io/async.c (destroy_adv_cond): Do not destroy lock. + (async_io): Make sure au->lock is locked for finishing of thread. + Do not lock/unlock around signalling emptysignal. Unlock au->lock + before return. + (init_adv_cond): Do not initialize lock. + (enqueue_transfer): Unlock after signal. + (enqueue_done_id): Likewise. + (enqueue_done): Likewise. + (enqueue_close): Likewise. + (enqueue_data_transfer): Likewise. + (async_wait_id): Do not lock/unlock around signalling au->work. + (async_wait): Unlock after signal. + * io/async.h (SIGNAL): Add comment about needed au->lock. + Remove locking/unlocking of advcond->lock. + (WAIT_SIGNAL_MUTEX): Add comment. Remove locking/unlocking of + advcond->lock. Unlock mutex only at the end. Loop on + __ghread_cond_wait returning zero. + (REVOKE_SIGNAL): Add comment. Remove locking/unlocking of + advcond->lock. + (struct adv_cond): Remove mutex from struct. + 2020-02-12 Sandra Loosemore PR libstdc++/79193 diff --git a/libgfortran/io/async.c b/libgfortran/io/async.c index ab214af8a66..63b9158c0ba 100644 --- a/libgfortran/io/async.c +++ b/libgfortran/io/async.c @@ -80,7 +80,6 @@ update_pdt (st_parameter_dt **old, st_parameter_dt *new) { static void destroy_adv_cond (struct adv_cond *ac) { - T_ERROR (__gthread_mutex_destroy, &ac->lock); T_ERROR (__gthread_cond_destroy, &ac->signal); } @@ -156,6 +155,7 @@ async_io (void *arg) case AIO_CLOSE: NOTE ("Received AIO_CLOSE"); + LOCK (&au->lock); goto finish_thread; default: @@ -175,7 +175,6 @@ async_io (void *arg) else if (ctq->type == AIO_CLOSE) { NOTE ("Received AIO_CLOSE during error condition"); - UNLOCK (&au->lock); goto finish_thread; } } @@ -189,9 +188,7 @@ async_io (void *arg) au->tail = NULL; au->head = NULL; au->empty = 1; - UNLOCK (&au->lock); SIGNAL (&au->emptysignal); - LOCK (&au->lock); } finish_thread: au->tail = NULL; @@ -199,6 +196,7 @@ async_io (void *arg) au->empty = 1; SIGNAL (&au->emptysignal); free (ctq); + UNLOCK (&au->lock); return NULL; } @@ -223,7 +221,6 @@ static void init_adv_cond (struct adv_cond *ac) { ac->pending = 0; - __GTHREAD_MUTEX_INIT_FUNCTION (&ac->lock); __GTHREAD_COND_INIT_FUNCTION (&ac->signal); } @@ -279,8 +276,8 @@ enqueue_transfer (async_unit *au, transfer_args *arg, enum aio_do type) au->tail = tq; REVOKE_SIGNAL (&(au->emptysignal)); au->empty = false; - UNLOCK (&au->lock); SIGNAL (&au->work); + UNLOCK (&au->lock); } /* Enqueue an st_write_done or st_read_done which contains an ID. */ @@ -303,8 +300,8 @@ enqueue_done_id (async_unit *au, enum aio_do type) au->empty = false; ret = au->id.high++; NOTE ("Enqueue id: %d", ret); - UNLOCK (&au->lock); SIGNAL (&au->work); + UNLOCK (&au->lock); return ret; } @@ -324,8 +321,8 @@ enqueue_done (async_unit *au, enum aio_do type) au->tail = tq; REVOKE_SIGNAL (&(au->emptysignal)); au->empty = false; - UNLOCK (&au->lock); SIGNAL (&au->work); + UNLOCK (&au->lock); } /* Enqueue a CLOSE statement. */ @@ -344,8 +341,8 @@ enqueue_close (async_unit *au) au->tail = tq; REVOKE_SIGNAL (&(au->emptysignal)); au->empty = false; - UNLOCK (&au->lock); SIGNAL (&au->work); + UNLOCK (&au->lock); } /* The asynchronous unit keeps the currently active PDT around. @@ -374,9 +371,9 @@ enqueue_data_transfer_init (async_unit *au, st_parameter_dt *dt, int read_flag) au->tail->next = tq; au->tail = tq; REVOKE_SIGNAL (&(au->emptysignal)); - au->empty = 0; - UNLOCK (&au->lock); + au->empty = false; SIGNAL (&au->work); + UNLOCK (&au->lock); } /* Collect the errors that may have happened asynchronously. Return true if @@ -430,9 +427,7 @@ async_wait_id (st_parameter_common *cmp, async_unit *au, int i) NOTE ("Waiting for id %d", i); if (au->id.waiting < i) au->id.waiting = i; - UNLOCK (&au->lock); SIGNAL (&(au->work)); - LOCK (&au->lock); WAIT_SIGNAL_MUTEX (&(au->id.done), (au->id.low >= au->id.waiting || au->empty), &au->lock); LOCK (&au->lock); @@ -454,8 +449,8 @@ async_wait (st_parameter_common *cmp, async_unit *au) if (cmp == NULL) cmp = au->error.cmp; - SIGNAL (&(au->work)); LOCK (&(au->lock)); + SIGNAL (&(au->work)); if (au->empty) { diff --git a/libgfortran/io/async.h b/libgfortran/io/async.h index c6b2e0f94bd..17d303c127b 100644 --- a/libgfortran/io/async.h +++ b/libgfortran/io/async.h @@ -229,44 +229,44 @@ #if ASYNC_IO +/* au->lock has to be held when calling this macro. */ + #define SIGNAL(advcond) do{ \ - INTERN_LOCK (&(advcond)->lock); \ (advcond)->pending = 1; \ DEBUG_PRINTF ("%s%-75s %20s():%-5d %18p\n", aio_prefix, DEBUG_ORANGE "SIGNAL: " DEBUG_NORM \ #advcond, __FUNCTION__, __LINE__, (void *) advcond); \ - T_ERROR (__gthread_cond_broadcast, &(advcond)->signal); \ - INTERN_UNLOCK (&(advcond)->lock); \ + T_ERROR (__gthread_cond_broadcast, &(advcond)->signal); \ } while (0) +/* Has to be entered with mutex locked. */ + #define WAIT_SIGNAL_MUTEX(advcond, condition, mutex) do{ \ __label__ finish; \ - INTERN_LOCK (&((advcond)->lock)); \ DEBUG_PRINTF ("%s%-75s %20s():%-5d %18p\n", aio_prefix, DEBUG_BLUE "WAITING: " DEBUG_NORM \ #advcond, __FUNCTION__, __LINE__, (void *) advcond); \ - if ((advcond)->pending || (condition)){ \ - UNLOCK (mutex); \ + if ((advcond)->pending || (condition)) \ goto finish; \ - } \ - UNLOCK (mutex); \ - while (!__gthread_cond_wait(&(advcond)->signal, &(advcond)->lock)) { \ - { int cond; \ - LOCK (mutex); cond = condition; UNLOCK (mutex); \ - if (cond){ \ - DEBUG_PRINTF ("%s%-75s %20s():%-5d %18p\n", aio_prefix, DEBUG_ORANGE "REC: " DEBUG_NORM \ - #advcond, __FUNCTION__, __LINE__, (void *)advcond); \ - break; \ - } \ + while (1) \ + { \ + int err_ret = __gthread_cond_wait(&(advcond)->signal, mutex); \ + if (err_ret) internal_error (NULL, "WAIT_SIGNAL_MUTEX failed"); \ + if (condition) \ + { \ + DEBUG_PRINTF ("%s%-75s %20s():%-5d %18p\n", aio_prefix, DEBUG_ORANGE \ + "REC: " DEBUG_NORM \ + #advcond, __FUNCTION__, __LINE__, (void *)advcond); \ + break; \ + } \ } \ - } \ finish: \ - (advcond)->pending = 0; \ - INTERN_UNLOCK (&((advcond)->lock)); \ - } while (0) + (advcond)->pending = 0; \ + UNLOCK (mutex); \ + } while (0) + +/* au->lock has to be held when calling this macro. */ #define REVOKE_SIGNAL(advcond) do{ \ - INTERN_LOCK (&(advcond)->lock); \ (advcond)->pending = 0; \ - INTERN_UNLOCK (&(advcond)->lock); \ } while (0) #else @@ -330,7 +330,6 @@ struct adv_cond { #if ASYNC_IO int pending; - __gthread_mutex_t lock; __gthread_cond_t signal; #endif }; -- 2.30.2