[PATCH, libgomp] Fix deadlock in acc_set_device_type

[PATCH, libgomp] Fix deadlock in acc_set_device_type

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

Hi Jakub, there's a path for deadlock on acc_device_lock when going
through the acc_set_device_type() OpenACC library function.
Basically, the gomp_init_targets_once() function should not be
called with that held. The attached patch moves it appropriately.

Also in this patch, there are several cases in acc_* functions
where gomp_init_targets_once() is guarded by a test of
!cached_base_dev. Since that function already uses pthread_once() to
call gomp_target_init(), and technically cached_base_dev
is protected by acc_device_lock, the cleanest way should be to
simply drop those "if(!cached_base_dev)" tests.

Tested libgomp without regressions on an nvptx offloaded system,
is this okay for trunk?

Thanks,
Chung-Lin

2016-03-28  Chung-Lin Tang  <cltang@codesourcery.com>

        * oacc-init.c (acc_init): Remove !cached_base_dev condition on call to
        gomp_init_targets_once().
        (acc_set_device_type): Remove !cached_base_dev condition on call to
        gomp_init_targets_once(), move call to before acc_device_lock acquire,
        to avoid deadlock.
        (acc_get_device_num): Remove !cached_base_dev condition on call to
        gomp_init_targets_once().
        (acc_set_device_num): Likewise.

