Revert "sched/group: move task group into task_tcb_s to improve performance"

This reverts commit 29e50ff.

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 guaranteed. If other free operations cause a block, problems still arise.

Signed-off-by: wangzhi16 <[email protected]>

Author: wangzhi16

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

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

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

sched/group/group_create.c

@@ -92,16 +92,16 @@ 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
  *   function is called as part of the task creation sequence.  The structure
  *   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;
 

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

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);
     }
 }

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

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");

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

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;