Corrected patch: call pthread_setspecific (gomp_tls_key, NULL) in
gomp_thread_start if HAVE_TLS is not defined.
2014-09-19 Varvara Rainchik <varvara.rainc...@intel.com>
* libgomp.h (gomp_thread): For non TLS case create thread data.
* team.c (non_tls_thread_data_destructor,
create_non_tls_thread_data): New functions.
---
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index bcd5b34..2f33d99 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -467,9 +467,15 @@ static inline struct gomp_thread *gomp_thread (void)
}
#else
extern pthread_key_t gomp_tls_key;
-static inline struct gomp_thread *gomp_thread (void)
+extern struct gomp_thread *create_non_tls_thread_data (void);
+static struct gomp_thread *gomp_thread (void)
{
- return pthread_getspecific (gomp_tls_key);
+ struct gomp_thread *thr = pthread_getspecific (gomp_tls_key);
+ if (thr == NULL)
+ {
+ thr = create_non_tls_thread_data ();
+ }
+ return thr;
}
#endif
diff --git a/libgomp/team.c b/libgomp/team.c
index e6a6d8f..1854d8a 100644
--- a/libgomp/team.c
+++ b/libgomp/team.c
@@ -41,6 +41,7 @@ pthread_key_t gomp_thread_destructor;
__thread struct gomp_thread gomp_tls_data;
#else
pthread_key_t gomp_tls_key;
+struct gomp_thread initial_thread_tls_data;
#endif
@@ -130,6 +131,9 @@ gomp_thread_start (void *xdata)
gomp_sem_destroy (&thr->release);
thr->thread_pool = NULL;
thr->task = NULL;
+#ifndef HAVE_TLS
+ pthread_setspecific (gomp_tls_key, NULL);
+#endif
return NULL;
}
@@ -222,8 +226,16 @@ gomp_free_pool_helper (void *thread_pool)
void
gomp_free_thread (void *arg __attribute__((unused)))
{
- struct gomp_thread *thr = gomp_thread ();
- struct gomp_thread_pool *pool = thr->thread_pool;
+ struct gomp_thread *thr;
+ struct gomp_thread_pool *pool;
+#ifdef HAVE_TLS
+ thr = gomp_thread ();
+#else
+ thr = pthread_getspecific (gomp_tls_key);
+ if (thr == NULL)
+ return;
+#endif
+ pool = thr->thread_pool;
if (pool)
{
if (pool->threads_used > 0)
@@ -910,6 +922,21 @@ gomp_team_end (void)
}
}
+/* Destructor for data created in create_non_tls_thread_data. */
+
+#ifndef HAVE_TLS
+void
+non_tls_thread_data_destructor (void *arg __attribute__((unused)))
+{
+ struct gomp_thread *thr = pthread_getspecific (gomp_tls_key);
+ if (thr != NULL && thr != &initial_thread_tls_data)
+ {
+ gomp_free_thread (arg);
+ free (thr);
+ pthread_setspecific (gomp_tls_key, NULL);
+ }
+}
+#endif
/* Constructors for this file. */
@@ -917,9 +944,7 @@ static void __attribute__((constructor))
initialize_team (void)
{
#ifndef HAVE_TLS
- static struct gomp_thread initial_thread_tls_data;
-
- pthread_key_create (&gomp_tls_key, NULL);
+ pthread_key_create (&gomp_tls_key, non_tls_thread_data_destructor);
pthread_setspecific (gomp_tls_key, &initial_thread_tls_data);
#endif
@@ -927,6 +952,19 @@ initialize_team (void)
gomp_fatal ("could not create thread pool destructor.");
}
+/* Create data for thread created by pthread_create. */
+
+#ifndef HAVE_TLS
+struct gomp_thread *create_non_tls_thread_data (void)
+{
+ struct gomp_thread *thr = gomp_malloc_cleared (sizeof (struct gomp_thread));
+ pthread_setspecific (gomp_tls_key, thr);
+ gomp_sem_init (&thr->release, 0);
+
+ return thr;
+}
+#endif
+
static void __attribute__((destructor))
team_destructor (void)
{
2014-09-24 14:19 GMT+04:00 Varvara Rainchik <varvara.s.rainc...@gmail.com>:
> *Ping*
>
> 2014-09-19 15:41 GMT+04:00 Varvara Rainchik <varvara.s.rainc...@gmail.com>:
>> I've corrected my patch accordingly to what you said. To diffirentiate
>> second case in destructor I've added pthread_setspecific
>> (gomp_tls_key, NULL) at the end of gomp_thread_start. So, destructor
>> can simply skip the case when pthread_getspecific (gomp_tls_key)
>> returns 0. I also think that it's better to set 0 in gomp_thread_start
>> explicitly as thread data is initialized by a local variable in this
>> function.
>>
>> But, I see that pthread_getspecific always returns 0 in destrucor
>> because data pointer is implicitly set to 0 before destructor call in
>> glibc:
>>
>> (pthread_create.c):
>>
>> /* Always clear the data. */
>> level2[inner].data = NULL;
>>
>> /* Make sure the data corresponds to a valid
>> key. This test fails if the key was
>> deallocated and also if it was
>> re-allocated. It is the user's
>> responsibility to free the memory in this
>> case. */
>> if (level2[inner].seq
>> == __pthread_keys[idx].seq
>> /* It is not necessary to register a destructor
>> function. */
>> && __pthread_keys[idx].destr != NULL)
>> /* Call the user-provided destructor. */
>> __pthread_keys[idx].destr (data);
>>
>> I suppose it's not necessary if everything is cleaned up in
>> gomp_thread_start and destructor. What do you think?
>>
>>
>> Changes are bootstrapped and regtested on x86_64-linux.
>>
>> 2014-09-19 Varvara Rainchik <varvara.rainc...@intel.com>
>>
>> * libgomp.h (gomp_thread): For non TLS case create thread data.
>> * team.c (non_tls_thread_data_destructor,
>> create_non_tls_thread_data): New functions.
>>
>>
>> ---
>> diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
>> index bcd5b34..2f33d99 100644
>> --- a/libgomp/libgomp.h
>> +++ b/libgomp/libgomp.h
>> @@ -467,9 +467,15 @@ static inline struct gomp_thread *gomp_thread (void)
>> }
>> #else
>> extern pthread_key_t gomp_tls_key;
>> -static inline struct gomp_thread *gomp_thread (void)
>> +extern struct gomp_thread *create_non_tls_thread_data (void);
>> +static struct gomp_thread *gomp_thread (void)
>> {
>> - return pthread_getspecific (gomp_tls_key);
>> + struct gomp_thread *thr = pthread_getspecific (gomp_tls_key);
>> + if (thr == NULL)
>> + {
>> + thr = create_non_tls_thread_data ();
>> + }
>> + return thr;
>> }
>> #endif
>>
>> diff --git a/libgomp/team.c b/libgomp/team.c
>> index e6a6d8f..a692df8 100644
>> --- a/libgomp/team.c
>> +++ b/libgomp/team.c
>> @@ -41,6 +41,7 @@ pthread_key_t gomp_thread_destructor;
>> __thread struct gomp_thread gomp_tls_data;
>> #else
>> pthread_key_t gomp_tls_key;
>> +struct gomp_thread initial_thread_tls_data;
>> #endif
>>
>>
>> @@ -130,6 +131,7 @@ gomp_thread_start (void *xdata)
>> gomp_sem_destroy (&thr->release);
>> thr->thread_pool = NULL;
>> thr->task = NULL;
>> + pthread_setspecific (gomp_tls_key, NULL);
>> return NULL;
>> }
>>
>> @@ -222,8 +224,16 @@ gomp_free_pool_helper (void *thread_pool)
>> void
>> gomp_free_thread (void *arg __attribute__((unused)))
>> {
>> - struct gomp_thread *thr = gomp_thread ();
>> - struct gomp_thread_pool *pool = thr->thread_pool;
>> + struct gomp_thread *thr;
>> + struct gomp_thread_pool *pool;
>> +#ifdef HAVE_TLS
>> + thr = gomp_thread ();
>> +#else
>> + thr = pthread_getspecific (gomp_tls_key);
>> + if (thr == NULL)
>> + return;
>> +#endif
>> + pool = thr->thread_pool;
>> if (pool)
>> {
>> if (pool->threads_used > 0)
>> @@ -910,6 +920,21 @@ gomp_team_end (void)
>> }
>> }
>>
>> +/* Destructor for data created in create_non_tls_thread_data. */
>> +
>> +#ifndef HAVE_TLS
>> +void
>> +non_tls_thread_data_destructor (void *arg __attribute__((unused)))
>> +{
>> + struct gomp_thread *thr = pthread_getspecific (gomp_tls_key);
>> + if (thr != NULL && thr != &initial_thread_tls_data)
>> + {
>> + gomp_free_thread (arg);
>> + free (thr);
>> + pthread_setspecific (gomp_tls_key, NULL);
>> + }
>> +}
>> +#endif
>>
>> /* Constructors for this file. */
>>
>> @@ -917,9 +942,7 @@ static void __attribute__((constructor))
>> initialize_team (void)
>> {
>> #ifndef HAVE_TLS
>> - static struct gomp_thread initial_thread_tls_data;
>> -
>> - pthread_key_create (&gomp_tls_key, NULL);
>> + pthread_key_create (&gomp_tls_key, non_tls_thread_data_destructor);
>> pthread_setspecific (gomp_tls_key, &initial_thread_tls_data);
>> #endif
>>
>> @@ -927,6 +950,19 @@ initialize_team (void)
>> gomp_fatal ("could not create thread pool destructor.");
>> }
>>
>> +/* Create data for thread created by pthread_create. */
>> +
>> +#ifndef HAVE_TLS
>> +struct gomp_thread *create_non_tls_thread_data (void)
>> +{
>> + struct gomp_thread *thr = gomp_malloc_cleared (sizeof (struct
>> gomp_thread));
>> + pthread_setspecific (gomp_tls_key, thr);
>> + gomp_sem_init (&thr->release, 0);
>> +
>> + return thr;
>> +}
>> +#endif
>> +
>> static void __attribute__((destructor))
>> team_destructor (void)
>> {
>>
>> 2014-09-02 14:36 GMT+04:00 Varvara Rainchik <varvara.s.rainc...@gmail.com>:
>>> May I use gomp_free_thread as a destructor for pthread_key_create?
>>> Then I'll make initial_thread_tls_data global for the first case, but
>>> how can I differentiate thread created by gomp_thread_start (second
>>> case)?
>>>
>>> 2014-09-01 14:51 GMT+04:00 Jakub Jelinek <ja...@redhat.com>:
>>>> On Fri, Aug 29, 2014 at 10:40:57AM -0700, Richard Henderson wrote:
>>>>> On 08/06/2014 03:05 AM, Varvara Rainchik wrote:
>>>>> > * libgomp.h (gomp_thread): For non TLS case create thread data.
>>>>> > * team.c (create_non_tls_thread_data): New function.
>>>>> >
>>>>> >
>>>>> > ---
>>>>> > diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
>>>>> > index a1482cc..cf3ec8f 100644
>>>>> > --- a/libgomp/libgomp.h
>>>>> > +++ b/libgomp/libgomp.h
>>>>> > @@ -479,9 +479,15 @@ static inline struct gomp_thread *gomp_thread
>>>>> > (void)
>>>>> > }
>>>>> > #else
>>>>> > extern pthread_key_t gomp_tls_key;
>>>>> > +extern struct gomp_thread *create_non_tls_thread_data (void);
>>>>> > static inline struct gomp_thread *gomp_thread (void)
>>>>> > {
>>>>> > - return pthread_getspecific (gomp_tls_key);
>>>>> > + struct gomp_thread *thr = pthread_getspecific (gomp_tls_key);
>>>>> > + if (thr == NULL)
>>>>> > + {
>>>>> > + thr = create_non_tls_thread_data ();
>>>>> > + }
>>>>> > + return thr;
>>>>> > }
>>>>>
>>>>> This should never happen.
>>>>
>>>> I guess it can happen if you mix up explicit pthread_create and libgomp
>>>> APIs.
>>>> initialize_team will only initialize it in the initial thread, while if you
>>>> use #pragma omp ... or omp_* calls from a thread created with
>>>> pthread_create, in the !HAVE_TLS case pthread_getspecific will return NULL.
>>>>
>>>> Now, the patch doesn't handle that case completely though (and is badly
>>>> formatted), the problem is that if we allocate in the !HAVE_TLS case
>>>> in non-initial thread the TLS data, we want to free them again, so that
>>>> would mean pthread_key_create with non-NULL destructor, and then we need to
>>>> differentiate in between the 3 cases - key equal to
>>>> &initial_thread_tls_data
>>>> (would need to move out of the block context), no freeing needed, thread
>>>> created by gomp_thread_start, no freeing needed, otherwise free.
>>>>
>>>>> The thread-specific data is set in gomp_thread_start and initialize_team.
>>>>>
>>>>> Where are you getting a call to gomp_thread that hasn't been through one
>>>>> of
>>>>> those functions?
>>>>
>>>> Jakub
