Re: [PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex
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>> >> 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 (_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
Re: [PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex
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
Re: [PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex
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> > 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 (_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
[PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex
Hi, 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 TangPR libgomp/67141 * oacc-int.h (goacc_host_init): Add declaration. * oacc-host.c (goacc_host_init): Remove static and constructor attribute * oacc-init.c (goacc_runtime_initialize): Call goacc_host_init() at end. 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 (_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 (_thread_lock); + + /* Initialize and register the 'host' device type. */ + goacc_host_init (); } /* Compiler helper functions */