Hi! It's still Friday afternoon -- so please bear with me once again...
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.
commit 702090223a8d43e7371d6cbfc9d8a39e3c5c2986
Author: Thomas Schwinge <tho...@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 (®ister_lock);
+ gomp_mutex_unlock (®ister_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 (®ister_lock);
+ gomp_mutex_unlock (®ister_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 (®ister_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 (®ister_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 (®ister_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 (®ister_lock);
}
}
Grüße,
Thomas
signature.asc
Description: PGP signature