[-- Attachment #2: a.diff --]
[-- Type: text/plain, Size: 1352 bytes --]

Index: oacc-init.c
===================================================================
--- oacc-init.c	(revision 234502)
+++ oacc-init.c	(working copy)
@@ -433,8 +433,7 @@ goacc_attach_host_thread_to_device (int ord)
 void
 acc_init (acc_device_t d)
 {
-  if (!cached_base_dev)
-    gomp_init_targets_once ();
+  gomp_init_targets_once ();
 
   gomp_mutex_lock (&acc_device_lock);
 
@@ -498,11 +497,10 @@ acc_set_device_type (acc_device_t d)
   struct gomp_device_descr *base_dev, *acc_dev;
   struct goacc_thread *thr = goacc_thread ();
 
+  gomp_init_targets_once ();
+
   gomp_mutex_lock (&acc_device_lock);
 
-  if (!cached_base_dev)
-    gomp_init_targets_once ();
-
   cached_base_dev = base_dev = resolve_device (d, true);
   acc_dev = &base_dev[goacc_device_num];
 
@@ -563,8 +561,7 @@ acc_get_device_num (acc_device_t d)
   if (d >= _ACC_device_hwm)
     gomp_fatal ("unknown device type %u", (unsigned) d);
 
-  if (!cached_base_dev)
-    gomp_init_targets_once ();
+  gomp_init_targets_once ();
 
   gomp_mutex_lock (&acc_device_lock);
   dev = resolve_device (d, true);
@@ -584,8 +581,7 @@ acc_set_device_num (int ord, acc_device_t d)
   struct gomp_device_descr *base_dev, *acc_dev;
   int num_devices;
 
-  if (!cached_base_dev)
-    gomp_init_targets_once ();
+  gomp_init_targets_once ();
 
   if (ord < 0)
     ord = goacc_device_num;

Re: [PATCH, libgomp] Fix deadlock in acc_set_device_type

[Chung-Lin Tang]

Ping.

On 2016/3/28 05:45 PM, Chung-Lin Tang wrote:
> Hi Jakub, there's a path for deadlock on acc_device_lock when going
> through the acc_set_device_type() OpenACC library function.
> Basically, the gomp_init_targets_once() function should not be
> called with that held. The attached patch moves it appropriately.
> 
> Also in this patch, there are several cases in acc_* functions
> where gomp_init_targets_once() is guarded by a test of
> !cached_base_dev. Since that function already uses pthread_once() to
> call gomp_target_init(), and technically cached_base_dev
> is protected by acc_device_lock, the cleanest way should be to
> simply drop those "if(!cached_base_dev)" tests.
> 
> Tested libgomp without regressions on an nvptx offloaded system,
> is this okay for trunk?
> 
> Thanks,
> Chung-Lin
> 
> 2016-03-28  Chung-Lin Tang  <cltang@codesourcery.com>
> 
>         * oacc-init.c (acc_init): Remove !cached_base_dev condition on call to
>         gomp_init_targets_once().
>         (acc_set_device_type): Remove !cached_base_dev condition on call to
>         gomp_init_targets_once(), move call to before acc_device_lock acquire,
>         to avoid deadlock.
>         (acc_get_device_num): Remove !cached_base_dev condition on call to
>         gomp_init_targets_once().
>         (acc_set_device_num): Likewise.
> 

Re: [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x2)

[Chung-Lin Tang]

Ping x2.

Hi Jakub,
This patch is fairly straightforward, and solves a easily encountered
deadlock. Please approve for trunk and gcc-6-branch.

Thanks,
Chung-Lin

On 2016/4/16 03:39 PM, Chung-Lin Tang wrote:
> Ping.
> 
> On 2016/3/28 05:45 PM, Chung-Lin Tang wrote:
>> Hi Jakub, there's a path for deadlock on acc_device_lock when going
>> through the acc_set_device_type() OpenACC library function.
>> Basically, the gomp_init_targets_once() function should not be
>> called with that held. The attached patch moves it appropriately.
>>
>> Also in this patch, there are several cases in acc_* functions
>> where gomp_init_targets_once() is guarded by a test of
>> !cached_base_dev. Since that function already uses pthread_once() to
>> call gomp_target_init(), and technically cached_base_dev
>> is protected by acc_device_lock, the cleanest way should be to
>> simply drop those "if(!cached_base_dev)" tests.
>>
>> Tested libgomp without regressions on an nvptx offloaded system,
>> is this okay for trunk?
>>
>> Thanks,
>> Chung-Lin
>>
>> 2016-03-28  Chung-Lin Tang  <cltang@codesourcery.com>
>>
>>         * oacc-init.c (acc_init): Remove !cached_base_dev condition on call to
>>         gomp_init_targets_once().
>>         (acc_set_device_type): Remove !cached_base_dev condition on call to
>>         gomp_init_targets_once(), move call to before acc_device_lock acquire,
>>         to avoid deadlock.
>>         (acc_get_device_num): Remove !cached_base_dev condition on call to
>>         gomp_init_targets_once().
>>         (acc_set_device_num): Likewise.
>>
> 

Re: [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x3)

[Chung-Lin Tang]

Ping x3

On 2016/4/19 10:30 PM, Chung-Lin Tang wrote:
> Ping x2.
> 
> Hi Jakub,
> This patch is fairly straightforward, and solves a easily encountered
> deadlock. Please approve for trunk and gcc-6-branch.
> 
> Thanks,
> Chung-Lin
> 
> On 2016/4/16 03:39 PM, Chung-Lin Tang wrote:
>> Ping.
>>
>> On 2016/3/28 05:45 PM, Chung-Lin Tang wrote:
>>> Hi Jakub, there's a path for deadlock on acc_device_lock when going
>>> through the acc_set_device_type() OpenACC library function.
>>> Basically, the gomp_init_targets_once() function should not be
>>> called with that held. The attached patch moves it appropriately.
>>>
>>> Also in this patch, there are several cases in acc_* functions
>>> where gomp_init_targets_once() is guarded by a test of
>>> !cached_base_dev. Since that function already uses pthread_once() to
>>> call gomp_target_init(), and technically cached_base_dev
>>> is protected by acc_device_lock, the cleanest way should be to
>>> simply drop those "if(!cached_base_dev)" tests.
>>>
>>> Tested libgomp without regressions on an nvptx offloaded system,
>>> is this okay for trunk?
>>>
>>> Thanks,
>>> Chung-Lin
>>>
>>> 2016-03-28  Chung-Lin Tang  <cltang@codesourcery.com>
>>>
>>>         * oacc-init.c (acc_init): Remove !cached_base_dev condition on call to
>>>         gomp_init_targets_once().
>>>         (acc_set_device_type): Remove !cached_base_dev condition on call to
>>>         gomp_init_targets_once(), move call to before acc_device_lock acquire,
>>>         to avoid deadlock.
>>>         (acc_get_device_num): Remove !cached_base_dev condition on call to
>>>         gomp_init_targets_once().
>>>         (acc_set_device_num): Likewise.
>>>
>>
> 

Re: [PATCH, libgomp] Fix deadlock in acc_set_device_type

[Jakub Jelinek]

On Mon, Mar 28, 2016 at 05:45:42PM +0800, Chung-Lin Tang wrote:
> Hi Jakub, there's a path for deadlock on acc_device_lock when going
> through the acc_set_device_type() OpenACC library function.
> Basically, the gomp_init_targets_once() function should not be
> called with that held. The attached patch moves it appropriately.
> 
> Also in this patch, there are several cases in acc_* functions
> where gomp_init_targets_once() is guarded by a test of
> !cached_base_dev. Since that function already uses pthread_once() to
> call gomp_target_init(), and technically cached_base_dev
> is protected by acc_device_lock, the cleanest way should be to
> simply drop those "if(!cached_base_dev)" tests.
> 
> Tested libgomp without regressions on an nvptx offloaded system,
> is this okay for trunk?

Ok, with ChangeLog nits:
> 
> 2016-03-28  Chung-Lin Tang  <cltang@codesourcery.com>
> 
>         * oacc-init.c (acc_init): Remove !cached_base_dev condition on call to
>         gomp_init_targets_once().
>         (acc_set_device_type): Remove !cached_base_dev condition on call to
>         gomp_init_targets_once(), move call to before acc_device_lock acquire,
>         to avoid deadlock.
>         (acc_get_device_num): Remove !cached_base_dev condition on call to
>         gomp_init_targets_once().
>         (acc_set_device_num): Likewise.

Please just use gomp_init_targets_once instead of gomp_init_targets_once()
in the ChangeLog.

> Index: oacc-init.c
> ===================================================================
> --- oacc-init.c	(revision 234502)
> +++ oacc-init.c	(working copy)
> @@ -433,8 +433,7 @@ goacc_attach_host_thread_to_device (int ord)
>  void
>  acc_init (acc_device_t d)
>  {
> -  if (!cached_base_dev)
> -    gomp_init_targets_once ();
> +  gomp_init_targets_once ();
>  
>    gomp_mutex_lock (&acc_device_lock);
>  
> @@ -498,11 +497,10 @@ acc_set_device_type (acc_device_t d)
>    struct gomp_device_descr *base_dev, *acc_dev;
>    struct goacc_thread *thr = goacc_thread ();
>  
> +  gomp_init_targets_once ();
> +
>    gomp_mutex_lock (&acc_device_lock);
>  
> -  if (!cached_base_dev)
> -    gomp_init_targets_once ();
> -
>    cached_base_dev = base_dev = resolve_device (d, true);
>    acc_dev = &base_dev[goacc_device_num];
>  
> @@ -563,8 +561,7 @@ acc_get_device_num (acc_device_t d)
>    if (d >= _ACC_device_hwm)
>      gomp_fatal ("unknown device type %u", (unsigned) d);
>  
> -  if (!cached_base_dev)
> -    gomp_init_targets_once ();
> +  gomp_init_targets_once ();
>  
>    gomp_mutex_lock (&acc_device_lock);
>    dev = resolve_device (d, true);
> @@ -584,8 +581,7 @@ acc_set_device_num (int ord, acc_device_t d)
>    struct gomp_device_descr *base_dev, *acc_dev;
>    int num_devices;
>  
> -  if (!cached_base_dev)
> -    gomp_init_targets_once ();
> +  gomp_init_targets_once ();
>  
>    if (ord < 0)
>      ord = goacc_device_num;


	Jakub