You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/05/09 08:34:54 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1009: Add support for preemptive scheduling in simulator build
xiaoxiang781216 commented on a change in pull request #1009:
URL: https://github.com/apache/incubator-nuttx/pull/1009#discussion_r422464666
##########
File path: arch/sim/Kconfig
##########
@@ -474,4 +474,22 @@ config SIM_QSPIFLASH_PAGESIZE
"wrap" causing the initial data sent to be overwritten.
This is consistent with standard SPI FLASH operation.
+config SIM_PREEMPTIBLE
+ bool "Preempt tasks on a periodic tick"
+ default false
+ ---help---
+ Setup a timer on the host that generates periodic signals
+ used to simulate the tick interrupt. In the context of this
+ tick interrupt we query the scheduler for the next task
+ and preempt the one that is running.
+
+if SIM_PREEMPTIBLE
+config SIM_PREEMPTIBLE_TASK_DURATION_MS
Review comment:
Should we reuse USEC_PER_TICK Kconfig? it doesn't make sense we use two Kconfig to describe the same thing.
##########
File path: arch/sim/include/irq.h
##########
@@ -83,7 +87,11 @@ struct xcptcontext
{
void *sigdeliver; /* Actual type is sig_deliver_t */
+#ifndef CONFIG_SIM_PREEMPTIBLE
Review comment:
Let's use the positive logic:
#ifdef CONFIG_SIM_PREEMPTIBLE
xcpt_reg_t regs[XCPTCONTEXT_REGS];
#else
ucontext_t context;
#endif
##########
File path: include/signal.h
##########
@@ -267,8 +267,9 @@
* special meaning in some circumstances (e.g., kill()).
*/
-#ifndef __SIGSET_T_DEFINED
+#if !defined(__sigset_t_defined) && !defined(__SIGSET_T_DEFINED)
Review comment:
Let's replace __SIGSET_T_DEFINED to __sigset_t_defined directly to more compatible with other implementation? __SIGINFO_T_DEFINED may need the same change to keep the consistence.
##########
File path: arch/sim/src/sim/up_blocktask.c
##########
@@ -145,7 +149,11 @@ void up_block_task(struct tcb_s *tcb, tstate_t task_state)
/* Then switch contexts */
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ swapcontext(&prev_rtcb->xcp.context, &rtcb->xcp.context);
Review comment:
we can't call swapcontext directly since NuttX may support swapcontext in the furture and then this code will call the implemenation form NuttX not from host which isn't what you want. You can reference how we do the same thing to call host timer API:
arch/sim/src/sim/up_hosttime.c
Here has more background information:
https://cwiki.apache.org/confluence/display/NUTTX/NuttX+Simulation
##########
File path: arch/sim/src/sim/up_blocktask.c
##########
@@ -118,7 +118,11 @@ void up_block_task(struct tcb_s *tcb, tstate_t task_state)
* value, then this is really the previously running task restarting!
*/
+#ifdef CONFIG_SIM_PREEMPTIBLE
Review comment:
It's better to combine this patch with:
https://github.com/apache/incubator-nuttx/pull/1009/commits/0e2bee8c4731ffa616a48e270be346947a4b489b
##########
File path: arch/sim/src/sim/up_blocktask.c
##########
@@ -145,7 +149,11 @@ void up_block_task(struct tcb_s *tcb, tstate_t task_state)
/* Then switch contexts */
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ swapcontext(&prev_rtcb->xcp.context, &rtcb->xcp.context);
Review comment:
One method could like other platform define:
up_switchcontext
up_fullcontextrestore
Andd implement these two api in the different way.
##########
File path: arch/sim/src/sim/up_initialstate.c
##########
@@ -73,6 +73,11 @@ void up_initial_state(struct tcb_s *tcb)
#else
int ret;
+ if (tcb->adj_stack_ptr == NULL)
Review comment:
Merge with the previous patch, we need each patch can work correctly.
##########
File path: arch/sim/src/sim/up_idle.c
##########
@@ -89,6 +89,15 @@ void up_idle(void)
}
#endif
+#ifndef CONFIG_SIM_PREEMPTIBLE
+ /* If the system is idle, then process "fake" timer interrupts.
+ * Hopefully, something will wake up.
+ */
+
+ nxsched_process_timer();
Review comment:
Why CONFIG_SIM_PREEMPTIBLE can't work with CONFIG_SCHED_TICKLESS?
##########
File path: arch/sim/src/sim/up_timer.c
##########
@@ -0,0 +1,169 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_timer.c
+ *
+ * Copyright (C) 2014, 2019 Gregory Nutt. All rights reserved.
+ * Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <se...@gmail.com>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * 3. Neither the name NuttX nor the names of its contributors may be
+ * used to endorse or promote products derived from this software
+ * without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
+ * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
+ * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
+ * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+#include <stdint.h>
+#include <stdarg.h>
+
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
+#include "../../../../include/nuttx/config.h"
Review comment:
Why need include this file?
##########
File path: arch/sim/src/sim/up_timer.c
##########
@@ -0,0 +1,169 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_timer.c
+ *
+ * Copyright (C) 2014, 2019 Gregory Nutt. All rights reserved.
Review comment:
All new file need Apache copyright, you can find the right head from the code base.
##########
File path: arch/sim/src/sim/up_timer.c
##########
@@ -0,0 +1,169 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_timer.c
+ *
+ * Copyright (C) 2014, 2019 Gregory Nutt. All rights reserved.
+ * Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <se...@gmail.com>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * 3. Neither the name NuttX nor the names of its contributors may be
+ * used to endorse or promote products derived from this software
+ * without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
+ * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
+ * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
+ * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+#include <stdint.h>
+#include <stdarg.h>
+
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
+#include "../../../../include/nuttx/config.h"
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+typedef void (*sched_timer_callback_t)(void);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static sched_timer_callback_t g_sched_process_timer_cb;
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: host_signal_handler
+ *
+ * Description:
+ * The signal handler is called periodically and is used to deliver TICK
+ * events to the OS.
+ *
+ ****************************************************************************/
+static void host_signal_handler(int sig, siginfo_t *si, void *old_ucontext)
+{
+ g_sched_process_timer_cb();
Review comment:
Does this really work? The signal can happen at any point, I don't think nxsched_process_timer can be called without any protection.
##########
File path: arch/sim/src/sim/up_timer.c
##########
@@ -0,0 +1,169 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_timer.c
+ *
+ * Copyright (C) 2014, 2019 Gregory Nutt. All rights reserved.
+ * Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <se...@gmail.com>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * 3. Neither the name NuttX nor the names of its contributors may be
+ * used to endorse or promote products derived from this software
+ * without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
+ * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
+ * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
+ * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+#include <stdint.h>
+#include <stdarg.h>
+
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
+#include "../../../../include/nuttx/config.h"
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+typedef void (*sched_timer_callback_t)(void);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static sched_timer_callback_t g_sched_process_timer_cb;
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: host_signal_handler
+ *
+ * Description:
+ * The signal handler is called periodically and is used to deliver TICK
+ * events to the OS.
+ *
+ ****************************************************************************/
+static void host_signal_handler(int sig, siginfo_t *si, void *old_ucontext)
+{
+ g_sched_process_timer_cb();
+}
+
+/****************************************************************************
+ * Name: host_setup_timer
+ *
+ * Description:
+ * Set up a timer to send periodic signals.
+ *
+ ****************************************************************************/
+
+static int host_setup_timer(void)
+{
+ int ret;
+ struct itimerval it;
+
+ it.it_interval.tv_sec = 0;
+ it.it_interval.tv_usec = CONFIG_SIM_PREEMPTIBLE_TASK_DURATION_MS * 1000;
+ it.it_value = it.it_interval;
+
+ ret = setitimer(ITIMER_REAL, &it, NULL);
+ if (ret < 0)
+ {
+ fprintf(stderr, "%d settimer\n", ret);
+ return ret;
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: host_setup_signals
+ *
+ * Description:
+ * Set up a signal to deliver periodic TICK events.
+ *
+ ****************************************************************************/
+
+static int host_setup_signals(void (*action)(int, siginfo_t *, void *))
+{
+ struct sigaction act;
+ int ret;
+ sigset_t set;
+
+ act.sa_sigaction = action;
+ sigemptyset(&act.sa_mask);
+ act.sa_flags = SA_SIGINFO;
+
+ sigemptyset(&set);
+ sigaddset(&set, SIGALRM);
+
+ if ((ret = sigaction(SIGALRM, &act, NULL)) != 0)
+ {
+ fprintf(stderr, "%d signal handler", ret);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_init_timer
+ *
+ * Description:
+ * Creates a periodic timer and sets up a signal handler on the host.
+ *
+ ****************************************************************************/
+
+int up_init_timer(void (*nxsched_process_timer)(void))
Review comment:
I think this logic is better integrate into up_oneshot.c. The timer interrupt generation is very easy, the hardest part is how to call nxsched_process_timer without synchronization issue.
##########
File path: arch/sim/src/sim/up_initialstate.c
##########
@@ -73,6 +73,10 @@ void up_initial_state(struct tcb_s *tcb)
#else
int ret;
+#if CONFIG_RR_INTERVAL > 0
+ tcb->flags |= TCB_FLAG_SCHED_RR;
Review comment:
The change is wrong, CONFIG_RR_INTERVAL doesn't mean all thread will use the route robin policy.
##########
File path: arch/sim/src/sim/up_interruptcontext.c
##########
@@ -43,10 +43,47 @@
#include <nuttx/arch.h>
#include "up_internal.h"
+#ifdef CONFIG_SIM_PREEMPTIBLE
+uint32_t host_signal_save(void);
Review comment:
@sebastianene07 please merge all preemptive scheduling related change in one patch. Without this patch, your previous change just break the system.
##########
File path: boards/sim/sim/sim/configs/nsh3/defconfig
##########
@@ -19,11 +19,26 @@ CONFIG_BOARD_LOOPSPERMSEC=0
CONFIG_BOOT_RUNFROMEXTSRAM=y
CONFIG_BUILTIN=y
CONFIG_BUILTIN_PROXY_STACKSIZE=8192
+CONFIG_DEBUG_ERROR=y
+CONFIG_DEBUG_FEATURES=y
+CONFIG_DEBUG_INFO=y
+CONFIG_DEBUG_NET=y
+CONFIG_DEBUG_NET_ERROR=y
+CONFIG_DEBUG_NET_INFO=y
+CONFIG_DEBUG_NET_WARN=y
+CONFIG_DEBUG_SCHED=y
+CONFIG_DEBUG_SCHED_ERROR=y
+CONFIG_DEBUG_SCHED_INFO=y
+CONFIG_DEBUG_SCHED_WARN=y
CONFIG_DEBUG_SYMBOLS=y
+CONFIG_DEBUG_WARN=y
CONFIG_DEV_LOOP=y
CONFIG_DEV_ZERO=y
CONFIG_EXAMPLES_HELLO=y
CONFIG_EXAMPLES_HELLO_STACKSIZE=8192
+CONFIG_EXAMPLES_TELNETD=y
+CONFIG_EXAMPLES_TELNETD_CLIENTSTACKSIZE=8192
Review comment:
you can change DEFAULT_TASK_STACKSIZE instead.
And please merge with the first nsh3/defconfig
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org