Message ID | 1436786144-8625-1-git-send-email-sebastian.huber@embedded-brains.de |
---|---|
State | New |
Headers | show |
On Mon, Jul 13, 2015 at 01:15:44PM +0200, Sebastian Huber wrote: > diff --git a/libgomp/team.c b/libgomp/team.c > index b98b233..0bcbaf8 100644 > --- a/libgomp/team.c > +++ b/libgomp/team.c > @@ -134,6 +134,25 @@ gomp_thread_start (void *xdata) > return NULL; > } > > +static struct gomp_team * > +get_recycable_team (unsigned nthreads) That would be recyclable. But I think get_last_team would be better. Also, please make it static inline. > + team = gomp_malloc (sizeof (*team) + nthreads * extra); > + > +#ifndef HAVE_SYNC_BUILTINS > + gomp_mutex_init (&team->work_share_list_free_lock); > +#endif Avoiding gomp_mutex_destroy/gomp_mutex_init is fine, but I must say I'm far less sure about gomp_sem_init (can you add there a temporary assert that it has the expected value) and even less about gomp_barrier_init (I think e.g. on Linux generation will be very unlikely 0 that it should be, and not sure about awaited_final value either). Jakub
On 13/07/15 16:17, Jakub Jelinek wrote: > On Mon, Jul 13, 2015 at 01:15:44PM +0200, Sebastian Huber wrote: >> diff --git a/libgomp/team.c b/libgomp/team.c >> index b98b233..0bcbaf8 100644 >> --- a/libgomp/team.c >> +++ b/libgomp/team.c >> @@ -134,6 +134,25 @@ gomp_thread_start (void *xdata) >> return NULL; >> } >> >> +static struct gomp_team * >> +get_recycable_team (unsigned nthreads) > That would be recyclable. > But I think get_last_team would be better. Ok. > Also, please make it static inline. Out of curiosity, does this make a difference for a static function in a module if it has the inline or not? > >> + team = gomp_malloc (sizeof (*team) + nthreads * extra); >> + >> +#ifndef HAVE_SYNC_BUILTINS >> + gomp_mutex_init (&team->work_share_list_free_lock); >> +#endif > Avoiding gomp_mutex_destroy/gomp_mutex_init is fine, > but I must say I'm far less sure about gomp_sem_init (can you > add there a temporary assert that it has the expected value) > and even less about gomp_barrier_init (I think e.g. on Linux > generation will be very unlikely 0 that it should be, and not > sure about awaited_final value either). > > Jakub I didn't observe any testsuite failures on x86_64-unknown-linux-gnu with this patch. I will add asserts and re-run the testsuite tomorrow.
On 13/07/15 16:33, Sebastian Huber wrote: > > > On 13/07/15 16:17, Jakub Jelinek wrote: >> On Mon, Jul 13, 2015 at 01:15:44PM +0200, Sebastian Huber wrote: >>> diff --git a/libgomp/team.c b/libgomp/team.c >>> index b98b233..0bcbaf8 100644 >>> --- a/libgomp/team.c >>> +++ b/libgomp/team.c >>> @@ -134,6 +134,25 @@ gomp_thread_start (void *xdata) >>> return NULL; >>> } >>> +static struct gomp_team * >>> +get_recycable_team (unsigned nthreads) >> That would be recyclable. >> But I think get_last_team would be better. > > Ok. > >> Also, please make it static inline. > > Out of curiosity, does this make a difference for a static function in > a module if it has the inline or not? > >> >>> + team = gomp_malloc (sizeof (*team) + nthreads * extra); >>> + >>> +#ifndef HAVE_SYNC_BUILTINS >>> + gomp_mutex_init (&team->work_share_list_free_lock); >>> +#endif >> Avoiding gomp_mutex_destroy/gomp_mutex_init is fine, >> but I must say I'm far less sure about gomp_sem_init (can you >> add there a temporary assert that it has the expected value) >> and even less about gomp_barrier_init (I think e.g. on Linux >> generation will be very unlikely 0 that it should be, and not >> sure about awaited_final value either). >> >> Jakub > > I didn't observe any testsuite failures on x86_64-unknown-linux-gnu > with this patch. I will add asserts and re-run the testsuite tomorrow. > The team->master_release semaphore is not always zero after use. I got a test failure here: pr29947-2.exe: libgomp/team.c:152: get_last_team: Assertion `last_team->master_release == 0' failed. The state of barrier seems to be all right. I added these assertions: #include <assert.h> static inline struct gomp_team * get_last_team (unsigned nthreads) { struct gomp_thread *thr = gomp_thread (); if (thr->ts.team == NULL) { struct gomp_thread_pool *pool = thr->thread_pool; if (pool != NULL) { struct gomp_team *last_team = pool->last_team; if (last_team != NULL && last_team->nthreads == nthreads) { pool->last_team = NULL; assert (last_team->barrier.total == nthreads); assert (last_team->barrier.awaited == nthreads); assert (last_team->barrier.awaited_final == nthreads); assert (last_team->barrier.generation % BAR_INCR != 0); return last_team; } } } return NULL; } None of them fails if I run the test suite. Running libgomp/testsuite/libgomp.c/c.exp ... Running libgomp/testsuite/libgomp.c++/c++.exp ... Running libgomp/testsuite/libgomp.fortran/fortran.exp ... Running libgomp/testsuite/libgomp.graphite/graphite.exp ... Running libgomp/testsuite/libgomp.oacc-c/c.exp ... Running libgomp/testsuite/libgomp.oacc-c++/c++.exp ... Running libgomp/testsuite/libgomp.oacc-fortran/fortran.exp ... === libgomp Summary === # of expected passes 5987 # of expected failures 8 # of unsupported tests 278 Should I still leave the barrier init as is?
On 14/07/15 09:04, Sebastian Huber wrote: > #include <assert.h> > > static inline struct gomp_team * > get_last_team (unsigned nthreads) > { > struct gomp_thread *thr = gomp_thread (); > if (thr->ts.team == NULL) > { > struct gomp_thread_pool *pool = thr->thread_pool; > if (pool != NULL) > { > struct gomp_team *last_team = pool->last_team; > if (last_team != NULL && last_team->nthreads == nthreads) > { > pool->last_team = NULL; > assert (last_team->barrier.total == nthreads); > assert (last_team->barrier.awaited == nthreads); > assert (last_team->barrier.awaited_final == nthreads); > assert (last_team->barrier.generation % BAR_INCR != 0); Sorry, this should be: assert (last_team->barrier.generation % BAR_INCR == 0); I pasted the wrong version, since I wanted to check that I get a failure in case generation % BAR_INCR != 0. > return last_team; > } > } > } > return NULL; > }
On Tue, Jul 14, 2015 at 09:09:01AM +0200, Sebastian Huber wrote: > I pasted the wrong version, since I wanted to check that I get a failure in > case generation % BAR_INCR != 0. There is also config/posix/bar* implementation, which would need to be checked too. I'd suggest first committing a patch with both sem and mutex being destroyed (note, config/posix/bar* has two semaphores in it), and perhaps if you feel strongly about it an incremental patch to avoid the destroying/initialization of the barrier if it works even with config/posix/bar*. Jakub
On 14/07/15 09:19, Jakub Jelinek wrote: > On Tue, Jul 14, 2015 at 09:09:01AM +0200, Sebastian Huber wrote: >> I pasted the wrong version, since I wanted to check that I get a failure in >> case generation % BAR_INCR != 0. > There is also config/posix/bar* implementation, which would need to be > checked too. I'd suggest first committing a patch with both sem and mutex > being destroyed (note, config/posix/bar* has two semaphores in it), and > perhaps if you feel strongly about it an incremental patch to avoid the > destroying/initialization of the barrier if it works even with > config/posix/bar*. > > Jakub If I destroy the barrier of the last team, and then initialize it later in gomp_new_team() +static inline struct gomp_team * +get_last_team (unsigned nthreads) +{ + struct gomp_thread *thr = gomp_thread (); + if (thr->ts.team == NULL) + { + struct gomp_thread_pool *pool = thr->thread_pool; + if (pool != NULL) + { + struct gomp_team *last_team = pool->last_team; + if (last_team != NULL && last_team->nthreads == nthreads) + { + pool->last_team = NULL; + gomp_barrier_destroy (&last_team->barrier); + return last_team; + } + } + } + return NULL; +} then I get test suite failures. Is it safe to destroy the team barrier here or do we circumvent the last team mechanism which is supposed to delay the destruction? Example failure: WARNING: program timed out. FAIL: libgomp.c/appendix-a/a.15.1.c execution test
On Tue, Jul 14, 2015 at 11:28:18AM +0200, Sebastian Huber wrote: > If I destroy the barrier of the last team, and then initialize it later in > gomp_new_team() > > +static inline struct gomp_team * > +get_last_team (unsigned nthreads) > +{ > + struct gomp_thread *thr = gomp_thread (); > + if (thr->ts.team == NULL) > + { > + struct gomp_thread_pool *pool = thr->thread_pool; > + if (pool != NULL) > + { > + struct gomp_team *last_team = pool->last_team; > + if (last_team != NULL && last_team->nthreads == nthreads) > + { > + pool->last_team = NULL; > + gomp_barrier_destroy (&last_team->barrier); > + return last_team; > + } > + } > + } > + return NULL; > +} > > then I get test suite failures. Is it safe to destroy the team barrier here > or do we circumvent the last team mechanism which is supposed to delay the > destruction? Then you indeed supposedly hit the reason why last_team exists. Thus, if not reinitializing the team barrier works even with --disable-linux-futex configured libgomp, I guess it is ok not to destroy it and reinit it again. Jakub
diff --git a/libgomp/team.c b/libgomp/team.c index b98b233..0bcbaf8 100644 --- a/libgomp/team.c +++ b/libgomp/team.c @@ -134,6 +134,25 @@ gomp_thread_start (void *xdata) return NULL; } +static struct gomp_team * +get_recycable_team (unsigned nthreads) +{ + struct gomp_thread *thr = gomp_thread (); + if (thr->ts.team == NULL) + { + struct gomp_thread_pool *pool = thr->thread_pool; + if (pool != NULL) + { + struct gomp_team *last_team = pool->last_team; + if (last_team != NULL && last_team->nthreads == nthreads) + { + pool->last_team = NULL; + return last_team; + } + } + } + return NULL; +} /* Create a new team data structure. */ @@ -141,18 +160,28 @@ struct gomp_team * gomp_new_team (unsigned nthreads) { struct gomp_team *team; - size_t size; int i; - size = sizeof (*team) + nthreads * (sizeof (team->ordered_release[0]) - + sizeof (team->implicit_task[0])); - team = gomp_malloc (size); + team = get_recycable_team (nthreads); + if (team == NULL) + { + size_t extra = sizeof (team->ordered_release[0]) + + sizeof (team->implicit_task[0]); + team = gomp_malloc (sizeof (*team) + nthreads * extra); + +#ifndef HAVE_SYNC_BUILTINS + gomp_mutex_init (&team->work_share_list_free_lock); +#endif + gomp_barrier_init (&team->barrier, nthreads); + gomp_sem_init (&team->master_release, 0); + gomp_mutex_init (&team->task_lock); + + team->nthreads = nthreads; + } team->work_share_chunk = 8; #ifdef HAVE_SYNC_BUILTINS team->single_count = 0; -#else - gomp_mutex_init (&team->work_share_list_free_lock); #endif team->work_shares_to_free = &team->work_shares[0]; gomp_init_work_share (&team->work_shares[0], false, nthreads); @@ -163,14 +192,9 @@ gomp_new_team (unsigned nthreads) team->work_shares[i].next_free = &team->work_shares[i + 1]; team->work_shares[i].next_free = NULL; - team->nthreads = nthreads; - gomp_barrier_init (&team->barrier, nthreads); - - gomp_sem_init (&team->master_release, 0); team->ordered_release = (void *) &team->implicit_task[nthreads]; team->ordered_release[0] = &team->master_release; - gomp_mutex_init (&team->task_lock); team->task_queue = NULL; team->task_count = 0; team->task_queued_count = 0; @@ -187,6 +211,10 @@ gomp_new_team (unsigned nthreads) static void free_team (struct gomp_team *team) { + gomp_sem_destroy (&team->master_release); +#ifndef HAVE_SYNC_BUILTINS + gomp_mutex_destroy (&team->work_share_list_free_lock); +#endif gomp_barrier_destroy (&team->barrier); gomp_mutex_destroy (&team->task_lock); free (team); @@ -894,10 +922,6 @@ gomp_team_end (void) } while (ws != NULL); } - gomp_sem_destroy (&team->master_release); -#ifndef HAVE_SYNC_BUILTINS - gomp_mutex_destroy (&team->work_share_list_free_lock); -#endif if (__builtin_expect (thr->ts.team != NULL, 0) || __builtin_expect (team->nthreads == 1, 0))