[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage

[GitBox <gi...@apache.org>]

xiaoxiang781216 commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r807985305



##########
File path: sched/task/task_tls_alloc.c
##########
@@ -0,0 +1,113 @@
+/****************************************************************************
+ * sched/task/task_tls_alloc.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <sys/types.h>
+#include <unistd.h>
+#include <sched.h>
+#include <errno.h>
+
+#include <nuttx/tls.h>
+#include <nuttx/semaphore.h>
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+#if CONFIG_TLS_TASK_NELEM > 0
+
+static int g_tlsset = 0;
+static sem_t g_tlssem = SEM_INITIALIZER(1);
+static tls_dtor_t g_tlsdtor[CONFIG_TLS_TASK_NELEM];
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+int task_tls_alloc(tls_dtor_t dtor)
+{
+  int ret;
+  int candidate;
+
+  ret = nxsem_wait(&g_tlssem);
+
+  if (ret < 0)
+    {
+      return ret;
+    }
+
+  ret = -EAGAIN;
+
+  for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+    {
+      int mask = 1 << candidate;
+      if ((g_tlsset & mask) == 0)
+        {
+          g_tlsset |= mask;
+          g_tlsdtor[candidate] = dtor;
+          ret = candidate;
+          break;
+        }
+    }
+
+  nxsem_post(&g_tlssem);
+  return ret;
+}
+
+/****************************************************************************
+ * Name: task_tls_destruct
+ *
+ * Description:
+ *   Destruct all TLS data element associated with allocated key
+ *
+ * Input Parameters:
+ *    None
+ *
+ * Returned Value:
+ *   A set of allocated TLS index
+ *
+ ****************************************************************************/
+
+void task_tls_destruct(void)

Review comment:
       > I would like to get some more explanation about operation of this API. Maybe I'm missing something and that brings my concerns up. For example `CONFIG_TLS_TASK_NELEM` is configured to `3`. Then each task (`struct task_info_s`) will be equipped with additional `3` elements of `uintptr_t` while globally we can have only `CONFIG_TLS_TASK_NELEM` number of destructors (`static tls_dtor_t g_tlsdtor[CONFIG_TLS_TASK_NELEM];`). So if I'm having `10` tasks the `3 * 10 * sizeof(uintptr_t)` will be additionally allocated for tasks and `3 * sizeof(tls_dtor_t)` for destructors. For example `task_A` call `task_tls_alloc()` one time and `task_B` call `task_tls_alloc()` two times. Then `task_A` exits and `task_tls_destruct` is called. The loop will iterate `3` times and globally `if ((g_tlsset & mask) != 0)` will always hit a `true` condition (one time for `task_A` and two times for `task_B`) and `dtor((void *)elem);` will be called `3` times passing 2 times `(void *)0` to `dtor()`. So the q
 uestions are next:
   > 
   > 1. Is it expected that `task_tls_destruct()` will call TLS destructors for other tasks?
   
   Yes, this API set is designed to fix the global variables issue. For example, one library use the global variable `a`, 'b'.... And multiple service call the function exported by this library. So we need support multiple instance of `a`, 'b'... at the same time. One approach is:
   
   1. Add xxx_initialize function to malloc and return a struct contain 'a', 'b'...
   2. All function add one argument to accept the pointer to the memory returned in step 1
   3. Use this struct instead of global variable
   3. Add xxx_deinialize function to free the memory
   
   The problem is that we need extend all pubic function accepted one instance pointer. It's hard to frequently-used library. Another approach (this PR) is:
   
   1. library call task_tls_alloc to allocate one task index once in the whole lifetime
   2. Call task_tls_get when need access global variable
   3. If return NULL, allocate and initialize the global variables and call task_tls_set to save this pointer
   4. Free the memory in task_tls_destruct callback when the task exit
   
   The second approach don't need modify the public function prototype which is critical in some case.
   
   > 2. If yes, then should we have a prerequisite that `dtor()` should check for `NULL` inside that call?
   
   It expect the callback check the special value.
   
   > 3. Is it expected that after `task_tls_destruct()` is called the while all `CONFIG_TLS_TASK_NELEM` are allocated the new `task_tls_alloc()` will always fail because `g_tlsset` never resets bits?
   
   Yes, as the above example, task index is global to prepare the next task which may use the library function again.
   
   > 4. What it `task_tls_destruct()` is called in the middle of `task_tls_alloc()`, for example after `g_tlsdtor[candidate] = dtor;` and before `ret = candidate;`? Will the `dtor` be ready to accept `NULL` (that actually comes back to 2)
   
   Yes, the callback need handle this case carefully.
   
   > 5. Is it expected to have `3 * (N - 1) * sizeof(uintptr_t)` (where `N` is the number of tasks in the system) of waste memory
   
   This interface is designed to the common library(e.g. libuv):
   
   1. Many processes use it
   2. it use global variable internally
   
   If the library is just used by one program, or multiple program at the different time, global variables is still the best choice.
   




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