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 2022/05/30 10:22:15 UTC

[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6337: Refine the implementation of TLS

pkarashchenko commented on code in PR #6337:
URL: https://github.com/apache/incubator-nuttx/pull/6337#discussion_r884652321


##########
libs/libc/pthread/pthread_keycreate.c:
##########
@@ -70,23 +72,45 @@
 int pthread_key_create(FAR pthread_key_t *key,
                        CODE void (*destructor)(FAR void *))
 {
-  int tlsindex;
+  FAR struct task_info_s *info = task_get_info();
+  int candidate;
+  int ret;
 
   DEBUGASSERT(key != NULL);
+  DEBUGASSERT(info != NULL);
 
-  /* Allocate a TLS index */
+  /* Search for an unused index.  This is done in a critical section here to
+   * avoid concurrent modification of the group TLS index set.
+   */
 
-  tlsindex = tls_alloc(destructor);
+  ret = _SEM_WAIT(&info->ta_sem);
 
-  /* Check if found a TLS index. */
+  if (ret < 0)
+    {
+      ret = _SEM_ERRNO(ret);
+      return ret;
+    }
+
+  ret = EAGAIN;
 
-  if (tlsindex >= 0)
+  for (candidate = 0; candidate < CONFIG_TLS_NELEM; candidate++)
     {
-      *key = tlsindex;
-      return OK;
+      /* Is this candidate index available? */
+
+      tls_ndxset_t mask = (tls_ndxset_t)1 << candidate;
+      if ((info->ta_tlsset & mask) == 0)
+        {
+          /* Yes.. allocate the index and break out of the loop */
+
+          info->ta_tlsset |= mask;
+          info->ta_tlsdtor[candidate] = destructor;
+          *key = candidate;
+          break;

Review Comment:
   I think `ret = OK;` is missing before `break;` ;)



##########
libs/libc/pthread/pthread_keycreate.c:
##########
@@ -70,23 +72,45 @@
 int pthread_key_create(FAR pthread_key_t *key,
                        CODE void (*destructor)(FAR void *))
 {
-  int tlsindex;
+  FAR struct task_info_s *info = task_get_info();
+  int candidate;
+  int ret;
 
   DEBUGASSERT(key != NULL);
+  DEBUGASSERT(info != NULL);
 
-  /* Allocate a TLS index */
+  /* Search for an unused index.  This is done in a critical section here to
+   * avoid concurrent modification of the group TLS index set.
+   */
 
-  tlsindex = tls_alloc(destructor);
+  ret = _SEM_WAIT(&info->ta_sem);
 
-  /* Check if found a TLS index. */
+  if (ret < 0)
+    {
+      ret = _SEM_ERRNO(ret);
+      return ret;
+    }

Review Comment:
   I think we need to wait for https://github.com/apache/incubator-nuttx/pull/6320 to be merged first as according to POSIX: `The pthread_key_create() function shall not return an error code of [EINTR]`



##########
libs/libc/pthread/pthread_keydelete.c:
##########
@@ -52,8 +57,33 @@
 
 int pthread_key_delete(pthread_key_t key)
 {
-  /* Free the TLS index */
+  FAR struct task_info_s *info = task_get_info();
+  tls_ndxset_t mask;
+  int ret = EINVAL;
+
+  DEBUGASSERT(info != NULL);
+  DEBUGASSERT(key >= 0 && key < CONFIG_TLS_NELEM);
+  if (key >= 0 && key < CONFIG_TLS_NELEM)
+    {
+      /* This is done while holding a semaphore here to avoid concurrent
+       * modification of the group TLS index set.
+       */
 
-  int ret = tls_free((int)key);
-  return ret < 0 ? -ret : 0;
+      mask = (tls_ndxset_t)1 << key;
+      ret = _SEM_WAIT(&info->ta_sem);

Review Comment:
   I think we need to wait for https://github.com/apache/incubator-nuttx/pull/6320 to be merged first as according to POSIX: `The pthread_key_delete() function shall not return an error code of [EINTR].`



-- 
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