Fortran: Async I/O - avoid unlocked unlocking [PR100352]

Message ID db25ac2d-3f5c-d49c-06c2-864a0b63cc3f@codesourcery.com
State New
Headers show
Series
  • Fortran: Async I/O - avoid unlocked unlocking [PR100352]
Related show

Commit Message

Tobias Burnus May 1, 2021, 12:59 p.m.
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

Comments

Paul Richard Thomas via Gcc-patches May 1, 2021, 4:15 p.m. | #1
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,

Patch

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 ();