libgomp: Guard all offload_images/num_offload_images access by register_lock (was: libgomp nvptx plugin: rework initialisation and support the proposed load/unload hooks)

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi!

It's still Friday afternoon -- so please bear with me once again...

On Thu, 26 Mar 2015 23:41:30 +0300, Ilya Verbin <iverbin@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.

commit 702090223a8d43e7371d6cbfc9d8a39e3c5c2986
Author: Thomas Schwinge <thomas@codesourcery.com>
Date:   Fri Sep 25 17:37:41 2015 +0200

    libgomp: Guard all offload_images/num_offload_images access by register_lock
---
 libgomp/target.c |   25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git libgomp/target.c libgomp/target.c
index 758ece5..1fbbe31 100644
--- libgomp/target.c
+++ libgomp/target.c
@@ -640,12 +640,13 @@ gomp_update (struct gomp_device_descr *devicep, size_t mapnum, void **hostaddrs,
 /* Load image pointed by TARGET_DATA to the device, specified by DEVICEP.
    And insert to splay tree the mapping between addresses from HOST_TABLE and
    from loaded target image.  We rely in the host and device compiler
-   emitting variable and functions in the same order.  */
+   emitting variable and functions in the same order.
+
+   The device must be locked, and REGISTER_LOCK must be held.  */
 
 static void
 gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
-			   const void *host_table, const void *target_data,
-			   bool is_register_lock)
+			   const void *host_table, const void *target_data)
 {
   void **host_func_table = ((void ***) host_table)[0];
   void **host_funcs_end  = ((void ***) host_table)[1];
@@ -668,8 +669,7 @@ gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
   if (num_target_entries != num_funcs + num_vars)
     {
       gomp_mutex_unlock (&devicep->lock);
-      if (is_register_lock)
-	gomp_mutex_unlock (&register_lock);
+      gomp_mutex_unlock (&register_lock);
       gomp_fatal ("Cannot map target functions or variables"
 		  " (expected %u, have %u)", num_funcs + num_vars,
 		  num_target_entries);
@@ -710,8 +710,7 @@ gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
 	  != (uintptr_t) host_var_table[i * 2 + 1])
 	{
 	  gomp_mutex_unlock (&devicep->lock);
-	  if (is_register_lock)
-	    gomp_mutex_unlock (&register_lock);
+	  gomp_mutex_unlock (&register_lock);
 	  gomp_fatal ("Can't map target variables (size mismatch)");
 	}
 
@@ -733,7 +732,8 @@ gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
 }
 
 /* Unload the mappings described by target_data from device DEVICE_P.
-   The device must be locked.   */
+
+   The device must be locked.  */
 
 static void
 gomp_unload_image_from_device (struct gomp_device_descr *devicep,
@@ -810,7 +810,7 @@ GOMP_offload_register_ver (unsigned version, const void *host_table,
       gomp_mutex_lock (&devicep->lock);
       if (devicep->type == target_type && devicep->is_initialized)
 	gomp_load_image_to_device (devicep, version,
-				   host_table, target_data, true);
+				   host_table, target_data);
       gomp_mutex_unlock (&devicep->lock);
     }
 
@@ -886,14 +886,15 @@ gomp_init_device (struct gomp_device_descr *devicep)
   devicep->init_device_func (devicep->target_id);
 
   /* Load to device all images registered by the moment.  */
+  gomp_mutex_lock (&register_lock);
   for (i = 0; i < num_offload_images; i++)
     {
       struct offload_image_descr *image = &offload_images[i];
       if (image->type == devicep->type)
 	gomp_load_image_to_device (devicep, image->version,
-				   image->host_table, image->target_data,
-				   false);
+				   image->host_table, image->target_data);
     }
+  gomp_mutex_unlock (&register_lock);
 
   devicep->is_initialized = true;
 }
@@ -906,6 +907,7 @@ gomp_unload_device (struct gomp_device_descr *devicep)
       unsigned i;
       
       /* Unload from device all images registered at the moment.  */
+      gomp_mutex_lock (&register_lock);
       for (i = 0; i < num_offload_images; i++)
 	{
 	  struct offload_image_descr *image = &offload_images[i];
@@ -914,6 +916,7 @@ gomp_unload_device (struct gomp_device_descr *devicep)
 					   image->host_table,
 					   image->target_data);
 	}
+      gomp_mutex_unlock (&register_lock);
     }
 }
 


GrÃÃe,
 Thomas

Attachment: signature.asc
Description: PGP signature