Skip to content

sched/group: move task group into task_tcb_s to improve performance #11832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acassis merged 1 commit into from
Mar 10, 2024
Merged

sched/group: move task group into task_tcb_s to improve performance #11832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/nuttx/mm/map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ void mm_map_unlock(void);
* Name: mm_map_initialize
*
* Description:
* Initialization function, called only by group_initialize
* Initialization function, called only by group_postinitialize
*
* Input Parameters:
* mm - Pointer to the mm_map structure to be initialized
Expand Down
4 changes: 4 additions & 0 deletions include/nuttx/sched.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,6 +680,10 @@ struct task_tcb_s

struct tcb_s cmn; /* Common TCB fields */

/* Task Group *************************************************************/

struct task_group_s group; /* Pointer to shared task group data */

@xiaoxiang781216 xiaoxiang781216 Mar 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but, main thread may exist before other thread, the used-after-free will happen.

@anchao anchao Mar 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task will kill all pthreads in the group before exiting. Will the task exit early than pthread?

#0  nxsched_release_tcb (tcb=0x409f04 <nxtask_exithook+96>, ttype=255 '\377') at sched/sched_releasetcb.c:99
#1  0x000000000040a0e5 in nxtask_terminate (pid=5) at task/task_terminate.c:122
#2  0x00000000004087c7 in pthread_cancel (thread=5) at pthread/pthread_cancel.c:110
#3  0x000000000040867b in group_cancel_children_handler (pid=5, arg=0x4) at group/group_killchildren.c:113
#4  0x000000000040a342 in group_foreachchild (group=0x7ffff3db53e0, handler=0x408613 <group_cancel_children_handler>, arg=0x4) at group/group_foreachchild.c:70
#5  0x000000000040870e in group_kill_children (tcb=0x7ffff3db52a0) at group/group_killchildren.c:212
#6  0x0000000000407adb in _exit (status=0) at task/exit.c:67
#7  0x0000000000404222 in exit (status=0) at stdlib/lib_exit.c:126
#8  0x000000000040dffe in nxtask_startup (entrypt=0x444546 <hello_main>, argc=1, argv=0x7ffff3db58c0) at sched/task_startup.c:70
#9  0x00000000004076b4 in nxtask_start () at task/task_start.c:134
#10 0x0000000000413081 in pre_start () at sim/sim_initialstate.c:52

@xiaoxiang781216 xiaoxiang781216 Mar 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exit terminate the whole proccess, but pthread_exit terminate the current thread. You can reproduce the case by:

  1. Create a work thread and call printf in a loop
  2. Exit the main thread by pthread_exit

@anchao anchao Mar 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got, I have a update which delayed the release of the parent tcb, the parent tcb will be released after all child threads exit.


/* Task Management Fields *************************************************/

#ifdef CONFIG_SCHED_STARTHOOK
Expand Down
4 changes: 2 additions & 2 deletions sched/group/group.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ void task_initialize(void);

/* Task group data structure management */

int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype);
void group_initialize(FAR struct task_tcb_s *tcb);
int group_initialize(FAR struct task_tcb_s *tcb, uint8_t ttype);
void group_postinitialize(FAR struct task_tcb_s *tcb);
#ifndef CONFIG_DISABLE_PTHREAD
int group_bind(FAR struct pthread_tcb_s *tcb);
int group_join(FAR struct pthread_tcb_s *tcb);
Expand Down
29 changes: 11 additions & 18 deletions sched/group/group_create.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,16 @@ static inline void group_inherit_identity(FAR struct task_group_s *group)
****************************************************************************/

/****************************************************************************
* Name: group_allocate
* Name: group_initialize
*
* 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_initialize() is called to set up the
* group membership.
* a group container, then (2) group_postinitialize() is called to set up
* the group membership.
*
* Input Parameters:
* tcb - The tcb in need of the task group.
Expand All @@ -112,7 +112,7 @@ static inline void group_inherit_identity(FAR struct task_group_s *group)
*
****************************************************************************/

int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype)
int group_initialize(FAR struct task_tcb_s *tcb, uint8_t ttype)
{
FAR struct task_group_s *group;
int ret;
Expand All @@ -121,11 +121,7 @@ int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype)

/* Allocate the group structure and assign it to the TCB */

group = kmm_zalloc(sizeof(struct task_group_s));
if (!group)
{
return -ENOMEM;
}
group = &tcb->group;

#if defined(CONFIG_MM_KERNEL_HEAP)
/* If this group is being created for a privileged thread, then all
Expand Down Expand Up @@ -161,7 +157,7 @@ int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype)
ret = task_init_info(group);
if (ret < 0)
{
goto errout_with_group;
return ret;
}

#ifndef CONFIG_DISABLE_PTHREAD
Expand All @@ -177,20 +173,17 @@ int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype)
#endif

return OK;

errout_with_group:
kmm_free(group);
return ret;
}

/****************************************************************************
* Name: group_initialize
* Name: group_postinitialize
*
* 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_allocate() early in the task creation sequence, then
* (2) this function is called to set up the initial group membership.
* allocated by group_initialize() 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.
Expand All @@ -204,7 +197,7 @@ int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype)
*
****************************************************************************/

