libgomp: Guard all devices/num_devices/num_devices_openmp access by register_lock (was: libgomp: Guard all offload_images/num_offload_images access by register_lock)

[Thomas Schwinge]

Hi!

On Fri, 25 Sep 2015 19:49:50 +0300, Ilya Verbin <iver...@gmail.com> wrote:
> On Fri, Sep 25, 2015 at 18:21:27 +0200, Thomas Schwinge wrote:
> > On Thu, 26 Mar 2015 23:41:30 +0300, Ilya Verbin <iver...@gmail.com> wrote:
> > > On Thu, Mar 26, 2015 at 13:09:19 +0100, Jakub Jelinek wrote:
> > > > the current code is majorly broken.  As I've said earlier, e.g. the lack
> > > > of mutex guarding gomp_target_init (which is using pthread_once 
> > > > guaranteed
> > > > to be run just once) vs. concurrent GOMP_offload_register calls
> > > > (if those are run from ctors, then I guess something like dl_load_lock
> > > > ensures at least on glibc that multiple GOMP_offload_register calls 
> > > > aren't
> > > > performed at the same time) in accessing/reallocating offload_images
> > > > and num_offload_images and the lack of support to register further
> > > > images after the gomp_target_init call (if you dlopen further shared
> > > > libraries) is really bad.  And it would be really nice to support the
> > > > unloading.
> > 
> > > Here is the latest patch for libgomp and mic plugin.
> > 
> > What about the scenario where one thread is inside
> > GOMP_offload_register_ver/GOMP_offload_register (say, due to opening a
> > shared library with such a mkoffload-generated constructor) and is
> > modifying offload_images with register_lock held, and another thread is
> > inside a GOMP_target* construct -> gomp_init_device and is accessing
> > offload_images without register_lock held?  Or, why isn't that a
> > reachable scenario?
> > 
> > Would the following patch (untested) do the right thing (locking added to
> > gomp_init_device and gomp_unload_device)?  We can then also remove the
> > is_register_lock parameter from gomp_load_image_to_device, and simplify
> > the code.
> 
> Looks like you're right, and this scenario is possible.

Thanks for your review!  Jakub, OK to commit the patch I had posted?


Then, in context of a similar scenario, I think we'll also want the
following.  Please confirm that my reasoning in gomp_get_num_devices and
resolve_device is correct.  OK for trunk?

commit b0cf4dcc588e432c0a0d19d85727a20210b4d837
Author: Thomas Schwinge <tho...@codesourcery.com>
Date:   Sat Sep 26 15:48:09 2015 +0200

    libgomp: Guard all devices/num_devices/num_devices_openmp access by 
register_lock
---
 libgomp/target.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git libgomp/target.c libgomp/target.c
index 1fbbe31..6f0a339 100644
--- libgomp/target.c
+++ libgomp/target.c
@@ -49,7 +49,7 @@ static void gomp_target_init (void);
 /* The whole initialization code for offloading plugins is only run one.  */
 static pthread_once_t gomp_is_initialized = PTHREAD_ONCE_INIT;
 
-/* Mutex for offload image registration.  */
+/* Mutex for offload targets setup and image registration.  */
 static gomp_mutex_t register_lock;
 
 /* This structure describes an offload image.
@@ -118,6 +118,8 @@ attribute_hidden int
 gomp_get_num_devices (void)
 {
   gomp_init_targets_once ();
+  /* As it is immutable once it has been initialized, it's safe to access
+     num_devices_openmp without register_lock held.  */
   return num_devices_openmp;
 }
 
@@ -133,6 +135,8 @@ resolve_device (int device_id)
   if (device_id < 0 || device_id >= gomp_get_num_devices ())
     return NULL;
 
+  /* As it is immutable once it has been initialized, it's safe to access
+     devices without register_lock held.  */
   return &devices[device_id];
 }
 
@@ -1228,6 +1232,8 @@ gomp_target_init (void)
   char *plugin_name;
   int i, new_num_devices;
 
+  gomp_mutex_lock (&register_lock);
+
   num_devices = 0;
   devices = NULL;
 
@@ -1317,6 +1323,8 @@ gomp_target_init (void)
       if (devices[i].capabilities & GOMP_OFFLOAD_CAP_OPENACC_200)
        goacc_register (&devices[i]);
     }
+
+  gomp_mutex_unlock (&register_lock);
 }
 
 #else /* PLUGIN_SUPPORT */


Grüße,
 Thomas

signature.asc
Description: PGP signature