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/09/28 23:35:00 UTC
[GitHub] [incubator-nuttx] masayuki2009 opened a new pull request #1912: Fix up_interrupt_context() for SMP
masayuki2009 opened a new pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912
## Summary
- I found an issue with up_interrupt_context() when testing.
- And finally found that up_interrupt_context() is not atomic.
- This commit fixes the issue
## Impact
- Affects SMP only
## Testing
- Tested with spresense:wifi_smp and sabre-6quad:smp (qemu)
- Tested with maix-bit:smp (qemu)
- Tested with esp32-core:smp (qemu)
----------------------------------------------------------------
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] masayuki2009 commented on a change in pull request #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r497354379
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
+ irqstate_t flags = up_irq_save();
Review comment:
@patacongo
This is the same as what I did for this_task() in sched/sched_thistask.c
So, may I merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r496308387
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
+ irqstate_t flags = up_irq_save();
Review comment:
Just to clarify, it is non-atomic in the SMP case because CURRENT_REGS expands to:
# define CURRENT_REGS (g_current_regs[up_cpu_index()])
In the non-SMP case, it expands to:
# define CURRENT_REGS (g_current_regs[0])
----------------------------------------------------------------
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] jerpelea commented on pull request #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
jerpelea commented on pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#issuecomment-701264896
LGTM
----------------------------------------------------------------
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 #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r497528189
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
+ irqstate_t flags = up_irq_save();
Review comment:
>
>
> @patacongo
>
> This is the same as what I did for this_task() in sched/sched_thistask.c
> So, may I merge this PR?
Well, we should never merge our own PRs. But I suppose we should merge this now. There is, however, a problem with the ARMv7-A with the GIC. The use of up_irq_save() will not disable the SGIs used for inter-processor signalling, at least not until use of the ICCMPR is used as you suggested.
Perhaps we should add a comment about this in the PR? 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] masayuki2009 commented on pull request #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#issuecomment-701685887
@patacongo
Thanks for adding the comments.
----------------------------------------------------------------
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 #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r497551864
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
+ irqstate_t flags = up_irq_save();
Review comment:
I already pushed the comment change and will merge in just a few minutes. Thanks! I did not expect you to still be working. So I can handle the rest.
----------------------------------------------------------------
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] masayuki2009 commented on a change in pull request #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r497532181
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
+ irqstate_t flags = up_irq_save();
Review comment:
> There is, however, a problem with this change for the ARMv7-A with the GIC. The use of up_irq_save() will not disable the SGIs used for inter-processor signalling, at least not until use of the ICCMPR is used as you suggested. So I think that the fix does not work for that case.
>
> Perhaps we should add a comment about this in the PR? What do you think?
@patacongo
OK, I will add a comment.
----------------------------------------------------------------
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] masayuki2009 commented on a change in pull request #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r496328310
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
+ irqstate_t flags = up_irq_save();
Review comment:
> Just to clarify, it is non-atomic in the SMP case because CURRENT_REGS expands to:
>
> ```
> # define CURRENT_REGS (g_current_regs[up_cpu_index()])
>```
@patacongo
Thanks for the comments.
Yes, what I noticed at a Wi-Fi streaming aging test with Spresense was
NuttX stopped with DEBUGASSERT at the following code.
```
nxplayer> [ 4916.595630] cxd56_configure: ERROR: Unknown feature unit: 2048
[ 4916.598621] cxd56_configure: ERROR: Unknown feature unit: 4
[ 4916.604175] cxd56_configure: ERROR: Unknown feature unit: 16
[ 4982.154812] cxd56_start_dma: Underrun
[ 4982.205165] up_assert: Assertion failed CPU1 at file:semaphore/sem_wait.c line: 78 task: gs2200m
int nxsem_wait(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
irqstate_t flags;
int ret = -EINVAL;
/* This API should not be called from interrupt handlers */
=> DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
````
Actually, the sem was not NULL but g_current_regs were the following values.
```
(gdb) p g_current_regs
$1 = {0x0, 0xd05df54}
```
Here, the DEBUGASSERT() happened on CPU1, and it was not in the interrupt context.
However, CPU0 was in the interrupt context, so I found that the task was moved from
CPU0 to CPU1 during calling up_interruput_context() in nxsem_wait().
With this PR, a Wi-Fi streaming aging test is still running over 12hrs.
----------------------------------------------------------------
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 #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r497531294
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
Review comment:
```suggestion
#ifdef CONFIG_SMP
/* REVISIT: Currently up_irq_save() will not disable the Software generated interrupts (SGIs)
* for the case of ARMv7-A architecture using the GIC. So this will not be sufficient in
* that case, at least not until we add support for the ICCMPR.
*/
```
----------------------------------------------------------------
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 #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r497538032
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
Review comment:
I tried to commit this so that I could merge your changes with this comment, but the lines are too long. I will try to modify you fork to fix the line width.
----------------------------------------------------------------
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 #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912#discussion_r497528189
##########
File path: arch/arm/src/common/arm_interruptcontext.c
##########
@@ -59,5 +59,15 @@
bool up_interrupt_context(void)
{
- return CURRENT_REGS != NULL;
+#ifdef CONFIG_SMP
+ irqstate_t flags = up_irq_save();
Review comment:
>
>
> @patacongo
>
> This is the same as what I did for this_task() in sched/sched_thistask.c
> So, may I merge this PR?
Well, we should never merge our own PRs. But I suppose we should merge this now. Alin or I can do that.
There is, however, a problem with this change for the ARMv7-A with the GIC. The use of up_irq_save() will not disable the SGIs used for inter-processor signalling, at least not until use of the ICCMPR is used as you suggested. So I think that the fix does not work for that case.
Perhaps we should add a comment about this in the PR? 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] patacongo merged pull request #1912: Fix up_interrupt_context() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #1912:
URL: https://github.com/apache/incubator-nuttx/pull/1912
----------------------------------------------------------------
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