You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "hujun260 (via GitHub)" <gi...@apache.org> on 2024/09/26 00:21:04 UTC
[PR] change the SMP scheduling policy from synchronous to asynchronous [nuttx]
hujun260 opened a new pull request, #13616:
URL: https://github.com/apache/nuttx/pull/13616
## Summary
Currently, if we need to schedule a task to another CPU, we have to completely halt the other CPU,
manipulate the scheduling linked list, and then resume the operation of that CPU. This process is both time-consuming and unnecessary.
During this process, both the current CPU and the target CPU are inevitably subjected to busyloop.
The improved strategy is to simply send a cross-core interrupt to the target CPU.
The current CPU continues to run while the target CPU responds to the interrupt, eliminating the certainty of a busyloop occurring.
## Impact
all smp arch
## Testing
We performed performance testing on the Cortex-R82 ARM64 SMP chip. Sending messages through a message queue which causes context switching. We recorded the time from sending a message to receiving it, with the time unit being cycles.
we also use monkey test in armv7-a
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] change the SMP scheduling policy from synchronous to asynchronous [nuttx]
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #13616:
URL: https://github.com/apache/nuttx/pull/13616#issuecomment-2375565355
> all smp arch
@hujun260
Which configurations did you test with this PR?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] change the SMP scheduling policy from synchronous to asynchronous [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #13616:
URL: https://github.com/apache/nuttx/pull/13616#discussion_r1777893327
##########
sched/sched/sched_process_delivered.c:
##########
@@ -0,0 +1,159 @@
+/****************************************************************************
+ * sched/sched/sched_process_delivered.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 <nuttx/config.h>
+
+#include <stdbool.h>
+#include <assert.h>
+
+#include <nuttx/queue.h>
+
+#include "irq/irq.h"
+#include "sched/sched.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: nxsched_process_delivered
+ *
+ * Description:
+ * This function is used to process the tcb in g_delivertasks.
+ * 1 We use direct locking instead of enter_critical_section
+ * to save processing time
+ * 2 If there is a higher priority task, we will still perform
+ * the higher priority task
+ * 3 If the schedule lock is on, the task will be placed in g_pendingtasks
+ *
+ * Input Parameters:
+ * cpu
+ *
+ * Returned Value:
+ * OK
+ *
+ * Assumptions:
+ * - The caller must be in irq
+ * - current cpu must not be locked
+ *
+ ****************************************************************************/
+
+void nxsched_process_delivered(int cpu)
+{
+ FAR dq_queue_t *tasklist;
+ FAR struct tcb_s *next;
+ FAR struct tcb_s *prev;
+ struct tcb_s *btcb = NULL;
+ struct tcb_s *tcb;
+
+ DEBUGASSERT(g_cpu_nestcount[cpu] == 0);
+ DEBUGASSERT(up_interrupt_context());
+
+ if ((g_cpu_irqset & (1 << cpu)) == 0)
+ {
+ while (!spin_trylock_wo_note(&g_cpu_irqlock))
+ {
+ if (up_cpu_pausereq(cpu))
+ {
+ up_cpu_paused(cpu);
+ }
+ }
+
+ g_cpu_irqset |= (1 << cpu);
+ }
+
+ if (g_delivertasks[cpu] == NULL)
+ {
+ tcb = current_task(cpu);
+ if (tcb->irqcount <= 0)
+ {
+ cpu_irqlock_clear();
+ }
+
+ return;
+ }
+
+ if (nxsched_islocked_global())
+ {
+ btcb = g_delivertasks[cpu];
+ g_delivertasks[cpu] = NULL;
+ nxsched_add_prioritized(btcb, &g_pendingtasks);
+ btcb->task_state = TSTATE_TASK_PENDING;
+ tcb = current_task(cpu);
+ if (tcb->irqcount <= 0)
+ {
+ cpu_irqlock_clear();
+ }
+
+ return;
+ }
+
+ btcb = g_delivertasks[cpu];
+ tasklist = &g_assignedtasks[cpu];
+
+ for (next = (FAR struct tcb_s *)tasklist->head;
+ (next && btcb->sched_priority <= next->sched_priority);
+ next = next->flink);
+
+ prev = next->blink;
+ if (prev == NULL)
+ {
+ /* Special case: Insert at the head of the list */
+
+ btcb->flink = next;
+ btcb->blink = NULL;
+ next->blink = btcb;
+ tasklist->head = (FAR dq_entry_t *)btcb;
+ btcb->cpu = cpu;
+ btcb->task_state = TSTATE_TASK_RUNNING;
+
+ DEBUGASSERT(btcb->flink != NULL);
+ next = btcb->flink;
+ next->task_state = TSTATE_TASK_ASSIGNED;
+
+ if (btcb->lockcount > 0)
+ {
+ g_cpu_lockset |= (1 << cpu);
+ }
+ }
+ else
+ {
+ /* Insert in the middle of the list */
+
+ btcb->flink = next;
Review Comment:
ditto
##########
sched/sched/sched_process_delivered.c:
##########
@@ -0,0 +1,159 @@
+/****************************************************************************
+ * sched/sched/sched_process_delivered.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 <nuttx/config.h>
+
+#include <stdbool.h>
+#include <assert.h>
+
+#include <nuttx/queue.h>
+
+#include "irq/irq.h"
+#include "sched/sched.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: nxsched_process_delivered
+ *
+ * Description:
+ * This function is used to process the tcb in g_delivertasks.
+ * 1 We use direct locking instead of enter_critical_section
+ * to save processing time
+ * 2 If there is a higher priority task, we will still perform
+ * the higher priority task
+ * 3 If the schedule lock is on, the task will be placed in g_pendingtasks
+ *
+ * Input Parameters:
+ * cpu
+ *
+ * Returned Value:
+ * OK
+ *
+ * Assumptions:
+ * - The caller must be in irq
+ * - current cpu must not be locked
+ *
+ ****************************************************************************/
+
+void nxsched_process_delivered(int cpu)
+{
+ FAR dq_queue_t *tasklist;
+ FAR struct tcb_s *next;
+ FAR struct tcb_s *prev;
+ struct tcb_s *btcb = NULL;
+ struct tcb_s *tcb;
+
+ DEBUGASSERT(g_cpu_nestcount[cpu] == 0);
+ DEBUGASSERT(up_interrupt_context());
+
+ if ((g_cpu_irqset & (1 << cpu)) == 0)
+ {
+ while (!spin_trylock_wo_note(&g_cpu_irqlock))
+ {
+ if (up_cpu_pausereq(cpu))
+ {
+ up_cpu_paused(cpu);
+ }
+ }
+
+ g_cpu_irqset |= (1 << cpu);
+ }
+
+ if (g_delivertasks[cpu] == NULL)
+ {
+ tcb = current_task(cpu);
+ if (tcb->irqcount <= 0)
+ {
+ cpu_irqlock_clear();
+ }
+
+ return;
+ }
+
+ if (nxsched_islocked_global())
+ {
+ btcb = g_delivertasks[cpu];
+ g_delivertasks[cpu] = NULL;
+ nxsched_add_prioritized(btcb, &g_pendingtasks);
+ btcb->task_state = TSTATE_TASK_PENDING;
+ tcb = current_task(cpu);
+ if (tcb->irqcount <= 0)
+ {
+ cpu_irqlock_clear();
+ }
+
+ return;
+ }
+
+ btcb = g_delivertasks[cpu];
+ tasklist = &g_assignedtasks[cpu];
+
+ for (next = (FAR struct tcb_s *)tasklist->head;
+ (next && btcb->sched_priority <= next->sched_priority);
+ next = next->flink);
+
+ prev = next->blink;
+ if (prev == NULL)
+ {
+ /* Special case: Insert at the head of the list */
+
+ btcb->flink = next;
Review Comment:
can we call the new special queue function added recently
##########
sched/sched/sched_addreadytorun.c:
##########
@@ -222,13 +222,35 @@ bool nxsched_add_readytorun(FAR struct tcb_s *btcb)
else /* (task_state == TSTATE_TASK_RUNNING) */
{
/* If we are modifying some assigned task list other than our own, we
- * will need to stop that CPU.
+ * will need to switch that CPU.
*/
me = this_cpu();
if (cpu != me)
{
- DEBUGVERIFY(up_cpu_pause(cpu));
+ if (g_delivertasks[cpu] == NULL)
+ {
+ g_delivertasks[cpu] = btcb;
+ btcb->cpu = cpu;
+ btcb->task_state = TSTATE_TASK_ASSIGNED;
+ up_cpu_pause_async(cpu);
+ }
+ else
+ {
+ rtcb = g_delivertasks[cpu];
+ if (rtcb->sched_priority < btcb->sched_priority)
+ {
+ g_delivertasks[cpu] = btcb;
+ btcb->cpu = cpu;
+ btcb->task_state = TSTATE_TASK_ASSIGNED;
+ btcb = rtcb;
Review Comment:
why not remove the assignment, and use rtcb in the following line
##########
sched/task/task_restart.c:
##########
@@ -135,14 +134,7 @@ static int nxtask_restart(pid_t pid)
* should no longer be accessible to the system
*/
-#ifdef CONFIG_SMP
- tasklist = TLIST_HEAD(&tcb->cmn, tcb->cmn.cpu);
-#else
- tasklist = TLIST_HEAD(&tcb->cmn);
-#endif
-
- dq_rem((FAR dq_entry_t *)tcb, tasklist);
- tcb->cmn.task_state = TSTATE_TASK_INVALID;
+ nxsched_remove_readytorun((FAR struct tcb_s *)tcb, false);
Review Comment:
can we call more simple variant
##########
sched/sched/sched_removereadytorun.c:
##########
@@ -325,6 +325,18 @@ bool nxsched_remove_readytorun(FAR struct tcb_s *tcb, bool merge)
{
FAR dq_queue_t *tasklist;
+ /* if tcb == g_delivertasks[i] we set NULL to g_delivertasks[i] */
+
+ for (int i = 0; i < CONFIG_SMP_NCPUS; i++)
Review Comment:
move int i out of loop
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] change the SMP scheduling policy from synchronous to asynchronous [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #13616:
URL: https://github.com/apache/nuttx/pull/13616#issuecomment-2375495211
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
## NuttX PR Requirements Review
This PR appears to meet most NuttX requirements, but some clarifications are needed:
**Strengths:**
* **Clear Summary:** The summary explains the problem (inefficient cross-core task scheduling), the solution (cross-core interrupts), and the benefits (reduced overhead, no busyloops).
* **Impact Well-Defined:** The impact on SMP architectures is noted, but more details are needed for each "Impact" sub-point (User, Build, Hardware, Documentation, Security, Compatibility). For example:
* **User:** Will applications need to change how they schedule tasks across cores?
* **Build:** Are any new configuration options introduced?
* **Hardware:** Are specific interrupt mechanisms required on target platforms?
* **Testing Provided:** Performance testing results are included, demonstrating improvements on a specific platform.
**Areas for Improvement:**
* **Complete Impact Assessment:** Provide specific answers for each impact sub-point. Even if the answer is "NO," briefly explain why.
* **Testing Details:**
* **Environment:** Specify the OS, CPU, compiler used for the build host in the "Testing" section.
* **Target Details:** Provide the full `board:config` used for the Cortex-R82 tests.
* **Monkey Test:** Briefly describe the "monkey test" and its significance.
* **Before/After Logs:** The "Testing logs" sections are empty. Include relevant log snippets showing the improvement.
**Conciseness:** The provided information is concise and well-written.
**Overall:**
This PR demonstrates a promising improvement, but requires a more thorough impact assessment and additional testing details before it can be considered complete.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] change the SMP scheduling policy from synchronous to asynchronous [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on PR #13616:
URL: https://github.com/apache/nuttx/pull/13616#issuecomment-2376227249
> > > > all smp arch
> > >
> > >
> > > @hujun260 Which configurations did you test with this PR?
> >
> >
> > qemu-armv7a:smp qemu-armv8a:nsh_smp
>
> Please test esp32-devkitc:smp, rv-virt:smp, rv-virt:smp64, rv-virt:ksmp64 and sim:smp, since the PR affects these architectures. Aslo, if you have spresense and raspberrypi-pico, please test the boards as well.
esp32-devkitc:smp ostest test ok
sim:smp can't boot, not caused by this pr
rv-virt:smp ostest test ok
rv-virt:smp64 ostest test ok
rv-virt:ksmp64 ostest test ok (except vfork break introdced by #12812)
raspberrypi-pico:smp ostest test 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] change the SMP scheduling policy from synchronous to asynchronous [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #13616:
URL: https://github.com/apache/nuttx/pull/13616
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] change the SMP scheduling policy from synchronous to asynchronous [nuttx]
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #13616:
URL: https://github.com/apache/nuttx/pull/13616#issuecomment-2375812605
> > > all smp arch
> >
> >
> > @hujun260 Which configurations did you test with this PR?
>
> qemu-armv7a:smp qemu-armv8a:nsh_smp
Please test esp32-devkitc:smp, rv-virt:smp, rv-virt:smp64, rv-virt:ksmp64 and sim:smp, since the PR affects these architectures.
Aslo, if you have spresense and raspberrypi-pico, please test the boards as well.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] change the SMP scheduling policy from synchronous to asynchronous [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on PR #13616:
URL: https://github.com/apache/nuttx/pull/13616#issuecomment-2375750957
> > all smp arch
>
> @hujun260 Which configurations did you test with this PR?
qemu-armv7a:smp
qemu-armv8a:nsh_smp
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org