xiaoxiang781216 commented on a change in pull request #1009:
URL: https://github.com/apache/incubator-nuttx/pull/1009#discussion_r422635361
##########
File path: arch/sim/include/irq.h
##########
@@ -44,6 +44,10 @@
* Included Files
****************************************************************************/
+#ifdef CONFIG_SIM_PREEMPTIBLE
+# include <ucontext.h>
Review comment:
Two method I can come up:
1.Define XCPTCONTEXT_REGS bigger than ucontext storage
2.Define a pointer and allocate memory dynamically
But, anyway the code build inside NuttX can't include host side header file.
It is't acceptable from the techincal point.
##########
File path: arch/sim/src/Makefile
##########
@@ -71,10 +71,16 @@ CSRCS += up_unblocktask.c up_blocktask.c up_releasepending.c
CSRCS += up_reprioritizertr.c up_exit.c up_schedulesigaction.c
CSRCS += up_allocateheap.c
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ CSRCS += up_tick_me.c
+ STDLIBS += -lpthread
+endif
+
VPATH = sim
DEPPATH = $(patsubst %,--dep-path %,$(subst :, ,$(VPATH)))
HOSTSRCS = up_hosttime.c
+HOSTCFLAGS +=
-DCONFIG_SIM_PREEMPTIBLE_TICK_DURATION_MS=$(CONFIG_SIM_PREEMPTIBLE_TICK_DURATION_MS)
Review comment:
I suggested to use USEC_PER_TICK before.
##########
File path: arch/sim/src/sim/up_oneshot.c
##########
@@ -357,6 +358,16 @@ FAR struct oneshot_lowerhalf_s *oneshot_initialize(int
chan,
void up_timer_initialize(void)
{
+ /* Block the signals for the new threads created on the host to prevent
+ * a race condition where the simulated interrupt handler runs on another
+ * host thread. The new threads will inherit the signal mask which has
+ * blocked signals.
+ */
+
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ up_prepare_timer();
Review comment:
As I said before, timer isn't compatible with oneshot timer, both can't
work together. you should integrate the alarm timer logic into oneshot.
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -156,3 +158,4 @@ HOSTCC = cc
HOSTINCLUDES = -I.
HOSTCFLAGS = $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
$(ARCHCPUFLAGS) $(HOSTINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+HOSTCFLAGS += -DCONFIG_USEC_PER_TICK=$(CONFIG_USEC_PER_TICK)
Review comment:
Move to arch/sim/sim/src/Makefile
##########
File path: arch/sim/src/sim/up_timer.c
##########
@@ -0,0 +1,362 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_timer.c
+ *
+ * Copyright (C) 2014, 2020 Gregory Nutt. All rights reserved.
+ * Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <sebastian.en...@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>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/* Number of microseconds per milisecond */
+
+#define USEC_PER_MSEC (1000L)
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+/* The callback invoked from signal handler */
+
+typedef void (*sched_timer_callback_t)(void);
+
+/* The primask bit index corresponding to a host signal */
+
+enum host_primask_bitset_e
+{
+ PRIMASK_BIT_0_SIGALARM = 0,
+ PRIMASK_BIT_SIZE
+};
+
+typedef enum host_primask_bitset_e host_primask_bitset_t;
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static void host_signal_handler(int sig, siginfo_t *si, void *old_ucontext);
+
+static int host_setup_timer(void);
+static int host_setup_signals(void (*action)(int, siginfo_t *, void *));
+
+static uint32_t get_irqstate_from_signal_mask(const sigset_t *signal_mask);
+static void get_signal_mask_from_irqstate(sigset_t *restored_mask,
+ uint32_t irqstate);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+/* The callback to the NuttX scheduler that will be invoked from sig
+ * handler.
+ */
+
+static sched_timer_callback_t g_sched_process_timer_cb;
+
+/* The map between the host signal and the bit index the irqstate_t from
+ * NuttX.
+ */
+
+static const int g_host_signal_map[PRIMASK_BIT_SIZE] =
+{
+ SIGALRM
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: host_signal_handler
+ *
+ * Description:
+ * The signal handler is called periodically and is used to deliver TICK
+ * events to the OS.
+ *
+ * Input Parameters:
+ * sig - the signal number
+ * si - the signal information
+ * old_ucontext - the previous context
+ *
+ ****************************************************************************/
+
+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_TICK_DURATION_MS *
+ USEC_PER_MSEC;
+
+ it.it_value = it.it_interval;
+ ret = setitimer(ITIMER_REAL, &it, NULL);
+ if (ret < 0)
+ {
+ fprintf(stderr, "ERROR %d settimer\n", ret);
+ return ret;
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: host_setup_signals
+ *
+ * Description:
+ * Set up a signal to deliver periodic TICK events.
+ *
+ * Input Parameters:
+ * action - the callback invoked when we are interrupted by a signal
+ *
+ * Returned Value:
+ * This function returns 0 on success, on error -1 is returned and host
+ * errno is set to indicate an error.
+ *
+ ****************************************************************************/
+
+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);
+ pthread_sigmask(SIG_UNBLOCK, &set, NULL);
+
+ if ((ret = sigaction(SIGALRM, &act, NULL)) != 0)
+ {
+ fprintf(stderr, "ERROR %d signal handler", ret);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: get_irqstate_from_signal_mask
+ *
+ * Description:
+ * A helper function that creates the NuttX IRQ mask from the host signal
+ * mask.
+ *
+ * Input Parameters:
+ * signal_mask - the signal mask for the current running process on host
+ *
+ * Description:
+ * Return a bitmask with the current active signals.
+ *
+ ****************************************************************************/
+
+static uint32_t get_irqstate_from_signal_mask(const sigset_t *signal_mask)
+{
+ uint32_t nx_irq_mask = 0;
+
+ for (int bit_index = 0; bit_index < PRIMASK_BIT_SIZE; bit_index++)
+ {
+ if (sigismember(signal_mask, g_host_signal_map[bit_index]))
+ {
+ nx_irq_mask |= (1 << bit_index);
+ }
+ }
+
+ return nx_irq_mask;
+}
+
+/****************************************************************************
+ * Name: get_signal_mask_from_irq_state
+ *
+ * Description:
+ * A helper function that creates a host signal mask from NuttX IRQ mask.
+ *
+ * Input Parameters:
+ * restored_mask - (out) buffer where we store the active signals
+ * irqstate - the mask with the active signals
+ *
+ ****************************************************************************/
+
+static void get_signal_mask_from_irqstate(sigset_t *restored_mask,
+ uint32_t irqstate)
+{
+ if (restored_mask == NULL)
+ {
+ return;
+ }
+
+ for (int bit_index = 0; bit_index < PRIMASK_BIT_SIZE; bit_index++)
+ {
+ if ((irqstate & (1 << bit_index)) != 0)
+ {
+ sigaddset(restored_mask, g_host_signal_map[bit_index]);
+ }
+ }
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_prepare_timer
+ *
+ * Description:
+ * Blocks all the signals in the current thread so that the new threads
+ * can inherit a blocked signal mask.
+ *
+ * Returned Value:
+ * This function returns 0 on success otherwise -1.
+ *
+ * Assumptions/Limitations:
+ * This function should be called prior to spawning new threads as we
+ * don't want to end up running nxsched_process_timer on a different
+ * host thread.
+ *
+ ****************************************************************************/
+
+int up_prepare_timer(void)
Review comment:
Please merge all related logic into up_oneshot.c. It doesn't make sense
that we implment timer twice.
##########
File path: include/signal.h
##########
@@ -267,9 +267,9 @@
* special meaning in some circumstances (e.g., kill()).
*/
-#ifndef __SIGSET_T_DEFINED
Review comment:
After some thought, your modify this just because you include ucontext.h
directly? if so, it's better to don't bring ucontext.h into NuttX space. then
we don't need modify this file anymore.
##########
File path: arch/sim/src/sim/up_unblocktask.c
##########
@@ -129,7 +133,11 @@ void up_unblock_task(FAR struct tcb_s *tcb)
/* Then switch contexts */
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ swapcontext(&prev_rtcb->xcp.context, &rtcb->xcp.context);
Review comment:
As I said before, you shouldn't call host function directly inside NuttX
code.
##########
File path: arch/sim/src/sim/up_releasepending.c
##########
@@ -114,7 +118,11 @@ void up_release_pending(void)
/* Then switch contexts */
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ swapcontext(&prev_rtcb->xcp.context, &rtcb->xcp.context);
Review comment:
Why not use sigsetjmp and siglongjmp:
https://linux.die.net/man/3/sigsetjmp
----------------------------------------------------------------
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:
us...@infra.apache.org
