You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by ma...@apache.org on 2020/11/21 22:48:03 UTC
[incubator-nuttx] branch releases/10.0 updated (224e0de -> f114189)
This is an automated email from the ASF dual-hosted git repository.
masayuki pushed a change to branch releases/10.0
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git.
from 224e0de boards: sabre-6quad: Fix README.txt
new 31a8167 arch: cxd56xx: Fix the pause handler for SMP
new 8fd7978 arch: armv7-a: Fix the pause handler for SMP
new 6f03cf4 arch: lc823450: Fix the pause handler for SMP
new 249dc11 arch: k210: Fix the pause handler for SMP
new d1a6f79 arch: xtensa: Fix the pause handler for SMP
new f114189 Update TODO regarding SMP
The 6 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
TODO | 66 +------------------------------
arch/arm/src/armv7-a/arm_cpupause.c | 16 +++++++-
arch/arm/src/cxd56xx/cxd56_cpupause.c | 16 +++++++-
arch/arm/src/lc823450/lc823450_cpupause.c | 16 +++++++-
arch/risc-v/src/k210/k210_cpupause.c | 16 +++++++-
arch/xtensa/src/common/xtensa_cpupause.c | 11 ++++++
6 files changed, 68 insertions(+), 73 deletions(-)
[incubator-nuttx] 03/06: arch: lc823450: Fix the pause handler for
SMP
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
masayuki pushed a commit to branch releases/10.0
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit 6f03cf488677721b8054792474efa590e89f482b
Author: Masayuki Ishikawa <ma...@gmail.com>
AuthorDate: Fri Nov 20 13:36:17 2020 +0900
arch: lc823450: Fix the pause handler for SMP
Summary:
- Apply the same logic added to cxd56_cpupause.c
Impact:
- SMP only
Testing:
- Tested with lc823450-xgevk:rndis
- Run smp and ostest
Signed-off-by: Masayuki Ishikawa <Ma...@jp.sony.com>
---
arch/arm/src/lc823450/lc823450_cpupause.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/arm/src/lc823450/lc823450_cpupause.c b/arch/arm/src/lc823450/lc823450_cpupause.c
index 2714fc3..b172df1 100644
--- a/arch/arm/src/lc823450/lc823450_cpupause.c
+++ b/arch/arm/src/lc823450/lc823450_cpupause.c
@@ -190,6 +190,7 @@ int up_cpu_paused(int cpu)
int lc823450_pause_handler(int irq, void *c, FAR void *arg)
{
int cpu = up_cpu_index();
+ int ret = OK;
/* Clear : Pause IRQ */
@@ -211,10 +212,21 @@ int lc823450_pause_handler(int irq, void *c, FAR void *arg)
if (spin_islocked(&g_cpu_paused[cpu]))
{
- return up_cpu_paused(cpu);
+ /* NOTE: up_cpu_paused() needs to be executed in a critical section
+ * to ensure that this CPU holds g_cpu_irqlock. However, adding
+ * a critical section in up_cpu_paused() is not a good idea,
+ * because it is also called in enter_critical_section() to break
+ * a deadlock
+ */
+
+ irqstate_t flags = enter_critical_section();
+
+ ret = up_cpu_paused(cpu);
+
+ leave_critical_section(flags);
}
- return OK;
+ return ret;
}
/****************************************************************************
[incubator-nuttx] 04/06: arch: k210: Fix the pause handler for SMP
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
masayuki pushed a commit to branch releases/10.0
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit 249dc11e0508a8ce11c93020af6e8c1627b6db3f
Author: Masayuki Ishikawa <ma...@gmail.com>
AuthorDate: Fri Nov 20 13:45:53 2020 +0900
arch: k210: Fix the pause handler for SMP
Summary:
- Apply the same logic added to cxd56_cpupause.c
Impact:
- SMP only
Testing:
- Tested with maix-bit:smp (QEMU)
- Run smp and ostest
Signed-off-by: Masayuki Ishikawa <Ma...@jp.sony.com>
---
arch/risc-v/src/k210/k210_cpupause.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/risc-v/src/k210/k210_cpupause.c b/arch/risc-v/src/k210/k210_cpupause.c
index 88af783..2eaa573 100644
--- a/arch/risc-v/src/k210/k210_cpupause.c
+++ b/arch/risc-v/src/k210/k210_cpupause.c
@@ -205,6 +205,7 @@ int up_cpu_paused(int cpu)
int riscv_pause_handler(int irq, void *c, FAR void *arg)
{
int cpu = up_cpu_index();
+ int ret = OK;
/* Clear machine software interrupt */
@@ -217,10 +218,21 @@ int riscv_pause_handler(int irq, void *c, FAR void *arg)
if (spin_islocked(&g_cpu_paused[cpu]))
{
- return up_cpu_paused(cpu);
+ /* NOTE: up_cpu_paused() needs to be executed in a critical section
+ * to ensure that this CPU holds g_cpu_irqlock. However, adding
+ * a critical section in up_cpu_paused() is not a good idea,
+ * because it is also called in enter_critical_section() to break
+ * a deadlock
+ */
+
+ irqstate_t flags = enter_critical_section();
+
+ ret = up_cpu_paused(cpu);
+
+ leave_critical_section(flags);
}
- return OK;
+ return ret;
}
/****************************************************************************
[incubator-nuttx] 02/06: arch: armv7-a: Fix the pause handler for
SMP
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
masayuki pushed a commit to branch releases/10.0
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit 8fd797841d32941d5e25e6c98367e0c172b27c4c
Author: Masayuki Ishikawa <ma...@gmail.com>
AuthorDate: Fri Nov 20 13:33:43 2020 +0900
arch: armv7-a: Fix the pause handler for SMP
Summary:
- Apply the same logic added to cxd56_cpupause.c
Impact:
- SMP only
Testing:
- Tested with sabre-6quad:smp (QEMU and dev board)
- Run smp and ostest
Signed-off-by: Masayuki Ishikawa <Ma...@jp.sony.com>
---
arch/arm/src/armv7-a/arm_cpupause.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/arm/src/armv7-a/arm_cpupause.c b/arch/arm/src/armv7-a/arm_cpupause.c
index 4725418..36d1eb0 100644
--- a/arch/arm/src/armv7-a/arm_cpupause.c
+++ b/arch/arm/src/armv7-a/arm_cpupause.c
@@ -205,6 +205,7 @@ int up_cpu_paused(int cpu)
int arm_pause_handler(int irq, FAR void *context, FAR void *arg)
{
int cpu = this_cpu();
+ int ret = OK;
/* Check for false alarms. Such false could occur as a consequence of
* some deadlock breaking logic that might have already serviced the SG2
@@ -214,10 +215,21 @@ int arm_pause_handler(int irq, FAR void *context, FAR void *arg)
if (spin_islocked(&g_cpu_paused[cpu]))
{
- return up_cpu_paused(cpu);
+ /* NOTE: up_cpu_paused() needs to be executed in a critical section
+ * to ensure that this CPU holds g_cpu_irqlock. However, adding
+ * a critical section in up_cpu_paused() is not a good idea,
+ * because it is also called in enter_critical_section() to break
+ * a deadlock
+ */
+
+ irqstate_t flags = enter_critical_section();
+
+ ret = up_cpu_paused(cpu);
+
+ leave_critical_section(flags);
}
- return OK;
+ return ret;
}
/****************************************************************************
[incubator-nuttx] 01/06: arch: cxd56xx: Fix the pause handler for
SMP
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
masayuki pushed a commit to branch releases/10.0
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit 31a8167821b0a82a384dfd0e4d3ef867e2a8184d
Author: Masayuki Ishikawa <ma...@gmail.com>
AuthorDate: Fri Nov 20 13:16:45 2020 +0900
arch: cxd56xx: Fix the pause handler for SMP
Summary:
- I noticed that sched_add_readytorun() runs on multiple CPUs simultaneously
- Finally, I found the root cause which was described in TODO
- Actually, the task newly scheduled on remote CPU did not acquire g_cpu_irqlock
- This commit fixes this issue by adding a critical section to the pause handler
- Which will acquire g_cpu_irqlock on the remote CPU explicitly
Impact:
- SMP only
Testing:
- Tested with spresense:wifi_smp (NCPUS=2 and 4)
- Run smp, ostest, nxplayer
Signed-off-by: Masayuki Ishikawa <Ma...@jp.sony.com>
---
arch/arm/src/cxd56xx/cxd56_cpupause.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/arm/src/cxd56xx/cxd56_cpupause.c b/arch/arm/src/cxd56xx/cxd56_cpupause.c
index c8bd410..1f5e6b4 100644
--- a/arch/arm/src/cxd56xx/cxd56_cpupause.c
+++ b/arch/arm/src/cxd56xx/cxd56_cpupause.c
@@ -283,6 +283,7 @@ int up_cpu_paused(int cpu)
int arm_pause_handler(int irq, void *c, FAR void *arg)
{
int cpu = up_cpu_index();
+ int ret = OK;
DPRINTF("cpu%d will be paused \n", cpu);
@@ -297,10 +298,21 @@ int arm_pause_handler(int irq, void *c, FAR void *arg)
if (spin_islocked(&g_cpu_paused[cpu]))
{
- return up_cpu_paused(cpu);
+ /* NOTE: up_cpu_paused() needs to be executed in a critical section
+ * to ensure that this CPU holds g_cpu_irqlock. However, adding
+ * a critical section in up_cpu_paused() is not a good idea,
+ * because it is also called in enter_critical_section() to break
+ * a deadlock
+ */
+
+ irqstate_t flags = enter_critical_section();
+
+ ret = up_cpu_paused(cpu);
+
+ leave_critical_section(flags);
}
- return OK;
+ return ret;
}
/****************************************************************************
[incubator-nuttx] 05/06: arch: xtensa: Fix the pause handler for SMP
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
masayuki pushed a commit to branch releases/10.0
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit d1a6f7969c06625981077ac623d042b14944ea48
Author: Masayuki Ishikawa <ma...@gmail.com>
AuthorDate: Fri Nov 20 13:47:56 2020 +0900
arch: xtensa: Fix the pause handler for SMP
Summary:
- Apply the same logic added to cxd56_cpupause.c
Impact:
- SMP only
Testing:
- Tested with esp32-core:smp (QEMU)
- Run smp and ostest
Signed-off-by: Masayuki Ishikawa <Ma...@jp.sony.com>
---
arch/xtensa/src/common/xtensa_cpupause.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/xtensa/src/common/xtensa_cpupause.c b/arch/xtensa/src/common/xtensa_cpupause.c
index 5b59200..0d7c486 100644
--- a/arch/xtensa/src/common/xtensa_cpupause.c
+++ b/arch/xtensa/src/common/xtensa_cpupause.c
@@ -192,7 +192,18 @@ void xtensa_pause_handler(void)
if (spin_islocked(&g_cpu_paused[cpu]))
{
+ /* NOTE: up_cpu_paused() needs to be executed in a critical section
+ * to ensure that this CPU holds g_cpu_irqlock. However, adding
+ * a critical section in up_cpu_paused() is not a good idea,
+ * because it is also called in enter_critical_section() to break
+ * a deadlock
+ */
+
+ irqstate_t flags = enter_critical_section();
+
up_cpu_paused(cpu);
+
+ leave_critical_section(flags);
}
}
[incubator-nuttx] 06/06: Update TODO regarding SMP
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
masayuki pushed a commit to branch releases/10.0
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit f11418934b0884235c50b7dc649585e336a0933d
Author: Masayuki Ishikawa <ma...@gmail.com>
AuthorDate: Fri Nov 20 14:27:54 2020 +0900
Update TODO regarding SMP
Summary:
- 'POSSIBLE FOR TWO CPUs TO HOLD A CRITICAL SECTION' was resolved
Signed-off-by: Masayuki Ishikawa <Ma...@jp.sony.com>
---
TODO | 66 +-----------------------------------------------------------------
1 file changed, 1 insertion(+), 65 deletions(-)
diff --git a/TODO b/TODO
index bffa549..d56cf2d 100644
--- a/TODO
+++ b/TODO
@@ -10,7 +10,7 @@ issues related to each board port.
nuttx/:
(16) Task/Scheduler (sched/)
- (3) SMP
+ (2) SMP
(1) Memory Management (mm/)
(0) Power Management (drivers/pm)
(5) Signals (sched/signal, arch/)
@@ -485,70 +485,6 @@ o SMP
an bugs caused by this. But I believe that failures are
possible.
- Title: POSSIBLE FOR TWO CPUs TO HOLD A CRITICAL SECTION?
- Description: The SMP design includes logic that will support multiple
- CPUs holding a critical section. Is this necessary? How
- can that occur? I think it can occur in the following
- situation:
-
- The log below was reported is Nuttx running on two cores
- Cortex-A7 architecture in SMP mode. You can notice see that
- when nxsched_add_readytorun() was called, the g_cpu_irqset is 3.
-
- nxsched_add_readytorun: irqset cpu 1, me 0 btcbname init, irqset 1 irqcount 2.
- nxsched_add_readytorun: nxsched_add_readytorun line 338 g_cpu_irqset = 3.
-
- This can happen, but only under a very certain condition.
- g_cpu_irqset only exists to support this certain condition:
-
- a. A task running on CPU 0 takes the critical section. So
- g_cpu_irqset == 0x1.
-
- b. A task exits on CPU 1 and a waiting, ready-to-run task
- is re-started on CPU 1. This new task also holds the
- critical section. So when the task is re-restarted on
- CPU 1, we than have g_cpu_irqset == 0x3
-
- So we are in a very perverse state! There are two tasks
- running on two different CPUs and both hold the critical
- section. I believe that is a dangerous situation and there
- could be undiscovered bugs that could happen in that case.
- However, as of this moment, I have not heard of any specific
- problems caused by this weird behavior.
-
- A possible solution would be to add a new task state that
- would exist only for SMP.
-
- - Add a new SMP-only task list and state. Say,
- g_csection_wait[]. It should be prioritized.
- - When a task acquires the critical section, all tasks in
- g_readytorun[] that need the critical section would be
- moved to g_csection_wait[].
- - When any task is unblocked for any reason and moved to the
- g_readytorun[] list, if that unblocked task needs the
- critical section, it would also be moved to the
- g_csection_wait[] list. No task that needs the critical
- section can be in the ready-to-run list if the critical
- section is not available.
- - When the task releases the critical section, all tasks in
- the g_csection_wait[] needs to be moved back to
- g_readytorun[].
- - This may result in a context switch. The tasks should be
- moved back to g_readytorun[] highest priority first. If a
- context switch occurs and the critical section to re-taken
- by the re-started task, the lower priority tasks in
- g_csection_wait[] must stay in that list.
-
- That is really not as much work as it sounds. It is
- something that could be done in 2-3 days of work if you know
- what you are doing. Getting the proper test setup and
- verifying the change would be the more difficult task.
-
-Status: Open
-Priority: Unknown. Might be high, but first we would need to confirm
- that this situation can occur and that is actually causes
- a failure.
-
o Memory Management (mm/)
^^^^^^^^^^^^^^^^^^^^^^^