@@ -61,20 +61,15 @@ st_close (st_parameter_close *clp)
find_option (&clp->common, clp->status, clp->status_len,
status_opt, "Bad STATUS parameter in CLOSE statement");
- u = find_unit (clp->common.unit);
+ LOCK (&unit_lock);
+ u = find_unit_locked (clp->common.unit);
if (ASYNC_IO && u && u->au)
if (async_wait (&(clp->common), u->au))
- {
- library_end ();
- return;
- }
+ goto done;
if ((clp->common.flags & IOPARM_LIBRETURN_MASK) != IOPARM_LIBRETURN_OK)
- {
- library_end ();
- return;
- }
+ goto done;
if (u != NULL)
{
@@ -123,6 +118,8 @@ st_close (st_parameter_close *clp)
#endif
}
- /* CLOSE on unconnected unit is legal and a no-op: F95 std., 9.3.5. */
+ done:
+ /* CLOSE on unconnected unit is legal and a no-op: F95 std., 9.3.5. */
+ UNLOCK (&unit_lock);
library_end ();
}
@@ -681,16 +681,6 @@ typedef struct gfc_unit
struct async_unit *au;
__gthread_mutex_t lock;
- /* Number of threads waiting to acquire this unit's lock.
- When non-zero, close_unit doesn't only removes the unit
- from the UNIT_ROOT tree, but doesn't free it and the
- last of the waiting threads will do that.
- This must be either atomically increased/decreased, or
- always guarded by UNIT_LOCK. */
- int waiting;
- /* Flag set by close_unit if the unit as been closed.
- Must be manipulated under unit's lock. */
- int closed;
/* For traversing arrays */
array_loop_spec *ls;
@@ -780,6 +770,9 @@ internal_proto(stash_internal_unit);
extern gfc_unit *find_unit (int);
internal_proto(find_unit);
+extern gfc_unit *find_unit_locked (int);
+internal_proto(find_unit_locked);
+
extern gfc_unit *find_or_create_unit (int);
internal_proto(find_or_create_unit);
@@ -973,50 +966,6 @@ internal_proto(size_from_complex_kind);
extern void free_ionml (st_parameter_dt *);
internal_proto(free_ionml);
-static inline void
-inc_waiting_locked (gfc_unit *u)
-{
-#ifdef HAVE_ATOMIC_FETCH_ADD
- (void) __atomic_fetch_add (&u->waiting, 1, __ATOMIC_RELAXED);
-#else
- u->waiting++;
-#endif
-}
-
-static inline int
-predec_waiting_locked (gfc_unit *u)
-{
-#ifdef HAVE_ATOMIC_FETCH_ADD
- /* Note that the pattern
-
- if (predec_waiting_locked (u) == 0)
- // destroy u
-
- could be further optimized by making this be an __ATOMIC_RELEASE,
- and then inserting a
-
- __atomic_thread_fence (__ATOMIC_ACQUIRE);
-
- inside the branch before destroying. But for now, lets keep it
- simple. */
- return __atomic_add_fetch (&u->waiting, -1, __ATOMIC_ACQ_REL);
-#else
- return --u->waiting;
-#endif
-}
-
-static inline void
-dec_waiting_unlocked (gfc_unit *u)
-{
-#ifdef HAVE_ATOMIC_FETCH_ADD
- (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED);
-#else
- __gthread_mutex_lock (&unit_lock);
- u->waiting--;
- __gthread_mutex_unlock (&unit_lock);
-#endif
-}
-
static inline void
memset4 (gfc_char4_t *p, gfc_char4_t c, int k)
@@ -2793,7 +2793,31 @@ nml_query (st_parameter_dt *dtp, char c)
/* Store the current unit and transfer to stdout. */
temp_unit = dtp->u.p.current_unit;
- dtp->u.p.current_unit = find_unit (options.stdout_unit);
+
+ /* Since we already hold the lock for stdin (temp_unit), trying to
+ acquire the unit_lock in order to find the stdout unit would be a
+ lock order inversion, which is not allowed as it could cause a
+ deadlock. Hence first try to lock unit_lock without blocking.
+ If that fails, fall back to unlocking temp_unit and then block
+ waiting for the lock in order to avoid the lock order inversion.
+ This is strictly speaking not correct, as it's possible another
+ thread could lock temp_unit inbetween causing inconsistent reads
+ from it. On the other hand it's a quite esoteric corner case, and
+ trying to read from stdin with multiple threads without other
+ synchronization is in any case a bad idea. */
+ if (!__gthread_mutex_trylock (&unit_lock))
+ {
+ dtp->u.p.current_unit = find_unit_locked (options.stdout_unit);
+ __gthread_mutex_unlock (&unit_lock);
+ }
+ else
+ {
+ __gthread_mutex_unlock (&temp_unit->lock);
+ __gthread_mutex_lock (&unit_lock);
+ __gthread_mutex_lock (&temp_unit->lock);
+ dtp->u.p.current_unit = find_unit_locked (options.stdout_unit);
+ __gthread_mutex_unlock (&unit_lock);
+ }
if (dtp->u.p.current_unit)
{
@@ -36,39 +36,30 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
UNIT_LOCK is a master lock, protecting UNIT_ROOT tree and UNIT_CACHE.
Concurrent use of different units should be supported, so
each unit has its own lock, LOCK.
- Open should be atomic with its reopening of units and list_read.c
- in several places needs find_unit another unit while holding stdin
- unit's lock, so it must be possible to acquire UNIT_LOCK while holding
- some unit's lock. Therefore to avoid deadlocks, it is forbidden
- to acquire unit's private locks while holding UNIT_LOCK, except
- for freshly created units (where no other thread can get at their
- address yet) or when using just trylock rather than lock operation.
- In addition to unit's private lock each unit has a WAITERS counter
- and CLOSED flag. WAITERS counter must be either only
- atomically incremented/decremented in all places (if atomic builtins
- are supported), or protected by UNIT_LOCK in all places (otherwise).
- CLOSED flag must be always protected by unit's LOCK.
- After finding a unit in UNIT_CACHE or UNIT_ROOT with UNIT_LOCK held,
- WAITERS must be incremented to avoid concurrent close from freeing
- the unit between unlocking UNIT_LOCK and acquiring unit's LOCK.
- Unit freeing is always done under UNIT_LOCK. If close_unit sees any
- WAITERS, it doesn't free the unit but instead sets the CLOSED flag
- and the thread that decrements WAITERS to zero while CLOSED flag is
- set is responsible for freeing it (while holding UNIT_LOCK).
- flush_all_units operation is iterating over the unit tree with
- increasing UNIT_NUMBER while holding UNIT_LOCK and attempting to
- flush each unit (and therefore needs the unit's LOCK held as well).
- To avoid deadlocks, it just trylocks the LOCK and if unsuccessful,
- remembers the current unit's UNIT_NUMBER, unlocks UNIT_LOCK, acquires
- unit's LOCK and after flushing reacquires UNIT_LOCK and restarts with
- the smallest UNIT_NUMBER above the last one flushed.
-
- If find_unit/find_or_create_unit/find_file/get_unit routines return
- non-NULL, the returned unit has its private lock locked and when the
- caller is done with it, it must call either unlock_unit or close_unit
- on it. unlock_unit or close_unit must be always called only with the
- private lock held. */
+ Lock ordering: UNIT_LOCK must be acquired before LOCK. It is NOT
+ allowed to acquire UNIT_LOCK while holding LOCK, as that can lead
+ to a deadlock. There is one exception to this rule, when reading a
+ namelist from stdin GFortran supports an extension where it can
+ print the namelist or elements thereof to stdout. To do this, both
+ stdin and stdout LOCK's must be acquired. See comment in
+ list_read.c for how this is done while avoiding deadlock.
+
+ Opening units: When creating a new unit, one can freely manipulate
+ it's LOCK as no other thread can yet access it. Only after
+ inserting the new unit into the UNIT_ROOT tree and unlocking
+ UNIT_LOCK other thread can access the new unit, and from that point
+ the same rules as for old units apply.
+
+ Opening units with NEWUNIT=: When opening a unit with the NEWUNIT=
+ specifier (and for internal units), the unit number allocator (see
+ below) is protected by newunit_lock. It is not permitted to acquire
+ any other lock with newunit_lock held.
+
+ Closing units: When closing a unit, UNIT_LOCK is first acquired,
+ then the unit's own LOCK, then the unit is closed (without
+ unlocking UNIT_LOCK!) and resources are freed. Only after
+ everything is done is UNIT_LOCK unlocked. */
/* Table of allocated newunit values. A simple solution would be to
@@ -91,6 +82,12 @@ static int newunit_size; /* Total number of elements in the newunits array. */
allocated and free units. */
static int newunit_lwi;
+#ifdef __GTHREAD_MUTEX_INIT
+static __gthread_mutex_t newunit_lock = __GTHREAD_MUTEX_INIT;
+#else
+static __gthread_mutex_t newunit_lock;
+#endif
+
/* Unit numbers assigned with NEWUNIT start from here. */
#define NEWUNIT_START -10
@@ -323,15 +320,15 @@ delete_unit (gfc_unit *old)
otherwise returns a locked unit. */
static gfc_unit *
-get_gfc_unit (int n, int do_create)
+get_gfc_unit (int n, int do_create, bool locked)
{
gfc_unit *p;
int c, created = 0;
NOTE ("Unit n=%d, do_create = %d", n, do_create);
- LOCK (&unit_lock);
+ if (!locked)
+ LOCK (&unit_lock);
-retry:
for (c = 0; c < CACHE_SIZE; c++)
if (unit_cache[c] != NULL && unit_cache[c]->unit_number == n)
{
@@ -369,41 +366,16 @@ retry:
{
/* Newly created units have their lock held already
from insert_unit. Just unlock UNIT_LOCK and return. */
- UNLOCK (&unit_lock);
+ if (!locked)
+ UNLOCK (&unit_lock);
return p;
}
found:
if (p != NULL && (p->child_dtio == 0))
- {
- /* Fast path. */
- if (! TRYLOCK (&p->lock))
- {
- /* assert (p->closed == 0); */
- UNLOCK (&unit_lock);
- return p;
- }
-
- inc_waiting_locked (p);
- }
-
-
- UNLOCK (&unit_lock);
-
- if (p != NULL && (p->child_dtio == 0))
- {
- LOCK (&p->lock);
- if (p->closed)
- {
- LOCK (&unit_lock);
- UNLOCK (&p->lock);
- if (predec_waiting_locked (p) == 0)
- destroy_unit_mutex (p);
- goto retry;
- }
-
- dec_waiting_unlocked (p);
- }
+ LOCK (&p->lock);
+ if (!locked)
+ UNLOCK (&unit_lock);
return p;
}
@@ -411,14 +383,19 @@ found:
gfc_unit *
find_unit (int n)
{
- return get_gfc_unit (n, 0);
+ return get_gfc_unit (n, 0, false);
}
+gfc_unit *
+find_unit_locked (int n)
+{
+ return get_gfc_unit (n, 0, true);
+}
gfc_unit *
find_or_create_unit (int n)
{
- return get_gfc_unit (n, 1);
+ return get_gfc_unit (n, 1, false);
}
@@ -553,7 +530,7 @@ get_unit (st_parameter_dt *dtp, int do_create)
dtp->u.p.unit_is_internal = 1;
dtp->common.unit = newunit_alloc ();
- unit = get_gfc_unit (dtp->common.unit, do_create);
+ unit = get_gfc_unit (dtp->common.unit, do_create, false);
set_internal_unit (dtp, unit, kind);
fbuf_init (unit, 128);
return unit;
@@ -571,10 +548,10 @@ get_unit (st_parameter_dt *dtp, int do_create)
{
if (dtp->common.unit > NEWUNIT_START) /* Reserved units. */
return NULL;
- return get_gfc_unit (dtp->common.unit, 0);
+ return get_gfc_unit (dtp->common.unit, 0, false);
}
- return get_gfc_unit (dtp->common.unit, do_create);
+ return get_gfc_unit (dtp->common.unit, do_create, false);
}
@@ -596,6 +573,7 @@ init_units (void)
#ifndef __GTHREAD_MUTEX_INIT
__GTHREAD_MUTEX_INIT_FUNCTION (&unit_lock);
+ __GTHREAD_MUTEX_INIT_FUNCTION (&newunit_lock);
#endif
if (sizeof (max_offset) == 8)
@@ -716,10 +694,13 @@ init_units (void)
static int
-close_unit_1 (gfc_unit *u, int locked)
+close_unit_1 (gfc_unit *u, bool locked)
{
int i, rc;
+ if (!locked)
+ LOCK (&u->lock);
+
if (ASYNC_IO && u->au)
async_close (u->au);
@@ -730,10 +711,6 @@ close_unit_1 (gfc_unit *u, int locked)
rc = (u->s == NULL) ? 0 : sclose (u->s) == -1;
- u->closed = 1;
- if (!locked)
- LOCK (&unit_lock);
-
for (i = 0; i < CACHE_SIZE; i++)
if (unit_cache[i] == u)
unit_cache[i] = NULL;
@@ -752,14 +729,7 @@ close_unit_1 (gfc_unit *u, int locked)
if (!locked)
UNLOCK (&u->lock);
- /* If there are any threads waiting in find_unit for this unit,
- avoid freeing the memory, the last such thread will free it
- instead. */
- if (u->waiting == 0)
- destroy_unit_mutex (u);
-
- if (!locked)
- UNLOCK (&unit_lock);
+ destroy_unit_mutex (u);
return rc;
}
@@ -774,12 +744,12 @@ unlock_unit (gfc_unit *u)
/* close_unit()-- Close a unit. The stream is closed, and any memory
associated with the stream is freed. Returns nonzero on I/O error.
- Should be called with the u->lock locked. */
+ Should be called with unit_lock and u->lock locked. */
int
close_unit (gfc_unit *u)
{
- return close_unit_1 (u, 0);
+ return close_unit_1 (u, true);
}
@@ -795,7 +765,7 @@ close_units (void)
{
LOCK (&unit_lock);
while (unit_root != NULL)
- close_unit_1 (unit_root, 1);
+ close_unit_1 (unit_root, false);
UNLOCK (&unit_lock);
free (newunits);
@@ -903,7 +873,7 @@ finish_last_advance_record (gfc_unit *u)
int
newunit_alloc (void)
{
- LOCK (&unit_lock);
+ LOCK (&newunit_lock);
if (!newunits)
{
newunits = xcalloc (16, 1);
@@ -917,7 +887,7 @@ newunit_alloc (void)
{
newunits[ii] = true;
newunit_lwi = ii + 1;
- UNLOCK (&unit_lock);
+ UNLOCK (&newunit_lock);
return -ii + NEWUNIT_START;
}
}
@@ -930,7 +900,7 @@ newunit_alloc (void)
memset (newunits + old_size, 0, old_size);
newunits[old_size] = true;
newunit_lwi = old_size + 1;
- UNLOCK (&unit_lock);
+ UNLOCK (&newunit_lock);
return -old_size + NEWUNIT_START;
}
@@ -943,7 +913,9 @@ newunit_free (int unit)
{
int ind = -unit + NEWUNIT_START;
assert(ind >= 0 && ind < newunit_size);
+ LOCK (&newunit_lock);
newunits[ind] = false;
if (ind < newunit_lwi)
newunit_lwi = ind;
+ UNLOCK (&newunit_lock);
}
@@ -1718,6 +1718,8 @@ find_file0 (gfc_unit *u, FIND_FILE0_DECL)
if (u == NULL)
return NULL;
+ LOCK (&u->lock);
+
#ifdef HAVE_WORKING_STAT
if (u->s != NULL)
{
@@ -1738,6 +1740,8 @@ find_file0 (gfc_unit *u, FIND_FILE0_DECL)
return u;
#endif
+ UNLOCK (&u->lock);
+
v = find_file0 (u->left, FIND_FILE0_ARGS);
if (v != NULL)
return v;
@@ -1775,100 +1779,33 @@ find_file (const char *file, gfc_charlen_type file_len)
#endif
LOCK (&unit_lock);
-retry:
u = find_file0 (unit_root, FIND_FILE0_ARGS);
- if (u != NULL)
- {
- /* Fast path. */
- if (! __gthread_mutex_trylock (&u->lock))
- {
- /* assert (u->closed == 0); */
- UNLOCK (&unit_lock);
- goto done;
- }
-
- inc_waiting_locked (u);
- }
UNLOCK (&unit_lock);
- if (u != NULL)
- {
- LOCK (&u->lock);
- if (u->closed)
- {
- LOCK (&unit_lock);
- UNLOCK (&u->lock);
- if (predec_waiting_locked (u) == 0)
- free (u);
- goto retry;
- }
-
- dec_waiting_unlocked (u);
- }
done:
free (path);
return u;
}
-static gfc_unit *
-flush_all_units_1 (gfc_unit *u, int min_unit)
+static void
+flush_all_units_1 (gfc_unit *u)
{
while (u != NULL)
{
- if (u->unit_number > min_unit)
- {
- gfc_unit *r = flush_all_units_1 (u->left, min_unit);
- if (r != NULL)
- return r;
- }
- if (u->unit_number >= min_unit)
- {
- if (__gthread_mutex_trylock (&u->lock))
- return u;
- if (u->s)
- sflush (u->s);
- UNLOCK (&u->lock);
- }
+ LOCK (&u->lock);
+ if (u->s)
+ sflush (u->s);
+ UNLOCK (&u->lock);
+ flush_all_units_1 (u->left);
u = u->right;
}
- return NULL;
}
void
flush_all_units (void)
{
- gfc_unit *u;
- int min_unit = 0;
-
LOCK (&unit_lock);
- do
- {
- u = flush_all_units_1 (unit_root, min_unit);
- if (u != NULL)
- inc_waiting_locked (u);
- UNLOCK (&unit_lock);
- if (u == NULL)
- return;
-
- LOCK (&u->lock);
-
- min_unit = u->unit_number + 1;
-
- if (u->closed == 0)
- {
- sflush (u->s);
- LOCK (&unit_lock);
- UNLOCK (&u->lock);
- (void) predec_waiting_locked (u);
- }
- else
- {
- LOCK (&unit_lock);
- UNLOCK (&u->lock);
- if (predec_waiting_locked (u) == 0)
- free (u);
- }
- }
- while (1);
+ flush_all_units_1 (unit_root);
+ UNLOCK (&unit_lock);
}