* [Patch] Fortran: Async I/O - avoid unlocked unlocking [PR100352]
@ 2021-05-01 12:59 Tobias Burnus
2021-05-01 16:15 ` Bernhard Reutner-Fischer
0 siblings, 1 reply; 2+ messages in thread
From: Tobias Burnus @ 2021-05-01 12:59 UTC (permalink / raw)
To: gcc-patches, fortran, Jerry DeLisle
[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]
As this patch is rather obvious, I intent to commit it tomorrow
as obvious, unless there is an OK earlier or other comments :-)
(And backport to GCC 11 in a couple of days.)
Before PR100352 (r11-7647),
st_write_done did:
st_write_done_worker (dtd)
unlock_unit (dtp->u.p.current_unit);
but st_write_done_worker did:
LOCK (&unit_lock);
newunit_free (dtp->common.unit);
UNLOCK (&unit_lock);
And this locking could lead to a deadlock.
Hence, 'unlock_unit' has been moved to st_write_done_worker
before the LOCK (&unit_lock).
That solved the issue, but async I/O does not use this lock
but, in async.c's async_io():
LOCK (&au->lock);
st_write_done_worker (au->pdt);
UNLOCK (&au->io_lock);
Which worked before the previous patch fine, but now
there is a bogus unlock_unit() alias UNLOCK (&u->lock);
although the unit is not locked.
Solution: Guard the unlock_unit.
Tobias
PS: The thread sanitizer still complains about a potential
cross-thread deadlock, see new PR100371.
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
[-- Attachment #2: fix-async.diff --]
[-- Type: text/x-patch, Size: 3623 bytes --]
Fortran: Async I/O - avoid unlocked unlocking [PR100352]
Follow up to PR100352, which moved unit unlocking to st_*_done_worker to
avoid lock order reversal; however, as async_io uses a different lock,
the (unlocked locked) unit lock shall not be unlocked there.
libgfortran/ChangeLog:
PR libgomp/100352
* io/transfer.c (st_read_done_worker, st_write_done_worker): Add new
arg whether to unlock unit.
(st_read_done, st_write_done): Call it with true.
* io/async.c (async_io): Call it with false.
* io/io.h (st_write_done_worker, st_read_done_worker): Update prototype.
libgfortran/io/async.c | 4 ++--
libgfortran/io/io.h | 4 ++--
libgfortran/io/transfer.c | 14 ++++++++------
3 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/libgfortran/io/async.c b/libgfortran/io/async.c
index d216ace1947..247008ca801 100644
--- a/libgfortran/io/async.c
+++ b/libgfortran/io/async.c
@@ -117,13 +117,13 @@ async_io (void *arg)
{
case AIO_WRITE_DONE:
NOTE ("Finalizing write");
- st_write_done_worker (au->pdt);
+ st_write_done_worker (au->pdt, false);
UNLOCK (&au->io_lock);
break;
case AIO_READ_DONE:
NOTE ("Finalizing read");
- st_read_done_worker (au->pdt);
+ st_read_done_worker (au->pdt, false);
UNLOCK (&au->io_lock);
break;
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index e0007c6bfe5..3355bc2fd8d 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -1083,11 +1083,11 @@ default_precision_for_float (int kind)
#endif
extern void
-st_write_done_worker (st_parameter_dt *);
+st_write_done_worker (st_parameter_dt *, bool);
internal_proto (st_write_done_worker);
extern void
-st_read_done_worker (st_parameter_dt *);
+st_read_done_worker (st_parameter_dt *, bool);
internal_proto (st_read_done_worker);
extern void
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 71a935652e3..36e35b48cd3 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -4337,7 +4337,7 @@ extern void st_read_done (st_parameter_dt *);
export_proto(st_read_done);
void
-st_read_done_worker (st_parameter_dt *dtp)
+st_read_done_worker (st_parameter_dt *dtp, bool unlock)
{
bool free_newunit = false;
finalize_transfer (dtp);
@@ -4367,7 +4367,8 @@ st_read_done_worker (st_parameter_dt *dtp)
free_format (dtp);
}
}
- unlock_unit (dtp->u.p.current_unit);
+ if (unlock)
+ unlock_unit (dtp->u.p.current_unit);
if (free_newunit)
{
/* Avoid inverse lock issues by placing after unlock_unit. */
@@ -4394,7 +4395,7 @@ st_read_done (st_parameter_dt *dtp)
unlock_unit (dtp->u.p.current_unit);
}
else
- st_read_done_worker (dtp); /* Calls unlock_unit. */
+ st_read_done_worker (dtp, true); /* Calls unlock_unit. */
}
library_end ();
@@ -4412,7 +4413,7 @@ st_write (st_parameter_dt *dtp)
void
-st_write_done_worker (st_parameter_dt *dtp)
+st_write_done_worker (st_parameter_dt *dtp, bool unlock)
{
bool free_newunit = false;
finalize_transfer (dtp);
@@ -4463,7 +4464,8 @@ st_write_done_worker (st_parameter_dt *dtp)
free_format (dtp);
}
}
- unlock_unit (dtp->u.p.current_unit);
+ if (unlock)
+ unlock_unit (dtp->u.p.current_unit);
if (free_newunit)
{
/* Avoid inverse lock issues by placing after unlock_unit. */
@@ -4496,7 +4498,7 @@ st_write_done (st_parameter_dt *dtp)
unlock_unit (dtp->u.p.current_unit);
}
else
- st_write_done_worker (dtp); /* Calls unlock_unit. */
+ st_write_done_worker (dtp, true); /* Calls unlock_unit. */
}
library_end ();
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Patch] Fortran: Async I/O - avoid unlocked unlocking [PR100352]
2021-05-01 12:59 [Patch] Fortran: Async I/O - avoid unlocked unlocking [PR100352] Tobias Burnus
@ 2021-05-01 16:15 ` Bernhard Reutner-Fischer
0 siblings, 0 replies; 2+ messages in thread
From: Bernhard Reutner-Fischer @ 2021-05-01 16:15 UTC (permalink / raw)
To: Tobias Burnus; +Cc: rep.dot.nop, gcc-patches, fortran, Jerry DeLisle
On Sat, 1 May 2021 14:59:01 +0200
Tobias Burnus <tobias@codesourcery.com> wrote:
> As this patch is rather obvious, I intent to commit it tomorrow
> as obvious, unless there is an OK earlier or other comments :-)
> (And backport to GCC 11 in a couple of days.)
>
> Before PR100352 (r11-7647),
> st_write_done did:
> st_write_done_worker (dtd)
> unlock_unit (dtp->u.p.current_unit);
>
> but st_write_done_worker did:
> LOCK (&unit_lock);
> newunit_free (dtp->common.unit);
> UNLOCK (&unit_lock);
>
> And this locking could lead to a deadlock.
>
> Hence, 'unlock_unit' has been moved to st_write_done_worker
> before the LOCK (&unit_lock).
>
> That solved the issue, but async I/O does not use this lock
> but, in async.c's async_io():
> LOCK (&au->lock);
> st_write_done_worker (au->pdt);
> UNLOCK (&au->io_lock);
>
> Which worked before the previous patch fine, but now
> there is a bogus unlock_unit() alias UNLOCK (&u->lock);
> although the unit is not locked.
>
> Solution: Guard the unlock_unit.
Doesn't look all that pretty TBH.
Doesn't it suggest that the worker's callers should eventually take
care of the locking (and newunit_free()ing) ?
I.e. have the workers return a bool whether to free the newunit or not.
But then, neither did i look closely nor do i volunteer to provide a
fix..
HTH,
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-05-01 16:16 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-01 12:59 [Patch] Fortran: Async I/O - avoid unlocked unlocking [PR100352] Tobias Burnus
2021-05-01 16:15 ` Bernhard Reutner-Fischer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).