| Message ID | 55FBC02A.5080306@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Sep 18, 2015 at 03:41:30PM +0800, Chung-Lin Tang wrote: > this patch fixes the uninitialized acc_device_lock mutex situation > reported in PR 67141. The patch attached on the bugzilla page > tries to solve it by constructor priorities, which we think will > probably be less manageable in general. > > This patch changes goacc_host_init() to be called from > goacc_runtime_initialize() instead, thereby ensuring the init order. > libgomp testsuite was re-run without regressions, okay for trunk? > > Thanks, > Chung-Lin > > 2015-09-18 Chung-Lin Tang <cltang@codesourcery.com> > > PR libgomp/67141 > No vertical space in between PR line and subsequent entries. > * oacc-int.h (goacc_host_init): Add declaration. > * oacc-host.c (goacc_host_init): Remove static and > constructor attribute Full stop at the end of entry. > * oacc-init.c (goacc_runtime_initialize): Call goacc_host_init() > at end. The patch is ok. Though, perhaps as a follow-up, I think I'd prefer getting rid of pthread_key_create (&goacc_cleanup_key, goacc_destroy_thread);, it is wasteful if we do the same thing in initialize_team. As the goacc_tls_data pointer is __thread anyway, I think just putting it into struct gomp_thread, arranging for init_team to be called from the env.c ctor and from the team TLS destructor call also some oacc freeing if the goacc_tls_data pointer is non-NULL (perhaps with __builtin_expect unlikely). Jakub
On 2015/9/18 04:02 PM, Jakub Jelinek wrote: > On Fri, Sep 18, 2015 at 03:41:30PM +0800, Chung-Lin Tang wrote: >> this patch fixes the uninitialized acc_device_lock mutex situation >> reported in PR 67141. The patch attached on the bugzilla page >> tries to solve it by constructor priorities, which we think will >> probably be less manageable in general. >> >> This patch changes goacc_host_init() to be called from >> goacc_runtime_initialize() instead, thereby ensuring the init order. >> libgomp testsuite was re-run without regressions, okay for trunk? >> >> Thanks, >> Chung-Lin >> >> 2015-09-18 Chung-Lin Tang <cltang@codesourcery.com> >> >> PR libgomp/67141 >> > > No vertical space in between PR line and subsequent entries. > >> * oacc-int.h (goacc_host_init): Add declaration. >> * oacc-host.c (goacc_host_init): Remove static and >> constructor attribute > > Full stop at the end of entry. > >> * oacc-init.c (goacc_runtime_initialize): Call goacc_host_init() >> at end. > > The patch is ok. Though, perhaps as a follow-up, I think I'd prefer getting > rid of pthread_key_create (&goacc_cleanup_key, goacc_destroy_thread);, > it is wasteful if we do the same thing in initialize_team. As the > goacc_tls_data pointer is __thread anyway, I think just putting it into > struct gomp_thread, arranging for init_team to be called from the env.c > ctor and from the team TLS destructor call also some oacc freeing if > the goacc_tls_data pointer is non-NULL (perhaps with __builtin_expect > unlikely). > > Jakub Committed, thanks for the review. I believe this patch is also needed for 5.x, okay for that branch as well? Thanks, Chung-Lin
On Tue, Sep 22, 2015 at 02:49:04PM +0800, Chung-Lin Tang wrote: > Committed, thanks for the review. > I believe this patch is also needed for 5.x, okay for that branch as well? Ok. Jakub
Index: oacc-host.c =================================================================== --- oacc-host.c (revision 227895) +++ oacc-host.c (working copy) @@ -256,7 +256,7 @@ static struct gomp_device_descr host_dispatch = }; /* Initialize and register this device type. */ -static __attribute__ ((constructor)) void +void goacc_host_init (void) { gomp_mutex_init (&host_dispatch.lock); Index: oacc-int.h =================================================================== --- oacc-int.h (revision 227895) +++ oacc-int.h (working copy) @@ -97,6 +97,7 @@ void goacc_runtime_initialize (void); void goacc_save_and_set_bind (acc_device_t); void goacc_restore_bind (void); void goacc_lazy_initialize (void); +void goacc_host_init (void); #ifdef HAVE_ATTRIBUTE_VISIBILITY # pragma GCC visibility pop Index: oacc-init.c =================================================================== --- oacc-init.c (revision 227895) +++ oacc-init.c (working copy) @@ -644,6 +644,9 @@ goacc_runtime_initialize (void) goacc_threads = NULL; gomp_mutex_init (&goacc_thread_lock); + + /* Initialize and register the 'host' device type. */ + goacc_host_init (); } /* Compiler helper functions */