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/08/18 09:29:00 UTC
[GitHub] [incubator-nuttx] sebastianene07 opened a new pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
sebastianene07 opened a new pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602
## Summary
This patch extends the existing functionality by adding a new option : `CONFIG_SIM_PREEMPTIBLE`. This option
changes the way we are generating timer events in `up_hosttime.c` by using an external host timer that sends periodic signals to the simulation. Previously the events were sent from the ` Idle Task` logic by periodically calling `up_timer_update`. The patch improves this logic by adding an event driven implementation and changes the way we are doing context switching using the `ucontext API`.
I have [another PR opened](https://github.com/apache/incubator-nuttx/pull/1009) that is using a different context switching mechanism with `sigsetjmp/siglongjmp` but this approach doesn't work on SMP configurations.
## Impact
The current implementation doesn't affect the existing implementation when `CONFIG_SIM_PREEMPTIBLE=n`.
## Testing
I validated the patch using `sim/ostest` config on both MacOS and Linux machines using `CONFIG_SIM_PREEMPTIBLE=y` and 'CONFIG_RR_INTERVAL=1'.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r475093740
##########
File path: sched/task/task_setup.c
##########
@@ -593,6 +598,11 @@ static inline int nxtask_setup_stackargs(FAR struct task_tcb_s *tcb,
stackargv[argc + 1] = NULL;
tcb->argv = stackargv;
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ up_setup_args_context(tcb->cmn.xcp.ucontext_buffer, tcb->cmn.start, argc,
Review comment:
I think that it is important to follow these rules but I didn't find a better approach. I was aware of this problem during the 'ucontext' patch redesign and I mentioned it here https://github.com/apache/incubator-nuttx/pull/1009#issuecomment-671011407
As ucontext has it's way of sending the stack arguments it can't be worked around in a better way. Do you think we can solve this more elegant ? Or we can use the first method with (sigsetjmp/siglongmp that doesn't have SMP support)
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r475093949
##########
File path: sched/task/task_setup.c
##########
@@ -593,6 +598,11 @@ static inline int nxtask_setup_stackargs(FAR struct task_tcb_s *tcb,
stackargv[argc + 1] = NULL;
tcb->argv = stackargv;
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ up_setup_args_context(tcb->cmn.xcp.ucontext_buffer, tcb->cmn.start, argc,
Review comment:
Then this change must NOT be merged into master. The integrity of the OS modularity is far, far, far most important than your change. The is no technical argument on the face of planet that is sufficient to violate those modularity principles. There is no new feature sufficiently importnat to violate those principles.
THIS MUST NOT BE MERGED. IT VIOLATES CORE ARCHITECTURAL PRINCIPLES OF THE OS. I will chnage the PR to a draft so that it is not accidentally merged.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-686239132
> > @sebastianene07 you can rebase this patch now, I have an idea which may avoid to touch the common code and will try in the next couple day.
>
> Thanks for the update, I figured it out and we don't have to modify the common code to pass arguments to new tasks. The entry point for a new task is `nxtask_start` and this function takes no arguments. It grabs the arguments (argc and argv) from the TCB and calls the `tcb->cmn.entry.main` with them. So the mechanism to pass the arguments is very generic and works like a charm in our case without any modification.
Yes, this is exactly what I am thinking!
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680096349
> This change MUST not come into master UNLESS all simulator specific changes outside of arch/sim and boards/sime are removed. The common OS must not be contaminated with platform-specific code.
I agree.
The platform independent logic is supposed to remain platform independent!!
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r473016370
##########
File path: sched/task/task_setup.c
##########
@@ -593,6 +598,11 @@ static inline int nxtask_setup_stackargs(FAR struct task_tcb_s *tcb,
stackargv[argc + 1] = NULL;
tcb->argv = stackargv;
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ up_setup_args_context(tcb->cmn.xcp.ucontext_buffer, tcb->cmn.start, argc,
Review comment:
it isn't good to modify the common code for the arch specific need.
##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -1,47 +1,251 @@
/****************************************************************************
* arch/sim/src/sim/up_hosttime.c
*
- * Copyright (C) 2008 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <nu...@fitbit.com>
Review comment:
Remove the author, the new apache license shouldn't contain the author.
##########
File path: arch/sim/src/sim/up_blocktask.c
##########
@@ -82,7 +82,7 @@ void up_block_task(struct tcb_s *tcb, tstate_t task_state)
DEBUGASSERT((tcb->task_state >= FIRST_READY_TO_RUN_STATE) &&
(tcb->task_state <= LAST_READY_TO_RUN_STATE));
- /* sinfo("Blocking TCB=%p\n", tcb); */
+ sinfo("Blocking TCB=%p %s\n", tcb, tcb->name);
Review comment:
tcb->name doesn't always exist.
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
+ifeq ($(CONFIG_HOST_MACOS), y)
+ CFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
Review comment:
remvoe CFLAGS change, only HOSTCFLAGS need to update
##########
File path: arch/sim/src/sim/up_initialstate.c
##########
@@ -66,7 +68,13 @@
void up_initial_state(struct tcb_s *tcb)
{
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ FAR struct tcb_s *rtcb = this_task();
+ tcb->xcp.ucontext_buffer = up_create_context(&tcb->xcp.ucontext_sp,
+ rtcb->xcp.ucontext_buffer, tcb->start);
Review comment:
rtcb may exit before tcb, it isn't safe to set tcb's uc_link to rtcb's uncontext
##########
File path: arch/sim/src/sim/up_initialstate.c
##########
@@ -39,11 +39,13 @@
#include <nuttx/config.h>
+#include <setjmp.h>
Review comment:
why need include setjmp?
##########
File path: arch/sim/src/sim/up_ucontext.c
##########
@@ -0,0 +1,363 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_ucontext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <assert.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <ucontext.h>
+
+/****************************************************************************
+ * Preprocessor Definition
+ ****************************************************************************/
+
+/* The minimum ucontext size */
+
+#define MIN_UCONTEXT_STACK_LENGTH (128 * 1024)
+#define STACK_ALIGNMENT_BYTES (8)
+#define ASSERT(x) assert((x))
+#define ARRAY_LENGTH(array) (sizeof((array))/sizeof((array)[0]))
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+typedef struct exit_data_context_s
+{
+ ucontext_t *next_ucontext;
+ ucontext_t *current_ucontext;
+ void *ucontext_sp;
+ int cpu_index;
+} exit_data_context_t;
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static ucontext_t g_uctx_main;
+
+#ifdef CONFIG_SMP
+static exit_data_context_t g_task_exit_details[CONFIG_SMP_NCPUS];
+static ucontext_t g_task_exit_context[CONFIG_SMP_NCPUS];
+#else
+static exit_data_context_t g_task_exit_details[1];
+static ucontext_t g_task_exit_context[1];
+#endif
+
+static volatile bool g_is_exit_context_init;
+static stack_t g_ss;
+
+/****************************************************************************
+ * Public Prototype
+ ****************************************************************************/
+
+#ifdef CONFIG_SMP
+int up_cpu_index(void);
+#endif
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: task_exit_point
+ *
+ * Description:
+ * This function is used to release the allocated stack memory for a task
+ * on it's exit path.
+ *
+ * Assumptions:
+ * This function does not return.
+ *
+ ****************************************************************************/
+
+static void task_exit_point(void)
+{
+ while (1)
+ {
+ int cpu_id = 0;
+
+#ifdef CONFIG_SMP
+ cpu_id = up_cpu_index();
+#endif
+ getcontext(&g_task_exit_context[cpu_id]);
+
+ if (g_task_exit_details[cpu_id].ucontext_sp)
+ {
+ free(g_task_exit_details[cpu_id].current_ucontext);
+ free(g_task_exit_details[cpu_id].ucontext_sp);
+ g_task_exit_details[cpu_id].ucontext_sp = NULL;
+ setcontext(g_task_exit_details[cpu_id].next_ucontext);
+ }
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_exit_context
+ *
+ * Description:
+ * Creates new exit context for tasks. In a SMP configuration every CPU
+ * has a different exit context.
+ *
+ ****************************************************************************/
+
+static void up_setup_exit_context(void)
+{
+ for (int i = 0; i < ARRAY_LENGTH(g_task_exit_context); i++)
+ {
+ ucontext_t *exit_context = &g_task_exit_context[i];
+ int ret = getcontext(exit_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ exit_context->uc_stack.ss_sp = aligned_stack;
+ exit_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH -
+ align_offset;
+ exit_context->uc_link = &g_uctx_main;
+
+ makecontext(exit_context, task_exit_point, 0);
+ }
+
+ g_is_exit_context_init = true;
+}
+
+/****************************************************************************
+ * Name: up_is_stack_downwards
+ *
+ * Description:
+ * This function returns true if the stack places elements downwards.
+ *
+ * Input Parameters:
+ * stack_arg - the address of a variable from the previous function stack
+ * frame
+ *
+ * Returned Value:
+ * True if the stack grows downwards otherwise false.
+ *
+ ****************************************************************************/
+
+static bool up_is_stack_downwards(void *stack_arg)
+{
+ int stack_arg_local;
+ return stack_arg > (void *)&stack_arg_local;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_create_context
+ *
+ * Description:
+ * Creates a new ucontext structure and initialize it.
+ *
+ * Input Parameters:
+ * ucontext_sp - buffer where we store the ucontext stack pointer
+ * prev_ucontext - the parent ucontext used%i to return to when the
+ * execution of the current context ends.
+ * entry_point - the entry point of the new context
+ *
+ * Return Value:
+ * A pointer to the ucontext structure or NULL in case something went
+ * wrong.
+ *
+ ****************************************************************************/
+
+void *up_create_context(void **ucontext_sp, void *prev_ucontext,
+ void (*entry_point)(void))
+{
+ ucontext_t *new_context = calloc(1, sizeof(ucontext_t));
+ ASSERT(new_context != NULL);
+
+ int ret = getcontext(new_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ *((uintptr_t *)ucontext_sp) = (uintptr_t)sp;
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ new_context->uc_stack.ss_sp = aligned_stack;
+ new_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH - align_offset;
+ new_context->uc_link = prev_ucontext == NULL ? &g_uctx_main :
+ prev_ucontext;
+
+ makecontext(new_context, entry_point, 0);
+
+ if (!g_is_exit_context_init)
+ up_setup_exit_context();
+
+ return new_context;
+}
+
+/****************************************************************************
+ * Name: up_destroy_context
+ *
+ * Description:
+ * This function releases the host memory for the context structure and the
+ * context stack and jumps to the next context specified as argument.
+ *
+ * Input Parameters:
+ * current_ucontext - pointer to the TCB context
+ * ucontext_sp - pointer to the ucontext stack
+ * new_ucontext - the next context that we want to jump to
+ * cpu_id - the CPU identificator
+ *
+ * Assumptions:
+ * This function does not return. In order to free the stack memory that we
+ * are currently executing on, we jump to a new exit context.
+ *
+ ****************************************************************************/
+
+void up_destroy_context(void *current_ucontext, void *ucontext_sp,
+ void *next_ucontext, int cpu_id)
+{
+ g_task_exit_details[cpu_id].next_ucontext = next_ucontext;
+ g_task_exit_details[cpu_id].current_ucontext = current_ucontext;
+ g_task_exit_details[cpu_id].cpu_index = cpu_id;
+ g_task_exit_details[cpu_id].ucontext_sp = ucontext_sp;
+
+ setcontext(&g_task_exit_context[cpu_id]);
+}
+
+/****************************************************************************
+ * Name: up_setup_args_context
+ *
+ * Description:
+ * This function specifies the entry point for the new context and sets up
+ * the arguments for the new task.
+ *
+ * Input Parameters:
+ * ucontext - pointer to the TCB context
+ * entry_point - the entry point for the task
+ * argc - the number of cmd arguments
+ * argv - array of arguments for the new task
+ *
+ ****************************************************************************/
+
+void up_setup_args_context(void *current_ucontext, void (*entry_point)(void),
+ int argc, char *argv[])
+{
+ makecontext(current_ucontext, entry_point, 2, argc, argv);
+}
+
+/****************************************************************************
+ * Name: up_swap_context
+ *
+ * Description:
+ * Save the current context in the old_ucontext and activate the
+ * context from activate_ucontext.
+ *
+ * Input Parameters:
+ * old_ucontext - place where we store the current context
+ * activate_ucontext - context that we will activate after function
+ * invocation.
+ *
+ ****************************************************************************/
+
+void up_swap_context(void *old_ucontext, void *activate_ucontext)
+{
+ swapcontext(old_ucontext, activate_ucontext);
+}
+
+/****************************************************************************
+ * Name: up_set_context
+ *
+ * Description:
+ * Set the current context to the specified ucontext
+ *
+ * Input Parameters:
+ * current_context - the current context
+ *
+ ****************************************************************************/
+
+void up_set_context(void *current_context)
+{
+ setcontext(current_context);
+}
+
+/****************************************************************************
+ * Name: up_cpu_simulated_interrupt
+ *
+ * Description:
+ * This function verifies if we are running on the signal handler stack.
+ *
+ * Returned Value:
+ * Return zero if we are not running on the signal stack otherwise return
+ * a non zero value.
+ *
+ ****************************************************************************/
+
+int up_cpu_simulated_interrupt(void)
+{
+ uint8_t test_address;
+ unsigned long verify_stack_arg = (unsigned long)&test_address;
+ unsigned long stack_start = (unsigned long)g_ss.ss_sp;
+ unsigned long stack_end;
+
+ if (up_is_stack_downwards(&verify_stack_arg) == true)
+ {
+ stack_end = stack_start - g_ss.ss_size;
+ return stack_start > verify_stack_arg && stack_end < verify_stack_arg;
+ }
+ else
+ {
+ stack_end = stack_start + g_ss.ss_size;
+ return stack_start < verify_stack_arg && stack_end < verify_stack_arg;
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_sigstackt
+ *
+ * Description:
+ * This function sets up an alternative stack for the signals from the
+ * host heap memory.
+ *
+ ****************************************************************************/
+
+void up_setup_sigstack(void)
+{
+ g_ss.ss_sp = malloc(SIGSTKSZ);
+ ASSERT(g_ss.ss_sp != NULL);
+
+ g_ss.ss_size = SIGSTKSZ;
+ g_ss.ss_flags = 0;
+
+ int ret = sigaltstack(&g_ss, NULL);
Review comment:
how to handle smp thread signal stack?
##########
File path: arch/sim/src/sim/up_oneshot.c
##########
@@ -357,6 +359,17 @@ 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
+ host_prepare_timer();
+ host_init_timer(up_timer_update);
Review comment:
Why not merge host_prepare_timer into host_init_timer?
##########
File path: arch/sim/include/irq.h
##########
@@ -82,11 +78,15 @@ typedef int xcpt_reg_t;
struct xcptcontext
{
- void *sigdeliver; /* Actual type is sig_deliver_t */
+ void *sigdeliver;
+
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ void *ucontext_buffer; /* Actual type is ucontext_t */
+ void *ucontext_sp;
+#endif
xcpt_reg_t regs[XCPTCONTEXT_REGS];
Review comment:
Should we change to:
```
#ifdef CONFIG_SIM_PREEMPTIBLE
void *ucontext_buffer; /* Actual type is ucontext_t */
void *ucontext_sp;
#else
xcpt_reg_t regs[XCPTCONTEXT_REGS];
#endif
```
since regs isn't used when CONFIG_SIM_PREEMPTIBLE == y.
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
Review comment:
Why need pass CONFIG_SIM_PREEMPTIBLE? the code in up_ucontext.c should know that CONFIG_SIM_PREEMPTIBLE is true.
##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -1,47 +1,251 @@
/****************************************************************************
* arch/sim/src/sim/up_hosttime.c
*
- * Copyright (C) 2008 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <nu...@fitbit.com>
*
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
*
- * 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.
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * 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.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
*
****************************************************************************/
/****************************************************************************
* Included Files
****************************************************************************/
+#include <signal.h>
+#include <stdarg.h>
#include <stdbool.h>
+#include <stdio.h>
#include <stdint.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
#include <time.h>
#include <unistd.h>
+/****************************************************************************
+ * 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_1_SIGUSR1 = 1,
Review comment:
The irq code should be generic to mask/restore all possible signals.
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
+ifeq ($(CONFIG_HOST_MACOS), y)
+ CFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
+ HOSTCFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
Review comment:
_XOPEN_SOURCE is normally defined in source code, please move to up_ucontext.c
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
+ifeq ($(CONFIG_HOST_MACOS), y)
+ CFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
+ HOSTCFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
+ HOSTCFLAGS += -pthread
Review comment:
why need -pthread?
##########
File path: arch/sim/src/sim/up_blocktask.c
##########
@@ -118,14 +118,18 @@ 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
+ FAR struct tcb_s *prev_tcb = rtcb;
+#else
if (!up_setjmp(rtcb->xcp.regs))
+#endif
{
/* Restore the exception context of the rtcb at the (new) head
* of the ready-to-run task list.
*/
rtcb = this_task();
- sinfo("New Active Task TCB=%p\n", rtcb);
+ sinfo("New Active Task TCB=%p %s\n", rtcb, rtcb->name);
Review comment:
tcb->name doesn't always exist.
##########
File path: arch/sim/src/sim/up_exit.c
##########
@@ -65,9 +65,13 @@
void up_exit(int status)
{
- FAR struct tcb_s *tcb;
+ FAR struct tcb_s *tcb = this_task();
- sinfo("TCB=%p exiting\n", this_task());
+ sinfo("TCB=%p exiting\n", tcb);
+
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ FAR struct tcb_s *prev_tcb = tcb;
Review comment:
variable declaration need to move to the beginning of function
##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -89,3 +293,107 @@ void host_sleepuntil(uint64_t nsec)
usleep((nsec - now) / 1000);
}
}
+
+#ifdef CONFIG_SIM_PREEMPTIBLE
+/****************************************************************************
+ * Name: host_prepare_timer
+ *
+ * Description:
+ * Blocks all the signals in the current host 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 host_prepare_timer(void)
+{
+ sigset_t signal_mask;
+ sigemptyset(&signal_mask);
+
+ return pthread_sigmask(SIG_SETMASK, &signal_mask, NULL);
+}
+
+/****************************************************************************
+ * Name: host_init_timer
+ *
+ * Description:
+ * Creates a periodic timer and sets up a signal handler on the host.
+ *
+ * Input Parameters:
+ * nxsched_process_timer - the NuttX scheduler callback
+ *
+ * Returned Value:
+ * This function returns 0 on success otherwise a negative error code.
+ *
+ ****************************************************************************/
+
+int host_init_timer(sched_timer_callback_t sched_cb)
+{
+ g_sched_process_timer_cb = sched_cb;
+
+ int ret = host_setup_signals(host_signal_handler);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ ret = host_setup_timer();
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: host_signal_save
+ *
+ * Description:
+ * Block signals and return the previous signal mask.
+ *
+ * Returned Value:
+ * This function returns the list of active signals before blocking them.
+ *
+ ****************************************************************************/
+
+uint32_t host_signal_save(void)
+{
+ sigset_t oldset, newset;
+ pthread_sigmask(0, NULL, &oldset);
+
+ /* Disable signals */
+
+ sigfillset(&newset);
+ pthread_sigmask(SIG_BLOCK, &newset, NULL);
Review comment:
change to:
pthread_sigmask(SIG_BLOCK, &newset, &oldset);
and remove line 370
##########
File path: arch/sim/src/sim/up_idle.c
##########
@@ -89,6 +89,14 @@ void up_idle(void)
}
#endif
+#ifndef CONFIG_SIM_PREEMPTIBLE
Review comment:
let' move the guard into up_timer_update, so we don't need change up_idle.c
##########
File path: arch/sim/src/sim/up_unblocktask.c
##########
@@ -78,7 +78,7 @@ void up_unblock_task(FAR struct tcb_s *tcb)
DEBUGASSERT((tcb->task_state >= FIRST_BLOCKED_STATE) &&
(tcb->task_state <= LAST_BLOCKED_STATE));
- sinfo("Unblocking TCB=%p\n", tcb);
+ sinfo("Unblocking TCB=%p %s\n", tcb, tcb->name);
Review comment:
name mayn't exit
##########
File path: arch/sim/src/sim/up_interruptcontext.c
##########
@@ -43,10 +43,49 @@
#include <nuttx/arch.h>
#include "up_internal.h"
+#ifdef CONFIG_SIM_PREEMPTIBLE
+uint32_t host_signal_save(void);
+void host_signal_restore(uint32_t flags);
+int up_cpu_simulated_interrupt(void);
+#else
+# define host_signal_save() 0
+# define host_signal_restore(f) (void)(f)
+# define up_cpu_simulated_interrupt() 0
+#endif
+
/****************************************************************************
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: up_irq_save
+ *
+ * Description:
+ * Disable interrupts and returned the mask before disabling them.
+ *
+ ****************************************************************************/
+
+irqstate_t up_irq_save(void)
+{
+ return host_signal_save();
Review comment:
SMP simulate IPI by signal, so we need disable/restore signal if CONFIG_SMP equals y too.
##########
File path: arch/sim/src/sim/up_unblocktask.c
##########
@@ -101,15 +101,19 @@ void up_unblock_task(FAR struct tcb_s *tcb)
* this is really the previously running task restarting!
*/
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ FAR struct tcb_s *prev_tcb = rtcb;
+#else
if (!up_setjmp(rtcb->xcp.regs))
+#endif
{
/* Restore the exception context of the new task that is ready to
* run (probably tcb). This is the new rtcb at the head of the
* ready-to-run task list.
*/
rtcb = this_task();
- sinfo("New Active Task TCB=%p\n", rtcb);
+ sinfo("New Active Task TCB=%p %s\n", rtcb, rtcb->name);
Review comment:
name mayn't exist
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-683309661
The patch not related to ucontext API is moved to here: https://github.com/apache/incubator-nuttx/pull/1663
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477286777
##########
File path: arch/sim/src/sim/up_unblocktask.c
##########
@@ -78,7 +78,7 @@ void up_unblock_task(FAR struct tcb_s *tcb)
DEBUGASSERT((tcb->task_state >= FIRST_BLOCKED_STATE) &&
(tcb->task_state <= LAST_BLOCKED_STATE));
- sinfo("Unblocking TCB=%p\n", tcb);
+ sinfo("Unblocking TCB=%p %s\n", tcb, tcb->name);
Review comment:
I will remove the name from all of the places where I added it
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477316055
##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -1,47 +1,251 @@
/****************************************************************************
* arch/sim/src/sim/up_hosttime.c
*
- * Copyright (C) 2008 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <nu...@fitbit.com>
*
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
*
- * 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.
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * 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.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
*
****************************************************************************/
/****************************************************************************
* Included Files
****************************************************************************/
+#include <signal.h>
+#include <stdarg.h>
#include <stdbool.h>
+#include <stdio.h>
#include <stdint.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
#include <time.h>
#include <unistd.h>
+/****************************************************************************
+ * 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_1_SIGUSR1 = 1,
Review comment:
Actually, I already split irq function from the patch and make it generic several months ago:
https://github.com/xiaoxiang781216/incubator-nuttx/commit/38fcaa6a9cb4893c090d9830a305d4bb209b4b35
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477407596
##########
File path: arch/sim/src/sim/up_initialstate.c
##########
@@ -39,11 +39,13 @@
#include <nuttx/config.h>
+#include <setjmp.h>
Review comment:
I think this can be removed we don't need it here
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680129897
There are some startup hooks that might be usable for such a purpose with some extensions??? sched/task/task_starthook.c
The the start hooks are not currently set up in a way that would be usable to you because it really only works if the TCB has been created (via nxtask_init()) but not yet started (via nxtask_activate()).
It is currently using only in binfmt/binfmt_execmodule.c when an ELF task is created in order to run the C++ static constuctors (This is a documented security Issue #1265 for this).
Hmm...
I think it would be fine to implement a abstracted, generalized, platform-specific hook in sched/task/task_init.c or maybe nxtask_activate.c. It would be wrapped and a general external task startup hook:
* It would have a name like up_start_hook()
* It would be prototyped in include/nuttx/arch.h
* There would be a configuration in sched/Kconfig like ARCH_STARTHOOK
That should work if I understand what you need correctly.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 closed pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 closed pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680129897
There are some startup hooks that might be usable for such a purpose with some extensions??? sched/task/task_starthook.c
The the start hooks are not currently set up in a way that would be usable to you because it really only works if the TCB has been created (via nxtask_init()) but not yet started (via nxtask_activate()).
It is currently using only in binfmt/binfmt_execmodule.c when an ELF task is created in order to run the C++ static constuctors (This is a documented security Issue #1265 for this).
Hmm...
I think it would be fine to implement a abstracted, generalized, platform-specific hook in sched/task/task_init.c or maybe nxtask_activate.c. It would be wrapped and a general external task startup hook:
* It would have a name like up_start_hook()
* It would be prototyped in include/nuttx/arch.h
* There would be a configuration in sched/Kconfig like ARCH_STARTHOOK
This hook would look something like:
#ifdef CONFIG_ARCH_STARTHOOK
up_start_hook(FAR struct task_tcb_s *tcb);
#endif
That should work if I understand what you need correctly. I presume that such a hook would not be needed for pthreads.
I think that such hooks are not 100% desirable, but this would prevent a direct coupling to the SIM architecture since it is a startup feature that could be exploited by any architecture.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680129897
There are some startup hooks that might be usable for such a purpose with some extensions??? sched/task/task_starthook.c
The the start hooks are not currently set up in a way that would be usable to you because it really only works if the TCB has been created (via nxtask_init()) but not yet started (via nxtask_activate()).
It is currently using only in binfmt/binfmt_execmodule.c when an ELF task is created in order to run the C++ static constuctors (This is a documented security Issue #1265 for this).
Hmm...
I think it would be fine to implement a abstracted, generalized, platform-specific hook in sched/task/task_init.c or maybe nxtask_activate.c before the task has started or more likely in task_start.c after the task has been started.. It would be wrapped and a general external task startup hook:
* It would have a name like up_start_hook()
* It would be prototyped in include/nuttx/arch.h
* There would be a configuration in sched/Kconfig like ARCH_STARTHOOK
This hook would look something like:
#ifdef CONFIG_ARCH_STARTHOOK
up_start_hook(FAR struct task_tcb_s *tcb);
#endif
That should work if I understand what you need correctly. I presume that such a hook would not be needed for pthreads.
I think that such hooks are not 100% desirable, but this would prevent a direct coupling to the SIM architecture since it is a startup feature that could be exploited by any architecture.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477332002
##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -1,47 +1,251 @@
/****************************************************************************
* arch/sim/src/sim/up_hosttime.c
*
- * Copyright (C) 2008 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <nu...@fitbit.com>
*
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
*
- * 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.
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * 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.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
*
****************************************************************************/
/****************************************************************************
* Included Files
****************************************************************************/
+#include <signal.h>
+#include <stdarg.h>
#include <stdbool.h>
+#include <stdio.h>
#include <stdint.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
#include <time.h>
#include <unistd.h>
+/****************************************************************************
+ * 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_1_SIGUSR1 = 1,
Review comment:
Cool, thanks
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680841255
> the previous comment would work better for you and is very simple. It is really the same solution that you have originally
> > In this way we don't have to modify the platform independent code but it adds additional complexity. What do you think ?
>
> I think my suggestion in the previous comment would work better for you and is very simple. It is really the same solution that you have originally except that it is dressed up to look like a platform independent feature. The decoupling here is purely semantic but, I think, acceptable.
>
> There are conditional hooks throughout the OS: start hooks, termination hooks, timer hooks, etc. So, although not a thing of beauty, I think that is consistent with how other sticky issues have been addressed.
Thanks for the suggestion I will update my patch.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477414325
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
+ifeq ($(CONFIG_HOST_MACOS), y)
+ CFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
+ HOSTCFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
Review comment:
I will remove it
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
+ifeq ($(CONFIG_HOST_MACOS), y)
+ CFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
Review comment:
I removed it
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r478119730
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
+ifeq ($(CONFIG_HOST_MACOS), y)
+ CFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
+ HOSTCFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
+ HOSTCFLAGS += -pthread
Review comment:
I saw OSX requires it to link correctly otherwise I'll get an error
##########
File path: arch/sim/src/sim/up_blocktask.c
##########
@@ -82,7 +82,7 @@ void up_block_task(struct tcb_s *tcb, tstate_t task_state)
DEBUGASSERT((tcb->task_state >= FIRST_READY_TO_RUN_STATE) &&
(tcb->task_state <= LAST_READY_TO_RUN_STATE));
- /* sinfo("Blocking TCB=%p\n", tcb); */
+ sinfo("Blocking TCB=%p %s\n", tcb, tcb->name);
Review comment:
I removed it
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r475088753
##########
File path: sched/task/task_setup.c
##########
@@ -593,6 +598,11 @@ static inline int nxtask_setup_stackargs(FAR struct task_tcb_s *tcb,
stackargv[argc + 1] = NULL;
tcb->argv = stackargv;
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ up_setup_args_context(tcb->cmn.xcp.ucontext_buffer, tcb->cmn.start, argc,
Review comment:
>
>
> it isn't good to modify the common code for the arch specific need.
Let me emphasize how important this is. There are modularity rules to keep NuttX from degrading into spaghetti code. This violates the core principle of the modular design. There must be NO sim-specific logic outside of the arch/srm and boards/sim "silos" (see https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=139629399). The common OS MUST be completely independent of all architectures.
Can you imagine what a mess it would be if everyone put platform-specific code throughout of the OS. It would be a miserable mess. We need to absolutely prohibit any violations of the modular architecture to keep it from falling into spaghetti code hell.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477206014
##########
File path: arch/sim/include/irq.h
##########
@@ -82,11 +78,15 @@ typedef int xcpt_reg_t;
struct xcptcontext
{
- void *sigdeliver; /* Actual type is sig_deliver_t */
+ void *sigdeliver;
+
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ void *ucontext_buffer; /* Actual type is ucontext_t */
+ void *ucontext_sp;
+#endif
xcpt_reg_t regs[XCPTCONTEXT_REGS];
Review comment:
If I do this I also need to guard the `xcp.regs `access with a negative gating CONFIG_SIM_PREEMPTIBLE from `up_stackframe.c` line 124. Is it ok with you ?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680113203
> solution and would not advocate a hack just to avoid the SIM dependency. We need to do things right. There are not comments added to the code to explain why you thought this was necessary. So it is hard to suggest alternatives. Comments are good! Comments are y
Thanks for considering an alternative solution and reviewing my code. I was thinking of something but I'm not sure if it's going to be accepted:
- jump to an alternative entry_point that fetches the arguments from the TCB and calls `makecontext` to setup the task arguments
In this way we don't have to modify the platform independent code but it adds additional complexity. What do you think ?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477302490
##########
File path: arch/sim/src/sim/up_interruptcontext.c
##########
@@ -43,10 +43,49 @@
#include <nuttx/arch.h>
#include "up_internal.h"
+#ifdef CONFIG_SIM_PREEMPTIBLE
+uint32_t host_signal_save(void);
+void host_signal_restore(uint32_t flags);
+int up_cpu_simulated_interrupt(void);
+#else
+# define host_signal_save() 0
+# define host_signal_restore(f) (void)(f)
+# define up_cpu_simulated_interrupt() 0
+#endif
+
/****************************************************************************
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: up_irq_save
+ *
+ * Description:
+ * Disable interrupts and returned the mask before disabling them.
+ *
+ ****************************************************************************/
+
+irqstate_t up_irq_save(void)
+{
+ return host_signal_save();
Review comment:
Yes, it's better to move up_irq_save/up_irq_restore to another patch.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 edited a comment on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 edited a comment on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680113203
> solution and would not advocate a hack just to avoid the SIM dependency. We need to do things right. There are not comments added to the code to explain why you thought this was necessary. So it is hard to suggest alternatives. Comments are good! Comments are y
Thanks for considering an alternative solution and reviewing my code. I was thinking of something but I'm not sure if it's going to be accepted:
- jump to an alternative entry_point that fetches the arguments from the TCB and calls `makecontext` to setup the task arguments in the `ucontext` manner
In this way we don't have to modify the platform independent code but it adds additional complexity. What do you think ?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 closed pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 closed pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r479412711
##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -1,47 +1,251 @@
/****************************************************************************
* arch/sim/src/sim/up_hosttime.c
*
- * Copyright (C) 2008 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <nu...@fitbit.com>
Review comment:
I replaced the license
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477285445
##########
File path: arch/sim/src/sim/up_interruptcontext.c
##########
@@ -43,10 +43,49 @@
#include <nuttx/arch.h>
#include "up_internal.h"
+#ifdef CONFIG_SIM_PREEMPTIBLE
+uint32_t host_signal_save(void);
+void host_signal_restore(uint32_t flags);
+int up_cpu_simulated_interrupt(void);
+#else
+# define host_signal_save() 0
+# define host_signal_restore(f) (void)(f)
+# define up_cpu_simulated_interrupt() 0
+#endif
+
/****************************************************************************
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: up_irq_save
+ *
+ * Description:
+ * Disable interrupts and returned the mask before disabling them.
+ *
+ ****************************************************************************/
+
+irqstate_t up_irq_save(void)
+{
+ return host_signal_save();
Review comment:
That's correct, I'm thinking of moving the up_irq_save/up_irq_restore implementation to a separate patch as it can be decoupled from this large change
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r478868577
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
Review comment:
I need it in `up_hosttime.c` to guard `host_init_timer` method
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r478119730
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
+ifeq ($(CONFIG_HOST_MACOS), y)
+ CFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
+ HOSTCFLAGS += -D_XOPEN_SOURCE -Wno-deprecated-declarations
+ HOSTCFLAGS += -pthread
Review comment:
I saw OSX requires it to link correctly otherwise I'll get a compilation error
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477288973
##########
File path: arch/sim/src/sim/up_ucontext.c
##########
@@ -0,0 +1,363 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_ucontext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <assert.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <ucontext.h>
+
+/****************************************************************************
+ * Preprocessor Definition
+ ****************************************************************************/
+
+/* The minimum ucontext size */
+
+#define MIN_UCONTEXT_STACK_LENGTH (128 * 1024)
+#define STACK_ALIGNMENT_BYTES (8)
+#define ASSERT(x) assert((x))
+#define ARRAY_LENGTH(array) (sizeof((array))/sizeof((array)[0]))
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+typedef struct exit_data_context_s
+{
+ ucontext_t *next_ucontext;
+ ucontext_t *current_ucontext;
+ void *ucontext_sp;
+ int cpu_index;
+} exit_data_context_t;
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static ucontext_t g_uctx_main;
+
+#ifdef CONFIG_SMP
+static exit_data_context_t g_task_exit_details[CONFIG_SMP_NCPUS];
+static ucontext_t g_task_exit_context[CONFIG_SMP_NCPUS];
+#else
+static exit_data_context_t g_task_exit_details[1];
+static ucontext_t g_task_exit_context[1];
+#endif
+
+static volatile bool g_is_exit_context_init;
+static stack_t g_ss;
+
+/****************************************************************************
+ * Public Prototype
+ ****************************************************************************/
+
+#ifdef CONFIG_SMP
+int up_cpu_index(void);
+#endif
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: task_exit_point
+ *
+ * Description:
+ * This function is used to release the allocated stack memory for a task
+ * on it's exit path.
+ *
+ * Assumptions:
+ * This function does not return.
+ *
+ ****************************************************************************/
+
+static void task_exit_point(void)
+{
+ while (1)
+ {
+ int cpu_id = 0;
+
+#ifdef CONFIG_SMP
+ cpu_id = up_cpu_index();
+#endif
+ getcontext(&g_task_exit_context[cpu_id]);
+
+ if (g_task_exit_details[cpu_id].ucontext_sp)
+ {
+ free(g_task_exit_details[cpu_id].current_ucontext);
+ free(g_task_exit_details[cpu_id].ucontext_sp);
+ g_task_exit_details[cpu_id].ucontext_sp = NULL;
+ setcontext(g_task_exit_details[cpu_id].next_ucontext);
+ }
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_exit_context
+ *
+ * Description:
+ * Creates new exit context for tasks. In a SMP configuration every CPU
+ * has a different exit context.
+ *
+ ****************************************************************************/
+
+static void up_setup_exit_context(void)
+{
+ for (int i = 0; i < ARRAY_LENGTH(g_task_exit_context); i++)
+ {
+ ucontext_t *exit_context = &g_task_exit_context[i];
+ int ret = getcontext(exit_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ exit_context->uc_stack.ss_sp = aligned_stack;
+ exit_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH -
+ align_offset;
+ exit_context->uc_link = &g_uctx_main;
+
+ makecontext(exit_context, task_exit_point, 0);
+ }
+
+ g_is_exit_context_init = true;
+}
+
+/****************************************************************************
+ * Name: up_is_stack_downwards
+ *
+ * Description:
+ * This function returns true if the stack places elements downwards.
+ *
+ * Input Parameters:
+ * stack_arg - the address of a variable from the previous function stack
+ * frame
+ *
+ * Returned Value:
+ * True if the stack grows downwards otherwise false.
+ *
+ ****************************************************************************/
+
+static bool up_is_stack_downwards(void *stack_arg)
+{
+ int stack_arg_local;
+ return stack_arg > (void *)&stack_arg_local;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_create_context
+ *
+ * Description:
+ * Creates a new ucontext structure and initialize it.
+ *
+ * Input Parameters:
+ * ucontext_sp - buffer where we store the ucontext stack pointer
+ * prev_ucontext - the parent ucontext used%i to return to when the
+ * execution of the current context ends.
+ * entry_point - the entry point of the new context
+ *
+ * Return Value:
+ * A pointer to the ucontext structure or NULL in case something went
+ * wrong.
+ *
+ ****************************************************************************/
+
+void *up_create_context(void **ucontext_sp, void *prev_ucontext,
+ void (*entry_point)(void))
+{
+ ucontext_t *new_context = calloc(1, sizeof(ucontext_t));
+ ASSERT(new_context != NULL);
+
+ int ret = getcontext(new_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ *((uintptr_t *)ucontext_sp) = (uintptr_t)sp;
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ new_context->uc_stack.ss_sp = aligned_stack;
+ new_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH - align_offset;
+ new_context->uc_link = prev_ucontext == NULL ? &g_uctx_main :
+ prev_ucontext;
+
+ makecontext(new_context, entry_point, 0);
+
+ if (!g_is_exit_context_init)
+ up_setup_exit_context();
+
+ return new_context;
+}
+
+/****************************************************************************
+ * Name: up_destroy_context
+ *
+ * Description:
+ * This function releases the host memory for the context structure and the
+ * context stack and jumps to the next context specified as argument.
+ *
+ * Input Parameters:
+ * current_ucontext - pointer to the TCB context
+ * ucontext_sp - pointer to the ucontext stack
+ * new_ucontext - the next context that we want to jump to
+ * cpu_id - the CPU identificator
+ *
+ * Assumptions:
+ * This function does not return. In order to free the stack memory that we
+ * are currently executing on, we jump to a new exit context.
+ *
+ ****************************************************************************/
+
+void up_destroy_context(void *current_ucontext, void *ucontext_sp,
+ void *next_ucontext, int cpu_id)
+{
+ g_task_exit_details[cpu_id].next_ucontext = next_ucontext;
+ g_task_exit_details[cpu_id].current_ucontext = current_ucontext;
+ g_task_exit_details[cpu_id].cpu_index = cpu_id;
+ g_task_exit_details[cpu_id].ucontext_sp = ucontext_sp;
+
+ setcontext(&g_task_exit_context[cpu_id]);
+}
+
+/****************************************************************************
+ * Name: up_setup_args_context
+ *
+ * Description:
+ * This function specifies the entry point for the new context and sets up
+ * the arguments for the new task.
+ *
+ * Input Parameters:
+ * ucontext - pointer to the TCB context
+ * entry_point - the entry point for the task
+ * argc - the number of cmd arguments
+ * argv - array of arguments for the new task
+ *
+ ****************************************************************************/
+
+void up_setup_args_context(void *current_ucontext, void (*entry_point)(void),
+ int argc, char *argv[])
+{
+ makecontext(current_ucontext, entry_point, 2, argc, argv);
+}
+
+/****************************************************************************
+ * Name: up_swap_context
+ *
+ * Description:
+ * Save the current context in the old_ucontext and activate the
+ * context from activate_ucontext.
+ *
+ * Input Parameters:
+ * old_ucontext - place where we store the current context
+ * activate_ucontext - context that we will activate after function
+ * invocation.
+ *
+ ****************************************************************************/
+
+void up_swap_context(void *old_ucontext, void *activate_ucontext)
+{
+ swapcontext(old_ucontext, activate_ucontext);
+}
+
+/****************************************************************************
+ * Name: up_set_context
+ *
+ * Description:
+ * Set the current context to the specified ucontext
+ *
+ * Input Parameters:
+ * current_context - the current context
+ *
+ ****************************************************************************/
+
+void up_set_context(void *current_context)
+{
+ setcontext(current_context);
+}
+
+/****************************************************************************
+ * Name: up_cpu_simulated_interrupt
+ *
+ * Description:
+ * This function verifies if we are running on the signal handler stack.
+ *
+ * Returned Value:
+ * Return zero if we are not running on the signal stack otherwise return
+ * a non zero value.
+ *
+ ****************************************************************************/
+
+int up_cpu_simulated_interrupt(void)
+{
+ uint8_t test_address;
+ unsigned long verify_stack_arg = (unsigned long)&test_address;
+ unsigned long stack_start = (unsigned long)g_ss.ss_sp;
+ unsigned long stack_end;
+
+ if (up_is_stack_downwards(&verify_stack_arg) == true)
+ {
+ stack_end = stack_start - g_ss.ss_size;
+ return stack_start > verify_stack_arg && stack_end < verify_stack_arg;
+ }
+ else
+ {
+ stack_end = stack_start + g_ss.ss_size;
+ return stack_start < verify_stack_arg && stack_end < verify_stack_arg;
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_sigstackt
+ *
+ * Description:
+ * This function sets up an alternative stack for the signals from the
+ * host heap memory.
+ *
+ ****************************************************************************/
+
+void up_setup_sigstack(void)
+{
+ g_ss.ss_sp = malloc(SIGSTKSZ);
+ ASSERT(g_ss.ss_sp != NULL);
+
+ g_ss.ss_size = SIGSTKSZ;
+ g_ss.ss_flags = 0;
+
+ int ret = sigaltstack(&g_ss, NULL);
Review comment:
The thread that installs the signal handler with a call to `sigaction` should specify the `SS_ONSTACK` flag.
`
sa.sa_flags = SA_ONSTACK;
sa.sa_handler = handler(); /* Address of a signal handler */
sigemptyset(&sa.sa_mask);
if (sigaction(SIGSEGV, &sa, NULL) == -1) {
perror("sigaction");
exit(EXIT_FAILURE);
}
`
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477407444
##########
File path: arch/sim/src/sim/up_idle.c
##########
@@ -89,6 +89,14 @@ void up_idle(void)
}
#endif
+#ifndef CONFIG_SIM_PREEMPTIBLE
Review comment:
I will moved it
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r475093949
##########
File path: sched/task/task_setup.c
##########
@@ -593,6 +598,11 @@ static inline int nxtask_setup_stackargs(FAR struct task_tcb_s *tcb,
stackargv[argc + 1] = NULL;
tcb->argv = stackargv;
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ up_setup_args_context(tcb->cmn.xcp.ucontext_buffer, tcb->cmn.start, argc,
Review comment:
Then this change must NOT be merged into master. The integrity of the OS modularity is far, far, far most important than your change.
THIS MUST NOT BE MERGED. IT VIOLATES CORE ARCHITECTURAL PRINCIPLES OF THE OS. I will chnage the PR to a draft so that it is not accidentally merged.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477286355
##########
File path: arch/sim/src/sim/up_oneshot.c
##########
@@ -357,6 +359,17 @@ 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
+ host_prepare_timer();
+ host_init_timer(up_timer_update);
Review comment:
That's a good idea I will do this
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477305260
##########
File path: arch/sim/src/sim/up_ucontext.c
##########
@@ -0,0 +1,363 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_ucontext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <assert.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <ucontext.h>
+
+/****************************************************************************
+ * Preprocessor Definition
+ ****************************************************************************/
+
+/* The minimum ucontext size */
+
+#define MIN_UCONTEXT_STACK_LENGTH (128 * 1024)
+#define STACK_ALIGNMENT_BYTES (8)
+#define ASSERT(x) assert((x))
+#define ARRAY_LENGTH(array) (sizeof((array))/sizeof((array)[0]))
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+typedef struct exit_data_context_s
+{
+ ucontext_t *next_ucontext;
+ ucontext_t *current_ucontext;
+ void *ucontext_sp;
+ int cpu_index;
+} exit_data_context_t;
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static ucontext_t g_uctx_main;
+
+#ifdef CONFIG_SMP
+static exit_data_context_t g_task_exit_details[CONFIG_SMP_NCPUS];
+static ucontext_t g_task_exit_context[CONFIG_SMP_NCPUS];
+#else
+static exit_data_context_t g_task_exit_details[1];
+static ucontext_t g_task_exit_context[1];
+#endif
+
+static volatile bool g_is_exit_context_init;
+static stack_t g_ss;
+
+/****************************************************************************
+ * Public Prototype
+ ****************************************************************************/
+
+#ifdef CONFIG_SMP
+int up_cpu_index(void);
+#endif
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: task_exit_point
+ *
+ * Description:
+ * This function is used to release the allocated stack memory for a task
+ * on it's exit path.
+ *
+ * Assumptions:
+ * This function does not return.
+ *
+ ****************************************************************************/
+
+static void task_exit_point(void)
+{
+ while (1)
+ {
+ int cpu_id = 0;
+
+#ifdef CONFIG_SMP
+ cpu_id = up_cpu_index();
+#endif
+ getcontext(&g_task_exit_context[cpu_id]);
+
+ if (g_task_exit_details[cpu_id].ucontext_sp)
+ {
+ free(g_task_exit_details[cpu_id].current_ucontext);
+ free(g_task_exit_details[cpu_id].ucontext_sp);
+ g_task_exit_details[cpu_id].ucontext_sp = NULL;
+ setcontext(g_task_exit_details[cpu_id].next_ucontext);
+ }
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_exit_context
+ *
+ * Description:
+ * Creates new exit context for tasks. In a SMP configuration every CPU
+ * has a different exit context.
+ *
+ ****************************************************************************/
+
+static void up_setup_exit_context(void)
+{
+ for (int i = 0; i < ARRAY_LENGTH(g_task_exit_context); i++)
+ {
+ ucontext_t *exit_context = &g_task_exit_context[i];
+ int ret = getcontext(exit_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ exit_context->uc_stack.ss_sp = aligned_stack;
+ exit_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH -
+ align_offset;
+ exit_context->uc_link = &g_uctx_main;
+
+ makecontext(exit_context, task_exit_point, 0);
+ }
+
+ g_is_exit_context_init = true;
+}
+
+/****************************************************************************
+ * Name: up_is_stack_downwards
+ *
+ * Description:
+ * This function returns true if the stack places elements downwards.
+ *
+ * Input Parameters:
+ * stack_arg - the address of a variable from the previous function stack
+ * frame
+ *
+ * Returned Value:
+ * True if the stack grows downwards otherwise false.
+ *
+ ****************************************************************************/
+
+static bool up_is_stack_downwards(void *stack_arg)
+{
+ int stack_arg_local;
+ return stack_arg > (void *)&stack_arg_local;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_create_context
+ *
+ * Description:
+ * Creates a new ucontext structure and initialize it.
+ *
+ * Input Parameters:
+ * ucontext_sp - buffer where we store the ucontext stack pointer
+ * prev_ucontext - the parent ucontext used%i to return to when the
+ * execution of the current context ends.
+ * entry_point - the entry point of the new context
+ *
+ * Return Value:
+ * A pointer to the ucontext structure or NULL in case something went
+ * wrong.
+ *
+ ****************************************************************************/
+
+void *up_create_context(void **ucontext_sp, void *prev_ucontext,
+ void (*entry_point)(void))
+{
+ ucontext_t *new_context = calloc(1, sizeof(ucontext_t));
+ ASSERT(new_context != NULL);
+
+ int ret = getcontext(new_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ *((uintptr_t *)ucontext_sp) = (uintptr_t)sp;
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ new_context->uc_stack.ss_sp = aligned_stack;
+ new_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH - align_offset;
+ new_context->uc_link = prev_ucontext == NULL ? &g_uctx_main :
+ prev_ucontext;
+
+ makecontext(new_context, entry_point, 0);
+
+ if (!g_is_exit_context_init)
+ up_setup_exit_context();
+
+ return new_context;
+}
+
+/****************************************************************************
+ * Name: up_destroy_context
+ *
+ * Description:
+ * This function releases the host memory for the context structure and the
+ * context stack and jumps to the next context specified as argument.
+ *
+ * Input Parameters:
+ * current_ucontext - pointer to the TCB context
+ * ucontext_sp - pointer to the ucontext stack
+ * new_ucontext - the next context that we want to jump to
+ * cpu_id - the CPU identificator
+ *
+ * Assumptions:
+ * This function does not return. In order to free the stack memory that we
+ * are currently executing on, we jump to a new exit context.
+ *
+ ****************************************************************************/
+
+void up_destroy_context(void *current_ucontext, void *ucontext_sp,
+ void *next_ucontext, int cpu_id)
+{
+ g_task_exit_details[cpu_id].next_ucontext = next_ucontext;
+ g_task_exit_details[cpu_id].current_ucontext = current_ucontext;
+ g_task_exit_details[cpu_id].cpu_index = cpu_id;
+ g_task_exit_details[cpu_id].ucontext_sp = ucontext_sp;
+
+ setcontext(&g_task_exit_context[cpu_id]);
+}
+
+/****************************************************************************
+ * Name: up_setup_args_context
+ *
+ * Description:
+ * This function specifies the entry point for the new context and sets up
+ * the arguments for the new task.
+ *
+ * Input Parameters:
+ * ucontext - pointer to the TCB context
+ * entry_point - the entry point for the task
+ * argc - the number of cmd arguments
+ * argv - array of arguments for the new task
+ *
+ ****************************************************************************/
+
+void up_setup_args_context(void *current_ucontext, void (*entry_point)(void),
+ int argc, char *argv[])
+{
+ makecontext(current_ucontext, entry_point, 2, argc, argv);
+}
+
+/****************************************************************************
+ * Name: up_swap_context
+ *
+ * Description:
+ * Save the current context in the old_ucontext and activate the
+ * context from activate_ucontext.
+ *
+ * Input Parameters:
+ * old_ucontext - place where we store the current context
+ * activate_ucontext - context that we will activate after function
+ * invocation.
+ *
+ ****************************************************************************/
+
+void up_swap_context(void *old_ucontext, void *activate_ucontext)
+{
+ swapcontext(old_ucontext, activate_ucontext);
+}
+
+/****************************************************************************
+ * Name: up_set_context
+ *
+ * Description:
+ * Set the current context to the specified ucontext
+ *
+ * Input Parameters:
+ * current_context - the current context
+ *
+ ****************************************************************************/
+
+void up_set_context(void *current_context)
+{
+ setcontext(current_context);
+}
+
+/****************************************************************************
+ * Name: up_cpu_simulated_interrupt
+ *
+ * Description:
+ * This function verifies if we are running on the signal handler stack.
+ *
+ * Returned Value:
+ * Return zero if we are not running on the signal stack otherwise return
+ * a non zero value.
+ *
+ ****************************************************************************/
+
+int up_cpu_simulated_interrupt(void)
+{
+ uint8_t test_address;
+ unsigned long verify_stack_arg = (unsigned long)&test_address;
+ unsigned long stack_start = (unsigned long)g_ss.ss_sp;
+ unsigned long stack_end;
+
+ if (up_is_stack_downwards(&verify_stack_arg) == true)
+ {
+ stack_end = stack_start - g_ss.ss_size;
+ return stack_start > verify_stack_arg && stack_end < verify_stack_arg;
+ }
+ else
+ {
+ stack_end = stack_start + g_ss.ss_size;
+ return stack_start < verify_stack_arg && stack_end < verify_stack_arg;
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_sigstackt
+ *
+ * Description:
+ * This function sets up an alternative stack for the signals from the
+ * host heap memory.
+ *
+ ****************************************************************************/
+
+void up_setup_sigstack(void)
+{
+ g_ss.ss_sp = malloc(SIGSTKSZ);
+ ASSERT(g_ss.ss_sp != NULL);
+
+ g_ss.ss_size = SIGSTKSZ;
+ g_ss.ss_flags = 0;
+
+ int ret = sigaltstack(&g_ss, NULL);
Review comment:
What I mean is that it's fine for non SMP case to just define one interrupt stack for signal handling, but the stack will corrupt in SMP case because the multiple signal handle could be activate at the same time and then utilize the same interrupted stack concurrently.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r475093949
##########
File path: sched/task/task_setup.c
##########
@@ -593,6 +598,11 @@ static inline int nxtask_setup_stackargs(FAR struct task_tcb_s *tcb,
stackargv[argc + 1] = NULL;
tcb->argv = stackargv;
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ up_setup_args_context(tcb->cmn.xcp.ucontext_buffer, tcb->cmn.start, argc,
Review comment:
Then this change must NOT be merged into master. The integrity of the OS modularity is far, far, far most important than your change. The is no technical argument on the face of planet that is sufficient to violate those modularity principles.
THIS MUST NOT BE MERGED. IT VIOLATES CORE ARCHITECTURAL PRINCIPLES OF THE OS. I will chnage the PR to a draft so that it is not accidentally merged.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680147750
> In this way we don't have to modify the platform independent code but it adds additional complexity. What do you think ?
I think my suggestion in the previous comment would work better for you and is very simple. It is really the same solution that you have originally except that it is dressed up to look like a platform independent feature. The decoupling here is purely semantic but, I think, acceptable.
There are conditional hooks throughout the OS: start hooks, termination hooks, timer hooks, etc. So, although not a thing of beauty, I think that is consistent with how other sticky issues have been addressed.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r478872797
##########
File path: arch/sim/src/sim/up_ucontext.c
##########
@@ -0,0 +1,363 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_ucontext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <assert.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <ucontext.h>
+
+/****************************************************************************
+ * Preprocessor Definition
+ ****************************************************************************/
+
+/* The minimum ucontext size */
+
+#define MIN_UCONTEXT_STACK_LENGTH (128 * 1024)
+#define STACK_ALIGNMENT_BYTES (8)
+#define ASSERT(x) assert((x))
+#define ARRAY_LENGTH(array) (sizeof((array))/sizeof((array)[0]))
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+typedef struct exit_data_context_s
+{
+ ucontext_t *next_ucontext;
+ ucontext_t *current_ucontext;
+ void *ucontext_sp;
+ int cpu_index;
+} exit_data_context_t;
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static ucontext_t g_uctx_main;
+
+#ifdef CONFIG_SMP
+static exit_data_context_t g_task_exit_details[CONFIG_SMP_NCPUS];
+static ucontext_t g_task_exit_context[CONFIG_SMP_NCPUS];
+#else
+static exit_data_context_t g_task_exit_details[1];
+static ucontext_t g_task_exit_context[1];
+#endif
+
+static volatile bool g_is_exit_context_init;
+static stack_t g_ss;
+
+/****************************************************************************
+ * Public Prototype
+ ****************************************************************************/
+
+#ifdef CONFIG_SMP
+int up_cpu_index(void);
+#endif
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: task_exit_point
+ *
+ * Description:
+ * This function is used to release the allocated stack memory for a task
+ * on it's exit path.
+ *
+ * Assumptions:
+ * This function does not return.
+ *
+ ****************************************************************************/
+
+static void task_exit_point(void)
+{
+ while (1)
+ {
+ int cpu_id = 0;
+
+#ifdef CONFIG_SMP
+ cpu_id = up_cpu_index();
+#endif
+ getcontext(&g_task_exit_context[cpu_id]);
+
+ if (g_task_exit_details[cpu_id].ucontext_sp)
+ {
+ free(g_task_exit_details[cpu_id].current_ucontext);
+ free(g_task_exit_details[cpu_id].ucontext_sp);
+ g_task_exit_details[cpu_id].ucontext_sp = NULL;
+ setcontext(g_task_exit_details[cpu_id].next_ucontext);
+ }
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_exit_context
+ *
+ * Description:
+ * Creates new exit context for tasks. In a SMP configuration every CPU
+ * has a different exit context.
+ *
+ ****************************************************************************/
+
+static void up_setup_exit_context(void)
+{
+ for (int i = 0; i < ARRAY_LENGTH(g_task_exit_context); i++)
+ {
+ ucontext_t *exit_context = &g_task_exit_context[i];
+ int ret = getcontext(exit_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ exit_context->uc_stack.ss_sp = aligned_stack;
+ exit_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH -
+ align_offset;
+ exit_context->uc_link = &g_uctx_main;
+
+ makecontext(exit_context, task_exit_point, 0);
+ }
+
+ g_is_exit_context_init = true;
+}
+
+/****************************************************************************
+ * Name: up_is_stack_downwards
+ *
+ * Description:
+ * This function returns true if the stack places elements downwards.
+ *
+ * Input Parameters:
+ * stack_arg - the address of a variable from the previous function stack
+ * frame
+ *
+ * Returned Value:
+ * True if the stack grows downwards otherwise false.
+ *
+ ****************************************************************************/
+
+static bool up_is_stack_downwards(void *stack_arg)
+{
+ int stack_arg_local;
+ return stack_arg > (void *)&stack_arg_local;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_create_context
+ *
+ * Description:
+ * Creates a new ucontext structure and initialize it.
+ *
+ * Input Parameters:
+ * ucontext_sp - buffer where we store the ucontext stack pointer
+ * prev_ucontext - the parent ucontext used%i to return to when the
+ * execution of the current context ends.
+ * entry_point - the entry point of the new context
+ *
+ * Return Value:
+ * A pointer to the ucontext structure or NULL in case something went
+ * wrong.
+ *
+ ****************************************************************************/
+
+void *up_create_context(void **ucontext_sp, void *prev_ucontext,
+ void (*entry_point)(void))
+{
+ ucontext_t *new_context = calloc(1, sizeof(ucontext_t));
+ ASSERT(new_context != NULL);
+
+ int ret = getcontext(new_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ *((uintptr_t *)ucontext_sp) = (uintptr_t)sp;
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ new_context->uc_stack.ss_sp = aligned_stack;
+ new_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH - align_offset;
+ new_context->uc_link = prev_ucontext == NULL ? &g_uctx_main :
+ prev_ucontext;
+
+ makecontext(new_context, entry_point, 0);
+
+ if (!g_is_exit_context_init)
+ up_setup_exit_context();
+
+ return new_context;
+}
+
+/****************************************************************************
+ * Name: up_destroy_context
+ *
+ * Description:
+ * This function releases the host memory for the context structure and the
+ * context stack and jumps to the next context specified as argument.
+ *
+ * Input Parameters:
+ * current_ucontext - pointer to the TCB context
+ * ucontext_sp - pointer to the ucontext stack
+ * new_ucontext - the next context that we want to jump to
+ * cpu_id - the CPU identificator
+ *
+ * Assumptions:
+ * This function does not return. In order to free the stack memory that we
+ * are currently executing on, we jump to a new exit context.
+ *
+ ****************************************************************************/
+
+void up_destroy_context(void *current_ucontext, void *ucontext_sp,
+ void *next_ucontext, int cpu_id)
+{
+ g_task_exit_details[cpu_id].next_ucontext = next_ucontext;
+ g_task_exit_details[cpu_id].current_ucontext = current_ucontext;
+ g_task_exit_details[cpu_id].cpu_index = cpu_id;
+ g_task_exit_details[cpu_id].ucontext_sp = ucontext_sp;
+
+ setcontext(&g_task_exit_context[cpu_id]);
+}
+
+/****************************************************************************
+ * Name: up_setup_args_context
+ *
+ * Description:
+ * This function specifies the entry point for the new context and sets up
+ * the arguments for the new task.
+ *
+ * Input Parameters:
+ * ucontext - pointer to the TCB context
+ * entry_point - the entry point for the task
+ * argc - the number of cmd arguments
+ * argv - array of arguments for the new task
+ *
+ ****************************************************************************/
+
+void up_setup_args_context(void *current_ucontext, void (*entry_point)(void),
+ int argc, char *argv[])
+{
+ makecontext(current_ucontext, entry_point, 2, argc, argv);
+}
+
+/****************************************************************************
+ * Name: up_swap_context
+ *
+ * Description:
+ * Save the current context in the old_ucontext and activate the
+ * context from activate_ucontext.
+ *
+ * Input Parameters:
+ * old_ucontext - place where we store the current context
+ * activate_ucontext - context that we will activate after function
+ * invocation.
+ *
+ ****************************************************************************/
+
+void up_swap_context(void *old_ucontext, void *activate_ucontext)
+{
+ swapcontext(old_ucontext, activate_ucontext);
+}
+
+/****************************************************************************
+ * Name: up_set_context
+ *
+ * Description:
+ * Set the current context to the specified ucontext
+ *
+ * Input Parameters:
+ * current_context - the current context
+ *
+ ****************************************************************************/
+
+void up_set_context(void *current_context)
+{
+ setcontext(current_context);
+}
+
+/****************************************************************************
+ * Name: up_cpu_simulated_interrupt
+ *
+ * Description:
+ * This function verifies if we are running on the signal handler stack.
+ *
+ * Returned Value:
+ * Return zero if we are not running on the signal stack otherwise return
+ * a non zero value.
+ *
+ ****************************************************************************/
+
+int up_cpu_simulated_interrupt(void)
+{
+ uint8_t test_address;
+ unsigned long verify_stack_arg = (unsigned long)&test_address;
+ unsigned long stack_start = (unsigned long)g_ss.ss_sp;
+ unsigned long stack_end;
+
+ if (up_is_stack_downwards(&verify_stack_arg) == true)
+ {
+ stack_end = stack_start - g_ss.ss_size;
+ return stack_start > verify_stack_arg && stack_end < verify_stack_arg;
+ }
+ else
+ {
+ stack_end = stack_start + g_ss.ss_size;
+ return stack_start < verify_stack_arg && stack_end < verify_stack_arg;
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_sigstackt
+ *
+ * Description:
+ * This function sets up an alternative stack for the signals from the
+ * host heap memory.
+ *
+ ****************************************************************************/
+
+void up_setup_sigstack(void)
+{
+ g_ss.ss_sp = malloc(SIGSTKSZ);
+ ASSERT(g_ss.ss_sp != NULL);
+
+ g_ss.ss_size = SIGSTKSZ;
+ g_ss.ss_flags = 0;
+
+ int ret = sigaltstack(&g_ss, NULL);
Review comment:
That's right but the behavior is that only the thread that installs the handler with `SS_ONSTACK` will run it on the alternate stack and the others will run the handler on their own stack. Anyways, I think we don't need this alternate stack method to decide if we are running in an interrupt or not. [Your method](https://github.com/xiaoxiang781216/incubator-nuttx/commit/38fcaa6a9cb4893c090d9830a305d4bb209b4b35) seems much better with keeping an `g_cpu_signal` array and marking true/false when we enter a handler.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477206014
##########
File path: arch/sim/include/irq.h
##########
@@ -82,11 +78,15 @@ typedef int xcpt_reg_t;
struct xcptcontext
{
- void *sigdeliver; /* Actual type is sig_deliver_t */
+ void *sigdeliver;
+
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ void *ucontext_buffer; /* Actual type is ucontext_t */
+ void *ucontext_sp;
+#endif
xcpt_reg_t regs[XCPTCONTEXT_REGS];
Review comment:
If I do this I also need to guard the `xcp.regs `access with a negative gating !CONFIG_SIM_PREEMPTIBLE from `up_stackframe.c` line 124. Is it ok with you ?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680129897
There are some startup hooks that might be usable for such a purpose with some extensions??? sched/task/task_starthook.c
The the start hooks are not currently set up in a way that would be usable to you because it really only works if the TCB has been created (via nxtask_init()) but not yet started (via nxtask_activate()).
It is currently using only in binfmt/binfmt_execmodule.c when an ELF task is created in order to run the C++ static constuctors (This is a documented security Issue #1265 for this).
Hmm...
I think it would be fine to implement a abstracted, generalized, platform-specific hook in sched/task/task_init.c or maybe nxtask_activate. It would be wrapped and a general external task startup hook:
* It would have a name like up_start_hook()
* It would be prototyped in include/nuttx/arch.h
* There would be a configuration in sched/Kconfig like ARCH_STARTHOOK
That should work if I understand what you need correctly.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680147750
> In this way we don't have to modify the platform independent code but it adds additional complexity. What do you think ?
I think my suggestion in the previous comment would work better for you and is very simple. It is really the same solution that you have originally except that it is dressed up to look like a platform independent feature. The decoupling here is purely semantic but, I think, acceptable.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477332002
##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -1,47 +1,251 @@
/****************************************************************************
* arch/sim/src/sim/up_hosttime.c
*
- * Copyright (C) 2008 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <nu...@fitbit.com>
*
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
*
- * 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.
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * 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.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
*
****************************************************************************/
/****************************************************************************
* Included Files
****************************************************************************/
+#include <signal.h>
+#include <stdarg.h>
#include <stdbool.h>
+#include <stdio.h>
#include <stdint.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
#include <time.h>
#include <unistd.h>
+/****************************************************************************
+ * 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_1_SIGUSR1 = 1,
Review comment:
Cool, thanks. I will use the updated version from you as it's more elegant
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477290621
##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -1,47 +1,251 @@
/****************************************************************************
* arch/sim/src/sim/up_hosttime.c
*
- * Copyright (C) 2008 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gn...@nuttx.org>
+ * Sebastian Ene <nu...@fitbit.com>
*
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
*
- * 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.
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * 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.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
*
****************************************************************************/
/****************************************************************************
* Included Files
****************************************************************************/
+#include <signal.h>
+#include <stdarg.h>
#include <stdbool.h>
+#include <stdio.h>
#include <stdint.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
#include <time.h>
#include <unistd.h>
+/****************************************************************************
+ * 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_1_SIGUSR1 = 1,
Review comment:
Currently we are using only these signals from the host. How can I make it more generic ?
##########
File path: arch/sim/src/sim/up_exit.c
##########
@@ -65,9 +65,13 @@
void up_exit(int status)
{
- FAR struct tcb_s *tcb;
+ FAR struct tcb_s *tcb = this_task();
- sinfo("TCB=%p exiting\n", this_task());
+ sinfo("TCB=%p exiting\n", tcb);
+
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ FAR struct tcb_s *prev_tcb = tcb;
Review comment:
I will move them, thank you
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-686215958
@sebastianene07 you can rebase this patch now, I have an idea which may avoid to touch the common code and will try in the next couple day.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r477288973
##########
File path: arch/sim/src/sim/up_ucontext.c
##########
@@ -0,0 +1,363 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_ucontext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <assert.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <ucontext.h>
+
+/****************************************************************************
+ * Preprocessor Definition
+ ****************************************************************************/
+
+/* The minimum ucontext size */
+
+#define MIN_UCONTEXT_STACK_LENGTH (128 * 1024)
+#define STACK_ALIGNMENT_BYTES (8)
+#define ASSERT(x) assert((x))
+#define ARRAY_LENGTH(array) (sizeof((array))/sizeof((array)[0]))
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+typedef struct exit_data_context_s
+{
+ ucontext_t *next_ucontext;
+ ucontext_t *current_ucontext;
+ void *ucontext_sp;
+ int cpu_index;
+} exit_data_context_t;
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static ucontext_t g_uctx_main;
+
+#ifdef CONFIG_SMP
+static exit_data_context_t g_task_exit_details[CONFIG_SMP_NCPUS];
+static ucontext_t g_task_exit_context[CONFIG_SMP_NCPUS];
+#else
+static exit_data_context_t g_task_exit_details[1];
+static ucontext_t g_task_exit_context[1];
+#endif
+
+static volatile bool g_is_exit_context_init;
+static stack_t g_ss;
+
+/****************************************************************************
+ * Public Prototype
+ ****************************************************************************/
+
+#ifdef CONFIG_SMP
+int up_cpu_index(void);
+#endif
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: task_exit_point
+ *
+ * Description:
+ * This function is used to release the allocated stack memory for a task
+ * on it's exit path.
+ *
+ * Assumptions:
+ * This function does not return.
+ *
+ ****************************************************************************/
+
+static void task_exit_point(void)
+{
+ while (1)
+ {
+ int cpu_id = 0;
+
+#ifdef CONFIG_SMP
+ cpu_id = up_cpu_index();
+#endif
+ getcontext(&g_task_exit_context[cpu_id]);
+
+ if (g_task_exit_details[cpu_id].ucontext_sp)
+ {
+ free(g_task_exit_details[cpu_id].current_ucontext);
+ free(g_task_exit_details[cpu_id].ucontext_sp);
+ g_task_exit_details[cpu_id].ucontext_sp = NULL;
+ setcontext(g_task_exit_details[cpu_id].next_ucontext);
+ }
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_exit_context
+ *
+ * Description:
+ * Creates new exit context for tasks. In a SMP configuration every CPU
+ * has a different exit context.
+ *
+ ****************************************************************************/
+
+static void up_setup_exit_context(void)
+{
+ for (int i = 0; i < ARRAY_LENGTH(g_task_exit_context); i++)
+ {
+ ucontext_t *exit_context = &g_task_exit_context[i];
+ int ret = getcontext(exit_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ exit_context->uc_stack.ss_sp = aligned_stack;
+ exit_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH -
+ align_offset;
+ exit_context->uc_link = &g_uctx_main;
+
+ makecontext(exit_context, task_exit_point, 0);
+ }
+
+ g_is_exit_context_init = true;
+}
+
+/****************************************************************************
+ * Name: up_is_stack_downwards
+ *
+ * Description:
+ * This function returns true if the stack places elements downwards.
+ *
+ * Input Parameters:
+ * stack_arg - the address of a variable from the previous function stack
+ * frame
+ *
+ * Returned Value:
+ * True if the stack grows downwards otherwise false.
+ *
+ ****************************************************************************/
+
+static bool up_is_stack_downwards(void *stack_arg)
+{
+ int stack_arg_local;
+ return stack_arg > (void *)&stack_arg_local;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_create_context
+ *
+ * Description:
+ * Creates a new ucontext structure and initialize it.
+ *
+ * Input Parameters:
+ * ucontext_sp - buffer where we store the ucontext stack pointer
+ * prev_ucontext - the parent ucontext used%i to return to when the
+ * execution of the current context ends.
+ * entry_point - the entry point of the new context
+ *
+ * Return Value:
+ * A pointer to the ucontext structure or NULL in case something went
+ * wrong.
+ *
+ ****************************************************************************/
+
+void *up_create_context(void **ucontext_sp, void *prev_ucontext,
+ void (*entry_point)(void))
+{
+ ucontext_t *new_context = calloc(1, sizeof(ucontext_t));
+ ASSERT(new_context != NULL);
+
+ int ret = getcontext(new_context);
+ ASSERT(ret >= 0);
+
+ uint8_t *sp = calloc(1, MIN_UCONTEXT_STACK_LENGTH);
+ ASSERT(sp != NULL);
+
+ *((uintptr_t *)ucontext_sp) = (uintptr_t)sp;
+
+ uint64_t align_offset = STACK_ALIGNMENT_BYTES - ((uint64_t)sp %
+ STACK_ALIGNMENT_BYTES);
+
+ uint8_t *aligned_stack = sp + align_offset;
+ new_context->uc_stack.ss_sp = aligned_stack;
+ new_context->uc_stack.ss_size = MIN_UCONTEXT_STACK_LENGTH - align_offset;
+ new_context->uc_link = prev_ucontext == NULL ? &g_uctx_main :
+ prev_ucontext;
+
+ makecontext(new_context, entry_point, 0);
+
+ if (!g_is_exit_context_init)
+ up_setup_exit_context();
+
+ return new_context;
+}
+
+/****************************************************************************
+ * Name: up_destroy_context
+ *
+ * Description:
+ * This function releases the host memory for the context structure and the
+ * context stack and jumps to the next context specified as argument.
+ *
+ * Input Parameters:
+ * current_ucontext - pointer to the TCB context
+ * ucontext_sp - pointer to the ucontext stack
+ * new_ucontext - the next context that we want to jump to
+ * cpu_id - the CPU identificator
+ *
+ * Assumptions:
+ * This function does not return. In order to free the stack memory that we
+ * are currently executing on, we jump to a new exit context.
+ *
+ ****************************************************************************/
+
+void up_destroy_context(void *current_ucontext, void *ucontext_sp,
+ void *next_ucontext, int cpu_id)
+{
+ g_task_exit_details[cpu_id].next_ucontext = next_ucontext;
+ g_task_exit_details[cpu_id].current_ucontext = current_ucontext;
+ g_task_exit_details[cpu_id].cpu_index = cpu_id;
+ g_task_exit_details[cpu_id].ucontext_sp = ucontext_sp;
+
+ setcontext(&g_task_exit_context[cpu_id]);
+}
+
+/****************************************************************************
+ * Name: up_setup_args_context
+ *
+ * Description:
+ * This function specifies the entry point for the new context and sets up
+ * the arguments for the new task.
+ *
+ * Input Parameters:
+ * ucontext - pointer to the TCB context
+ * entry_point - the entry point for the task
+ * argc - the number of cmd arguments
+ * argv - array of arguments for the new task
+ *
+ ****************************************************************************/
+
+void up_setup_args_context(void *current_ucontext, void (*entry_point)(void),
+ int argc, char *argv[])
+{
+ makecontext(current_ucontext, entry_point, 2, argc, argv);
+}
+
+/****************************************************************************
+ * Name: up_swap_context
+ *
+ * Description:
+ * Save the current context in the old_ucontext and activate the
+ * context from activate_ucontext.
+ *
+ * Input Parameters:
+ * old_ucontext - place where we store the current context
+ * activate_ucontext - context that we will activate after function
+ * invocation.
+ *
+ ****************************************************************************/
+
+void up_swap_context(void *old_ucontext, void *activate_ucontext)
+{
+ swapcontext(old_ucontext, activate_ucontext);
+}
+
+/****************************************************************************
+ * Name: up_set_context
+ *
+ * Description:
+ * Set the current context to the specified ucontext
+ *
+ * Input Parameters:
+ * current_context - the current context
+ *
+ ****************************************************************************/
+
+void up_set_context(void *current_context)
+{
+ setcontext(current_context);
+}
+
+/****************************************************************************
+ * Name: up_cpu_simulated_interrupt
+ *
+ * Description:
+ * This function verifies if we are running on the signal handler stack.
+ *
+ * Returned Value:
+ * Return zero if we are not running on the signal stack otherwise return
+ * a non zero value.
+ *
+ ****************************************************************************/
+
+int up_cpu_simulated_interrupt(void)
+{
+ uint8_t test_address;
+ unsigned long verify_stack_arg = (unsigned long)&test_address;
+ unsigned long stack_start = (unsigned long)g_ss.ss_sp;
+ unsigned long stack_end;
+
+ if (up_is_stack_downwards(&verify_stack_arg) == true)
+ {
+ stack_end = stack_start - g_ss.ss_size;
+ return stack_start > verify_stack_arg && stack_end < verify_stack_arg;
+ }
+ else
+ {
+ stack_end = stack_start + g_ss.ss_size;
+ return stack_start < verify_stack_arg && stack_end < verify_stack_arg;
+ }
+}
+
+/****************************************************************************
+ * Name: up_setup_sigstackt
+ *
+ * Description:
+ * This function sets up an alternative stack for the signals from the
+ * host heap memory.
+ *
+ ****************************************************************************/
+
+void up_setup_sigstack(void)
+{
+ g_ss.ss_sp = malloc(SIGSTKSZ);
+ ASSERT(g_ss.ss_sp != NULL);
+
+ g_ss.ss_size = SIGSTKSZ;
+ g_ss.ss_flags = 0;
+
+ int ret = sigaltstack(&g_ss, NULL);
Review comment:
The thread that installs the signal handler with a call to `sigaction` should specify the `SS_ONSTACK` flag.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#discussion_r479663554
##########
File path: arch/sim/include/irq.h
##########
@@ -82,11 +78,15 @@ typedef int xcpt_reg_t;
struct xcptcontext
{
- void *sigdeliver; /* Actual type is sig_deliver_t */
+ void *sigdeliver;
+
+#ifdef CONFIG_SIM_PREEMPTIBLE
+ void *ucontext_buffer; /* Actual type is ucontext_t */
+ void *ucontext_sp;
+#endif
xcpt_reg_t regs[XCPTCONTEXT_REGS];
Review comment:
I am considering to always use ucontext API, I don't see there is any benefit to maintain two context switch mechanism.
##########
File path: arch/sim/src/Makefile
##########
@@ -92,6 +93,17 @@ ifeq ($(CONFIG_SPINLOCK),y)
HOSTSRCS += up_testset.c
endif
+ifeq ($(CONFIG_SIM_PREEMPTIBLE),y)
+ HOSTSRCS += up_ucontext.c
+ HOSTCFLAGS += -DCONFIG_SIM_PREEMPTIBLE=$(CONFIG_SIM_PREEMPTIBLE)
Review comment:
OK.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680102911
>
>
> > This change MUST not come into master UNLESS all simulator specific changes outside of arch/sim and boards/sime are removed. The common OS must not be contaminated with platform-specific code.
>
> I agree.
>
> The platform independent logic is supposed to remain platform independent!!
I wonder if there is some design alternative that would eliminate the need for this coupling. It would be good if we could bring this code in, but we do have to enforce the OS modularity.
There are other examples where we had to implement special features to support specific platforms or hardware, but we did these things by extending interfaces and configuration options. For example, we added the capabilities method to the SDIO interface (included/nuttx/sdio.h).
I am not proposing that as a solution and would not advocate a hack just to avoid the SIM dependency. We need to do things right. There are not comments added to the code to explain why you thought this was necessary. So it is hard to suggest alternatives. Comments are good! Comments are your friends!
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-680102911
>
>
> > This change MUST not come into master UNLESS all simulator specific changes outside of arch/sim and boards/sime are removed. The common OS must not be contaminated with platform-specific code.
>
> I agree.
>
> The platform independent logic is supposed to remain platform independent!!
I wonder if there is some design alternative that would eliminate the need for this coupling. It would be good if we could bring this code in, but we do have to enforce the OS modularity.
There are other examples where we had to implement special features to support specific platforms or hardware, but we did these things by extending interfaces and configuration options. For example, we added the capabilities method to the SDIO interface (included/nuttx/sdio.h).
I am not proposing that as a solution and would not advocate a hack just to avoid the SIM dependency. The point is that you can retain functionality while removing platform dependencies by generalizing and abstracting the functionality.
We need to do things right. There are no comments added to the code to explain why you thought this was necessary. So it is hard to suggest alternatives. Comments are good! Comments are your friends!
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] sebastianene07 commented on pull request #1602: arch/sim: Add support for preemptive scheduling in the simulator build using ucontext API
Posted by GitBox <gi...@apache.org>.
sebastianene07 commented on pull request #1602:
URL: https://github.com/apache/incubator-nuttx/pull/1602#issuecomment-686238069
> @sebastianene07 you can rebase this patch now, I have an idea which may avoid to touch the common code and will try in the next couple day.
Thanks for the update, I figured it out and we don't have to modify the common code to pass arguments to new tasks. The entry point for a new task is ```nxtask_start``` and this function takes no arguments. It grabs the arguments (argc and argv) from the TCB and calls the `tcb->cmn.entry.main` with them. So the mechanism to pass the arguments is very generic and works like a charm in our case without any modification.
----------------------------------------------------------------
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