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/02/15 02:19:12 UTC
[GitHub] [incubator-nuttx] no1wudi opened a new pull request #5498: sched: Implement task local storage
no1wudi opened a new pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498
## Summary
Implement a non-standard thread local storage like task local storage API for some software that required per-task storage (like libuv).
## Impact
None, new API
## Testing
Custom application.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r809213159
##########
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:
this change seems to be more `TGS` (task global storage) then `TLS`
##########
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:
this change seems to be more `TGS` (task global storage) than `TLS`
--
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] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r809779062
##########
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:
> Why do we need `ta_telem[CONFIG_TLS_TASK_NELEM];` to be a member of `struct task_info_s`? I mean why it can't be global? and have only allocated index bits in `struct task_info_s`
Because, each task need point to the different instance(please, consider my example, the library has the global variables, but want to support multiple clients at the same time):
1. All task(at the same index) use the same destructor to free the resource
2. Different task point to the different instance
If you compare to TLS:
1. All thread in the same task share the same destruct callback
2. Different thread point to the different instance
From the concept, task local storage is very similar to thread local storge, but at the task level.
1. Thread local storage allow the code allocate a task wide index with a destructor and attach a different pointer to the different thread at the same index in the same task.
2. Task local storage allow the code allocate a global index with a destructor and attach a different pointer to the different task at the same index.
> We can have 0 .. 64 as a limit for `CONFIG_TLS_TASK_NELEM` and use up to `uint64_t` for index storage. Or add save task `pid` together with each allocated index so that can be reused in `task_tls_destruct` to filter destructors to call
could you explanation more? The code is almost same as the thread local storage, just lift up one level.
> this change seems to be more `TGS` (Thread-global storage) than `TLS`
TLS here mean Task local storage, collide with thread local storage.:(
--
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] acassis commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806862494
##########
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];
Review comment:
@no1wudi please include the custom application as examples/testing.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r809143638
##########
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:
Why do we need `ta_telem[CONFIG_TLS_TASK_NELEM];` to be a member of `struct task_info_s`? I mean why it can't be global? and have only allocated index bits in `struct task_info_s`
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r809143638
##########
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:
Why do we need `ta_telem[CONFIG_TLS_TASK_NELEM];` to be a member of `struct task_info_s`? I mean why it can't be global?
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r811265527
##########
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 tls_task_ndxset_t g_tlsset;
+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;
Review comment:
Maybe `EUSERS` and not `EAGAIN` since indexes are never released?
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810680217
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
+ {
+ elem = info->ta_telem[candidate];
+ dtor = g_tlsdtor[candidate];
+ if (dtor)
Review comment:
Please apply changes back
--
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] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810169659
##########
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 will explain what I mean and also make some comments based on your answers.
>
> > TLS here mean Task local storage, collide with thread local storage.:(
>
> Let's use `task_ls_` and not `task_tls_` so the confusion will be removed. Do you agree?
>
> > From the concept, task local storage is very similar to thread local storage, but at the task level.
>
> Because I do not have visibility about conditions (when and how) of `task_tls_alloc` call, I can't get a full picture. Based on your example let's assume that it is a time when the library is instantiated for the first time. Then the allocated ID will be shared across the library instances while global data will be stored in Task LS. For example I have 2 different libraries that rely on top of Task LS. The library A attach `dtorA` at index 0 and library B attach `dtorB` at index 1. When one of tasks that use library A exits then `task_tls_destruct` is called and we will call `dtorB` from library A user context point of view. Yes, the `0` will be passed as a parameter, but I'm not sure if such situation is valid from isolation point of view.
Yes, it's possible. Another possibility is that the task use both libraries and then dtorA/dtorB get the different non-NULL pointer.
> Even more, if there is a but in library A code and it somehow at some point pass a wrong index to `task_tls_set_value` then `dtorB` will be called with non-zero argument and that value can match with the same value that is allocated by library B users. This potentially can lead to security issues.
>
Yes, the wrong usage will crash the system, but it doesn't increase the security risk, because the task local storage is used only in FLAT/PROTECTED build, not KERNEL build(The static/dynamic library get a new copy for each task in this case). Task in FLAT/PROTECTED mode already can access other tasks and do any evil thing.
> > > We can have 0 .. 64 as a limit for CONFIG_TLS_TASK_NELEM and use up to uint64_t for index storage. Or add save task pid together with each allocated index so that can be reused in task_tls_destruct to filter destructors to call
> >
> >
> > could you explanation more? The code is almost same as the thread local storage, just lift up one level.
>
> There are actually two issues here:
>
> 1. `CONFIG_TLS_TASK_NELEM` is not limited in Kconfig and there is next code:
>
> ```
> int task_tls_alloc(tls_dtor_t dtor)
> {
> int ret;
> int candidate;
> ...
> 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;
> }
> }
> ...
> return ret;
> }
> ```
>
> so if `CONFIG_TLS_TASK_NELEM` is more then number of bits that `int` can carry `if ((g_tlsset & mask) == 0)` will start be `true` always and out of boundary array access will occur. Hopefully this will be caught by compiler.
@no1wudi please add range 0 64 to Kconfig for both CONFIG_TLS_NELEM and CONFIG_TLS_TASK_NELEM.
> 2. Because of lack of use case description I was thinking that each library A user will call `task_tls_alloc()` at the start and will use that allocated index, but maybe it is not a use case (but that is a more like a traditional TLS).
Yes, you are right, each library need allocate the index when it need access the global variables at the first time.
> So I was thinking that we can store a bit mask to keep tracking of indexes allocated by a task. Again even now I'm not aware about this API usage point of view assuming that it can be called by any task in the system.
The API usage it's simple, @no1wudi will provide an usage in libuv like this:
```
static void uv__global_free(void* global) {
if (global) {
uv_library_shutdown();
uv__free(global);
}
}
uv__global_t* uv__global_get(void) {
static int index = -1;
uv__global_t* global = NULL;
if (index < 0) {
index = task_tls_alloc(uv__global_free);
}
if (index >= 0) {
global = (uv__global_t*)task_tls_get_value(index);
if (global == NULL) {
global = (uv__global_t*)uv__calloc(1, sizeof(uv__global_t));
if (global) {
global->once = UV_ONCE_INIT;
global->uv__signal_global_init_guard = UV_ONCE_INIT;
global->uv__signal_lock_pipefd[0] = -1;
global->uv__signal_lock_pipefd[1] = -1;
task_tls_set_value(index, (uintptr_t)global);
}
}
}
ASSERT(global != NULL);
return global;
}
```
Basically, libuv define several global variables, but we have many services call libuv functions in the background. The above code support the multiple libuv's instances with minimal change.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810681107
##########
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 tls_ndxset_t g_tlsset = 0;
Review comment:
We either need to add
```
#if CONFIG_TLS_TASK_NELEM > 0
# if CONFIG_TLS_TASK_NELEM > 64
# error Too many task TLS elements
# elif CONFIG_TLS_TASK_NELEM > 32
typedef uint64_t tls_task_ndxset_t;
# elif CONFIG_TLS_TASK_NELEM > 16
typedef uint32_t tls_task_ndxset_t;
# elif CONFIG_TLS_TASK_NELEM > 8
typedef uint16_t tls_task_ndxset_t;
# else
typedef uint8_t tls_task_ndxset_t;
# endif
```
and use `tls_task_ndxset_t` here or just use `uint64_t` for simplicity.
##########
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 tls_ndxset_t 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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
Review comment:
`tls_task_ndxset_t` or `uint64_t` (not `int`).
##########
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 tls_ndxset_t g_tlsset = 0;
Review comment:
```suggestion
static tls_ndxset_t g_tlsset;
```
##########
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 tls_ndxset_t 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;
Review comment:
`tls_task_ndxset_t` or `uint64_t` (not `int`).
--
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] pkarashchenko commented on pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#issuecomment-1047904134
This is the third time I notice that already discussed and applied changes a missing again. That is not good. Please go via resolved comments to find what I'm talking about
--
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] xiaoxiang781216 merged pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r809220542
##########
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:
Maybe I'm missing some exact use case that you are trying to address, so posting a lot of question. If you can share use case then I can take a look from a different angle
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r807698825
##########
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 questions are next:
1. Is it expected that `task_tls_destruct()` will call TLS destructors for other tasks?
2. If yes, then should we have a prerequisite that `dtor()` should check for `NULL` inside that call?
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?
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)
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810681107
##########
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 tls_ndxset_t g_tlsset = 0;
Review comment:
We either need to add
```
#if CONFIG_TLS_TASK_NELEM > 0
# if CONFIG_TLS_TASK_NELEM > 64
# error Too many task TLS elements
# elif CONFIG_TLS_TASK_NELEM > 32
typedef uint64_t tls_task_ndxset_t;
# elif CONFIG_TLS_TASK_NELEM > 16
typedef uint32_t tls_task_ndxset_t;
# elif CONFIG_TLS_TASK_NELEM > 8
typedef uint16_t tls_task_ndxset_t;
# else
typedef uint8_t tls_task_ndxset_t;
# endif
```
and use `tls_task_ndxset_t` here or just use `uint64_t` for simplicity. Since this is globally allocated, then we can go with `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.
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] xiaoxiang781216 commented on pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#issuecomment-1047942634
@no1wudi you can get the last version from github on the new computer by:
git fetch origin
git branch --track patch origin/patch
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806720452
##########
File path: libs/libc/tls/Kconfig
##########
@@ -54,4 +54,14 @@ config TLS_NELEM
NOTE that the special value of CONFIG_TLS_NELEM disables these
TLS interfaces.
+config TLS_TASK_NELEM
+ int "Number of Task Local Storage elements"
+ default 0
Review comment:
I think that together with ``uintptr_t ta_telem[CONFIG_TLS_TASK_NELEM]; it breaks C89 compatibility
##########
File path: libs/libc/tls/Kconfig
##########
@@ -54,4 +54,14 @@ config TLS_NELEM
NOTE that the special value of CONFIG_TLS_NELEM disables these
TLS interfaces.
+config TLS_TASK_NELEM
+ int "Number of Task Local Storage elements"
+ default 0
Review comment:
Sorry. Missed that!
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
+ {
+ elem = info->ta_telem[candidate];
+ dtor = g_tlsdtor[candidate];
+ if (dtor)
Review comment:
```suggestion
if (dtor != NULL)
```
##########
File path: include/nuttx/tls.h
##########
@@ -267,6 +270,13 @@ uintptr_t tls_get_value(int tlsindex);
int tls_set_value(int tlsindex, uintptr_t tlsvalue);
#endif
+#if CONFIG_TLS_TASK_NELEM > 0
+int task_tls_alloc(tls_dtor_t dtor);
+void task_tls_destruct(void);
+int task_tls_set_value(int tlsindex, uintptr_t tlsvalue);
+uintptr_t task_tls_get_value(int tlsindex);
Review comment:
Do we need to add comment with AP description? Similar to APIs above
##########
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];
Review comment:
Why this is global and not located in `struct task_group_s`?
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
Review comment:
```suggestion
if ((g_tlsset & mask) != 0)
```
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806720452
##########
File path: libs/libc/tls/Kconfig
##########
@@ -54,4 +54,14 @@ config TLS_NELEM
NOTE that the special value of CONFIG_TLS_NELEM disables these
TLS interfaces.
+config TLS_TASK_NELEM
+ int "Number of Task Local Storage elements"
+ default 0
Review comment:
I think that together with ``uintptr_t ta_telem[CONFIG_TLS_TASK_NELEM]; it breaks C89 compatibility
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810141082
##########
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 will explain what I mean and also make some comments based on your answers.
> TLS here mean Task local storage, collide with thread local storage.:(
Let's use `task_ls_` and not `task_tls_` so the confusion will be removed. Do you agree?
> From the concept, task local storage is very similar to thread local storage, but at the task level.
Because I do not have visibility about conditions (when and how) of `task_tls_alloc` call, I can't get a full picture. Based on your example let's assume that it is a time when the library is instantiated for the first time. Then the allocated ID will be shared across the library instances while global data will be stored in Task LS.
For example I have 2 different libraries that rely on top of Task LS. The library A attach `dtorA` at index 0 and library B attach `dtorB` at index 1. When one of tasks that use library A exits then `task_tls_destruct` is called and we will call `dtorB` from library A user context point of view. Yes, the `0` will be passed as a parameter, but I'm not sure if such situation is valid from isolation point of view. Even more, if there is a but in library A code and it somehow at some point pass a wrong index to `task_tls_set_value` then `dtorB` will be called with non-zero argument and that value can match with the same value that is allocated by library B users. This potentially can lead to security issues.
>> We can have 0 .. 64 as a limit for CONFIG_TLS_TASK_NELEM and use up to uint64_t for index storage. Or add save task pid together with each allocated index so that can be reused in task_tls_destruct to filter destructors to call
>
> could you explanation more? The code is almost same as the thread local storage, just lift up one level.
There are actually two issues here:
1. `CONFIG_TLS_TASK_NELEM` is not limited in Kconfig and there is next code:
```
int task_tls_alloc(tls_dtor_t dtor)
{
int ret;
int candidate;
...
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;
}
}
...
return ret;
}
```
so if `CONFIG_TLS_TASK_NELEM` is more then number of bits that `int` can carry `if ((g_tlsset & mask) == 0)` will start be `true` always and out of boundary array access will occur. Hopefully this will be caught by compiler.
2. Because of lack of use case description I was thinking that each library A user will call `task_tls_alloc()` at the start and will use that allocated index, but maybe it is not a use case (but that is a more like a traditional TLS). So I was thinking that we can store a bit mask to keep tracking of indexes allocated by a task. Again even now I'm not aware about this API usage point of view assuming that it can be called by any task in the system.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810681886
##########
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 tls_ndxset_t 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
Review comment:
```suggestion
* None
```
##########
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:
Now I see what you are trying to overcome. Current version is fine as an initial approach. We can enhance it later to overcome a global limitation of `CONFIG_TLS_TASK_NELEM` indexes per system.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810367115
##########
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:
Yes. I will examine the use case more carefully early next week. I briefly went thru lines now and I have a feeling that I solved a similar problem with `pthread_once` + `pthread_key_create` on the library init and then the created key was used to access allocated data (please see example from https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html). I will try to understand what is the difference, but definitely need some more deep dive into what you are trying to solve.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r809181408
##########
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:
We can have 0 .. 64 as a limit for `CONFIG_TLS_TASK_NELEM` and use up to `uint64_t` for index storage. Or add save task `pid` together with each allocated index so that can be reused in `task_tls_destruct` to filter destructors to call
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810367115
##########
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:
Yes. I will examine the use case more carefully early next week. I briefly went thru lines now and I have a feeling that I solved a similar problem with `pthread_once` + `pthread_key_create` on the library init and then the created key was used to access allocated data. I will try to understand what is the difference, but definitely need some more deep dive into what you are trying to solve.
--
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] no1wudi commented on pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#issuecomment-1047927722
> This is the third time I notice that already discussed and applied changes a missing again. That is not good. Please go via resolved comments to find what I'm talking about
Sorry for that, I'm working on two computer so forget to get synchronized with latest version sometimes. I'll update this PR carefully tomorrow.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r809213159
##########
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:
this change seems to be more `TGS` (Thread-global storage) than `TLS`
--
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] acassis commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806862494
##########
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];
Review comment:
@no1wudi please include the custom application as examples/testing.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r807698825
##########
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 questions are next:
1. Is it expected that `task_tls_destruct()` will call TLS destructors for other tasks?
2. If yes, then should we have a prerequisite that `dtor()` should check for `NULL` inside that call?
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?
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)
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 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] no1wudi commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r807984699
##########
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:
These APIs is very similar to pthread_key_create series APIs, the only different is it works at task level, then:
> 1. Is it expected that task_tls_destruct() will call TLS destructors for other tasks?
Yes, if any tls key allocated, we must call task_tls_destruct for all tasks since scheduler don't know wether was the task specific context used.
> If yes, then should we have a prerequisite that dtor() should check for NULL inside that call?
Yes, we can check the data element as pthread does.
> 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 in current implementation, maybe we can add a reference counter for each bit to recycle it ? Or maybe it's not a problem since in current workload, only few application or midlleware like libuv need it, and in such a complex system, the counter is hard to get chance to return zero.
> 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)
It's safe in this situation since task_tls_destruct and task_tls_alloc is independent in different task, if task A use relative API (task_tls_alloc), it's dtor is used for all tasks but it's data element is only used in task A.
> 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
Yes, so these APIs disabled by default.
--
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] no1wudi commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806808283
##########
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];
Review comment:
It's shared by all tasks.
--
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] no1wudi commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806810800
##########
File path: include/nuttx/tls.h
##########
@@ -267,6 +270,13 @@ uintptr_t tls_get_value(int tlsindex);
int tls_set_value(int tlsindex, uintptr_t tlsvalue);
#endif
+#if CONFIG_TLS_TASK_NELEM > 0
+int task_tls_alloc(tls_dtor_t dtor);
+void task_tls_destruct(void);
+int task_tls_set_value(int tlsindex, uintptr_t tlsvalue);
+uintptr_t task_tls_get_value(int tlsindex);
Review comment:
Thanks, I'll add it later
--
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] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810468502
##########
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:
> Yes. I will examine the use case more carefully early next week. I briefly went thru lines now and I have a feeling that I solved a similar problem with `pthread_once` + `pthread_key_create` on the library init and then the created key was used to access allocated data (please see example from https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html). I will try to understand what is the difference, but definitely need some more deep dive into what you are trying to solve.
Your method can't handle multiple task case since pthread_key is specific to one task, in the new task you have to call pthread_key_create again. Basically, task local storage(attach pointer to each task) try to handle the similar problem as thread local storage(attach pointer to each thread in one task), but at the task level.
It isn't a problem in the traditional POSIX OS(e.g. Linux, BSD, NuttX' Kernel mode), since they utilize MMU and each task/process has separate virtual address space, but it's an essential combability issue in NuttX's FLAT/PROTECTED mode.
Apache NuttX Wiki has some article describe this problem:
https://cwiki.apache.org/confluence/display/NUTTX/Linux+Processes+vs+NuttX+Tasks
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=152115079
--
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] xiaoxiang781216 commented on pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#issuecomment-1047010811
@no1wudi please rebase your patch to the latest.
--
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] no1wudi commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806781547
##########
File path: libs/libc/tls/Kconfig
##########
@@ -54,4 +54,14 @@ config TLS_NELEM
NOTE that the special value of CONFIG_TLS_NELEM disables these
TLS interfaces.
+config TLS_TASK_NELEM
+ int "Number of Task Local Storage elements"
+ default 0
Review comment:
There is a macro guard `#if CONFIG_TLS_TASK_NELEM > 0` for it.
##########
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];
Review comment:
It's shared by all tasks.
##########
File path: include/nuttx/tls.h
##########
@@ -267,6 +270,13 @@ uintptr_t tls_get_value(int tlsindex);
int tls_set_value(int tlsindex, uintptr_t tlsvalue);
#endif
+#if CONFIG_TLS_TASK_NELEM > 0
+int task_tls_alloc(tls_dtor_t dtor);
+void task_tls_destruct(void);
+int task_tls_set_value(int tlsindex, uintptr_t tlsvalue);
+uintptr_t task_tls_get_value(int tlsindex);
Review comment:
Thanks, I'll add it later
--
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] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage
Posted by 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
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810428516
##########
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:
@xiaoxiang781216 Range limit added.
--
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] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810169659
##########
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 will explain what I mean and also make some comments based on your answers.
>
> > TLS here mean Task local storage, collide with thread local storage.:(
>
> Let's use `task_ls_` and not `task_tls_` so the confusion will be removed. Do you agree?
>
> > From the concept, task local storage is very similar to thread local storage, but at the task level.
>
> Because I do not have visibility about conditions (when and how) of `task_tls_alloc` call, I can't get a full picture. Based on your example let's assume that it is a time when the library is instantiated for the first time. Then the allocated ID will be shared across the library instances while global data will be stored in Task LS. For example I have 2 different libraries that rely on top of Task LS. The library A attach `dtorA` at index 0 and library B attach `dtorB` at index 1. When one of tasks that use library A exits then `task_tls_destruct` is called and we will call `dtorB` from library A user context point of view. Yes, the `0` will be passed as a parameter, but I'm not sure if such situation is valid from isolation point of view.
Yes, it's possible. Another possibility is that the task use both libraries and then dtorA/dtorB get the different non-NULL pointer.
> Even more, if there is a but in library A code and it somehow at some point pass a wrong index to `task_tls_set_value` then `dtorB` will be called with non-zero argument and that value can match with the same value that is allocated by library B users. This potentially can lead to security issues.
>
Yes, the wrong usage will crash the system, but it doesn't increase the security risk, because the task local storage is used only in FLAT/PROTECTED build, not KERNEL build(The static/dynamic library get a new copy for each task in this case). Task in FLAT/PROTECTED mode already can access other tasks and do any evil thing.
> > > We can have 0 .. 64 as a limit for CONFIG_TLS_TASK_NELEM and use up to uint64_t for index storage. Or add save task pid together with each allocated index so that can be reused in task_tls_destruct to filter destructors to call
> >
> >
> > could you explanation more? The code is almost same as the thread local storage, just lift up one level.
>
> There are actually two issues here:
>
> 1. `CONFIG_TLS_TASK_NELEM` is not limited in Kconfig and there is next code:
>
> ```
> int task_tls_alloc(tls_dtor_t dtor)
> {
> int ret;
> int candidate;
> ...
> 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;
> }
> }
> ...
> return ret;
> }
> ```
>
> so if `CONFIG_TLS_TASK_NELEM` is more then number of bits that `int` can carry `if ((g_tlsset & mask) == 0)` will start be `true` always and out of boundary array access will occur. Hopefully this will be caught by compiler.
@no1wudi please add range 0 64 to Kconfig for both CONFIG_TLS_NELEM and CONFIG_TLS_TASK_NELEM.
> 2. Because of lack of use case description I was thinking that each library A user will call `task_tls_alloc()` at the start and will use that allocated index, but maybe it is not a use case (but that is a more like a traditional TLS).
Yes, you are right, each library need allocate the index when it need access the global variables at the first time.
> So I was thinking that we can store a bit mask to keep tracking of indexes allocated by a task. Again even now I'm not aware about this API usage point of view assuming that it can be called by any task in the system.
The API usage it's simple like the traditional TLS, @no1wudi will provide an usage in libuv like this:
```
static void uv__global_free(void* global) {
if (global) {
uv_library_shutdown();
uv__free(global);
}
}
uv__global_t* uv__global_get(void) {
static int index = -1;
uv__global_t* global = NULL;
if (index < 0) {
index = task_tls_alloc(uv__global_free);
}
if (index >= 0) {
global = (uv__global_t*)task_tls_get_value(index);
if (global == NULL) {
global = (uv__global_t*)uv__calloc(1, sizeof(uv__global_t));
if (global) {
global->once = UV_ONCE_INIT;
global->uv__signal_global_init_guard = UV_ONCE_INIT;
global->uv__signal_lock_pipefd[0] = -1;
global->uv__signal_lock_pipefd[1] = -1;
task_tls_set_value(index, (uintptr_t)global);
}
}
}
ASSERT(global != NULL);
return global;
}
```
Basically, libuv define several global variables, but we have many services call libuv functions in the background. The above code support the multiple libuv's instances with minimal change.
1. uv__global_get allocate index and uv_global_t at the first call of the first task
2. uv__global_get return the same global in the subsequent calls of the first task
3. uv__global_get allocate a new global when the second task call libuv's function at the fist time
4. uv__global_get return the second global in the subsequent calls of the second task
I hope this case help you understand the usage.
--
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] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r809779062
##########
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:
> Why do we need `ta_telem[CONFIG_TLS_TASK_NELEM];` to be a member of `struct task_info_s`? I mean why it can't be global? and have only allocated index bits in `struct task_info_s`
Because, each task need point to the different instance(please, consider my example, the library has the global variables, but want to support multiple clients at the same time):
1. All task(at the same index) use the same destructor to free the resource
2. Different task point to the different instance
If you compare to TLS:
1. All thread in the same task share the same destruct callback
2. Different thread point to the different instance
From the concept, task local storage is very similar to thread local storge, but at the task level.
1. Thread local storage allow the code allocate a task wide index with a destructor and attach a different pointer to the different thread at the same index in the same task.
2. Task local storage allow the code allocate a global index with a destructor and attach a different pointer to the different task at the same index.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810680217
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
+ {
+ elem = info->ta_telem[candidate];
+ dtor = g_tlsdtor[candidate];
+ if (dtor)
Review comment:
Please apply changes back, but let's modify condition a bit:
```suggestion
if (dtor != NULL && elem != 0)
```
In this way we will filter desctructors of the tasks that didn't call `task_tls_set_value`. Similar to `pthread_key`
--
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] acassis commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r812186282
##########
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)
Review comment:
I think the Description from functions existent in the header file should be duplicated here. Not sure, but I think this is way we are doing it currently. I don't know how we will handle it when we use some script to generate documentation from these functions Description, the duplicated Description (in header and in C file) could case some issue.
--
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] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810169659
##########
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 will explain what I mean and also make some comments based on your answers.
>
> > TLS here mean Task local storage, collide with thread local storage.:(
>
> Let's use `task_ls_` and not `task_tls_` so the confusion will be removed. Do you agree?
>
> > From the concept, task local storage is very similar to thread local storage, but at the task level.
>
> Because I do not have visibility about conditions (when and how) of `task_tls_alloc` call, I can't get a full picture. Based on your example let's assume that it is a time when the library is instantiated for the first time. Then the allocated ID will be shared across the library instances while global data will be stored in Task LS. For example I have 2 different libraries that rely on top of Task LS. The library A attach `dtorA` at index 0 and library B attach `dtorB` at index 1. When one of tasks that use library A exits then `task_tls_destruct` is called and we will call `dtorB` from library A user context point of view. Yes, the `0` will be passed as a parameter, but I'm not sure if such situation is valid from isolation point of view.
Yes, it's possible. Another possibility is that the task use both libraries and then dtorA/dtorB get the different non-NULL pointer.
> Even more, if there is a but in library A code and it somehow at some point pass a wrong index to `task_tls_set_value` then `dtorB` will be called with non-zero argument and that value can match with the same value that is allocated by library B users. This potentially can lead to security issues.
>
Yes, the wrong usage will crash the system, but it doesn't increase the security risk, because the task local storage is used only in FLAT/PROTECTED build, not KERNEL build(The static/dynamic library get a new copy for each task in this case). Task in FLAT/PROTECTED mode already can access other tasks and do any evil thing.
> > > We can have 0 .. 64 as a limit for CONFIG_TLS_TASK_NELEM and use up to uint64_t for index storage. Or add save task pid together with each allocated index so that can be reused in task_tls_destruct to filter destructors to call
> >
> >
> > could you explanation more? The code is almost same as the thread local storage, just lift up one level.
>
> There are actually two issues here:
>
> 1. `CONFIG_TLS_TASK_NELEM` is not limited in Kconfig and there is next code:
>
> ```
> int task_tls_alloc(tls_dtor_t dtor)
> {
> int ret;
> int candidate;
> ...
> 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;
> }
> }
> ...
> return ret;
> }
> ```
>
> so if `CONFIG_TLS_TASK_NELEM` is more then number of bits that `int` can carry `if ((g_tlsset & mask) == 0)` will start be `true` always and out of boundary array access will occur. Hopefully this will be caught by compiler.
@no1wudi please add range 0 64 to Kconfig for both CONFIG_TLS_NELEM and CONFIG_TLS_TASK_NELEM.
> 2. Because of lack of use case description I was thinking that each library A user will call `task_tls_alloc()` at the start and will use that allocated index, but maybe it is not a use case (but that is a more like a traditional TLS).
Yes, you are right, each library need allocate the index when it need access the global variables at the first time.
> So I was thinking that we can store a bit mask to keep tracking of indexes allocated by a task. Again even now I'm not aware about this API usage point of view assuming that it can be called by any task in the system.
The API usage it's simple like the traditional TLS, @no1wudi will provide an usage in libuv like this:
```
static void uv__global_free(void* global) {
if (global) {
uv_library_shutdown();
uv__free(global);
}
}
uv__global_t* uv__global_get(void) {
static int index = -1;
uv__global_t* global = NULL;
if (index < 0) {
index = task_tls_alloc(uv__global_free);
}
if (index >= 0) {
global = (uv__global_t*)task_tls_get_value(index);
if (global == NULL) {
global = (uv__global_t*)uv__calloc(1, sizeof(uv__global_t));
if (global) {
global->once = UV_ONCE_INIT;
global->uv__signal_global_init_guard = UV_ONCE_INIT;
global->uv__signal_lock_pipefd[0] = -1;
global->uv__signal_lock_pipefd[1] = -1;
task_tls_set_value(index, (uintptr_t)global);
}
}
}
ASSERT(global != NULL);
return global;
}
```
Basically, libuv define several global variables, but we have many services call libuv functions in the background. The above code support the multiple libuv's instances with minimal change.
--
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] xiaoxiang781216 commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810169659
##########
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 will explain what I mean and also make some comments based on your answers.
>
> > TLS here mean Task local storage, collide with thread local storage.:(
>
> Let's use `task_ls_` and not `task_tls_` so the confusion will be removed. Do you agree?
>
> > From the concept, task local storage is very similar to thread local storage, but at the task level.
>
> Because I do not have visibility about conditions (when and how) of `task_tls_alloc` call, I can't get a full picture. Based on your example let's assume that it is a time when the library is instantiated for the first time. Then the allocated ID will be shared across the library instances while global data will be stored in Task LS. For example I have 2 different libraries that rely on top of Task LS. The library A attach `dtorA` at index 0 and library B attach `dtorB` at index 1. When one of tasks that use library A exits then `task_tls_destruct` is called and we will call `dtorB` from library A user context point of view. Yes, the `0` will be passed as a parameter, but I'm not sure if such situation is valid from isolation point of view.
Yes, it's possible. Another possibility is that the task use both libraries and then dtorA/dtorB get the different non-NULL pointer.
> Even more, if there is a but in library A code and it somehow at some point pass a wrong index to `task_tls_set_value` then `dtorB` will be called with non-zero argument and that value can match with the same value that is allocated by library B users. This potentially can lead to security issues.
>
Yes, the wrong usage will crash the system, but it doesn't increase the security risk, because the task local storage is used only in FLAT/PROTECTED build, not KERNEL build(The static/dynamic library get a new copy for each task in this case). Task in FLAT/PROTECTED mode already can access other tasks and do any evil thing.
> > > We can have 0 .. 64 as a limit for CONFIG_TLS_TASK_NELEM and use up to uint64_t for index storage. Or add save task pid together with each allocated index so that can be reused in task_tls_destruct to filter destructors to call
> >
> >
> > could you explanation more? The code is almost same as the thread local storage, just lift up one level.
>
> There are actually two issues here:
>
> 1. `CONFIG_TLS_TASK_NELEM` is not limited in Kconfig and there is next code:
>
> ```
> int task_tls_alloc(tls_dtor_t dtor)
> {
> int ret;
> int candidate;
> ...
> 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;
> }
> }
> ...
> return ret;
> }
> ```
>
> so if `CONFIG_TLS_TASK_NELEM` is more then number of bits that `int` can carry `if ((g_tlsset & mask) == 0)` will start be `true` always and out of boundary array access will occur. Hopefully this will be caught by compiler.
@no1wudi please add range 0 64 to Kconfig for both CONFIG_TLS_NELEM and CONFIG_TLS_TASK_NELEM.
> 2. Because of lack of use case description I was thinking that each library A user will call `task_tls_alloc()` at the start and will use that allocated index, but maybe it is not a use case (but that is a more like a traditional TLS).
Yes, you are right, each library need allocate the index when it need access the global variables at the first time.
> So I was thinking that we can store a bit mask to keep tracking of indexes allocated by a task. Again even now I'm not aware about this API usage point of view assuming that it can be called by any task in the system.
The API usage it's simple like the traditional TLS, @no1wudi will provide an usage in libuv like this:
```
static void uv__global_free(void* global) {
if (global) {
uv_library_shutdown();
uv__free(global);
}
}
uv__global_t* uv__global_get(void) {
static int index = -1;
uv__global_t* global = NULL;
if (index < 0) {
index = task_tls_alloc(uv__global_free);
}
if (index >= 0) {
global = (uv__global_t*)task_tls_get_value(index);
if (global == NULL) {
global = (uv__global_t*)uv__calloc(1, sizeof(uv__global_t));
if (global) {
global->once = UV_ONCE_INIT;
global->uv__signal_global_init_guard = UV_ONCE_INIT;
global->uv__signal_lock_pipefd[0] = -1;
global->uv__signal_lock_pipefd[1] = -1;
task_tls_set_value(index, (uintptr_t)global);
}
}
}
ASSERT(global != NULL);
return global;
}
```
Basically, libuv define several global variables, but we have many services call libuv functions in the background. The above code support the multiple libuv's instances with minimal change.
1. uv__global_get allocate index and uv_global_t at the first call of the first task
2. uv__global_get return the same global in the subsequent calls of the first task
3. uv__global_get allocate a new global when the second task call libuv's function at the fist time
4. uv__global_get return the second global in the subsequent calls of the second task
5. If task 3 never use libuv, the callback(uv__global_free) is still called that is why the NULL check exist in uv__global_free.
I hope this case help you understand the usage.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806786798
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
+ {
+ elem = info->ta_telem[candidate];
+ dtor = g_tlsdtor[candidate];
+ if (dtor)
Review comment:
```suggestion
if (dtor != NULL)
```
##########
File path: include/nuttx/tls.h
##########
@@ -267,6 +270,13 @@ uintptr_t tls_get_value(int tlsindex);
int tls_set_value(int tlsindex, uintptr_t tlsvalue);
#endif
+#if CONFIG_TLS_TASK_NELEM > 0
+int task_tls_alloc(tls_dtor_t dtor);
+void task_tls_destruct(void);
+int task_tls_set_value(int tlsindex, uintptr_t tlsvalue);
+uintptr_t task_tls_get_value(int tlsindex);
Review comment:
Do we need to add comment with AP description? Similar to APIs above
##########
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];
Review comment:
Why this is global and not located in `struct task_group_s`?
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
Review comment:
```suggestion
if ((g_tlsset & mask) != 0)
```
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806782575
##########
File path: libs/libc/tls/Kconfig
##########
@@ -54,4 +54,14 @@ config TLS_NELEM
NOTE that the special value of CONFIG_TLS_NELEM disables these
TLS interfaces.
+config TLS_TASK_NELEM
+ int "Number of Task Local Storage elements"
+ default 0
Review comment:
Sorry. Missed that!
--
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] no1wudi commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r806781547
##########
File path: libs/libc/tls/Kconfig
##########
@@ -54,4 +54,14 @@ config TLS_NELEM
NOTE that the special value of CONFIG_TLS_NELEM disables these
TLS interfaces.
+config TLS_TASK_NELEM
+ int "Number of Task Local Storage elements"
+ default 0
Review comment:
There is a macro guard `#if CONFIG_TLS_TASK_NELEM > 0` for 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.
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810950472
##########
File path: include/nuttx/tls.h
##########
@@ -67,8 +67,10 @@
*/
#if CONFIG_TLS_NELEM > 0
-# if CONFIG_TLS_NELEM > 32
+# if CONFIG_TLS_NELEM > 64
# error Too many TLS elements
+# elif CONFIG_TLS_NELEM > 32
+ typedef uint64_t tls_ndxset_t;
Review comment:
Either revert this or apply type cast for `1 << ...` case in TLS related files.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810853485
##########
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 tls_task_ndxset_t g_tlsset;
+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++)
+ {
+ tls_task_ndxset_t 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:
+ * None
+ *
+ ****************************************************************************/
+
+void task_tls_destruct(void)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ tls_task_ndxset_t mask = 1 << candidate;
Review comment:
I'm really not sure if this will work here and in pure `tls` case as well after you added 64 bit extension. We should go either with
```suggestion
tls_task_ndxset_t mask = 1llu << candidate;
```
or
```suggestion
tls_task_ndxset_t mask = (tls_task_ndxset_t)1 << candidate;
```
--
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] no1wudi commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r807604891
##########
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];
Review comment:
I'll update ostest for it later
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810680217
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
+ {
+ elem = info->ta_telem[candidate];
+ dtor = g_tlsdtor[candidate];
+ if (dtor)
Review comment:
Please apply changes back, but let's modify condition a bit:
```suggestion
if (dtor != NULL && elem != 0)
```
In this way we will filter desctructors of the tasks that didn't call `task_tls_set_value`. Similar to `pthread_key` -> `tls_destruct`.
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810680217
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
+ {
+ elem = info->ta_telem[candidate];
+ dtor = g_tlsdtor[candidate];
+ if (dtor)
Review comment:
Please apply changes back, but let's modify condition a bit:
```suggestion
if (dtor != NULL && elem != 0)
```
--
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] pkarashchenko commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810680197
##########
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)
+{
+ int candidate;
+ uintptr_t elem;
+ tls_dtor_t dtor;
+ FAR struct task_info_s *info = task_get_info();
+
+ for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++)
+ {
+ int mask = 1 << candidate;
+ if (g_tlsset & mask)
Review comment:
Please apply changes back
--
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] no1wudi commented on a change in pull request #5498: sched: Implement task local storage
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5498:
URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810847416
##########
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 tls_ndxset_t g_tlsset = 0;
Review comment:
Thanks
--
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