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 2021/03/01 23:52:38 UTC
[GitHub] [incubator-nuttx] masayuki2009 opened a new pull request #2946: sched: task: Call nxtask_flushstreams() without critical section
masayuki2009 opened a new pull request #2946:
URL: https://github.com/apache/incubator-nuttx/pull/2946
## Summary
- During investigating critical section with semaphores, I noticed
that nxtask_flushstreams() is called with a critical section.
- The function calls lib_flushall() which handles a semaphore
in userspace.
- So it should be done without a critical section
## Impact
- SMP only
## Testing
- Tested with ostest the following configs
- esp32-devkitc:smp (QEMU), sabre-6quad:smp (QEMU)
- maix-bit:smp (QEMU), sim:smp
- spresense:smp
- Tested with nxplayer and stress test with spresense:wifi_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] masayuki2009 commented on a change in pull request #2946: sched: task: Call nxtask_flushstreams() without critical section
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #2946:
URL: https://github.com/apache/incubator-nuttx/pull/2946#discussion_r585251195
##########
File path: sched/task/task_exithook.c
##########
@@ -645,7 +645,20 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
if (!nonblocking)
{
+ /* NOTE: nxtask_flushstreams() calls lib_flushall()
+ * which handles a semaphore in userspace.
+ * It should be done without a critical section.
+ */
+
+#ifdef CONFIG_SMP
+ leave_critical_section(flags);
+#endif
+
nxtask_flushstreams(tcb);
Review comment:
@xiaoxiang781216
Do you mean that we should move the code block before nxtask_exitwakeup() like
```
diff --git a/sched/task/task_exithook.c b/sched/task/task_exithook.c
index 8378c0c8fc..0afec6a007 100644
--- a/sched/task/task_exithook.c
+++ b/sched/task/task_exithook.c
@@ -612,6 +612,22 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
}
#endif
+ /* If this is the last thread in the group, then flush all streams (File
+ * descriptors will be closed when the TCB is deallocated).
+ *
+ * NOTES:
+ * 1. We cannot flush the buffered I/O if nonblocking is requested.
+ * that might cause this logic to block.
+ * 2. This function will only be called with non-blocking == true
+ * only when called through _exit(). _exit() behavior does not
+ * require that the streams be flushed
+ */
+
+ if (!nonblocking)
+ {
+ nxtask_flushstreams(tcb);
+ }
+
/* If the task was terminated by another task, it may be in an unknown
* state. Make some feeble effort to recover the state.
*/
@@ -632,22 +648,6 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
nxtask_exitwakeup(tcb, status);
- /* If this is the last thread in the group, then flush all streams (File
- * descriptors will be closed when the TCB is deallocated).
- *
- * NOTES:
- * 1. We cannot flush the buffered I/O if nonblocking is requested.
- * that might cause this logic to block.
- * 2. This function will only be called with non-blocking == true
- * only when called through _exit(). _exit() behavior does not
- * require that the streams be flushed
- */
-
- if (!nonblocking)
- {
- nxtask_flushstreams(tcb);
- }
-
/* Leave the task group. Perhaps discarding any un-reaped child
* status (no zombies 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] xiaoxiang781216 commented on a change in pull request #2946: sched: task: Call nxtask_flushstreams() without critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2946:
URL: https://github.com/apache/incubator-nuttx/pull/2946#discussion_r585283915
##########
File path: sched/task/task_exithook.c
##########
@@ -645,7 +645,20 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
if (!nonblocking)
{
+ /* NOTE: nxtask_flushstreams() calls lib_flushall()
+ * which handles a semaphore in userspace.
+ * It should be done without a critical section.
+ */
+
+#ifdef CONFIG_SMP
+ leave_critical_section(flags);
+#endif
+
nxtask_flushstreams(tcb);
Review comment:
Yes, but we can merge two "if (!nonblocking)..." into one
----------------------------------------------------------------
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 #2946: sched: task: Call nxtask_flushstreams() without critical section
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #2946:
URL: https://github.com/apache/incubator-nuttx/pull/2946#discussion_r585295414
##########
File path: sched/task/task_exithook.c
##########
@@ -645,7 +645,20 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
if (!nonblocking)
{
+ /* NOTE: nxtask_flushstreams() calls lib_flushall()
+ * which handles a semaphore in userspace.
+ * It should be done without a critical section.
+ */
+
+#ifdef CONFIG_SMP
+ leave_critical_section(flags);
+#endif
+
nxtask_flushstreams(tcb);
Review comment:
@xiaoxiang781216
OK. How about this change?
```
diff --git a/sched/task/task_exithook.c b/sched/task/task_exithook.c
index 8378c0c8fc..0e34a99ab7 100644
--- a/sched/task/task_exithook.c
+++ b/sched/task/task_exithook.c
@@ -586,7 +586,6 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
tcb->cpcount = 0;
#endif
-#if defined(CONFIG_SCHED_ATEXIT) || defined(CONFIG_SCHED_ONEXIT)
/* If exit function(s) were registered, call them now before we do any un-
* initialization.
*
@@ -604,14 +603,28 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
if (!nonblocking)
{
+#if defined(CONFIG_SCHED_ATEXIT) || defined(CONFIG_SCHED_ONEXIT)
nxtask_atexit(tcb);
/* Call any registered on_exit function(s) */
nxtask_onexit(tcb, status);
- }
#endif
+ /* If this is the last thread in the group, then flush all streams
+ * (File descriptors will be closed when the TCB is deallocated).
+ *
+ * NOTES:
+ * 1. We cannot flush the buffered I/O if nonblocking is requested.
+ * that might cause this logic to block.
+ * 2. This function will only be called with non-blocking == true
+ * only when called through _exit(). _exit() behavior does not
+ * require that the streams be flushed
+ */
+
+ nxtask_flushstreams(tcb);
+ }
+
/* If the task was terminated by another task, it may be in an unknown
* state. Make some feeble effort to recover the state.
*/
@@ -632,22 +645,6 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
nxtask_exitwakeup(tcb, status);
- /* If this is the last thread in the group, then flush all streams (File
- * descriptors will be closed when the TCB is deallocated).
- *
- * NOTES:
- * 1. We cannot flush the buffered I/O if nonblocking is requested.
- * that might cause this logic to block.
- * 2. This function will only be called with non-blocking == true
- * only when called through _exit(). _exit() behavior does not
- * require that the streams be flushed
- */
-
- if (!nonblocking)
- {
- nxtask_flushstreams(tcb);
- }
-
/* Leave the task group. Perhaps discarding any un-reaped child
* status (no zombies 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] xiaoxiang781216 merged pull request #2946: sched: task: Call nxtask_flushstreams() without critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2946:
URL: https://github.com/apache/incubator-nuttx/pull/2946
----------------------------------------------------------------
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 #2946: sched: task: Call nxtask_flushstreams() without critical section
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #2946:
URL: https://github.com/apache/incubator-nuttx/pull/2946#discussion_r585434684
##########
File path: sched/task/task_exithook.c
##########
@@ -645,7 +645,20 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
if (!nonblocking)
{
+ /* NOTE: nxtask_flushstreams() calls lib_flushall()
+ * which handles a semaphore in userspace.
+ * It should be done without a critical section.
+ */
+
+#ifdef CONFIG_SMP
+ leave_critical_section(flags);
+#endif
+
nxtask_flushstreams(tcb);
Review comment:
@xiaoxiang781216
Done
----------------------------------------------------------------
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 #2946: sched: task: Call nxtask_flushstreams() without critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2946:
URL: https://github.com/apache/incubator-nuttx/pull/2946#discussion_r585226542
##########
File path: sched/task/task_exithook.c
##########
@@ -645,7 +645,20 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
if (!nonblocking)
{
+ /* NOTE: nxtask_flushstreams() calls lib_flushall()
+ * which handles a semaphore in userspace.
+ * It should be done without a critical section.
+ */
+
+#ifdef CONFIG_SMP
+ leave_critical_section(flags);
+#endif
+
nxtask_flushstreams(tcb);
Review comment:
should we move after line 611? since:
1. It mayn't safe to exit the critical section temporarily at this point
2. It look safe to move the flush ahead since the code from line 611 to here is just for notification
----------------------------------------------------------------
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 #2946: sched: task: Call nxtask_flushstreams() without critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2946:
URL: https://github.com/apache/incubator-nuttx/pull/2946#discussion_r585377480
##########
File path: sched/task/task_exithook.c
##########
@@ -645,7 +645,20 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)
if (!nonblocking)
{
+ /* NOTE: nxtask_flushstreams() calls lib_flushall()
+ * which handles a semaphore in userspace.
+ * It should be done without a critical section.
+ */
+
+#ifdef CONFIG_SMP
+ leave_critical_section(flags);
+#endif
+
nxtask_flushstreams(tcb);
Review comment:
Looks good.
----------------------------------------------------------------
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