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 2022/02/18 07:18:19 UTC
[GitHub] [incubator-nuttx] no1wudi opened a new pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
no1wudi opened a new pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543
## Summary
# Fix the issue found in #5382
## Impact
Cancellation poin
## Testing
sim:ostest
--
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
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543
--
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
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810426239
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
Keep them is not harmful, but there are two case to call the `pthread_exit`:
1. Application call `pthread_exit` directly, we can mark pthread as non-cancelable since we are already exiting, as line63 does
2. `pthread_exit` called by cancellation point, if happend, the cpcount must be 0 and we do the same logic as case 1 immediate, means the state had been set during `nx_pthread_exit`, no need to set 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810635337
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
Ok. I'm fine with this change. Just want to make sure when we are changing existing behavior so we do not loose some important cases
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810176134
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
We can't touch the fields in tcb_s from libc since it doesn't work in protected and kernel build.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810132271
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
One more point: if we move TASK_CANCEL_DISABLE to nx_pthread_exit, the callback from pthread_cleanup_popall at line 67 may trigger the recursive pthread_exit call which we want to avoid in this patch.
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810147263
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
Ok. But should be keep
```
tcb->flags &= ~TCB_FLAG_CANCEL_PENDING;
tcb->cpcount = 0;
```
in `nx_pthread_exit` since this action is not done by `task_setcancelstate(TASK_CANCEL_DISABLE, NULL);`?
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810185989
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
I propose to keep those two lines at the original place in `nx_pthread_exit`.
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810070433
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
What if `nx_pthread_exit` is called outside of `pthread_exit`? I mean is it theoretically possible?
--
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
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810426239
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
Keep them is not harmful, but there are two case to call the `pthread_exit`:
1. Application call `pthread_exit` directly, we can mark pthread as non-cancelable since we are already exiting, as line63 does
2. `pthread_exit` called by cancellation point, if happend, the cpcount must be 0 and we do the same logic as case 1 immediate, means the state had been set during `nx_pthread_exit`, no need to set it 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #5543: sched: Mark pthread as non-cancelable to avoid additional calls to pthread_exit()
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5543:
URL: https://github.com/apache/incubator-nuttx/pull/5543#discussion_r810095157
##########
File path: libs/libc/pthread/pthread_exit.c
##########
@@ -54,6 +54,15 @@
void pthread_exit(FAR void *exit_value)
{
+ /* Mark the pthread as non-cancelable to avoid additional calls to
+ * pthread_exit() due to any cancellation point logic that might get
+ * kicked off by actions taken during pthread_exit processing.
+ */
+
+#ifdef CONFIG_CANCELLATION_POINTS
+ task_setcancelstate(TASK_CANCEL_DISABLE, NULL);
Review comment:
`nx_pthread_exit` is only called from `pthread_exit`, and if this happened it would not be a problem since #5382 is caused by pthread cleanup callback , these callbacks only called from pthread_exit.
--
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