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/06/03 17:47:48 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1187: sched: nxsig_abnormal_termination should terminate the whole task
xiaoxiang781216 opened a new pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187
## Summary
## Impact
## Testing
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438331920
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
Certainly I will never merge this code. I believe that it is an abminination to the concept of a properly modularized Unix system. But I leave that to the good sense of others if they want it or not. (God, I hope there is no one out there so foolish).
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642028348
The PR summary does not does not describe the changes of all of the 11 commits. This must be broken into separately traceable PRs.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438319593
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
I made this patch, because it's a critical error that:
1.main thread can't call pthread_cleanup_push pthread_cleanup_pop
2.main thread can't use robust mutex
----------------------------------------------------------------
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 edited a comment on pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642031583
> @xiaoxiang781216 why is this needed? Why did you take the approach of removing the union?
You mean the union inside exitinfo_s, this struct is added by me in commit:
https://github.com/apache/incubator-nuttx/pull/1189/commits/843ad796f511043d5b28ce8100b65ba078955b7e
And find that the logic can be simplified more by removing the union.
> Where are the test cases with all the permutations of the CONFIG_* that control the changes?
Now we just have ostest. I run the change with ostest, no problem found so far. Yes, the test case is very little. Can you improve in this area?
If your concern is whether the change can build successfully with all possible CONFIG_* combination, I think the github action and apache jenkins already build all most board config.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438423719
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
This is a shoddy, incorrect solution to an important issue. The correct solution that does not contaminate the kernel thread is significantly more complex. I cut corners and implemented a bad simple solution that is bad of the OS and must not be merged. You are doing one of the "Enemies of Inviolables" in INVIOLABLES.txt:
The Enemies
===========
No Short Cuts
-------------
o Doing things the easy way instead of the correct way.
o Reducing effort at the expense of Quality, Portability, or
Consistency.
o Focus on the values of the organization, not the values of the Open
Source project. Need to support both.
o It takes work to support the Inviolables. There are no shortcuts.
That is unacceptable.
----------------------------------------------------------------
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 pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-641978683
> In any event, this should not be merged until the 9.1 release has branched
We are expecting to branch for 9.1 in 5 days, it should be reasonable to wait.
----------------------------------------------------------------
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 edited a comment on pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642142655
In general, I think these changes are pretty ill-conceived. But I have not looked at all of the details; that is just my impression. There are many good ideas mish-mashed together with some not so good ideas. I don't think it should go to master.
It should be broken up into multiple PRs that can be reviewed, approved, and merged. It should never come into master in this form (and we still do not want to perturb the code base prior the branching off 9.1!).
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437871620
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
because main/kernel thread type is TCB_FLAG_TTYPE_TASK/TCB_FLAG_TTYPE_KERNEL, but both types should be able to call pthread_cleanup_push/pthread_cleanup_pop. The major target of whole patch set is ensure the main/kernel thread can call all most pthread API like pthread.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438277789
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
I don't believe that use any pthread APIs should be permitted within the OS.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187
----------------------------------------------------------------
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] yamt commented on a change in pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437899404
##########
File path: sched/task/task_delete.c
##########
@@ -143,6 +144,15 @@ int task_delete(pid_t pid)
goto errout;
}
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(rtcb);
Review comment:
why is it dangerous? it's something i believe the most of pthread implementations allow.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438270516
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
> because main/kernel thread type is TCB_FLAG_TTYPE_TASK/TCB_FLAG_TTYPE_KERNEL, but both types should be able to call pthread_cleanup_push/pthread_cleanup_pop. The major target of whole patch set is ensure the main/kernel thread can call all most pthread API like pthread.
But kernel threads cannot create pthreads. pthreads reside only in user space.
What would that even mean in a kernal build. It makes no sense. Which user space?
No pthread APIs should be availble to kernel threads. That must be forbidden by the OS.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438239737
##########
File path: sched/task/task_delete.c
##########
@@ -143,6 +144,15 @@ int task_delete(pid_t pid)
goto errout;
}
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(rtcb);
Review comment:
Most people call task_delete mean to kill the whole task/process, just like you kill the process from task manager windows on Linux/Windows, but now this function just delete the main thread but let other threads in the same task keep running. And worse you can't kill other threads in this task by task_delete, then we don't have any API to terminate all threads in a task.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437872559
##########
File path: sched/task/task_delete.c
##########
@@ -143,6 +144,15 @@ int task_delete(pid_t pid)
goto errout;
}
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(rtcb);
Review comment:
> why is this necessary?
I think that It's dangerous to terminate the main thread but leave other threads in the task keep running. And other similar function also do the same thing(e.g. task_restart, nxsig_abnormal_termination, exit...).
> why is this rtcb, not dtcb?
good catch, I will fix it.
----------------------------------------------------------------
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] yamt commented on a change in pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437902321
##########
File path: sched/pthread/pthread_cancel.c
##########
@@ -85,6 +76,15 @@ int pthread_cancel(pthread_t thread)
pthread_exit(PTHREAD_CANCELED);
}
Review comment:
i don't think pthread_cancel(self) is supposed to terminate the thread in the case of deferred cancellation.
after your changes, it seems to do.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437874556
##########
File path: sched/task/task_cancelpt.c
##########
@@ -339,51 +344,89 @@ void nxnotify_cancellation(FAR struct tcb_s *tcb)
flags = enter_critical_section();
- /* Make sure that the cancellation pending indication is set. */
-
- tcb->flags |= TCB_FLAG_CANCEL_PENDING;
-
/* We only notify the cancellation if (1) the thread has not disabled
* cancellation, (2) the thread uses the deferred cancellation mode,
* (3) the thread is waiting within a cancellation point.
*/
- if (((tcb->flags & TCB_FLAG_NONCANCELABLE) == 0 &&
- (tcb->flags & TCB_FLAG_CANCEL_DEFERRED) != 0) ||
- tcb->cpcount > 0)
+ /* Check to see if this task has the non-cancelable bit set. */
+
+ if ((tcb->flags & TCB_FLAG_NONCANCELABLE) != 0)
{
- /* If the thread is blocked waiting for a semaphore, then the thread
- * must be unblocked to handle the cancellation.
+ /* Then we cannot cancel the thread now. Here is how this is
+ * supposed to work:
+ *
+ * "When cancellability is disabled, all cancels are held pending
+ * in the target thread until the thread changes the cancellability.
+ * When cancellability is deferred, all cancels are held pending in
+ * the target thread until the thread changes the cancellability,
+ * calls a function which is a cancellation point or calls
+ * pthread_testcancel(), thus creating a cancellation point. When
+ * cancellability is asynchronous, all cancels are acted upon
+ * immediately, interrupting the thread with its processing."
+ *
+ * REVISIT: Does this rule apply to equally to both SIGKILL and
+ * SIGINT?
Review comment:
The comment is copied from sig_default, I will remove it.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438302967
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
Mutexes cannot be support in the kernel address space at all. That must be forbidden. Nor can the kernel reliably access a user space mutex in the KERNEL build. This is very bad design and must not be permitted onto master.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438277789
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
I don't believe that use any pthread APIs should be permitted within the OS. Nor do I believe that the kernel thread TCB should carry the burden of the pthread data structures.
----------------------------------------------------------------
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] davids5 commented on pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642099295
> > @xiaoxiang781216 why is this needed? Why did you take the approach of removing the union?
>
> You mean the union inside exitinfo_s, this struct is added by me in commit:
> [843ad79](https://github.com/apache/incubator-nuttx/commit/843ad796f511043d5b28ce8100b65ba078955b7e)
> And find that the logic can be simplified more by removing the union.
>
> > Where are the test cases with all the permutations of the CONFIG_* that control the changes?
>
> Now we just have ostest. I run the change with ostest, no problem found so far. Yes, the test case is very little. Can you improve in this area?
> If your concern is whether the change can build successfully with all possible CONFIG_* combination, I think the github action and apache jenkins already build all most board config.
I just ran into this myself, I was thinking dma preflight was lit is the some of the test cases it was not. So it did not break the build, but it also was never tested.
----------------------------------------------------------------
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 pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-641737101
> this PR is complex.
Yes, it's better to review the patch one by one in commit tab.
> can you consider to separate quick_exit into another PR?
It's too difficult because quick_exit depend the previous patch which clean up the exit logic. If need, I can remove quick_exit patch from the PR and send a new PR once this PR get merged.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438313789
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
> Mutexes cannot be support in the kernel address space at all. That must be forbidden. Nor can the kernel reliably access a user space mutex in the KERNEL build. This is very bad design and must not be permitted onto master.
Yes, normally we don't use mutex in kernel thread, but the mutex should just work inside kernel thread from the pure technology view. Could you point out what code snippet in mutex can't work in kernel thread? But anyway, the goal of this patchset is to fix main thread problem. If user follow the guide and never use mutex and the clean stack inside kernel thread, pthread_mutex_inconsistent/pthread_cleanup_popall is always no-op for kernel thread.
----------------------------------------------------------------
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] davids5 commented on pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-641839694
@xiaoxiang781216 why is this needed? Why did you take the approach of removing the union? Where are the test cases with all the permutations of the CONFIG_* that control the changes?
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438252132
##########
File path: sched/task/task_delete.c
##########
@@ -143,6 +144,15 @@ int task_delete(pid_t pid)
goto errout;
}
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(rtcb);
Review comment:
> The PR summary does not does not describe the changes of all of the 11 commits. This must be broken into separately traceable PRs.
Because nobody review my PR more than 7 days and some late patch depend on the previous one, I have to accumulate the patch in this PR. Anyway, I will remove the new patch from the list and send new PR when this PR get merged.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438520100
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
> To me it is a totally unacceptable partitioning of user space and kernel interfaces. I am very opposed to supported this. It is wrong. It is a serious architectural mistake. Let's not go that way... it is very sickening design concept. Let's keep logic in place to prohibit the use of any pthread APIs from kernel threads.
Ok, we can make the change like this:
```
DEBUGASSERT((tcb->flags & TCB_FLAG_TTYPE_MASK) !=
TCB_FLAG_TTYPE_KTHREAD);
```
or
```
if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KTHREAD)
{
...
}
```
So the kernel thread will fail or no-op for pthread API, but main/pthread can work correctly.
> Please. Don't shit on the OS! We are entrusted you to maintain good functional partitioning and not to trash all of the interfaces.
>
> I am super opposed to the concept. I am very disappointed in what you are doing to the OS. Very disappointed.
> My long term goals include:
>
> 1. To separate kernel threads from all groups. To eliminate streams and file descriptors from strings,
> 2. Move all pthread logic possible into user space as will all other Unix system.
>
> You are ruining that.
This change doesn't ruining your goal, but a path to your final target:
1.Unify the main and thread behaviour:
a.main thread can call pthread_cleanup_[push|pop]
b.robust mutex work with main thread too
c.pthread_exit in main shouldn't terminate the whole process
b.cancellation point in main shouldn't terminate the whole process
2.The next step is reorg tcb_s maybe like this:
```
tcb_s
|
+-------+--------+
| |
tcb_kernel_s tcb_user_s
|
+-------+--------+
| |
tcb_task_s tcb_pthread_s
```
3.Finally merge tls_info_s into tcb_user_s and place userspace tcb at the beginning of stack
You can see the first step help this path because it make main thread same as pthread which is always required.
----------------------------------------------------------------
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 edited a comment on pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642153040
I already split the PR into small one, please give your comment and suggestion there. Thanks.
----------------------------------------------------------------
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] yamt commented on a change in pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437857953
##########
File path: sched/task/task.h
##########
@@ -48,6 +48,13 @@
#include <nuttx/sched.h>
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define EXIT_NORMAL 0
+#define EXIT_BYPASS 1
Review comment:
can you document what this bypasses?
##########
File path: sched/task/task_cancelpt.c
##########
@@ -339,51 +344,89 @@ void nxnotify_cancellation(FAR struct tcb_s *tcb)
flags = enter_critical_section();
- /* Make sure that the cancellation pending indication is set. */
-
- tcb->flags |= TCB_FLAG_CANCEL_PENDING;
-
/* We only notify the cancellation if (1) the thread has not disabled
* cancellation, (2) the thread uses the deferred cancellation mode,
* (3) the thread is waiting within a cancellation point.
*/
- if (((tcb->flags & TCB_FLAG_NONCANCELABLE) == 0 &&
- (tcb->flags & TCB_FLAG_CANCEL_DEFERRED) != 0) ||
- tcb->cpcount > 0)
+ /* Check to see if this task has the non-cancelable bit set. */
+
+ if ((tcb->flags & TCB_FLAG_NONCANCELABLE) != 0)
{
- /* If the thread is blocked waiting for a semaphore, then the thread
- * must be unblocked to handle the cancellation.
+ /* Then we cannot cancel the thread now. Here is how this is
+ * supposed to work:
+ *
+ * "When cancellability is disabled, all cancels are held pending
+ * in the target thread until the thread changes the cancellability.
+ * When cancellability is deferred, all cancels are held pending in
+ * the target thread until the thread changes the cancellability,
+ * calls a function which is a cancellation point or calls
+ * pthread_testcancel(), thus creating a cancellation point. When
+ * cancellability is asynchronous, all cancels are acted upon
+ * immediately, interrupting the thread with its processing."
+ *
+ * REVISIT: Does this rule apply to equally to both SIGKILL and
+ * SIGINT?
Review comment:
i don't understand this REVISIT comment. how signals are related here?
##########
File path: sched/pthread/pthread_cancel.c
##########
@@ -85,6 +76,15 @@ int pthread_cancel(pthread_t thread)
pthread_exit(PTHREAD_CANCELED);
}
Review comment:
does this mean to make pthread_cancel a cancellation point?
##########
File path: sched/task/task_delete.c
##########
@@ -143,6 +144,15 @@ int task_delete(pid_t pid)
goto errout;
}
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(rtcb);
Review comment:
why is this necessary?
why is this rtcb, not dtcb?
----------------------------------------------------------------
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 pull request #1187: sched: Main thread should support cleanup and robust mutex too
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-640075734
@patacongo could you give some feedback? so other committer can merge the code.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438270516
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
> because main/kernel thread type is TCB_FLAG_TTYPE_TASK/TCB_FLAG_TTYPE_KERNEL, but both types should be able to call pthread_cleanup_push/pthread_cleanup_pop. The major target of whole patch set is ensure the main/kernel thread can call all most pthread API like pthread.
But kernel threads cannot create pthreads. pthreads reside only in user space.
What would that even mean in a kernal build. It makes no sense. Which user space?
No pthread APIs should be availble to kernel threads. That must be forbidden by the OS. I believe that would result in severe, catastrophic breakage if permitted.
----------------------------------------------------------------
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 pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642031583
> @xiaoxiang781216 why is this needed? Why did you take the approach of removing the union?
You mean the union inside exitinfo_s, this struct is added by me in commit:
https://github.com/apache/incubator-nuttx/pull/1189/commits/843ad796f511043d5b28ce8100b65ba078955b7e
And find that the logic can be simplified more by removing the union.
> Where are the test cases with all the permutations of the CONFIG_* that control the changes?
Now we just have ostest. I run the change with ostest, no problem found so far. Yes, the test case is very little. Can you improve in this area?
If your concern is whetherthe change can build successfully with all possible CONFIG_* combination, I think the github action and apache jenkin already build all most board config.
----------------------------------------------------------------
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 pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-641701999
Does anyone can help review the change?
----------------------------------------------------------------
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] btashton commented on a change in pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437865951
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
Why are we not bailing out here when called with a non pthread type?
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438520100
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
> To me it is a totally unacceptable partitioning of user space and kernel interfaces. I am very opposed to supported this. It is wrong. It is a serious architectural mistake. Let's not go that way... it is very sickening design concept. Let's keep logic in place to prohibit the use of any pthread APIs from kernel threads.
Ok, we can make the change like this:
```
DEBUGASSERT((tcb->flags & TCB_FLAG_TTYPE_MASK) !=
TCB_FLAG_TTYPE_KTHREAD);
```
or
```
if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KTHREAD)
{
...
}
```
So the kernel thread will fail or no-op for pthread API, but main/pthread can work correctly.
> Please. Don't shit on the OS! We are entrusted you to maintain good functional partitioning and not to trash all of the interfaces.
>
> I am super opposed to the concept. I am very disappointed in what you are doing to the OS. Very disappointed.
> My long term goals include:
>
> 1. To separate kernel threads from all groups. To eliminate streams and file descriptors from strings,
> 2. Move all pthread logic possible into user space as will all other Unix system.
>
> You are ruining that.
This change doesn't ruining your goal, but a path to your final target:
1.Unify the main and thread behaviour:
a.main thread can call pthread_cleanup_[push|pop]
b.robust mutex work with main thread too
c.pthread_exit in main doesn't terminate the whole process
b.cancellation point in main doesn't terminate the whole process
2.The next step is reorg tcb_s maybe like this:
```
tcb_s
|
+-------+--------+
| |
tcb_kernel_s tcb_user_s
|
+-------+--------+
| |
tcb_task_s tcb_pthread_s
```
3.Finally merge tls_info_s into tcb_user_s and place userspace tcb at the beginning of stack
You can see the first step help this path because it make main thread same as pthread which is always required.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438321525
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
To me it is a totally unacceptable partitioning of user space and kernel interfaces. I am very opposed to supported this. It is wrong. It is a serious architectural mistake. Let's not go that way... it is very sickening design concept. Let's keep logic in place to prohibit the use of any pthread APIs from kernel threads. Please. Don't shit on the OS! We are entrusted you to maintain good functional partitioning and not to trash all of the interfaces.
I am super opposed to the concept. I am very disappointed in what you are doing to the OS. Very disappointed.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437872559
##########
File path: sched/task/task_delete.c
##########
@@ -143,6 +144,15 @@ int task_delete(pid_t pid)
goto errout;
}
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(rtcb);
Review comment:
> why is this necessary?
I think that It's dangerous to terminate the main thread but leave other threads in the task keep running. And other similar function also do the same thing(e.g. task_restart, nxsig_abnormal_termination, exit...).
> why is this rtcb, not dtcb?
good catch, I will fix it.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438960780
##########
File path: include/nuttx/sched.h
##########
@@ -714,6 +714,23 @@ struct tcb_s
FAR struct mqueue_inode_s *msgwaitq; /* Waiting for this message queue */
#endif
+ /* Robust mutex support *******************************************************/
+
+#if !defined(CONFIG_DISABLE_PTHREAD) && !defined(CONFIG_PTHREAD_MUTEX_UNSAFE)
+ FAR struct pthread_mutex_s *mhead; /* List of mutexes held by thread */
+#endif
+
+ /* Clean-up stack *************************************************************/
+
+#ifdef CONFIG_PTHREAD_CLEANUP
+ /* tos - The index to the next available entry at the top of the stack.
+ * stack - The pre-allocated clean-up stack memory.
+ */
+
+ uint8_t tos;
+ struct pthread_cleanup_s stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+#endif
+
Review comment:
Wouldn't it be better to move these into TLS? They should only be used by user space applications and, hence, should be accessed in the same way that the errno variable is.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r440188444
##########
File path: include/nuttx/sched.h
##########
@@ -714,6 +714,23 @@ struct tcb_s
FAR struct mqueue_inode_s *msgwaitq; /* Waiting for this message queue */
#endif
+ /* Robust mutex support *******************************************************/
+
+#if !defined(CONFIG_DISABLE_PTHREAD) && !defined(CONFIG_PTHREAD_MUTEX_UNSAFE)
+ FAR struct pthread_mutex_s *mhead; /* List of mutexes held by thread */
+#endif
+
+ /* Clean-up stack *************************************************************/
+
+#ifdef CONFIG_PTHREAD_CLEANUP
+ /* tos - The index to the next available entry at the top of the stack.
+ * stack - The pre-allocated clean-up stack memory.
+ */
+
+ uint8_t tos;
+ struct pthread_cleanup_s stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+#endif
+
Review comment:
@patacongo I will provide a patch to move them to TLS when I finish upgrade libcxx to the latest version.
----------------------------------------------------------------
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 pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642153040
I already spli the PR into small one, please give your comment and suggestion there. Thanks.
----------------------------------------------------------------
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 edited a comment on pull request #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642031583
> @xiaoxiang781216 why is this needed? Why did you take the approach of removing the union?
You mean the union inside exitinfo_s, this struct is added by me in commit:
https://github.com/apache/incubator-nuttx/pull/1189/commits/843ad796f511043d5b28ce8100b65ba078955b7e
And find that the logic can be simplified more by removing the union.
> Where are the test cases with all the permutations of the CONFIG_* that control the changes?
Now we just have ostest. I run the change with ostest, no problem found so far. Yes, the test case is very little. Can you improve in this area?
If your concern is whether the change can build successfully with all possible CONFIG_* combination, I think the github action and apache jenkin already build all most board config.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438323194
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
My long term goals include:
1. To separate kernel threads from all groups. To eliminate streams and file descriptors from strings,
2. Move all pthread logic possible into user space as will all other Unix system.
You are ruining that.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438270516
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
> because main/kernel thread type is TCB_FLAG_TTYPE_TASK/TCB_FLAG_TTYPE_KERNEL, but both types should be able to call pthread_cleanup_push/pthread_cleanup_pop. The major target of whole patch set is ensure the main/kernel thread can call all most pthread API like pthread.
But kernel threads cannot create pthreads. pthreads reside only in user space.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437875000
##########
File path: sched/task/task.h
##########
@@ -48,6 +48,13 @@
#include <nuttx/sched.h>
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define EXIT_NORMAL 0
+#define EXIT_BYPASS 1
Review comment:
Ok.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-641972738
In any event, this should not be merged until the 9.1 release has branched. This is far to risky to go into a release unverified.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438254814
##########
File path: sched/pthread/pthread_cancel.c
##########
@@ -85,6 +76,15 @@ int pthread_cancel(pthread_t thread)
pthread_exit(PTHREAD_CANCELED);
}
Review comment:
After re-reading the related document, you are right we need defer the termination even pthread_cancel self. I will drop this patch from 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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438960780
##########
File path: include/nuttx/sched.h
##########
@@ -714,6 +714,23 @@ struct tcb_s
FAR struct mqueue_inode_s *msgwaitq; /* Waiting for this message queue */
#endif
+ /* Robust mutex support *******************************************************/
+
+#if !defined(CONFIG_DISABLE_PTHREAD) && !defined(CONFIG_PTHREAD_MUTEX_UNSAFE)
+ FAR struct pthread_mutex_s *mhead; /* List of mutexes held by thread */
+#endif
+
+ /* Clean-up stack *************************************************************/
+
+#ifdef CONFIG_PTHREAD_CLEANUP
+ /* tos - The index to the next available entry at the top of the stack.
+ * stack - The pre-allocated clean-up stack memory.
+ */
+
+ uint8_t tos;
+ struct pthread_cleanup_s stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
+#endif
+
Review comment:
Wouldn't it be better to move these into TLS? They should only be used by user space applications and, hence, should be accessed in the same way that the errno variable is.
We should move as much as pthread logic as possible out of the operating system. Over time, the entire pthread implementation (other than a a few OS hooks) should be moved into the C library. That is a proper and standard roadmap for pthread support.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-642142655
In general, I think these changes are pretty ill-conceived. But I have not looked at all of the details; that is just my impression. There are many good ideas mish-mashed together with some not so good ideas. I don't think it should go to master.
It should be broken up into multiple PRs that can be reviewed, approved, and merged. It should never come into master in this form.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438292551
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
tcb_s is shared by three entities(main task, kernel thread and pthread). To confirm POSIX standard, both main thread and pthread MUST support pthread_cleanup_[push|pop] and robust mutex. For kernel thread, it's not bad to support the cleanup stack. Yes, the robust mutex isn't need. But we only waste four byes for the late case.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
jerpelea commented on pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#issuecomment-641980191
+1 for the wait
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r437876538
##########
File path: sched/pthread/pthread_cancel.c
##########
@@ -85,6 +76,15 @@ int pthread_cancel(pthread_t thread)
pthread_exit(PTHREAD_CANCELED);
}
Review comment:
The behaviour is same as before, the change is just an optimizition. If you look at nxnotify_cancellation, the main job is call nxsem_wait_irq/nxsig_wait_irq/nxmq_wait_irq but the code unnecessary if some thread call pthread_cancel on self because the thread is impossible in the wait state.
----------------------------------------------------------------
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 #1187: sched: Unify the main thread and pthread behaivour
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1187:
URL: https://github.com/apache/incubator-nuttx/pull/1187#discussion_r438304876
##########
File path: sched/pthread/pthread_cleanup.c
##########
@@ -173,8 +165,7 @@ void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg)
*/
sched_lock();
- if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD &&
- tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
+ if (tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE)
Review comment:
I believe that you are being very careless and are endangering the OS. You must slow down everything and properly and thorough test and review everything. I think you are misguided.
----------------------------------------------------------------
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