* [PATCH, libgomp] Fix deadlock in acc_set_device_type
@ 2016-03-28 11:18 Chung-Lin Tang
2016-04-16 7:40 ` Chung-Lin Tang
2016-05-12 10:06 ` [PATCH, libgomp] Fix deadlock in acc_set_device_type Jakub Jelinek
0 siblings, 2 replies; 5+ messages in thread
From: Chung-Lin Tang @ 2016-03-28 11:18 UTC (permalink / raw)
To: gcc-patches, Jakub Jelinek; +Cc: Thomas Schwinge
[-- 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;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, libgomp] Fix deadlock in acc_set_device_type
2016-03-28 11:18 [PATCH, libgomp] Fix deadlock in acc_set_device_type Chung-Lin Tang
@ 2016-04-16 7:40 ` Chung-Lin Tang
2016-04-19 14:31 ` [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x2) Chung-Lin Tang
2016-05-12 10:06 ` [PATCH, libgomp] Fix deadlock in acc_set_device_type Jakub Jelinek
1 sibling, 1 reply; 5+ messages in thread
From: Chung-Lin Tang @ 2016-04-16 7:40 UTC (permalink / raw)
To: gcc-patches, Jakub Jelinek; +Cc: Thomas Schwinge
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.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x2)
2016-04-16 7:40 ` Chung-Lin Tang
@ 2016-04-19 14:31 ` Chung-Lin Tang
2016-05-11 6:48 ` [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x3) Chung-Lin Tang
0 siblings, 1 reply; 5+ messages in thread
From: Chung-Lin Tang @ 2016-04-19 14:31 UTC (permalink / raw)
To: gcc-patches, Jakub Jelinek; +Cc: Thomas Schwinge
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.
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x3)
2016-04-19 14:31 ` [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x2) Chung-Lin Tang
@ 2016-05-11 6:48 ` Chung-Lin Tang
0 siblings, 0 replies; 5+ messages in thread
From: Chung-Lin Tang @ 2016-05-11 6:48 UTC (permalink / raw)
To: gcc-patches, Jakub Jelinek; +Cc: Thomas Schwinge
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.
>>>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, libgomp] Fix deadlock in acc_set_device_type
2016-03-28 11:18 [PATCH, libgomp] Fix deadlock in acc_set_device_type Chung-Lin Tang
2016-04-16 7:40 ` Chung-Lin Tang
@ 2016-05-12 10:06 ` Jakub Jelinek
1 sibling, 0 replies; 5+ messages in thread
From: Jakub Jelinek @ 2016-05-12 10:06 UTC (permalink / raw)
To: Chung-Lin Tang; +Cc: gcc-patches, Thomas Schwinge
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-05-12 10:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-28 11:18 [PATCH, libgomp] Fix deadlock in acc_set_device_type Chung-Lin Tang
2016-04-16 7:40 ` Chung-Lin Tang
2016-04-19 14:31 ` [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x2) Chung-Lin Tang
2016-05-11 6:48 ` [PATCH, libgomp] Fix deadlock in acc_set_device_type (ping x3) Chung-Lin Tang
2016-05-12 10:06 ` [PATCH, libgomp] Fix deadlock in acc_set_device_type Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).