Re: [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts

[Jakub Jelinek]

On Tue, Jan 19, 2016 at 03:02:48PM +0900, Chung-Lin Tang wrote:
> Ping x 2.

I like the general idea of moving the gomp_fatal stuff up, out of the
plugins, but we now have another plugin, so it can't apply as is.
And in the intelmic plugin, adding return true; after abort (); looks wrong.
So, can you please update this series and repost?

Jakub

Re: [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts

[Chung-Lin Tang]

Ping x 2.

On 2016/1/5 11:22 PM, Chung-Lin Tang wrote:
> Patch has been updated to accommodate the gomp_fini_device() removal changes.
> And ping.
> 
> On 2015/12/14 11:47 PM, Chung-Lin Tang wrote:
>> [sorry, forgot to C gcc-patches in last send]
>>
>> Hi Jakub,
>> these patches are a revision of 
>> https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01701.html
>> since that patch set have bitrotten by now.
>>
>> To recap the original situation, due to the way that device locks are held
>> when entering plugin code, a GOMP_PLUGIN_fatal() call will deadlock when the
>> GOMP_unregister_var() exit destructor tries to obtain the same device lock.
>>
>> This patch set revises many functions on libgomp plugin interface to return 
>> false on error,
>> and back to libgomp to release the lock and call gomp_fatal() there.
>>
>> This first patch is the changes for the machine independent libgomp proper. 
>> The entire patch
>> set was tested without regressions. Is this okay for trunk?
>>
>> Thanks,
>> Chung-Lin
>>
>> 2015-12-14  Chung-Lin Tang  
>>
>> * target.c (gomp_device_copy): New function.
>> (gomp_copy_host2dev): Likewise.
>> (gomp_copy_dev2host): Likewise.
>> (gomp_free_device_memory): Likewise.
>> (gomp_map_vars_existing): Adjust to call gomp_copy_host2dev().
>> (gomp_map_pointer): Likewise.
>> (gomp_map_vars): Adjust to call gomp_copy_host2dev(), handle
>> NULL value from alloc_func plugin hook.
>> (gomp_unmap_tgt): Adjust to call gomp_free_device_memory().
>> (gomp_copy_from_async): Adjust to call gomp_copy_dev2host().
>> (gomp_unmap_vars): Likewise.
>> (gomp_update): Adjust to call gomp_copy_dev2host() and
>> gomp_copy_host2dev() functions.
>> (gomp_init_device): Handle false value from init_device_func
>> plugin hook.
>> (gomp_fini_device): Handle false value from fini_device_func
>> plugin hook.
>> (gomp_exit_data): Adjust to call gomp_copy_dev2host().
>> (omp_target_free): Adjust to call gomp_free_device_memory().
>> (omp_target_memcpy): Handle return values from host2dev_func,
>> dev2host_func, and dev2dev_func plugin hooks.
>> (omp_target_memcpy_rect_worker): Likewise.
>> * libgomp.h (struct gomp_device_descr): Adjust return type of
>> init_device_func, fini_device_func, free_func, dev2host_func,
>> host2dev_func, and dev2dev_func plugin hooks from 'void *' to
>> bool.
>> * oacc-host.c (host_init_device): Change return type to bool.
>> (host_fini_device): Likewise.
>> (host_free): Likewise.
>> (host_dev2host): Likewise.
>> (host_host2dev): Likewise.
>> * oacc-mem.c (acc_free): Handle plugin hook fatal error case.
>> (acc_memcpy_to_device): Likewise.
>> (acc_memcpy_from_device): Likewise.
>> (delete_copyout): Add libfnname parameter, handle free_func
>> hook fatal error case.
>> (acc_delete): Adjust delete_copyout call.
>> (acc_copyout): Likewise.
>>
>>
>>
> 

Re: [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts

[Chung-Lin Tang]

Patch has been updated to accommodate the gomp_fini_device() removal changes.
And ping.

On 2015/12/14 11:47 PM, Chung-Lin Tang wrote:
> [sorry, forgot to C gcc-patches in last send]
> 
> Hi Jakub,
> these patches are a revision of 
> https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01701.html
> since that patch set have bitrotten by now.
> 
> To recap the original situation, due to the way that device locks are held
> when entering plugin code, a GOMP_PLUGIN_fatal() call will deadlock when the
> GOMP_unregister_var() exit destructor tries to obtain the same device lock.
> 
> This patch set revises many functions on libgomp plugin interface to return 
> false on error,
> and back to libgomp to release the lock and call gomp_fatal() there.
> 
> This first patch is the changes for the machine independent libgomp proper. 
> The entire patch
> set was tested without regressions. Is this okay for trunk?
> 
> Thanks,
> Chung-Lin
> 
> 2015-12-14  Chung-Lin Tang  
> 
> * target.c (gomp_device_copy): New function.
> (gomp_copy_host2dev): Likewise.
> (gomp_copy_dev2host): Likewise.
> (gomp_free_device_memory): Likewise.
> (gomp_map_vars_existing): Adjust to call gomp_copy_host2dev().
> (gomp_map_pointer): Likewise.
> (gomp_map_vars): Adjust to call gomp_copy_host2dev(), handle
> NULL value from alloc_func plugin hook.
> (gomp_unmap_tgt): Adjust to call gomp_free_device_memory().
> (gomp_copy_from_async): Adjust to call gomp_copy_dev2host().
> (gomp_unmap_vars): Likewise.
> (gomp_update): Adjust to call gomp_copy_dev2host() and
> gomp_copy_host2dev() functions.
> (gomp_init_device): Handle false value from init_device_func
> plugin hook.
> (gomp_fini_device): Handle false value from fini_device_func
> plugin hook.
> (gomp_exit_data): Adjust to call gomp_copy_dev2host().
> (omp_target_free): Adjust to call gomp_free_device_memory().
> (omp_target_memcpy): Handle return values from host2dev_func,
> dev2host_func, and dev2dev_func plugin hooks.
> (omp_target_memcpy_rect_worker): Likewise.
> * libgomp.h (struct gomp_device_descr): Adjust return type of
> init_device_func, fini_device_func, free_func, dev2host_func,
> host2dev_func, and dev2dev_func plugin hooks from 'void *' to
> bool.
> * oacc-host.c (host_init_device): Change return type to bool.
> (host_fini_device): Likewise.
> (host_free): Likewise.
> (host_dev2host): Likewise.
> (host_host2dev): Likewise.
> * oacc-mem.c (acc_free): Handle plugin hook fatal error case.
> (acc_memcpy_to_device): Likewise.
> (acc_memcpy_from_device): Likewise.
> (delete_copyout): Add libfnname parameter, handle free_func
> hook fatal error case.
> (acc_delete): Adjust delete_copyout call.
> (acc_copyout): Likewise.
> 
> 
> 

Index: libgomp.h
===
--- libgomp.h   (revision 232047)
+++ libgomp.h   (working copy)
@@ -927,16 +927,17 @@ struct gomp_device_descr
   unsigned int (*get_caps_func) (void);
   int (*get_type_func) (void);
   int (*get_num_devices_func) (void);
-  void (*init_device_func) (int);
-  void (*fini_device_func) (int);
+  bool (*init_device_func) (int);
+  bool (*fini_device_func) (int);
   unsigned (*version_func) (void);
   int (*load_image_func) (int, unsigned, const void *, struct addr_pair **);
   void (*unload_image_func) (int, unsigned, const void *);
   void *(*alloc_func) (int, size_t);
-  void (*free_func) (int, void *);
-  void *(*dev2host_func) (int, void *, const void *, size_t);
-  void *(*host2dev_func) (int, void *, const void *, size_t);
-  void *(*dev2dev_func) (int, void *, const void *, size_t);
+  bool (*free_func) (int, void *);
+  bool (*dev2host_func) (int, void *, const void *, size_t);
+  bool (*host2dev_func) (int, void *, const void *, size_t);
+  /*xxx*/
+  bool (*dev2dev_func) (int, void *, const void *, size_t);
   void (*run_func) (int, void *, void *);
   void (*async_run_func) (int, void *, void *, void *);
 
Index: oacc-host.c
===
--- oacc-host.c (revision 232047)
+++ oacc-host.c (working copy)
@@ -60,14 +60,16 @@ host_get_num_devices (void)
   return 1;
 }
 
-static void
+static bool
 host_init_device (int n __attribute__ ((unused)))
 {
+  return true;
 }
 
-static void
+static bool
 host_fini_device (int n __attribute__ ((unused)))
 {
+  return true;
 }
 
 static unsigned
@@ -98,28 +100,29 @@ host_alloc (int n __attribute__ ((unused)), size_t
   return gomp_malloc (s);
 }
 
-static void
+static bool
 host_free (int n __attribute__ ((unused)), void *p)
 {
   free (p);
+  return true;
 }
 
-static void *
+static bool
 host_dev2host (int n __attribute__ ((unused)),
   void *h 

[PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts

[Chung-Lin Tang]

[sorry, forgot to C gcc-patches in last send]

Hi Jakub,
these patches are a revision of 
https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01701.html
since that patch set have bitrotten by now.

To recap the original situation, due to the way that device locks are held
when entering plugin code, a GOMP_PLUGIN_fatal() call will deadlock when the
GOMP_unregister_var() exit destructor tries to obtain the same device lock.

This patch set revises many functions on libgomp plugin interface to return 
false on error,
and back to libgomp to release the lock and call gomp_fatal() there.

This first patch is the changes for the machine independent libgomp proper. The 
entire patch
set was tested without regressions. Is this okay for trunk?

Thanks,
Chung-Lin

2015-12-14  Chung-Lin Tang  

* target.c (gomp_device_copy): New function.
(gomp_copy_host2dev): Likewise.
(gomp_copy_dev2host): Likewise.
(gomp_free_device_memory): Likewise.
(gomp_map_vars_existing): Adjust to call gomp_copy_host2dev().
(gomp_map_pointer): Likewise.
(gomp_map_vars): Adjust to call gomp_copy_host2dev(), handle
NULL value from alloc_func plugin hook.
(gomp_unmap_tgt): Adjust to call gomp_free_device_memory().
(gomp_copy_from_async): Adjust to call gomp_copy_dev2host().
(gomp_unmap_vars): Likewise.
(gomp_update): Adjust to call gomp_copy_dev2host() and
gomp_copy_host2dev() functions.
(gomp_init_device): Handle false value from init_device_func
plugin hook.
(gomp_fini_device): Handle false value from fini_device_func
plugin hook.
(gomp_exit_data): Adjust to call gomp_copy_dev2host().
(omp_target_free): Adjust to call gomp_free_device_memory().
(omp_target_memcpy): Handle return values from host2dev_func,
dev2host_func, and dev2dev_func plugin hooks.
(omp_target_memcpy_rect_worker): Likewise.
* libgomp.h (struct gomp_device_descr): Adjust return type of
init_device_func, fini_device_func, free_func, dev2host_func,
host2dev_func, and dev2dev_func plugin hooks from 'void *' to
bool.
* oacc-host.c (host_init_device): Change return type to bool.
(host_fini_device): Likewise.
(host_free): Likewise.
(host_dev2host): Likewise.
(host_host2dev): Likewise.
* oacc-mem.c (acc_free): Handle plugin hook fatal error case.
(acc_memcpy_to_device): Likewise.
(acc_memcpy_from_device): Likewise.
(delete_copyout): Add libfnname parameter, handle free_func
hook fatal error case.
(acc_delete): Adjust delete_copyout call.
(acc_copyout): Likewise.



Index: libgomp/libgomp.h
===
--- libgomp/libgomp.h	(revision 231613)
+++ libgomp/libgomp.h	(working copy)
@@ -914,16 +914,17 @@ struct gomp_device_descr
   unsigned int (*get_caps_func) (void);
   int (*get_type_func) (void);
   int (*get_num_devices_func) (void);
-  void (*init_device_func) (int);
-  void (*fini_device_func) (int);
+  bool (*init_device_func) (int);
+  bool (*fini_device_func) (int);
   unsigned (*version_func) (void);
   int (*load_image_func) (int, unsigned, const void *, struct addr_pair **);
   void (*unload_image_func) (int, unsigned, const void *);
   void *(*alloc_func) (int, size_t);
-  void (*free_func) (int, void *);
-  void *(*dev2host_func) (int, void *, const void *, size_t);
-  void *(*host2dev_func) (int, void *, const void *, size_t);
-  void *(*dev2dev_func) (int, void *, const void *, size_t);
+  bool (*free_func) (int, void *);
+  bool (*dev2host_func) (int, void *, const void *, size_t);
+  bool (*host2dev_func) (int, void *, const void *, size_t);
+  /*xxx*/
+  bool (*dev2dev_func) (int, void *, const void *, size_t);
   void (*run_func) (int, void *, void *);
   void (*async_run_func) (int, void *, void *, void *);
 
Index: libgomp/oacc-host.c
===
--- libgomp/oacc-host.c	(revision 231613)
+++ libgomp/oacc-host.c	(working copy)
@@ -60,14 +60,16 @@ host_get_num_devices (void)
   return 1;
 }
 
-static void
+static bool
 host_init_device (int n __attribute__ ((unused)))
 {
+  return true;
 }
 
-static void
+static bool
 host_fini_device (int n __attribute__ ((unused)))
 {
+  return true;
 }
 
 static unsigned
@@ -98,28 +100,29 @@ host_alloc (int n __attribute__ ((unused)), size_t
   return gomp_malloc (s);
 }
 
-static void
+static bool
 host_free (int n __attribute__ ((unused)), void *p)
 {
   free (p);
+  return true;
 }
 
-static void *
+static bool
 host_dev2host (int n __attribute__ ((unused)),
 	   void *h __attribute__ ((unused)),
 	   const void *d __attribute__ ((unused)),
 	   size_t s __attribute__ ((unused)))
 {
-  return NULL;
+  return true;
 }
 
-static void *
+static bool
 host_host2dev (int n __attribute__