This is an automated email from the ASF dual-hosted git repository. masayuki pushed a commit to branch revert-11848-24030602 in repository https://gitbox.apache.org/repos/asf/nuttx.git
commit 09be02816a43613f1fd0b1f8e7670aa77ee16bfd Author: Masayuki Ishikawa <[email protected]> AuthorDate: Thu Mar 7 15:48:00 2024 +0900 Revert "sched/group: change type of task group member to single queue" This reverts commit ec08031e4bf01f0abc9e590484382c166d90915e. --- binfmt/binfmt_execmodule.c | 2 +- fs/procfs/fs_procfsproc.c | 16 ++--- include/nuttx/sched.h | 10 +-- sched/clock/clock_gettime.c | 30 ++++----- sched/group/group_create.c | 33 ++++++++-- sched/group/group_foreachchild.c | 18 ++---- sched/group/group_join.c | 127 +++++++++++++++++++++++++++++++++++++-- sched/group/group_killchildren.c | 2 +- sched/group/group_leave.c | 121 ++++++++++++++++++++++++++++++++++--- sched/task/task_exithook.c | 6 +- 10 files changed, 293 insertions(+), 72 deletions(-) diff --git a/binfmt/binfmt_execmodule.c b/binfmt/binfmt_execmodule.c index 0f5fffc0a6..85978b411b 100644 --- a/binfmt/binfmt_execmodule.c +++ b/binfmt/binfmt_execmodule.c @@ -121,7 +121,7 @@ static void exec_swap(FAR struct tcb_s *ptcb, FAR struct tcb_s *chtcb) pid_t pid; irqstate_t flags; #ifdef HAVE_GROUP_MEMBERS - sq_queue_t tg_members; + FAR pid_t *tg_members; #endif #ifdef CONFIG_SCHED_HAVE_PARENT # ifdef CONFIG_SCHED_CHILD_STATUS diff --git a/fs/procfs/fs_procfsproc.c b/fs/procfs/fs_procfsproc.c index 4f5911e130..c6b7145c0f 100644 --- a/fs/procfs/fs_procfsproc.c +++ b/fs/procfs/fs_procfsproc.c @@ -43,7 +43,6 @@ # include <time.h> #endif -#include <nuttx/nuttx.h> #include <nuttx/irq.h> #include <nuttx/tls.h> #include <nuttx/sched.h> @@ -54,7 +53,6 @@ #include <nuttx/fs/procfs.h> #include <nuttx/fs/ioctl.h> #include <nuttx/mm/mm.h> -#include <nuttx/queue.h> #if !defined(CONFIG_SCHED_CPULOAD_NONE) || defined(CONFIG_SCHED_CRITMONITOR) # include <nuttx/clock.h> @@ -1124,8 +1122,7 @@ static ssize_t proc_groupstatus(FAR struct proc_file_s *procfile, size_t copysize; size_t totalsize; #ifdef HAVE_GROUP_MEMBERS - FAR sq_entry_t *curr; - FAR sq_entry_t *next; + int i; #endif DEBUGASSERT(group != NULL); @@ -1171,14 +1168,13 @@ static ssize_t proc_groupstatus(FAR struct proc_file_s *procfile, buffer += copysize; remaining -= copysize; -#ifdef HAVE_GROUP_MEMBERS if (totalsize >= buflen) { return totalsize; } - linesize = procfs_snprintf(procfile->line, STATUS_LINELEN, "%-12s%zu\n", - "Members:", sq_count(&group->tg_members)); + linesize = procfs_snprintf(procfile->line, STATUS_LINELEN, "%-12s%d\n", + "Members:", group->tg_nmembers); copysize = procfs_memcpy(procfile->line, linesize, buffer, remaining, &offset); @@ -1186,6 +1182,7 @@ static ssize_t proc_groupstatus(FAR struct proc_file_s *procfile, buffer += copysize; remaining -= copysize; +#ifdef HAVE_GROUP_MEMBERS if (totalsize >= buflen) { return totalsize; @@ -1205,11 +1202,10 @@ static ssize_t proc_groupstatus(FAR struct proc_file_s *procfile, return totalsize; } - sq_for_every_safe(&group->tg_members, curr, next) + for (i = 0; i < group->tg_nmembers; i++) { - tcb = container_of(curr, struct tcb_s, member); linesize = procfs_snprintf(procfile->line, STATUS_LINELEN, " %d", - tcb->pid); + group->tg_members[i]); copysize = procfs_memcpy(procfile->line, linesize, buffer, remaining, &offset); diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index 879619874d..cfc0eb9c8b 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -429,8 +429,10 @@ struct task_group_s /* Group membership *******************************************************/ + uint8_t tg_nmembers; /* Number of members in the group */ #ifdef HAVE_GROUP_MEMBERS - sq_queue_t tg_members; /* List of members for task */ + uint8_t tg_mxmembers; /* Number of members in allocation */ + FAR pid_t *tg_members; /* Members of the group */ #endif #ifdef CONFIG_BINFMT_LOADABLE @@ -536,12 +538,6 @@ struct tcb_s FAR struct task_group_s *group; /* Pointer to shared task group data */ - /* Group membership *******************************************************/ - -#ifdef HAVE_GROUP_MEMBERS - sq_entry_t member; /* List entry of task member */ -#endif - /* Address Environment ****************************************************/ #ifdef CONFIG_ARCH_ADDRENV diff --git a/sched/clock/clock_gettime.c b/sched/clock/clock_gettime.c index 808ca7aefa..0dffa8ae13 100644 --- a/sched/clock/clock_gettime.c +++ b/sched/clock/clock_gettime.c @@ -30,11 +30,9 @@ #include <errno.h> #include <debug.h> -#include <nuttx/nuttx.h> #include <nuttx/arch.h> #include <nuttx/sched.h> #include <nuttx/spinlock.h> -#include <nuttx/queue.h> #include "clock/clock.h" #ifdef CONFIG_CLOCK_TIMEKEEPING @@ -166,14 +164,11 @@ int clock_gettime(clockid_t clock_id, struct timespec *tp) } else if (clock_type == CLOCK_PROCESS_CPUTIME_ID) { - unsigned long runtime; - FAR struct tcb_s *tcb; -# ifdef HAVE_GROUP_MEMBERS FAR struct task_group_s *group; - FAR sq_entry_t *curr; - FAR sq_entry_t *next; + unsigned long runtime; irqstate_t flags; -# endif + int i; + FAR struct tcb_s *tcb; if (pid == 0) { @@ -188,23 +183,20 @@ int clock_gettime(clockid_t clock_id, struct timespec *tp) if (tcb != NULL) { -# ifdef HAVE_GROUP_MEMBERS group = tcb->group; runtime = 0; - flags = spin_lock_irqsave(NULL); - sq_for_every_safe(&group->tg_members, curr, next) + flags = enter_critical_section(); + for (i = group->tg_nmembers - 1; i >= 0; i--) { - tcb = container_of(curr, struct tcb_s, member); - - runtime += tcb->run_time; + tcb = nxsched_get_tcb(group->tg_members[i]); + if (tcb != NULL) + { + runtime += tcb->run_time; + } } - spin_unlock_irqrestore(NULL, flags); -# else /* HAVE_GROUP_MEMBERS */ - runtime = tcb->run_time; -# endif /* HAVE_GROUP_MEMBERS */ - + leave_critical_section(flags); perf_convert(runtime, tp); } else diff --git a/sched/group/group_create.c b/sched/group/group_create.c index 01262d64bf..c14660f6c7 100644 --- a/sched/group/group_create.c +++ b/sched/group/group_create.c @@ -39,6 +39,14 @@ #include "group/group.h" #include "tls/tls.h" +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* Is this worth making a configuration option? */ + +#define GROUP_INITIAL_MEMBERS 4 + /**************************************************************************** * Public Data ****************************************************************************/ @@ -139,9 +147,18 @@ int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype) #endif /* defined(CONFIG_MM_KERNEL_HEAP) */ #ifdef HAVE_GROUP_MEMBERS - /* Initialize member list of the group */ + /* Allocate space to hold GROUP_INITIAL_MEMBERS members of the group */ - sq_init(&group->tg_members); + group->tg_members = kmm_malloc(GROUP_INITIAL_MEMBERS * sizeof(pid_t)); + if (!group->tg_members) + { + ret = -ENOMEM; + goto errout_with_group; + } + + /* Number of members in allocation */ + + group->tg_mxmembers = GROUP_INITIAL_MEMBERS; #endif /* Attach the group to the TCB */ @@ -161,7 +178,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; + goto errout_with_member; } #ifndef CONFIG_DISABLE_PTHREAD @@ -178,7 +195,11 @@ int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype) return OK; +errout_with_member: +#ifdef HAVE_GROUP_MEMBERS + kmm_free(group->tg_members); errout_with_group: +#endif kmm_free(group); return ret; } @@ -219,7 +240,7 @@ void group_initialize(FAR struct task_tcb_s *tcb) #ifdef HAVE_GROUP_MEMBERS /* Assign the PID of this new task as a member of the group. */ - sq_addlast(&tcb->cmn.member, &group->tg_members); + group->tg_members[0] = tcb->cmn.pid; #endif /* Save the ID of the main task within the group of threads. This needed @@ -229,4 +250,8 @@ void group_initialize(FAR struct task_tcb_s *tcb) */ group->tg_pid = tcb->cmn.pid; + + /* Mark that there is one member in the group, the main task */ + + group->tg_nmembers = 1; } diff --git a/sched/group/group_foreachchild.c b/sched/group/group_foreachchild.c index 827b53a651..81761ba6e8 100644 --- a/sched/group/group_foreachchild.c +++ b/sched/group/group_foreachchild.c @@ -25,9 +25,7 @@ #include <nuttx/config.h> #include <assert.h> -#include <nuttx/nuttx.h> #include <nuttx/sched.h> -#include <nuttx/queue.h> #include "group/group.h" @@ -60,27 +58,23 @@ int group_foreachchild(FAR struct task_group_s *group, foreachchild_t handler, FAR void *arg) { - FAR sq_entry_t *curr; - FAR sq_entry_t *next; int ret; + int i; DEBUGASSERT(group); /* Visit the main thread last (if present) */ - sq_for_every_safe(&group->tg_members, curr, next) + for (i = group->tg_nmembers - 1; i >= 0; i--) { - FAR struct tcb_s *mtcb = - container_of(curr, struct tcb_s, member); - - ret = handler(mtcb->pid, arg); - if (ret != OK) + ret = handler(group->tg_members[i], arg); + if (ret != 0) { - break; + return ret; } } - return ret; + return 0; } #endif /* HAVE_GROUP_MEMBERS */ diff --git a/sched/group/group_join.c b/sched/group/group_join.c index 48befc1d3b..42d82310f1 100644 --- a/sched/group/group_join.c +++ b/sched/group/group_join.c @@ -38,6 +38,102 @@ #ifndef CONFIG_DISABLE_PTHREAD +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* Is this worth making a configuration option? */ + +#define GROUP_REALLOC_MEMBERS 4 + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: group_addmember + * + * Description: + * Add a new member to a group. + * + * Input Parameters: + * group - The task group to add the new member + * pid - The new member + * + * Returned Value: + * 0 (OK) on success; a negated errno value on failure. + * + * Assumptions: + * Called during thread creation and during reparenting in a safe context. + * No special precautions are required here. + * + ****************************************************************************/ + +#ifdef HAVE_GROUP_MEMBERS +static inline int group_addmember(FAR struct task_group_s *group, pid_t pid) +{ + FAR pid_t *oldmembers = NULL; + irqstate_t flags; + + DEBUGASSERT(group && group->tg_nmembers < UINT8_MAX); + + /* Will we need to extend the size of the array of groups? */ + + if (group->tg_nmembers >= group->tg_mxmembers) + { + FAR pid_t *newmembers; + unsigned int newmax; + + /* Yes... reallocate the array of members */ + + newmax = group->tg_mxmembers + GROUP_REALLOC_MEMBERS; + if (newmax > UINT8_MAX) + { + newmax = UINT8_MAX; + } + + newmembers = kmm_malloc(sizeof(pid_t) * newmax); + + if (!newmembers) + { + serr("ERROR: Failed to reallocate tg_members\n"); + return -ENOMEM; + } + + /* Save the new number of members in the reallocated members array. + * We need to make the following atomic because the member list + * may be traversed from an interrupt handler (read-only). + */ + + flags = spin_lock_irqsave(NULL); + memcpy(newmembers, group->tg_members, + sizeof(pid_t) * group->tg_mxmembers); + oldmembers = group->tg_members; + group->tg_members = newmembers; + group->tg_mxmembers = newmax; + } + else + { + flags = spin_lock_irqsave(NULL); + } + + /* Assign this new pid to the group; group->tg_nmembers will be incremented + * by the caller. + */ + + group->tg_members[group->tg_nmembers] = pid; + group->tg_nmembers++; + spin_unlock_irqrestore(NULL, flags); + + if (oldmembers != NULL) + { + kmm_free(oldmembers); + } + + return OK; +} +#endif /* HAVE_GROUP_MEMBERS */ + /**************************************************************************** * Public Functions ****************************************************************************/ @@ -46,7 +142,12 @@ * Name: group_bind * * Description: - * A thread joins the group when it is created. + * A thread joins the group when it is created. This is a two step process, + * first, the group must bound to the new threads TCB. group_bind() does + * this (at the return from group_join, things are a little unstable: The + * group has been bound, but tg_nmembers has not yet been incremented). + * Then, after the new thread is initialized and has a PID assigned to it, + * group_join() is called, incrementing the tg_nmembers count on the group. * * Input Parameters: * tcb - The TCB of the new "child" task that need to join the group. @@ -78,7 +179,12 @@ int group_bind(FAR struct pthread_tcb_s *tcb) * Name: group_join * * Description: - * A thread joins the group when it is created. + * A thread joins the group when it is created. This is a two step process, + * first, the group must bound to the new threads TCB. group_bind() does + * this (at the return from group_join, things are a little unstable: The + * group has been bound, but tg_nmembers has not yet been incremented). + * Then, after the new thread is initialized and has a PID assigned to it, + * group_join() is called, incrementing the tg_nmembers count on the group. * * Input Parameters: * tcb - The TCB of the new "child" task that need to join the group. @@ -97,19 +203,32 @@ int group_bind(FAR struct pthread_tcb_s *tcb) int group_join(FAR struct pthread_tcb_s *tcb) { FAR struct task_group_s *group; +#ifdef HAVE_GROUP_MEMBERS + int ret; +#else irqstate_t flags; +#endif - DEBUGASSERT(tcb && tcb->cmn.group); + DEBUGASSERT(tcb && tcb->cmn.group && + tcb->cmn.group->tg_nmembers < UINT8_MAX); /* Get the group from the TCB */ group = tcb->cmn.group; +#ifdef HAVE_GROUP_MEMBERS /* Add the member to the group */ + ret = group_addmember(group, tcb->cmn.pid); + if (ret < 0) + { + return ret; + } +#else flags = spin_lock_irqsave(NULL); - sq_addfirst(&tcb->cmn.member, &group->tg_members); + group->tg_nmembers++; spin_unlock_irqrestore(NULL, flags); +#endif return OK; } diff --git a/sched/group/group_killchildren.c b/sched/group/group_killchildren.c index 7807af43ec..bafe734071 100644 --- a/sched/group/group_killchildren.c +++ b/sched/group/group_killchildren.c @@ -191,7 +191,7 @@ int group_kill_children(FAR struct tcb_s *tcb) ret = CONFIG_GROUP_KILL_CHILDREN_TIMEOUT_MS; while (1) { - if (sq_empty(&tcb->group->tg_members)) + if (tcb->group->tg_nmembers <= 1) { break; } diff --git a/sched/group/group_leave.c b/sched/group/group_leave.c index 04d4134aac..89f8e6ee68 100644 --- a/sched/group/group_leave.c +++ b/sched/group/group_leave.c @@ -105,6 +105,16 @@ static inline void group_release(FAR struct task_group_s *group) mm_map_destroy(&group->tg_mm_map); +#ifdef HAVE_GROUP_MEMBERS + /* Release the members array */ + + if (group->tg_members) + { + kmm_free(group->tg_members); + group->tg_members = NULL; + } +#endif + #ifdef CONFIG_BINFMT_LOADABLE /* If the exiting task was loaded into RAM from a file, then we need to * lease all of the memory resource when the last thread exits the task @@ -127,6 +137,58 @@ static inline void group_release(FAR struct task_group_s *group) group_drop(group); } +/**************************************************************************** + * Name: group_removemember + * + * Description: + * Remove a member from a group. + * + * Input Parameters: + * group - The group from which to remove the member. + * pid - The member to be removed. + * + * Returned Value: + * On success, returns the number of members remaining in the group (>=0). + * Can fail only if the member is not found in the group. On failure, + * returns -ENOENT + * + * Assumptions: + * Called during task deletion and also from the reparenting logic, both + * in a safe context. No special precautions are required here. + * + ****************************************************************************/ + +#ifdef HAVE_GROUP_MEMBERS +static inline void group_removemember(FAR struct task_group_s *group, + pid_t pid) +{ + irqstate_t flags; + int i; + + DEBUGASSERT(group); + + /* Find the member in the array of members and remove it */ + + for (i = 0; i < group->tg_nmembers; i++) + { + /* Does this member have the matching pid */ + + if (group->tg_members[i] == pid) + { + /* Remove the member from the array of members. This must be an + * atomic operation because the member array may be accessed from + * interrupt handlers (read-only). + */ + + flags = enter_critical_section(); + group->tg_members[i] = group->tg_members[group->tg_nmembers - 1]; + group->tg_nmembers--; + leave_critical_section(flags); + } + } +} +#endif /* HAVE_GROUP_MEMBERS */ + /**************************************************************************** * Public Functions ****************************************************************************/ @@ -152,12 +214,10 @@ static inline void group_release(FAR struct task_group_s *group) * ****************************************************************************/ +#ifdef HAVE_GROUP_MEMBERS void group_leave(FAR struct tcb_s *tcb) { FAR struct task_group_s *group; -#ifdef HAVE_GROUP_MEMBERS - irqstate_t flags; -#endif DEBUGASSERT(tcb); @@ -166,17 +226,17 @@ void group_leave(FAR struct tcb_s *tcb) group = tcb->group; if (group) { - /* Remove the member from group. */ + /* Remove the member from group. This function may be called + * during certain error handling before the PID has been + * added to the group. In this case tcb->pid will be uninitialized + * group_removemember() will fail. + */ -#ifdef HAVE_GROUP_MEMBERS - flags = spin_lock_irqsave(NULL); - sq_rem(&tcb->member, &group->tg_members); - spin_unlock_irqrestore(NULL, flags); + group_removemember(group, tcb->pid); /* Have all of the members left the group? */ - if (sq_empty(&group->tg_members)) -#endif + if (group->tg_nmembers == 0) { /* Yes.. Release all of the resource held by the task group */ @@ -191,6 +251,47 @@ void group_leave(FAR struct tcb_s *tcb) } } +#else /* HAVE_GROUP_MEMBERS */ + +void group_leave(FAR struct tcb_s *tcb) +{ + FAR struct task_group_s *group; + + DEBUGASSERT(tcb); + + /* Make sure that we have a group */ + + group = tcb->group; + if (group) + { + /* Yes, we have a group.. Is this the last member of the group? */ + + if (group->tg_nmembers > 1) + { + /* No.. just decrement the number of members in the group */ + + group->tg_nmembers--; + } + + /* Yes.. that was the last member remaining in the group */ + + else + { + /* Release all of the resource held by the task group */ + + group_release(group); + } + + /* In any event, we can detach the group from the TCB so we won't do + * this again. + */ + + tcb->group = NULL; + } +} + +#endif /* HAVE_GROUP_MEMBERS */ + /**************************************************************************** * Name: group_drop * diff --git a/sched/task/task_exithook.c b/sched/task/task_exithook.c index 0a586f8908..b8afa5e977 100644 --- a/sched/task/task_exithook.c +++ b/sched/task/task_exithook.c @@ -174,7 +174,7 @@ static inline void nxtask_sigchild(pid_t ppid, FAR struct tcb_s *ctcb, * should generate SIGCHLD. */ - if (sq_is_singular(&chgrp->tg_members)) + if (chgrp->tg_nmembers == 1) { /* Mark that all of the threads in the task group have exited */ @@ -360,9 +360,7 @@ static inline void nxtask_exitwakeup(FAR struct tcb_s *tcb, int status) /* Is this the last thread in the group? */ -#ifndef CONFIG_DISABLE_PTHREAD - if (sq_is_singular(&group->tg_members)) -#endif + if (group->tg_nmembers == 1) { /* Yes.. Wakeup any tasks waiting for this task to exit */
