This is an automated email from the ASF dual-hosted git repository.
acassis pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
The following commit(s) were added to refs/heads/master by this push:
new 31e562b10fe Revert "sched/group: move task group into task_tcb_s to
improve performance"
31e562b10fe is described below
commit 31e562b10fe3425a07d760a8f806c68c3efa280c
Author: wangzhi16 <[email protected]>
AuthorDate: Thu Dec 18 19:51:21 2025 +0800
Revert "sched/group: move task group into task_tcb_s to improve performance"
This reverts commit 29e50ffa7374fa4c473d8d4f8cb3506665443d3e.
reason:
Placing the main thread and the gourd in the same memory block, and
allocating and freeing memory simultaneously, presents the following two
problems：
When the main thread creates a child thread and performs a detach
operation, there is a possibility that the main thread may have exited, but the
main thread's TCB (Transaction Control Block) may not have been released.
This could potentially cause the main thread's TCB to be double-freed. The
core contradiction in this problem lies in binding the main thread's TCB (Trust
Container Registry) and the group together. When releasing the main thread's
TCB, an additional check is needed to ensure the main thread was the last to
leave the group. If this check and the free operation are atomically
guaranteed, the logic is sound, and double freeing won't occur. However, this
atomicity cannot be completely gu [...]
Signed-off-by: wangzhi16 <[email protected]>
---
include/nuttx/mm/map.h | 2 +-
include/nuttx/sched.h | 4 ----
sched/group/group.h | 4 ++--
sched/group/group_create.c | 30 ++++++++++++++++++-----------
sched/group/group_foreachchild.c | 2 +-
sched/group/group_leave.c | 41 ++++++++++++++--------------------------
sched/init/nx_start.c | 4 ++--
sched/pthread/pthread_create.c | 3 ++-
sched/sched/sched_releasetcb.c | 22 ---------------------
sched/task/task_fork.c | 4 ++--
sched/task/task_init.c | 4 ++--
11 files changed, 45 insertions(+), 75 deletions(-)
diff --git a/include/nuttx/mm/map.h b/include/nuttx/mm/map.h
index 22df037e72c..eff3bac8254 100644
--- a/include/nuttx/mm/map.h
+++ b/include/nuttx/mm/map.h
@@ -129,7 +129,7 @@ void mm_map_unlock(void);
* Name: mm_map_initialize
*
* Description:
- * Initialization function, called only by group_postinitialize
+ * Initialization function, called only by group_initialize
*
* Input Parameters:
* mm - Pointer to the mm_map structure to be initialized
diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h
index a565bd373d3..1059bb9f004 100644
--- a/include/nuttx/sched.h
+++ b/include/nuttx/sched.h
@@ -746,10 +746,6 @@ struct task_tcb_s
/* Common TCB fields ******************************************************/
struct tcb_s cmn; /* Common TCB fields */
-
- /* Task Group *************************************************************/
-
- struct task_group_s group; /* Shared task group data */
};
/* struct pthread_tcb_s *****************************************************/
diff --git a/sched/group/group.h b/sched/group/group.h
index 69831ead8ab..a8c4f908c2c 100644
--- a/sched/group/group.h
+++ b/sched/group/group.h
@@ -58,8 +58,8 @@ void task_initialize(void);
/* Task group data structure management */
-int group_initialize(FAR struct task_tcb_s *tcb, uint8_t ttype);
-void group_postinitialize(FAR struct task_tcb_s *tcb);
+int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype);
+void group_initialize(FAR struct task_tcb_s *tcb);
#ifndef CONFIG_DISABLE_PTHREAD
void group_bind(FAR struct pthread_tcb_s *tcb);
void group_join(FAR struct pthread_tcb_s *tcb);
diff --git a/sched/group/group_create.c b/sched/group/group_create.c
index 49a797d1f3c..1c5f5f81e41 100644
--- a/sched/group/group_create.c
+++ b/sched/group/group_create.c
@@ -92,7 +92,7 @@ static inline void group_inherit_identity(FAR struct
task_group_s *group)
****************************************************************************/
/****************************************************************************
- * Name: group_initialize
+ * Name: group_allocate
*
* Description:
* Create and a new task group structure for the specified TCB. This
@@ -100,8 +100,8 @@ static inline void group_inherit_identity(FAR struct
task_group_s *group)
* allocated and zeroed, but otherwise uninitialized. The full creation
* of the group of a two step process: (1) First, this function allocates
* group structure early in the task creation sequence in order to provide
- * a group container, then (2) group_postinitialize() is called to set up
- * the group membership.
+ * a group container, then (2) group_initialize() is called to set up the
+ * group membership.
*
* Input Parameters:
* tcb - The tcb in need of the task group.
@@ -116,7 +116,7 @@ static inline void group_inherit_identity(FAR struct
task_group_s *group)
*
****************************************************************************/
-int group_initialize(FAR struct task_tcb_s *tcb, uint8_t ttype)
+int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype)
{
FAR struct task_group_s *group;
int ret;
@@ -138,7 +138,12 @@ int group_initialize(FAR struct task_tcb_s *tcb, uint8_t
ttype)
}
else
{
- group = &tcb->group;
+ group = kmm_zalloc(sizeof(struct task_group_s));
+ }
+
+ if (!group)
+ {
+ return -ENOMEM;
}
#if defined(CONFIG_MM_KERNEL_HEAP)
@@ -175,7 +180,7 @@ int group_initialize(FAR struct task_tcb_s *tcb, uint8_t
ttype)
ret = task_init_info(group);
if (ret < 0)
{
- return ret;
+ goto errout_with_group;
}
nxrmutex_init(&group->tg_mutex);
@@ -193,17 +198,20 @@ int group_initialize(FAR struct task_tcb_s *tcb, uint8_t
ttype)
#endif
return OK;
+
+errout_with_group:
+ kmm_free(group);
+ return ret;
}
/****************************************************************************
- * Name: group_postinitialize
+ * Name: group_initialize
*
* Description:
* Add the task as the initial member of the group. The full creation of
* the group of a two step process: (1) First, this group structure is
- * allocated by group_initialize() early in the task creation sequence,
- * then (2) this function is called to set up the initial group
- * membership.
+ * allocated by group_allocate() early in the task creation sequence, then
+ * (2) this function is called to set up the initial group membership.
*
* Input Parameters:
* tcb - The tcb in need of the task group.
@@ -217,7 +225,7 @@ int group_initialize(FAR struct task_tcb_s *tcb, uint8_t
ttype)
*
****************************************************************************/
-void group_postinitialize(FAR struct task_tcb_s *tcb)
+void group_initialize(FAR struct task_tcb_s *tcb)
{
FAR struct task_group_s *group;
diff --git a/sched/group/group_foreachchild.c b/sched/group/group_foreachchild.c
index a0cf1f25458..8b830837833 100644
--- a/sched/group/group_foreachchild.c
+++ b/sched/group/group_foreachchild.c
@@ -64,7 +64,7 @@ int group_foreachchild(FAR struct task_group_s *group,
{
FAR sq_entry_t *curr;
FAR sq_entry_t *next;
- int ret = OK;
+ int ret;
DEBUGASSERT(group);
diff --git a/sched/group/group_leave.c b/sched/group/group_leave.c
index 470a3c49fb0..5e69dc2d349 100644
--- a/sched/group/group_leave.c
+++ b/sched/group/group_leave.c
@@ -31,7 +31,6 @@
#include <errno.h>
#include <debug.h>
-#include <nuttx/nuttx.h>
#include <nuttx/irq.h>
#include <nuttx/fs/fs.h>
#include <nuttx/net/net.h>
@@ -71,8 +70,7 @@
*
****************************************************************************/
-static inline void
-group_release(FAR struct task_group_s *group, uint8_t ttype)
+static inline void group_release(FAR struct task_group_s *group)
{
/* Destroy the mutex */
@@ -125,18 +123,13 @@ group_release(FAR struct task_group_s *group, uint8_t
ttype)
}
#endif
- /* Then drop the group freeing the allocated memory */
+ /* Mark the group as deleted now */
-#ifndef CONFIG_DISABLE_PTHREAD
- if (ttype == TCB_FLAG_TTYPE_PTHREAD)
- {
- /* Mark the group as deleted now */
+ group->tg_flags |= GROUP_FLAG_DELETED;
- group->tg_flags |= GROUP_FLAG_DELETED;
+ /* Then drop the group freeing the allocated memory */
- group_drop(group);
- }
-#endif
+ group_drop(group);
}
/****************************************************************************
@@ -178,12 +171,6 @@ void group_leave(FAR struct tcb_s *tcb)
group = tcb->group;
if (group)
{
- /* In any event, we can detach the group from the TCB so that we won't
- * do this again.
- */
-
- tcb->group = NULL;
-
/* Remove the member from group. */
#ifdef HAVE_GROUP_MEMBERS
@@ -198,8 +185,14 @@ void group_leave(FAR struct tcb_s *tcb)
{
/* Yes.. Release all of the resource held by the task group */
- group_release(group, tcb->flags & TCB_FLAG_TTYPE_MASK);
+ group_release(group);
}
+
+ /* In any event, we can detach the group from the TCB so that we won't
+ * do this again.
+ */
+
+ tcb->group = NULL;
}
}
@@ -226,8 +219,6 @@ void group_leave(FAR struct tcb_s *tcb)
void group_drop(FAR struct task_group_s *group)
{
- FAR struct task_tcb_s *tcb;
-
#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT)
/* If there are threads waiting for this group to be freed, then we cannot
* yet free the memory resources. Instead just mark the group deleted
@@ -242,17 +233,13 @@ void group_drop(FAR struct task_group_s *group)
}
else
#endif
+
/* Finally, if no one needs the group and it has been deleted, remove it */
if (group->tg_flags & GROUP_FLAG_DELETED)
{
- tcb = container_of(group, struct task_tcb_s, group);
-
/* Release the group container itself */
- if (tcb->cmn.flags & TCB_FLAG_FREE_TCB)
- {
- kmm_free(tcb);
- }
+ kmm_free(group);
}
}
diff --git a/sched/init/nx_start.c b/sched/init/nx_start.c
index 3f4a4e1820c..79f57e4029e 100644
--- a/sched/init/nx_start.c
+++ b/sched/init/nx_start.c
@@ -445,7 +445,7 @@ static void idle_group_initialize(void)
/* Allocate the IDLE group */
DEBUGVERIFY(
- group_initialize((FAR struct task_tcb_s *)tcb, tcb->flags));
+ group_allocate((FAR struct task_tcb_s *)tcb, tcb->flags));
/* Initialize the task join */
@@ -478,7 +478,7 @@ static void idle_group_initialize(void)
* of child status in the IDLE group.
*/
- group_postinitialize((FAR struct task_tcb_s *)tcb);
+ group_initialize((FAR struct task_tcb_s *)tcb);
tcb->group->tg_flags = GROUP_FLAG_NOCLDWAIT | GROUP_FLAG_PRIVILEGED;
}
}
diff --git a/sched/pthread/pthread_create.c b/sched/pthread/pthread_create.c
index 108e1651028..023e15eeea0 100644
--- a/sched/pthread/pthread_create.c
+++ b/sched/pthread/pthread_create.c
@@ -209,7 +209,8 @@ int nx_pthread_create(pthread_trampoline_t trampoline, FAR
pthread_t *thread,
/* Allocate a TCB for the new task. */
- ptcb = kmm_zalloc(sizeof(struct pthread_tcb_s));
+ ptcb = (FAR struct pthread_tcb_s *)
+ kmm_zalloc(sizeof(struct pthread_tcb_s));
if (!ptcb)
{
serr("ERROR: Failed to allocate TCB\n");
diff --git a/sched/sched/sched_releasetcb.c b/sched/sched/sched_releasetcb.c
index 52ce5f3159e..96b3e736e79 100644
--- a/sched/sched/sched_releasetcb.c
+++ b/sched/sched/sched_releasetcb.c
@@ -100,9 +100,6 @@ static void nxsched_releasepid(pid_t pid)
int nxsched_release_tcb(FAR struct tcb_s *tcb, uint8_t ttype)
{
-#ifndef CONFIG_DISABLE_PTHREAD
- FAR struct task_tcb_s *ttcb;
-#endif
int ret = OK;
if (tcb)
@@ -175,25 +172,6 @@ int nxsched_release_tcb(FAR struct tcb_s *tcb, uint8_t
ttype)
/* Destroy the pthread join mutex */
nxtask_joindestroy(tcb);
-
- /* Task still referenced by pthread */
-
- if (ttype == TCB_FLAG_TTYPE_TASK)
- {
- ttcb = (FAR struct task_tcb_s *)tcb;
- if (!sq_empty(&ttcb->group.tg_members)
-#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT)
- || ttcb->group.tg_nwaiters > 0
-#endif
- )
- {
- /* Mark the group as deleted now */
-
- ttcb->group.tg_flags |= GROUP_FLAG_DELETED;
-
- return ret;
- }
- }
#endif
/* And, finally, release the TCB itself */
diff --git a/sched/task/task_fork.c b/sched/task/task_fork.c
index 0176e57dfb5..a9b7f071b84 100644
--- a/sched/task/task_fork.c
+++ b/sched/task/task_fork.c
@@ -170,7 +170,7 @@ FAR struct task_tcb_s *nxtask_setup_fork(start_t retaddr)
/* Allocate a new task group with the same privileges as the parent */
- ret = group_initialize(child, ttype);
+ ret = group_allocate(child, ttype);
if (ret < 0)
{
goto errout_with_tcb;
@@ -257,7 +257,7 @@ FAR struct task_tcb_s *nxtask_setup_fork(start_t retaddr)
/* Now we have enough in place that we can join the group */
- group_postinitialize(child);
+ group_initialize(child);
sinfo("parent=%p, returning child=%p\n", parent, child);
return child;
diff --git a/sched/task/task_init.c b/sched/task/task_init.c
index a2f49431390..a3f8d9c78ab 100644
--- a/sched/task/task_init.c
+++ b/sched/task/task_init.c
@@ -113,7 +113,7 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char
*name, int priority,
/* Create a new task group */
- ret = group_initialize(tcb, tcb->cmn.flags);
+ ret = group_allocate(tcb, tcb->cmn.flags);
if (ret < 0)
{
sched_trace_end();
@@ -195,7 +195,7 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char
*name, int priority,
/* Now we have enough in place that we can join the group */
- group_postinitialize(tcb);
+ group_initialize(tcb);
sched_trace_end();
return ret;
