You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/06/05 15:35:14 UTC
[GitHub] [incubator-nuttx] no1wudi opened a new pull request #3858: tls: Move pthread key destructor to libc
no1wudi opened a new pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858
## Summary
See #3626
This patch move the pthread key destructor from task_group_s to task_info_s (in main task stack) to let destructors can access in userspace.
## Impact
Should be none
## Testing
Test on maix-bit, both PROTECTED build & FLAT build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646083075
##########
File path: libs/libc/tls/tls_getset.c
##########
@@ -56,16 +53,14 @@
tls_ndxset_t tls_get_set(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t tlsset;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
- flags = spin_lock_irqsave(NULL);
- tlsset = group->tg_tlsset;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ tlsset = tinfo->ta_tlsset;
+ sem_post(&tinfo->ta_tlssem);
return tlsset;
Review comment:
I find that only tls_alloc & tls_free need protection
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033162
##########
File path: libs/libc/tls/tls_free.c
##########
@@ -59,26 +56,22 @@
int tls_free(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t mask;
- irqstate_t flags;
int ret = -EINVAL;
- DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && group != NULL);
+ DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && tinfo != NULL);
if ((unsigned)tlsindex < CONFIG_TLS_NELEM)
{
/* This is done in a critical section here to avoid concurrent
* modification of the group TLS index set.
*/
mask = (1 << tlsindex);
- flags = spin_lock_irqsave(NULL);
-
- DEBUGASSERT((group->tg_tlsset & mask) != 0);
- group->tg_tlsset &= ~mask;
- spin_unlock_irqrestore(NULL, flags);
-
+ sem_wait(&tinfo->ta_tlssem);
+ DEBUGASSERT((tinfo->ta_tlsset & mask) != 0);
Review comment:
What if the semaphore wait is awakened by a receipt of a signal? Or if the thread is canceled while waiting? In these cases, the function must return EINTR or ECANCELED, respectively, for an error indication. Higher levels must abort the operation and propagate the error indication.
##########
File path: libs/libc/tls/tls_getdtor.c
##########
@@ -57,19 +54,17 @@
tls_dtor_t tls_get_dtor(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
- tls_dtor_t destr;
+ FAR struct task_info_s *tinfo = task_get_info();
+ tls_dtor_t dtor;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- destr = group->tg_tlsdestr[tlsindex];
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ dtor = tinfo->ta_tlsdtor[tlsindex];
Review comment:
What if the semaphore wait is awakened by a receipt of a signal? Or if the thread is canceled while waiting? In these cases, the function must return EINTR or ECANCELED, respectively, for an error indication. Higher levels must abort the operation and propagate the error indication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033253
##########
File path: libs/libc/tls/tls_getdtor.c
##########
@@ -57,19 +54,17 @@
tls_dtor_t tls_get_dtor(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
- tls_dtor_t destr;
+ FAR struct task_info_s *tinfo = task_get_info();
+ tls_dtor_t dtor;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- destr = group->tg_tlsdestr[tlsindex];
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ dtor = tinfo->ta_tlsdtor[tlsindex];
Review comment:
The fetch is atomic and does not require that you take the semaphore. This is true provided that tls_ndxset_t is not a uint64_t.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646065548
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
> The TLS key relative interface wouldn't be called from OS side, execpt **nx_pthread_exit** by syscall (drop to user space)
Then either ifdef __KERNEL__ should be used in the Make.defs to specifically exclude access to the kernel or _SEM_WAIT should be used instead of sem_wait() so that if it is called from the OS, nothing bad happens. I think everything else in libs/libc uses _SEM_WAIT, I suspect.
Handling the errors from sem_wait is important. For example, catching the ECANCELED error and unwinding to return an error indication to the cancellation point logic. I am not certain if any of these are involved in cancellation point interfaces, but supporting that behavior is the safest option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646065548
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
> The TLS key relative interface wouldn't be called from OS side, execpt **nx_pthread_exit** by syscall (drop to user space)
Then either ifdef __KERNEL__ should be used in the Make.defs to specifically exclude access to the kernel or _SEM_WAIT should be used instead of sem_wait() so that if it is called from the OS, nothing bad happens. I think everything else in libs/libc uses _SEM_WAIT, I suspect.
Handling the errors from sem_wait() is important; if sem_wait() fails then you do not hold the semaphore. That breaks the protect and the following sem_post() screws up the counting. Errors will occur rather frequently in some conditions: From signals and pthread cancellation
For pthread cancellation, catching the ECANCELED error and unwinding to return an error indication to the cancellation point logic may be important too. I am not certain if any of these are involved in cancellation point interfaces, but supporting that behavior is the safest option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033253
##########
File path: libs/libc/tls/tls_getdtor.c
##########
@@ -57,19 +54,17 @@
tls_dtor_t tls_get_dtor(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
- tls_dtor_t destr;
+ FAR struct task_info_s *tinfo = task_get_info();
+ tls_dtor_t dtor;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- destr = group->tg_tlsdestr[tlsindex];
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ dtor = tinfo->ta_tlsdtor[tlsindex];
Review comment:
The fetch is atomic and does not require that you take the semaphore. The critical section was not necessary in the original code either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033168
##########
File path: libs/libc/tls/tls_getdtor.c
##########
@@ -57,19 +54,17 @@
tls_dtor_t tls_get_dtor(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
- tls_dtor_t destr;
+ FAR struct task_info_s *tinfo = task_get_info();
+ tls_dtor_t dtor;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- destr = group->tg_tlsdestr[tlsindex];
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ dtor = tinfo->ta_tlsdtor[tlsindex];
Review comment:
What if the semaphore wait is awakened by a receipt of a signal? Or if the thread is canceled while waiting?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646036213
##########
File path: libs/libc/tls/tls_alloc.c
##########
@@ -57,34 +54,33 @@
int tls_alloc(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
int candidate;
int ret = -EAGAIN;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
/* Search for an unused index. This is done in a critical section here to
* avoid concurrent modification of the group TLS index set.
*/
- flags = spin_lock_irqsave(NULL);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
Should _SEM_WAIT be used? It should be if this could ever be called from within the OS. If not, then it should not be built if __KERNEL__ is defined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033116
##########
File path: libs/libc/tls/tls_alloc.c
##########
@@ -57,34 +54,33 @@
int tls_alloc(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
int candidate;
int ret = -EAGAIN;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
/* Search for an unused index. This is done in a critical section here to
* avoid concurrent modification of the group TLS index set.
*/
- flags = spin_lock_irqsave(NULL);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
What if the semaphore wait is awakened by a receipt of a signal?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646065548
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
> The TLS key relative interface wouldn't be called from OS side, execpt **nx_pthread_exit** by syscall (drop to user space)
Then either ifdef __KERNEL__ should be used to specifically exclude access to the kernel or _SEM_WAIT should be used instead of sem_wait() so that if it is called from the OS, nothing bad happens. I think everything else in libs/libc uses _SEM_WAIT, I suspect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646035957
##########
File path: libs/libc/tls/tls_getset.c
##########
@@ -56,16 +53,14 @@
tls_ndxset_t tls_get_set(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t tlsset;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
- flags = spin_lock_irqsave(NULL);
- tlsset = group->tg_tlsset;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ tlsset = tinfo->ta_tlsset;
Review comment:
What if the semaphore wait is awakened by a receipt of a signal? Or if the thread is canceled while waiting?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646036872
##########
File path: libs/libc/tls/tls_getdtor.c
##########
@@ -57,19 +54,17 @@
tls_dtor_t tls_get_dtor(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
- tls_dtor_t destr;
+ FAR struct task_info_s *tinfo = task_get_info();
+ tls_dtor_t dtor;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- destr = group->tg_tlsdestr[tlsindex];
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ dtor = tinfo->ta_tlsdtor[tlsindex];
+ sem_post(&tinfo->ta_tlssem);
- return destr;
+ return dtor;
Review comment:
Needs to return an error if sem_wait() fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033253
##########
File path: libs/libc/tls/tls_getdtor.c
##########
@@ -57,19 +54,17 @@
tls_dtor_t tls_get_dtor(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
- tls_dtor_t destr;
+ FAR struct task_info_s *tinfo = task_get_info();
+ tls_dtor_t dtor;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- destr = group->tg_tlsdestr[tlsindex];
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ dtor = tinfo->ta_tlsdtor[tlsindex];
Review comment:
The fetch is atomic and does not require that you take the semaphore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033116
##########
File path: libs/libc/tls/tls_alloc.c
##########
@@ -57,34 +54,33 @@
int tls_alloc(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
int candidate;
int ret = -EAGAIN;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
/* Search for an unused index. This is done in a critical section here to
* avoid concurrent modification of the group TLS index set.
*/
- flags = spin_lock_irqsave(NULL);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
What if the semaphore wait is awakened by a receipt of a signal? Or if the thread is canceled while waiting?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#issuecomment-869467433
Still think that https://github.com/apache/incubator-nuttx/pull/3858 is the right direction. We just need handle the special case found by @yamt: https://github.com/apache/incubator-nuttx/pull/3877, since it could:
1. Move all tls destructor to userspace
2. Move all exit callback to userspace
3. Move all environment function to userspace
4. More(e.g. cancelation pointer).
and also:
1. Avoid the complex interaction between the userspace and kernel.
2. Is more aligned with the tranditional OS implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646036830
##########
File path: libs/libc/tls/tls_free.c
##########
@@ -59,26 +56,22 @@
int tls_free(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t mask;
- irqstate_t flags;
int ret = -EINVAL;
- DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && group != NULL);
+ DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && tinfo != NULL);
if ((unsigned)tlsindex < CONFIG_TLS_NELEM)
{
/* This is done in a critical section here to avoid concurrent
* modification of the group TLS index set.
*/
mask = (1 << tlsindex);
- flags = spin_lock_irqsave(NULL);
-
- DEBUGASSERT((group->tg_tlsset & mask) != 0);
- group->tg_tlsset &= ~mask;
- spin_unlock_irqrestore(NULL, flags);
-
+ sem_wait(&tinfo->ta_tlssem);
+ DEBUGASSERT((tinfo->ta_tlsset & mask) != 0);
+ tinfo->ta_tlsset &= ~mask;
+ sem_post(&tinfo->ta_tlssem);
ret = OK;
Review comment:
Needs to return an error if sem_wait() fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646080451
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
Yes, I got it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646065548
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
> The TLS key relative interface wouldn't be called from OS side, execpt **nx_pthread_exit** by syscall (drop to user space)
Then either ifdef __ KERNEL __ should be used in the Make.defs to specifically exclude access to the kernel or _SEM_WAIT should be used instead of sem_wait() so that if it is called from the OS, nothing bad happens. I think everything else in libs/libc uses _SEM_WAIT, I suspect.
Handling the errors from sem_wait() is important; if sem_wait() fails then you do not hold the semaphore. That breaks the protect and the following sem_post() screws up the counting. Errors will occur rather frequently in some conditions: From signals and pthread cancellation
For pthread cancellation, catching the ECANCELED error and unwinding to return an error indication to the cancellation point logic may be important too. I am not certain if any of these are involved in cancellation point interfaces, but supporting that behavior is the safest option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033162
##########
File path: libs/libc/tls/tls_free.c
##########
@@ -59,26 +56,22 @@
int tls_free(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t mask;
- irqstate_t flags;
int ret = -EINVAL;
- DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && group != NULL);
+ DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && tinfo != NULL);
if ((unsigned)tlsindex < CONFIG_TLS_NELEM)
{
/* This is done in a critical section here to avoid concurrent
* modification of the group TLS index set.
*/
mask = (1 << tlsindex);
- flags = spin_lock_irqsave(NULL);
-
- DEBUGASSERT((group->tg_tlsset & mask) != 0);
- group->tg_tlsset &= ~mask;
- spin_unlock_irqrestore(NULL, flags);
-
+ sem_wait(&tinfo->ta_tlssem);
+ DEBUGASSERT((tinfo->ta_tlsset & mask) != 0);
Review comment:
What if the semaphore wait is awakened by a receipt of a signal? Or if the thread is canceled while waiting?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646032244
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ tinfo->ta_tlsdtor[tlsindex] = dtor;
+ sem_post(&tinfo->ta_tlssem);
Review comment:
You don't need to take the semaphore here. The assignment is an atomic operation. This is true provided that tls_ndxset_t is not a uint64_t.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646032244
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ tinfo->ta_tlsdtor[tlsindex] = dtor;
+ sem_post(&tinfo->ta_tlssem);
Review comment:
You don't need to take the semaphore here. The assignment is an atomic operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646032244
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ tinfo->ta_tlsdtor[tlsindex] = dtor;
+ sem_post(&tinfo->ta_tlssem);
Review comment:
You don't need to take the semaphore here. The assignment is an atomic operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646065548
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
> The TLS key relative interface wouldn't be called from OS side, execpt **nx_pthread_exit** by syscall (drop to user space)
Then either ifdef __KERNEL__ should be used in the Make.defs to specifically exclude access to the kernel or _SEM_WAIT should be used instead of sem_wait() so that if it is called from the OS, nothing bad happens. I think everything else in libs/libc uses _SEM_WAIT, I suspect.
Handling the errors from sem_wait is important. For example, catching the ECANCELED error and unwinding to return an error indication to the cancellation point logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033116
##########
File path: libs/libc/tls/tls_alloc.c
##########
@@ -57,34 +54,33 @@
int tls_alloc(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
int candidate;
int ret = -EAGAIN;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
/* Search for an unused index. This is done in a critical section here to
* avoid concurrent modification of the group TLS index set.
*/
- flags = spin_lock_irqsave(NULL);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
What if the semaphore wait is awakened by a receipt of a signal? Or if the thread is canceled while waiting? In these cases, the function must return EINTR or ECANCELED, respectively, for an error indication. Higher levels must abort the operation and propagate the error indication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646065548
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
> The TLS key relative interface wouldn't be called from OS side, execpt **nx_pthread_exit** by syscall (drop to user space)
Then either ifdef __KERNEL__ should be used in the Make.defs to specifically exclude access to the kernel or _SEM_WAIT should be used instead of sem_wait() so that if it is called from the OS, nothing bad happens. I think everything else in libs/libc uses _SEM_WAIT, I suspect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646031778
##########
File path: include/nuttx/sched.h
##########
@@ -530,14 +530,6 @@ 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 */
-
Review comment:
Nevermind... I see you replaced with critical section with a semaphore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646036272
##########
File path: libs/libc/tls/tls_getset.c
##########
@@ -56,16 +53,14 @@
tls_ndxset_t tls_get_set(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t tlsset;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
- flags = spin_lock_irqsave(NULL);
- tlsset = group->tg_tlsset;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
Should _SEM_WAIT be used? It should be if this could ever be called from within the OS. If not, then it should not be built if __KERNEL__ is defined.
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
Should _SEM_WAIT be used? It should be if this could ever be called from within the OS. If not, then it should not be built if __KERNEL__ is defined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646035957
##########
File path: libs/libc/tls/tls_getset.c
##########
@@ -56,16 +53,14 @@
tls_ndxset_t tls_get_set(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t tlsset;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
- flags = spin_lock_irqsave(NULL);
- tlsset = group->tg_tlsset;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ tlsset = tinfo->ta_tlsset;
Review comment:
What if the semaphore wait is awakened by a receipt of a signal? Or if the thread is canceled while waiting? In these cases, the function must return EINTR or ECANCELED, respectively, for an error indication. Higher levels must abort the operation and propagate the error indication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646031730
##########
File path: libs/libc/tls/tls_alloc.c
##########
@@ -57,34 +54,33 @@
int tls_alloc(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
int candidate;
int ret = -EAGAIN;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
/* Search for an unused index. This is done in a critical section here to
* avoid concurrent modification of the group TLS index set.
*/
- flags = spin_lock_irqsave(NULL);
+ sem_wait(&tinfo->ta_tlssem);
+
Review comment:
I see that you added a semaphore to replace the critical section. I suppose that makes my comment above not so relevant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646037122
##########
File path: libs/libc/tls/tls_alloc.c
##########
@@ -57,34 +54,33 @@
int tls_alloc(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
int candidate;
int ret = -EAGAIN;
Review comment:
Inconsistent error reporting. Some functions call sem_wait() which sets the errno and returns ERROR on and error. Others follow the internal OS convention of returning a negated errno value. This needs to be made consistent and documented.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646036908
##########
File path: libs/libc/tls/tls_getset.c
##########
@@ -56,16 +53,14 @@
tls_ndxset_t tls_get_set(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t tlsset;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
- flags = spin_lock_irqsave(NULL);
- tlsset = group->tg_tlsset;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ tlsset = tinfo->ta_tlsset;
+ sem_post(&tinfo->ta_tlssem);
return tlsset;
Review comment:
Needs to return an error if sem_wait() fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo merged pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646031567
##########
File path: include/nuttx/sched.h
##########
@@ -530,14 +530,6 @@ 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 */
-
Review comment:
I think that tg_tlsset should remain inside the group structure. It is not accessible to user's and should be protected. Access must be atomic and this was done with a critical section, I believe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033162
##########
File path: libs/libc/tls/tls_free.c
##########
@@ -59,26 +56,22 @@
int tls_free(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t mask;
- irqstate_t flags;
int ret = -EINVAL;
- DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && group != NULL);
+ DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && tinfo != NULL);
if ((unsigned)tlsindex < CONFIG_TLS_NELEM)
{
/* This is done in a critical section here to avoid concurrent
* modification of the group TLS index set.
*/
mask = (1 << tlsindex);
- flags = spin_lock_irqsave(NULL);
-
- DEBUGASSERT((group->tg_tlsset & mask) != 0);
- group->tg_tlsset &= ~mask;
- spin_unlock_irqrestore(NULL, flags);
-
+ sem_wait(&tinfo->ta_tlssem);
+ DEBUGASSERT((tinfo->ta_tlsset & mask) != 0);
Review comment:
What if the semaphore wait is awakened by a receipt of a signal?
##########
File path: libs/libc/tls/tls_getdtor.c
##########
@@ -57,19 +54,17 @@
tls_dtor_t tls_get_dtor(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
- tls_dtor_t destr;
+ FAR struct task_info_s *tinfo = task_get_info();
+ tls_dtor_t dtor;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- destr = group->tg_tlsdestr[tlsindex];
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
+ dtor = tinfo->ta_tlsdtor[tlsindex];
Review comment:
What if the semaphore wait is awakened by a receipt of a signal?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646031976
##########
File path: libs/libc/tls/tls_free.c
##########
@@ -59,26 +56,22 @@
int tls_free(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t mask;
- irqstate_t flags;
int ret = -EINVAL;
- DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && group != NULL);
+ DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && tinfo != NULL);
if ((unsigned)tlsindex < CONFIG_TLS_NELEM)
{
/* This is done in a critical section here to avoid concurrent
Review comment:
```suggestion
/* This is done while holding a semaphore here to avoid concurrent
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646064494
##########
File path: libs/libc/tls/tls_setdtor.c
##########
@@ -59,18 +56,16 @@
*
****************************************************************************/
-int tls_set_dtor(int tlsindex, tls_dtor_t destr)
+int tls_set_dtor(int tlsindex, tls_dtor_t dtor)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
DEBUGASSERT(tlsindex >= 0 && tlsindex < CONFIG_TLS_NELEM);
- flags = spin_lock_irqsave(NULL);
- group->tg_tlsdestr[tlsindex] = destr;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
The TLS key relative interface wouldn't be called from OS side, execpt **nx_pthread_exit** by syscall (drop to user space)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646033321
##########
File path: libs/libc/tls/tls_getset.c
##########
@@ -56,16 +53,14 @@
tls_ndxset_t tls_get_set(void)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
- irqstate_t flags;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t tlsset;
- DEBUGASSERT(group != NULL);
+ DEBUGASSERT(tinfo != NULL);
- flags = spin_lock_irqsave(NULL);
- tlsset = group->tg_tlsset;
- spin_unlock_irqrestore(NULL, flags);
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
Unlike some of the other comments, since modification of the tlsset is not atomic, the semphore protection is probably a good idea in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3858: tls: Move pthread key destructor to libc
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3858:
URL: https://github.com/apache/incubator-nuttx/pull/3858#discussion_r646036255
##########
File path: libs/libc/tls/tls_free.c
##########
@@ -59,26 +56,22 @@
int tls_free(int tlsindex)
{
- FAR struct tcb_s *rtcb = this_task();
- FAR struct task_group_s *group = rtcb->group;
+ FAR struct task_info_s *tinfo = task_get_info();
tls_ndxset_t mask;
- irqstate_t flags;
int ret = -EINVAL;
- DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && group != NULL);
+ DEBUGASSERT((unsigned)tlsindex < CONFIG_TLS_NELEM && tinfo != NULL);
if ((unsigned)tlsindex < CONFIG_TLS_NELEM)
{
/* This is done in a critical section here to avoid concurrent
* modification of the group TLS index set.
*/
mask = (1 << tlsindex);
- flags = spin_lock_irqsave(NULL);
-
- DEBUGASSERT((group->tg_tlsset & mask) != 0);
- group->tg_tlsset &= ~mask;
- spin_unlock_irqrestore(NULL, flags);
-
+ sem_wait(&tinfo->ta_tlssem);
Review comment:
Should _SEM_WAIT be used? It should be if this could ever be called from within the OS. If not, then it should not be built if __KERNEL__ is defined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org