From 52fbbaf778c9f48d1ad9e8237b7f5b69e2f1c40b Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Thu, 18 Feb 2016 08:34:11 -0600 Subject: [PATCH] 1. SMP: Fix an assertion. SMP-specific change accidentally made in non-SMP code 2. Move list of signal actions from the task TCB to the task group. Signal handlers are a property of the entire task group and not of individual threads in the group. I know, I preferred it the other way too but this is more compliant with POSIX. --- ChangeLog | 7 +- TODO | 106 ++++++++++++++++++++++++- include/nuttx/sched.h | 4 +- sched/group/group_signal.c | 12 ++- sched/sched/sched_addreadytorun.c | 2 +- sched/signal/sig_action.c | 16 ++-- sched/signal/sig_cleanup.c | 20 ++--- sched/signal/sig_dispatch.c | 12 ++- sched/signal/sig_findaction.c | 8 +- sched/signal/sig_pending.c | 2 +- sched/signal/sig_removependingsignal.c | 6 +- sched/signal/signal.h | 2 +- 12 files changed, 157 insertions(+), 40 deletions(-) diff --git a/ChangeLog b/ChangeLog index 7ce513e2f0..47354c5a4a 100755 --- a/ChangeLog +++ b/ChangeLog @@ -11491,5 +11491,8 @@ your code still uses them, please switch to enter_critical_section() and leave_critical_section() (2016-02-14). * Also rename irqdisable() and irqenable() to up_irq_disable() and - up_irq_enable (2016-02-14). - + up_irq_enable() (2016-02-14). + * sched/signal and include/nuttx/sched.h: Move the list of signal + actions from the TCB to the group structure. Signal handlers are not + per thread but, rather, per task group. I know, I preferred it the + other way too, but this is more compliant with POSIX (2016-02-18). diff --git a/TODO b/TODO index 5d484fd1da..5351334ad5 100644 --- a/TODO +++ b/TODO @@ -1,4 +1,4 @@ -NuttX TODO List (Last updated February 8, 2016) +NuttX TODO List (Last updated February 18, 2016) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This file summarizes known NuttX bugs, limitations, inconsistencies with @@ -25,7 +25,7 @@ nuttx/ (8) Graphics subsystem (graphics/) (1) Pascal add-on (pcode/) (1) Build system / Toolchains - (3) Linux/Cywgin simulation (arch/sim) + (4) Linux/Cywgin simulation (arch/sim) (4) ARM (arch/arm/) apps/ @@ -1620,6 +1620,108 @@ o Linux/Cywgin simulation (arch/sim) Status: Open Priority: Low + Title: SMP SIMULATION INCOMPLETE + Description: The configuration has basic support SMP testing. The simulation + supports the emulation of multiple CPUs by creating multiple + pthreads, each run a copy of the simulation in the same process + address space. + + The simulation SMP implemention is incomplete, however. Two + critical SMP functions are not implemented: + + - int up_cpu_pause(void) + - int up_cpu_resume(void) + + These are used to start a new task on a different CPU: (1) + the other CPU is stopped or paused, (2) the OS datastructures + for that CPU are modified, then (2) the other CPU is resumed. + Unfortunately, I have not yet thought of a way to implement + them in the simulation. + + Currently, for example, you can enable SMP for ostest + configuration by enabling: + + -# CONFIG_EXPERIMENTAL is not set + +CONFIG_EXPERIMENTAL=y + + +CONFIG_SPINLOCK=y + +CONFIG_SMP=y + +CONFIG_SMP_NCPUS=2 + +CONFIG_SMP_IDLETHREAD_STACKSIZE=2048 + + And you can enable some additional debug output with: + + -# CONFIG_DEBUG_SCHED is not set + +CONFIG_DEBUG_SCHED=y + + -# CONFIG_SCHED_INSTRUMENTATION is not set + +CONFIG_SCHED_INSTRUMENTATION=y + + The result should be as follows: + + - CPU0 initializes and starts CPU1. CPU1 is running an + executing the IDLE task. + + os_start: Entry + os_idletask: CPU1: Beginning Idle Loop + + - CPU0 brings up the the OS test application on CPU0 + + os_do_appstart: Starting init thread + CPU0: Start init, TCB@42f490, state=5 + up_unblock_task: Unblocking TCB=42f490 + CPU0: Suspend CPU0 IDLE, TCB@42c180, state=4 + CPU0: Resume init, TCB@42f490, state=0 + up_unblock_task: New Active Task TCB=42f490 + + - OS test runs and performs some basic checks. + + stdio_test: write fd=1 + stdio_test: Standard I/O Check: printf + stdio_test: write fd=2 + stdio_test: Standard I/O Check: fprintf to stderr + ostest_main: putenv(Variable1=BadValue3) + + up_block_task: Blocking TCB=42f490 + CPU0: Suspend init, TCB@42f490, state=4 + CPU0: Resume CPU0 IDLE, TCB@42c180, state=3 + up_block_task: New Active Task TCB=42c180 + up_unblock_task: Unblocking TCB=42f490 + CPU0: Suspend CPU0 IDLE, TCB@42c180, state=4 + CPU0: Resume init, TCB@42f490, state=0 + up_unblock_task: New Active Task TCB=42f490 + + ostest_main: setenv(Variable1, GoodValue1, TRUE) + ostest_main: setenv(Variable2, BadValue1, FALSE) + ostest_main: setenv(Variable2, GoodValue2, TRUE) + ostest_main: setenv(Variable3, Variable3, FALSE) + ostest_main: setenv(Variable3, Variable3, FALSE) + show_variable: Variable=Variable1 has value=GoodValue1 + show_variable: Variable=Variable2 has value=GoodValue2 + show_variable: Variable=Variable3 has value=GoodValue3 + + - Then OS test tries to start the task ostest on CPU1 + + CPU0: Start ostest, TCB@430e90, state=5 + up_unblock_task: Unblocking TCB=430e90 + CPU1: Suspend CPU1 IDLE, TCB@42c2c0, state=4 + CPU0: Resume ostest, TCB@430e90, state=0 + ostest_main: Started user_main at PID=4 + + There is no failure but, of course, the task on CPU1 does not + run, i.e., it does not replace the IDLE task running on CPU1. + + 2016-02-16: The NSH configuration can be forced to run, but + only if (1) You don't try to execute built-in commands or to + execute commands in the background. Those thoses cases, it + will try to start the command on CPU1 and the same problem + as for the ostest case occurs. + + Also, for NSH you have to modify arch/sim/src/up_idle.c so + that the IDLE loop only runfs for CPU0. Otherwise, often + simuart_post() will be called from CPU1 and it will try to + restart NSH on CPU0 and, again, the same problem occurs. + o ARM (arch/arm/) ^^^^^^^^^^^^^^^ diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index cade6cde3e..586c1a81e9 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -445,7 +445,8 @@ struct task_group_s #ifndef CONFIG_DISABLE_SIGNALS /* POSIX Signal Control Fields ************************************************/ - sq_queue_t sigpendingq; /* List of pending signals */ + sq_queue_t tg_sigactionq; /* List of actions for signals */ + sq_queue_t tg_sigpendingq; /* List of pending signals */ #endif #ifndef CONFIG_DISABLE_ENVIRON @@ -599,7 +600,6 @@ struct tcb_s #ifndef CONFIG_DISABLE_SIGNALS sigset_t sigprocmask; /* Signals that are blocked */ sigset_t sigwaitmask; /* Waiting for pending signals */ - sq_queue_t sigactionq; /* List of actions for signals */ sq_queue_t sigpendactionq; /* List of pending signal actions */ sq_queue_t sigpostedq; /* List of posted signals */ siginfo_t sigunbinfo; /* Signal info when task unblocked */ diff --git a/sched/group/group_signal.c b/sched/group/group_signal.c index f3ed143bbd..7b29e3d5d6 100644 --- a/sched/group/group_signal.c +++ b/sched/group/group_signal.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/group/group_signal.c * - * Copyright (C) 2013 Gregory Nutt. All rights reserved. + * Copyright (C) 2013, 2016 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -89,6 +89,7 @@ struct group_signal_s static int group_signal_handler(pid_t pid, FAR void *arg) { FAR struct group_signal_s *info = (FAR struct group_signal_s *)arg; + FAR struct task_group_s *group; FAR struct tcb_s *tcb; FAR sigactq_t *sigact; int ret; @@ -98,7 +99,7 @@ static int group_signal_handler(pid_t pid, FAR void *arg) /* Get the TCB associated with the group member */ tcb = sched_gettcb(pid); - DEBUGASSERT(tcb); + DEBUGASSERT(tcb ! = NULL); if (tcb) { /* Set this one as the default if we have not already set the default. */ @@ -151,9 +152,12 @@ static int group_signal_handler(pid_t pid, FAR void *arg) info->utcb = tcb; } - /* Is there also an action associated with the task? */ + /* Is there also a action associated with the task group? */ - sigact = sig_findaction(tcb, info->siginfo->si_signo); + group = tcb->group; + DEBUGASSERT(group != NULL); + + sigact = sig_findaction(group, info->siginfo->si_signo); if (sigact) { /* Yes.. then use this thread. The requirement is this: diff --git a/sched/sched/sched_addreadytorun.c b/sched/sched/sched_addreadytorun.c index 6beb3267ff..2bcc3f9087 100644 --- a/sched/sched/sched_addreadytorun.c +++ b/sched/sched/sched_addreadytorun.c @@ -113,7 +113,7 @@ bool sched_addreadytorun(FAR struct tcb_s *btcb) * is now the new active task! */ - ASSERT(!spin_islocked(&g_cpu_schedlock) && btcb->flink != NULL); + DEBUGASSERT(rtcb->lockcount == 0 && btcb->flink != NULL); btcb->task_state = TSTATE_TASK_RUNNING; btcb->flink->task_state = TSTATE_TASK_READYTORUN; diff --git a/sched/signal/sig_action.c b/sched/signal/sig_action.c index c0f7dcc743..4b61dc1d49 100644 --- a/sched/signal/sig_action.c +++ b/sched/signal/sig_action.c @@ -156,12 +156,16 @@ static FAR sigactq_t *sig_allocateaction(void) int sigaction(int signo, FAR const struct sigaction *act, FAR struct sigaction *oact) { FAR struct tcb_s *rtcb = this_task(); + FAR struct task_group_s *group; FAR sigactq_t *sigact; /* Since sigactions can only be installed from the running thread of * execution, no special precautions should be necessary. */ + DEBUGASSERT(rtcb != NULL && rtcb->group != NULL); + group = rtcb->group; + /* Verify the signal number */ if (!GOOD_SIGNO(signo)) @@ -170,9 +174,9 @@ int sigaction(int signo, FAR const struct sigaction *act, FAR struct sigaction * return ERROR; } - /* Find the signal in the sigactionq */ + /* Find the signal in the signal action queue */ - sigact = sig_findaction(rtcb, signo); + sigact = sig_findaction(group, signo); /* Return the old sigaction value if so requested */ @@ -242,9 +246,9 @@ int sigaction(int signo, FAR const struct sigaction *act, FAR struct sigaction * if (sigact) { - /* Yes.. Remove it from sigactionq */ + /* Yes.. Remove it from signal action queue */ - sq_rem((FAR sq_entry_t *)sigact, &rtcb->sigactionq); + sq_rem((FAR sq_entry_t *)sigact, &group->tg_sigactionq); /* And deallocate it */ @@ -278,9 +282,9 @@ int sigaction(int signo, FAR const struct sigaction *act, FAR struct sigaction * sigact->signo = (uint8_t)signo; - /* Add the new sigaction to sigactionq */ + /* Add the new sigaction to signal action queue */ - sq_addlast((FAR sq_entry_t *)sigact, &rtcb->sigactionq); + sq_addlast((FAR sq_entry_t *)sigact, &group->tg_sigactionq); } /* Set the new sigaction */ diff --git a/sched/signal/sig_cleanup.c b/sched/signal/sig_cleanup.c index 5847a3f83c..4a37e67b5f 100644 --- a/sched/signal/sig_cleanup.c +++ b/sched/signal/sig_cleanup.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/signal/sig_cleanup.c * - * Copyright (C) 2007, 2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2007, 2009, 2016 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -58,16 +58,8 @@ void sig_cleanup(FAR struct tcb_s *stcb) { - FAR sigactq_t *sigact; FAR sigq_t *sigq; - /* Deallocate all entries in the list of signal actions */ - - while ((sigact = (FAR sigactq_t *)sq_remfirst(&stcb->sigactionq)) != NULL) - { - sig_releaseaction(sigact); - } - /* Deallocate all entries in the list of pending signal actions */ while ((sigq = (FAR sigq_t *)sq_remfirst(&stcb->sigpendactionq)) != NULL) @@ -101,11 +93,19 @@ void sig_cleanup(FAR struct tcb_s *stcb) void sig_release(FAR struct task_group_s *group) { + FAR sigactq_t *sigact; FAR sigpendq_t *sigpend; + /* Deallocate all entries in the list of signal actions */ + + while ((sigact = (FAR sigactq_t *)sq_remfirst(&group->tg_sigactionq)) != NULL) + { + sig_releaseaction(sigact); + } + /* Deallocate all entries in the list of pending signals */ - while ((sigpend = (FAR sigpendq_t *)sq_remfirst(&group->sigpendingq)) != NULL) + while ((sigpend = (FAR sigpendq_t *)sq_remfirst(&group->tg_sigpendingq)) != NULL) { sig_releasependingsignal(sigpend); } diff --git a/sched/signal/sig_dispatch.c b/sched/signal/sig_dispatch.c index b52b89290f..2fedb96088 100644 --- a/sched/signal/sig_dispatch.c +++ b/sched/signal/sig_dispatch.c @@ -72,6 +72,7 @@ static int sig_queueaction(FAR struct tcb_s *stcb, siginfo_t *info) { + FAR struct task_group_s *group; FAR sigactq_t *sigact; FAR sigq_t *sigq; irqstate_t flags; @@ -79,9 +80,12 @@ static int sig_queueaction(FAR struct tcb_s *stcb, siginfo_t *info) sched_lock(); - /* Find the sigaction associated with this signal */ + /* Find the group sigaction associated with this signal */ - sigact = sig_findaction(stcb, info->si_signo); + DEBUGASSERT(stcb != NULL && stcb->group != NULL); + + group = stcb->group; + sigact = sig_findaction(group, info->si_signo); /* Check if a valid signal handler is available and if the signal is * unblocked. NOTE: There is no default action. @@ -206,7 +210,7 @@ static FAR sigpendq_t *sig_findpendingsignal(FAR struct task_group_s *group, /* Seach the list for a sigpendion on this signal */ - for (sigpend = (FAR sigpendq_t *)group->sigpendingq.head; + for (sigpend = (FAR sigpendq_t *)group->tg_sigpendingq.head; (sigpend && sigpend->info.si_signo != signo); sigpend = sigpend->flink); @@ -260,7 +264,7 @@ static FAR sigpendq_t *sig_addpendingsignal(FAR struct tcb_s *stcb, /* Add the structure to the pending signal list */ flags = enter_critical_section(); - sq_addlast((FAR sq_entry_t *)sigpend, &group->sigpendingq); + sq_addlast((FAR sq_entry_t *)sigpend, &group->tg_sigpendingq); leave_critical_section(flags); } } diff --git a/sched/signal/sig_findaction.c b/sched/signal/sig_findaction.c index 0980e0658d..769a678d3c 100644 --- a/sched/signal/sig_findaction.c +++ b/sched/signal/sig_findaction.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/signal/sig_findaction.c * - * Copyright (C) 2007, 2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2007, 2009, 2016 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -52,13 +52,13 @@ * ****************************************************************************/ -FAR sigactq_t *sig_findaction(FAR struct tcb_s *stcb, int signo) +FAR sigactq_t *sig_findaction(FAR struct task_group_s *group, int signo) { FAR sigactq_t *sigact = NULL; /* Verify the caller's sanity */ - if (stcb) + if (group) { /* Sigactions can only be assigned to the currently executing * thread. So, a simple lock ought to give us sufficient @@ -69,7 +69,7 @@ FAR sigactq_t *sig_findaction(FAR struct tcb_s *stcb, int signo) /* Seach the list for a sigaction on this signal */ - for (sigact = (FAR sigactq_t *)stcb->sigactionq.head; + for (sigact = (FAR sigactq_t *)group->tg_sigactionq.head; ((sigact) && (sigact->signo != signo)); sigact = sigact->flink); diff --git a/sched/signal/sig_pending.c b/sched/signal/sig_pending.c index 91a272c547..aadef0b86b 100644 --- a/sched/signal/sig_pending.c +++ b/sched/signal/sig_pending.c @@ -103,7 +103,7 @@ sigset_t sig_pendingset(FAR struct tcb_s *stcb) sigpendset = NULL_SIGNAL_SET; flags = enter_critical_section(); - for (sigpend = (FAR sigpendq_t *)group->sigpendingq.head; + for (sigpend = (FAR sigpendq_t *)group->tg_sigpendingq.head; (sigpend); sigpend = sigpend->flink) { sigaddset(&sigpendset, sigpend->info.si_signo); diff --git a/sched/signal/sig_removependingsignal.c b/sched/signal/sig_removependingsignal.c index 3de79184f7..90485c2dc5 100644 --- a/sched/signal/sig_removependingsignal.c +++ b/sched/signal/sig_removependingsignal.c @@ -76,7 +76,7 @@ FAR sigpendq_t *sig_removependingsignal(FAR struct tcb_s *stcb, int signo) flags = enter_critical_section(); - for (prevsig = NULL, currsig = (FAR sigpendq_t *)group->sigpendingq.head; + for (prevsig = NULL, currsig = (FAR sigpendq_t *)group->tg_sigpendingq.head; (currsig && currsig->info.si_signo != signo); prevsig = currsig, currsig = currsig->flink); @@ -84,11 +84,11 @@ FAR sigpendq_t *sig_removependingsignal(FAR struct tcb_s *stcb, int signo) { if (prevsig) { - sq_remafter((FAR sq_entry_t *)prevsig, &group->sigpendingq); + sq_remafter((FAR sq_entry_t *)prevsig, &group->tg_sigpendingq); } else { - sq_remfirst(&group->sigpendingq); + sq_remfirst(&group->tg_sigpendingq); } } diff --git a/sched/signal/signal.h b/sched/signal/signal.h index bce55af8bd..e8cfda81b6 100644 --- a/sched/signal/signal.h +++ b/sched/signal/signal.h @@ -186,7 +186,7 @@ void sig_release(FAR struct task_group_s *group); FAR sigq_t *sig_allocatependingsigaction(void); void sig_deliver(FAR struct tcb_s *stcb); -FAR sigactq_t *sig_findaction(FAR struct tcb_s *stcb, int signo); +FAR sigactq_t *sig_findaction(FAR struct task_group_s *group, int signo); int sig_lowest(FAR sigset_t *set); #ifdef CONFIG_CAN_PASS_STRUCTS int sig_mqnotempty(int tid, int signo, union sigval value);