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/11/14 08:08:06 UTC
[GitHub] [incubator-nuttx] masayuki2009 opened a new pull request #2298: sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
masayuki2009 opened a new pull request #2298:
URL: https://github.com/apache/incubator-nuttx/pull/2298
## Summary
- I noticed waitpid_test stops with lc823450-xgevk:rndis
- The condition was CONFIG_DEBUG_ASSERTION=y
- Actually, the child task sent SIGCHILD but the parent couldn't catch the signal
- Then, I found that nx_waitid(), nx_waitpid() use sched_lock()
- However, a parent task and a child task are running on different CPUs
- So, sched_lock() is not enough and need to use a critical section
- Also, signal handling in nxtask_exithook() must be done in a critical section
## Impact
- SMP only
## Testing
- Tested with ostest with the following configurations
- lc823450-xgevk:rndis (CONFIG_DEBUG_ASSERTION=y and n)
- spresense:smp
- spresense:wifi_smp (NCPUS=2 and 4)
- sabre-6quad:smp (QEMU)
- esp32-core:smp (QEMU)
- maix-bit:smp (QEMU)
- sim: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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #2298: sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2298:
URL: https://github.com/apache/incubator-nuttx/pull/2298
----------------------------------------------------------------
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] Ouss4 commented on a change in pull request #2298: sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2298:
URL: https://github.com/apache/incubator-nuttx/pull/2298#discussion_r523418880
##########
File path: sched/sched/sched_waitid.c
##########
@@ -143,6 +143,14 @@ int nx_waitid(int idtype, id_t id, FAR siginfo_t *info, int options)
sigemptyset(&set);
nxsig_addset(&set, SIGCHLD);
+ /* NOTE: sched_lock() is not enough for SMP
+ * because the child task is running on another CPU
+ */
+
+#ifdef CONFIG_SMP
+ irqstate_t flags = enter_critical_section();
Review comment:
Isn't the call to `sched_(un)lock` in line 156 not needed then?
----------------------------------------------------------------
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 #2298: sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2298:
URL: https://github.com/apache/incubator-nuttx/pull/2298#discussion_r523594022
##########
File path: sched/sched/sched_waitid.c
##########
@@ -143,6 +143,14 @@ int nx_waitid(int idtype, id_t id, FAR siginfo_t *info, int options)
sigemptyset(&set);
nxsig_addset(&set, SIGCHLD);
+ /* NOTE: sched_lock() is not enough for SMP
+ * because the child task is running on another CPU
+ */
+
+#ifdef CONFIG_SMP
+ irqstate_t flags = enter_critical_section();
Review comment:
> I think replacing sched_lock() and sched_unlock() with a critical section should be a future task.
Discussed a little in issue #1138, Related to issue #1137
----------------------------------------------------------------
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] Ouss4 commented on a change in pull request #2298: sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2298:
URL: https://github.com/apache/incubator-nuttx/pull/2298#discussion_r523418880
##########
File path: sched/sched/sched_waitid.c
##########
@@ -143,6 +143,14 @@ int nx_waitid(int idtype, id_t id, FAR siginfo_t *info, int options)
sigemptyset(&set);
nxsig_addset(&set, SIGCHLD);
+ /* NOTE: sched_lock() is not enough for SMP
+ * because the child task is running on another CPU
+ */
+
+#ifdef CONFIG_SMP
+ irqstate_t flags = enter_critical_section();
Review comment:
Isn't the call to `sched_(un)lock` not needed then?
----------------------------------------------------------------
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 #2298: sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2298:
URL: https://github.com/apache/incubator-nuttx/pull/2298#issuecomment-727276818
See issue #1137
----------------------------------------------------------------
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 #2298: sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #2298:
URL: https://github.com/apache/incubator-nuttx/pull/2298#discussion_r523468256
##########
File path: sched/sched/sched_waitid.c
##########
@@ -143,6 +143,14 @@ int nx_waitid(int idtype, id_t id, FAR siginfo_t *info, int options)
sigemptyset(&set);
nxsig_addset(&set, SIGCHLD);
+ /* NOTE: sched_lock() is not enough for SMP
+ * because the child task is running on another CPU
+ */
+
+#ifdef CONFIG_SMP
+ irqstate_t flags = enter_critical_section();
Review comment:
@Ouss4
Thanks for your comment. I will modify the code and do the regression test again.
----------------------------------------------------------------
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 #2298: sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #2298:
URL: https://github.com/apache/incubator-nuttx/pull/2298#discussion_r523497562
##########
File path: sched/sched/sched_waitid.c
##########
@@ -143,6 +143,14 @@ int nx_waitid(int idtype, id_t id, FAR siginfo_t *info, int options)
sigemptyset(&set);
nxsig_addset(&set, SIGCHLD);
+ /* NOTE: sched_lock() is not enough for SMP
+ * because the child task is running on another CPU
+ */
+
+#ifdef CONFIG_SMP
+ irqstate_t flags = enter_critical_section();
Review comment:
@Ouss4
I tried the following change in sched_waitid.c and sched_waitpid.c
The ostest passed for the all SMP configurations.
However, Wi-Fi streaming plus stress testing with spresense:wifi_smp (NCPUS=4, DEBUG_ASSERTIONS=y)
failed with DEBUGASSERTION after 3.5 hours. (FYI, without the following changes, the test continued over 9 hours)
I think replacing sched_lock() and sched_unlock() with a critical section should be a future task.
What do you think?
```
#ifdef CONFIG_SMP
/* NOTE: sched_lock() is not enough for SMP
* because the child task is running on another CPU
*/
irqstate_t flags = enter_critical_section();
#else
/* Disable pre-emption so that nothing changes while the loop executes */
sched_lock();
#endif
...
#ifdef CONFIG_SMP
leave_critical_section(flags);
#else
sched_unlock();
#endif
```
----------------------------------------------------------------
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