void group_initialize(FAR struct task_tcb_s *tcb)
void group_postinitialize(FAR struct task_tcb_s *tcb)
{
FAR struct task_group_s *group;

Expand Down
2 changes: 1 addition & 1 deletion sched/group/group_foreachchild.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ int group_foreachchild(FAR struct task_group_s *group,
{
FAR sq_entry_t *curr;
FAR sq_entry_t *next;
int ret;
int ret = OK;

DEBUGASSERT(group);

Expand Down
35 changes: 24 additions & 11 deletions sched/group/group_leave.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <errno.h>
#include <debug.h>

#include <nuttx/nuttx.h>
#include <nuttx/irq.h>
#include <nuttx/fs/fs.h>
#include <nuttx/net/net.h>
Expand Down Expand Up @@ -67,7 +68,8 @@
*
****************************************************************************/

static inline void group_release(FAR struct task_group_s *group)
static inline void
group_release(FAR struct task_group_s *group, uint8_t ttype)
{
task_uninit_info(group);

Expand Down Expand Up @@ -124,7 +126,12 @@ static inline void group_release(FAR struct task_group_s *group)

/* Then drop the group freeing the allocated memory */

group_drop(group);
#ifndef CONFIG_DISABLE_PTHREAD
if (ttype == TCB_FLAG_TTYPE_PTHREAD)
{
group_drop(group);
}
#endif
}

/****************************************************************************
Expand Down Expand Up @@ -166,6 +173,12 @@ 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
Expand All @@ -180,14 +193,8 @@ void group_leave(FAR struct tcb_s *tcb)
{
/* Yes.. Release all of the resource held by the task group */

group_release(group);
group_release(group, tcb->flags & TCB_FLAG_TTYPE_MASK);
}

/* In any event, we can detach the group from the TCB so that we won't
* do this again.
*/

tcb->group = NULL;
}
}

Expand All @@ -214,6 +221,8 @@ 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
Expand All @@ -228,13 +237,17 @@ 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 */

kmm_free(group);
if (tcb->cmn.flags & TCB_FLAG_FREE_TCB)
{
kmm_free(tcb);
}
}
}
4 changes: 2 additions & 2 deletions sched/init/nx_start.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ void nx_start(void)

/* Allocate the IDLE group */

DEBUGVERIFY(group_allocate(&g_idletcb[i], g_idletcb[i].cmn.flags));
DEBUGVERIFY(group_initialize(&g_idletcb[i], g_idletcb[i].cmn.flags));
g_idletcb[i].cmn.group->tg_info->ta_argv = &g_idleargv[i][0];

#ifdef CONFIG_SMP
Expand All @@ -543,7 +543,7 @@ void nx_start(void)
* of child status in the IDLE group.
*/

group_initialize(&g_idletcb[i]);
group_postinitialize(&g_idletcb[i]);
g_idletcb[i].cmn.group->tg_flags = GROUP_FLAG_NOCLDWAIT |
GROUP_FLAG_PRIVILEGED;
}
Expand Down
3 changes: 1 addition & 2 deletions sched/pthread/pthread_create.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,7 @@ int nx_pthread_create(pthread_trampoline_t trampoline, FAR pthread_t *thread,

/* Allocate a TCB for the new task. */

ptcb = (FAR struct pthread_tcb_s *)
kmm_zalloc(sizeof(struct pthread_tcb_s));
ptcb = kmm_zalloc(sizeof(struct pthread_tcb_s));
if (!ptcb)
{
serr("ERROR: Failed to allocate TCB\n");
Expand Down
20 changes: 20 additions & 0 deletions sched/sched/sched_releasetcb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ 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)
Expand Down Expand Up @@ -161,6 +164,23 @@ int nxsched_release_tcb(FAR struct tcb_s *tcb, uint8_t ttype)

group_leave(tcb);

/* Kernel thread and group still reference by pthread */

#ifndef CONFIG_DISABLE_PTHREAD
if (ttype != TCB_FLAG_TTYPE_PTHREAD)
Comment thread
xiaoxiang781216 marked this conversation as resolved.
{
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
)
{
return ret;
}
}
#endif

/* And, finally, release the TCB itself */

if (tcb->flags & TCB_FLAG_FREE_TCB)
Expand Down
6 changes: 4 additions & 2 deletions sched/task/task_fork.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,11 @@ FAR struct task_tcb_s *nxtask_setup_fork(start_t retaddr)
goto errout;
}

child->cmn.flags |= TCB_FLAG_FREE_TCB;

/* Allocate a new task group with the same privileges as the parent */

ret = group_allocate(child, ttype);
ret = group_initialize(child, ttype);
if (ret < 0)
{
goto errout_with_tcb;
Expand Down Expand Up @@ -214,7 +216,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_initialize(child);
group_postinitialize(child);
sinfo("parent=%p, returning child=%p\n", parent, child);
return child;

Expand Down
4 changes: 2 additions & 2 deletions sched/task/task_init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority,

/* Create a new task group */

ret = group_allocate(tcb, tcb->cmn.flags);
ret = group_initialize(tcb, tcb->cmn.flags);
if (ret < 0)
{
sched_trace_end();
Expand Down Expand Up @@ -179,7 +179,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_initialize(tcb);
group_postinitialize(tcb);
sched_trace_end();
return ret;

Expand Down