From b3e8535ad62581e081b4bfb947fb28dba2a9062b Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Wed, 9 Jun 2021 15:53:23 +0900 Subject: [PATCH] Revert "tls: Move pthread key destructor to libc" This reverts commit cc514d7791dae0808124249ff545d9dda39b24b7. * It introduced a regression. https://github.com/apache/incubator-nuttx/issues/3868 * It seems conceptually wrong to have per-process data in the main thread's stack. --- include/nuttx/sched.h | 8 ++++ include/nuttx/tls.h | 23 ----------- include/sys/syscall_lookup.h | 8 ++++ libs/libc/pthread/pthread_keycreate.c | 2 +- libs/libc/tls/Make.defs | 3 -- sched/group/Make.defs | 5 +++ .../group/group_tlsalloc.c | 29 ++++++-------- .../tls_free.c => sched/group/group_tlsfree.c | 39 +++++++++---------- .../group/group_tlsgetdtor.c | 21 ++++++---- .../group/group_tlsgetset.c | 15 +++++-- .../group/group_tlssetdtor.c | 21 ++++++---- sched/task/task_init.c | 8 ---- sched/task/task_setup.c | 26 ------------- syscall/syscall.csv | 5 +++ 14 files changed, 97 insertions(+), 116 deletions(-) rename libs/libc/tls/tls_alloc.c => sched/group/group_tlsalloc.c (85%) rename libs/libc/tls/tls_free.c => sched/group/group_tlsfree.c (75%) rename libs/libc/tls/tls_getdtor.c => sched/group/group_tlsgetdtor.c (84%) rename libs/libc/tls/tls_getset.c => sched/group/group_tlsgetset.c (85%) rename libs/libc/tls/tls_setdtor.c => sched/group/group_tlssetdtor.c (82%) diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index fc196680fc..8f324402a7 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -530,6 +530,14 @@ struct task_group_s FAR struct join_s *tg_jointail; /* Tail of a list of join data */ #endif + /* Thread local storage ***************************************************/ + +#if CONFIG_TLS_NELEM > 0 + tls_ndxset_t tg_tlsset; /* Set of TLS indexes allocated */ + + tls_dtor_t tg_tlsdestr[CONFIG_TLS_NELEM]; /* List of TLS destructors */ +#endif + /* POSIX Signal Control Fields ********************************************/ sq_queue_t tg_sigactionq; /* List of actions for signals */ diff --git a/include/nuttx/tls.h b/include/nuttx/tls.h index 6950751568..d45230dd06 100644 --- a/include/nuttx/tls.h +++ b/include/nuttx/tls.h @@ -118,13 +118,6 @@ struct task_info_s struct tls_info_s ta_tls; /* Must be first field */ #ifndef CONFIG_BUILD_KERNEL struct getopt_s ta_getopt; /* Globals used by getopt() */ -#endif - /* Thread local storage ***************************************************/ - -#if CONFIG_TLS_NELEM > 0 - sem_t ta_tlssem; - tls_ndxset_t ta_tlsset; /* Set of TLS index allocated */ - tls_dtor_t ta_tlsdtor[CONFIG_TLS_NELEM]; /* List of TLS destructors */ #endif }; @@ -327,20 +320,4 @@ void tls_destruct(void); FAR struct task_info_s *task_get_info(void); -/**************************************************************************** - * Name: task_setup_info - * - * Description: - * Setup task_info_s for task - * - * Input Parameters: - * info - New created task_info_s - * - * Returned Value: - * OK on success; ERROR on failure - * - ****************************************************************************/ - -int task_setup_info(FAR struct task_info_s *info); - #endif /* __INCLUDE_NUTTX_TLS_H */ diff --git a/include/sys/syscall_lookup.h b/include/sys/syscall_lookup.h index 03de33c0d2..a0e44d7dbd 100644 --- a/include/sys/syscall_lookup.h +++ b/include/sys/syscall_lookup.h @@ -294,6 +294,14 @@ SYSCALL_LOOKUP(telldir, 1) SYSCALL_LOOKUP(shmdt, 1) #endif +#if CONFIG_TLS_NELEM > 0 + SYSCALL_LOOKUP(tls_alloc, 0) + SYSCALL_LOOKUP(tls_free, 1) + SYSCALL_LOOKUP(tls_get_set, 1) + SYSCALL_LOOKUP(tls_get_dtor, 1) + SYSCALL_LOOKUP(tls_set_dtor, 2) +#endif + /* The following are defined if pthreads are enabled */ #ifndef CONFIG_DISABLE_PTHREAD diff --git a/libs/libc/pthread/pthread_keycreate.c b/libs/libc/pthread/pthread_keycreate.c index d162fbf8c0..798cca7d38 100644 --- a/libs/libc/pthread/pthread_keycreate.c +++ b/libs/libc/pthread/pthread_keycreate.c @@ -89,7 +89,7 @@ int pthread_key_create(FAR pthread_key_t *key, return OK; } - return ERROR; + return -tlsindex; } #endif /* CONFIG_TLS_NELEM */ diff --git a/libs/libc/tls/Make.defs b/libs/libc/tls/Make.defs index ab1da9069a..67e830f71a 100644 --- a/libs/libc/tls/Make.defs +++ b/libs/libc/tls/Make.defs @@ -22,9 +22,6 @@ CSRCS += task_getinfo.c ifneq ($(CONFIG_TLS_NELEM),0) CSRCS += tls_setvalue.c tls_getvalue.c tls_destruct.c -CSRCS += tls_getdtor.c tls_setdtor.c -CSRCS += tls_alloc.c tls_free.c -CSRCS += tls_getset.c endif ifneq ($(CONFIG_TLS_ALIGNED),y) diff --git a/sched/group/Make.defs b/sched/group/Make.defs index b0a2cf7e13..35eee89702 100644 --- a/sched/group/Make.defs +++ b/sched/group/Make.defs @@ -53,6 +53,11 @@ ifneq ($(CONFIG_BUILD_FLAT),y) CSRCS += group_malloc.c group_zalloc.c group_free.c endif +ifneq ($(CONFIG_TLS_NELEM),0) +CSRCS += group_tlsalloc.c group_tlsfree.c +CSRCS += group_tlsgetset.c group_tlsgetdtor.c group_tlssetdtor.c +endif + # Include group build support DEPPATH += --dep-path group diff --git a/libs/libc/tls/tls_alloc.c b/sched/group/group_tlsalloc.c similarity index 85% rename from libs/libc/tls/tls_alloc.c rename to sched/group/group_tlsalloc.c index 2630e7e415..5b09073abc 100644 --- a/libs/libc/tls/tls_alloc.c +++ b/sched/group/group_tlsalloc.c @@ -1,5 +1,5 @@ /**************************************************************************** - * libs/libc/tls/tls_alloc.c + * sched/group/group_tlsalloc.c * * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with @@ -32,6 +32,9 @@ #include #include +#include "sched/sched.h" +#include "group/group.h" + #if CONFIG_TLS_NELEM > 0 /**************************************************************************** @@ -49,45 +52,39 @@ * * Returned Value: * A TLS index that is unique for use within this task group. - * If unsuccessful, an errno value will be returned and set to errno. * ****************************************************************************/ int tls_alloc(void) { - FAR struct task_info_s *tinfo = task_get_info(); + FAR struct tcb_s *rtcb = this_task(); + FAR struct task_group_s *group = rtcb->group; + irqstate_t flags; int candidate; int ret = -EAGAIN; - DEBUGASSERT(tinfo != NULL); + DEBUGASSERT(group != NULL); /* Search for an unused index. This is done in a critical section here to * avoid concurrent modification of the group TLS index set. */ - ret = _SEM_WAIT(&tinfo->ta_tlssem); - - if (ERROR == ret) - { - ret = -get_errno(); - goto errout_with_errno; - } - + flags = spin_lock_irqsave(NULL); for (candidate = 0; candidate < CONFIG_TLS_NELEM; candidate++) { /* Is this candidate index available? */ tls_ndxset_t mask = (1 << candidate); - if ((tinfo->ta_tlsset & mask) == 0) + if ((group->tg_tlsset & mask) == 0) { /* Yes.. allocate the index and break out of the loop */ - tinfo->ta_tlsset |= mask; + group->tg_tlsset |= mask; break; } } - _SEM_POST(&tinfo->ta_tlssem); + spin_unlock_irqrestore(NULL, flags); /* Check if found a valid TLS data index. */ @@ -98,8 +95,6 @@ int tls_alloc(void) ret = candidate; } -errout_with_errno: - return ret; } diff --git a/libs/libc/tls/tls_free.c b/sched/group/group_tlsfree.c similarity index 75% rename from libs/libc/tls/tls_free.c rename to sched/group/group_tlsfree.c index 7455e08cb3..c1eccaf25d 100644 --- a/libs/libc/tls/tls_free.c +++ b/sched/group/group_tlsfree.c @@ -1,5 +1,5 @@ /**************************************************************************** - * libs/libc/tls/tls_free.c + * sched/group/group_tlsfree.c * * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with @@ -31,6 +31,9 @@ #include #include +#include "sched/sched.h" +#include "group/group.h" + #if CONFIG_TLS_NELEM > 0 /**************************************************************************** @@ -47,40 +50,36 @@ * tlsindex - The previously allocated TLS index to be freed * * Returned Value: - * OK is returned on success; a negated errno value will be returned and - * set to errno on failure: + * OK is returned on success; a negated errno value will be returned on + * failure: * - * -EINVAL - the index to be freed is out of range. - * -EINTR - the wait operation interrupted by signal - * -ECANCELED - the thread was canceled during waiting + * -EINVAL - the index to be freed is out of range. * ****************************************************************************/ int tls_free(int tlsindex) { - FAR struct task_info_s *tinfo = task_get_info(); + FAR struct tcb_s *rtcb = this_task(); + FAR struct task_group_s *group = rtcb->group; tls_ndxset_t mask; + irqstate_t flags; int ret = -EINVAL; - DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && tinfo != NULL); + DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && group != NULL); if ((unsigned)tlsindex < CONFIG_TLS_NELEM) { - /* This is done while holding a semaphore here to avoid concurrent + /* This is done in a critical section here to avoid concurrent * modification of the group TLS index set. */ mask = (1 << tlsindex); - ret = _SEM_WAIT(&tinfo->ta_tlssem); - if (OK == ret) - { - DEBUGASSERT((tinfo->ta_tlsset & mask) != 0); - tinfo->ta_tlsset &= ~mask; - _SEM_POST(&tinfo->ta_tlssem); - } - else - { - ret = -get_errno(); - } + flags = spin_lock_irqsave(NULL); + + DEBUGASSERT((group->tg_tlsset & mask) != 0); + group->tg_tlsset &= ~mask; + spin_unlock_irqrestore(NULL, flags); + + ret = OK; } return ret; diff --git a/libs/libc/tls/tls_getdtor.c b/sched/group/group_tlsgetdtor.c similarity index 84% rename from libs/libc/tls/tls_getdtor.c rename to sched/group/group_tlsgetdtor.c index 1e33b327d1..c6e25f3f70 100644 --- a/libs/libc/tls/tls_getdtor.c +++ b/sched/group/group_tlsgetdtor.c @@ -1,5 +1,5 @@ /**************************************************************************** - * libs/libc/tls/tls_getdtor.c + * sched/group/group_tlsgetdtor.c * * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with @@ -32,6 +32,9 @@ #include #include +#include "sched/sched.h" +#include "group/group.h" + #if CONFIG_TLS_NELEM > 0 /**************************************************************************** @@ -42,7 +45,7 @@ * Name: tls_get_dtor * * Description: - * Get the TLS element destructor associated with the 'tlsindex' to 'dtor' + * Get the TLS element destructor associated with the 'tlsindex' to 'destr' * * Input Parameters: * tlsindex - Index of TLS data destructor to get @@ -54,15 +57,19 @@ tls_dtor_t tls_get_dtor(int tlsindex) { - FAR struct task_info_s *tinfo = task_get_info(); - tls_dtor_t dtor; + FAR struct tcb_s *rtcb = this_task(); + FAR struct task_group_s *group = rtcb->group; + irqstate_t flags; + tls_dtor_t destr; - DEBUGASSERT(tinfo != NULL); + DEBUGASSERT(group != NULL); DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM); - dtor = tinfo->ta_tlsdtor[tlsindex]; + flags = spin_lock_irqsave(NULL); + destr = group->tg_tlsdestr[tlsindex]; + spin_unlock_irqrestore(NULL, flags); - return dtor; + return destr; } #endif /* CONFIG_TLS_NELEM > 0 */ diff --git a/libs/libc/tls/tls_getset.c b/sched/group/group_tlsgetset.c similarity index 85% rename from libs/libc/tls/tls_getset.c rename to sched/group/group_tlsgetset.c index cb3c9587a9..a3405c44a5 100644 --- a/libs/libc/tls/tls_getset.c +++ b/sched/group/group_tlsgetset.c @@ -1,5 +1,5 @@ /**************************************************************************** - * libs/libc/tls/tls_getset.c + * sched/group/group_tlsgetset.c * * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with @@ -32,6 +32,9 @@ #include #include +#include "sched/sched.h" +#include "group/group.h" + #if CONFIG_TLS_NELEM > 0 /**************************************************************************** @@ -53,12 +56,16 @@ tls_ndxset_t tls_get_set(void) { - FAR struct task_info_s *tinfo = task_get_info(); + FAR struct tcb_s *rtcb = this_task(); + FAR struct task_group_s *group = rtcb->group; + irqstate_t flags; tls_ndxset_t tlsset; - DEBUGASSERT(tinfo != NULL); + DEBUGASSERT(group != NULL); - tlsset = tinfo->ta_tlsset; + flags = spin_lock_irqsave(NULL); + tlsset = group->tg_tlsset; + spin_unlock_irqrestore(NULL, flags); return tlsset; } diff --git a/libs/libc/tls/tls_setdtor.c b/sched/group/group_tlssetdtor.c similarity index 82% rename from libs/libc/tls/tls_setdtor.c rename to sched/group/group_tlssetdtor.c index 4ed813b393..040ff65822 100644 --- a/libs/libc/tls/tls_setdtor.c +++ b/sched/group/group_tlssetdtor.c @@ -1,5 +1,5 @@ /**************************************************************************** - * libs/libc/tls/tls_setdtor.c + * sched/group/group_tlssetdtor.c * * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with @@ -32,6 +32,9 @@ #include #include +#include "sched/sched.h" +#include "group/group.h" + #if CONFIG_TLS_NELEM > 0 /**************************************************************************** @@ -42,11 +45,11 @@ * Name: tls_set_dtor * * Description: - * Set the TLS element destructor associated with the 'tlsindex' to 'dtor' + * Set the TLS element destructor associated with the 'tlsindex' to 'destr' * * Input Parameters: * tlsindex - Index of TLS data destructor to set - * dtor - The dtor of TLS data element + * destr - The destr of TLS data element * * Returned Value: * Zero is returned on success, a negated errno value is return on @@ -56,14 +59,18 @@ * ****************************************************************************/ -int tls_set_dtor(int tlsindex, tls_dtor_t dtor) +int tls_set_dtor(int tlsindex, tls_dtor_t destr) { - FAR struct task_info_s *tinfo = task_get_info(); + FAR struct tcb_s *rtcb = this_task(); + FAR struct task_group_s *group = rtcb->group; + irqstate_t flags; - DEBUGASSERT(tinfo != NULL); + DEBUGASSERT(group != NULL); DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM); - tinfo->ta_tlsdtor[tlsindex] = dtor; + flags = spin_lock_irqsave(NULL); + group->tg_tlsdestr[tlsindex] = destr; + spin_unlock_irqrestore(NULL, flags); return OK; } diff --git a/sched/task/task_init.c b/sched/task/task_init.c index fd774ec1f4..dfd7c7f1dc 100644 --- a/sched/task/task_init.c +++ b/sched/task/task_init.c @@ -142,14 +142,6 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority, DEBUGASSERT(info == tcb->cmn.stack_alloc_ptr); - ret = task_setup_info(info); - - if (ret < OK) - { - ret = -EINVAL; - goto errout_with_group; - } - /* Initialize the task control block */ ret = nxtask_setup_scheduler(tcb, priority, nxtask_start, diff --git a/sched/task/task_setup.c b/sched/task/task_setup.c index 830754614e..73f681512a 100644 --- a/sched/task/task_setup.c +++ b/sched/task/task_setup.c @@ -35,7 +35,6 @@ #include #include #include -#include #include "sched/sched.h" #include "pthread/pthread.h" @@ -722,28 +721,3 @@ int nxtask_setup_arguments(FAR struct task_tcb_s *tcb, FAR const char *name, return nxtask_setup_stackargs(tcb, argv); } - -/**************************************************************************** - * Name: task_setup_info - * - * Description: - * Setup task_info_s for task - * - * Input Parameters: - * info - New created task_info_s - * - * Returned Value: - * OK on success; ERROR on failure - * - ****************************************************************************/ - -int task_setup_info(FAR struct task_info_s *info) -{ - int ret = OK; - -#if CONFIG_TLS_NELEM > 0 - ret = _SEM_INIT(&info->ta_tlssem, 0, 1); -#endif - - return ret; -} diff --git a/syscall/syscall.csv b/syscall/syscall.csv index 0f96f4ed95..523806cf3c 100644 --- a/syscall/syscall.csv +++ b/syscall/syscall.csv @@ -177,6 +177,11 @@ "timer_getoverrun","time.h","!defined(CONFIG_DISABLE_POSIX_TIMERS)","int","timer_t" "timer_gettime","time.h","!defined(CONFIG_DISABLE_POSIX_TIMERS)","int","timer_t","FAR struct itimerspec *" "timer_settime","time.h","!defined(CONFIG_DISABLE_POSIX_TIMERS)","int","timer_t","int","FAR const struct itimerspec *","FAR struct itimerspec *" +"tls_alloc","nuttx/tls.h","CONFIG_TLS_NELEM > 0","int" +"tls_free","nuttx/tls.h","CONFIG_TLS_NELEM > 0","int","int" +"tls_get_dtor","nuttx/tls.h","CONFIG_TLS_NELEM > 0","tls_dtor_t","int" +"tls_get_set","nuttx/tls.h","CONFIG_TLS_NELEM > 0","tls_ndxset_t" +"tls_set_dtor","nuttx/tls.h","CONFIG_TLS_NELEM > 0","int","int","tls_dtor_t" "umount2","sys/mount.h","!defined(CONFIG_DISABLE_MOUNTPOINT)","int","FAR const char *","unsigned int" "unlink","unistd.h","!defined(CONFIG_DISABLE_MOUNTPOINT)","int","FAR const char *" "unsetenv","stdlib.h","!defined(CONFIG_DISABLE_ENVIRON)","int","FAR const char *"