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/29 15:04:50 UTC
[GitHub] [incubator-nuttx] patacongo opened a new pull request #1328: pthread_cleanup functions must be called from user space
patacongo opened a new pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328
## Summary
1. A user-space shim is needed to catch any return from a pthead main function and to automatically call pthread_exit() from user space.
Rename pthread_create() in sched/pthread/pthread_create.c to nx_pthread_create(). Add one new parameter: The address of the user-space pthread startup function. Instead of calling the pthread main entry (directly or indirectly), pthread_start() would call the pthread startup function, passing it the real address of the pthread main function.
The call to pthread_exist would be removed from pthread_startup() and move into a new function in user space.
2. Add libs/libc/pthread/lib_pthread_start.c that would contain two trivial functions:
static void pthread_startup(pthread_startroutine_t startroutine, pthread_addr_t arg)
{
pthread_exit(startroutine(arg));
}
int pthread_create(FAR pthread_t *thread, FAR const pthread_attr_t *attr,
pthread_startroutine_t startroutine, pthread_addr_t arg)
{
return nx_pthread_create(pthread_startup, thread, attr, startroutine, arg);
}
Still to do:
a. Modifiy up_pthread_start so that it takes three parameters: startup, entry, and arg.
b. Remove pthread_startup function pointer from struct userspace_s; it is no longer needed.
c. Modify how pthread is started in pthread_start() (and up_pthread_startup()) so that the startup function is called with the entry and arg paramgers.
d. Remove call to pthread_exit() from pthread_start()
e. Rename pthread_exit() to nx_pthread_exit(). Remove logic that calls pthread_cleanup() functions.
f. Make nx_pthread_exit() a system call.
g. Create libc/pthread/pthread_exit() that contains only i) the logic that calls the pthread_cleanup functions, and ii) calls the nx_pthread_exit() system call.
h. Extend TLS and pthread-specific data function so that the destructor is retained in TLS
i. Extend pthread_exit() so that it also calls the pthread-specific data destructors from user-space.
## Impact
This PR would effect all logic that uses pthread and, hence, must be well verified before merging.
## Testing
Most testing is performed with sim:ostest which exercises pthread logic pretty well.
----------------------------------------------------------------
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] antmerlino edited a comment on pull request #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
antmerlino edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652509617
> > There is a complexity... pthread_exit() is also called from within the OS from task_cancelpt.c, pthread_cancel.c, pthread_create.c, sig_default.c, task_cancelpt.c, task_setcancelstate.c, task_setcanceltype.c. This may require another system call analogous to up_pthread_start().
>
> The calls to pthread_exit() from the cancellation logic within the OS is a potential show-stopper. Or certainly a reason to stop and re-consider how thread cancellation should work. Thread cancellation should also be primarily a user-space implementation as well.
@gregory-nutt
I've now merged my branch in and am ready to put some time towards this. It sounds like the next big step is to unify the thread cancellation logic - we will definitely need to do some good testing on this!
All of these make sense. I could take care of those.
a. Rename pthread_exit() to nx_pthread_exit(). Remove logic that calls pthread_cleanup() functions.
b. Make nx_pthread_exit() a system call.
c. Create libc/pthread/pthread_exit() that contains only i) the logic that calls the pthread_cleanup functions, and ii) calls the nx_pthread_exit() system call.
d. Extend TLS and pthread-specific data function so that the destructor is retained in TLS
e. Extend pthread_exit() so that it also calls the pthread-specific data destructors from user-space.
But I feel like we should figure out the cancellation stuff before I do any of that.
> There is a complexity... pthread_exit() is also called from within the OS from task_cancelpt.c, pthread_cancel.c, pthread_create.c, sig_default.c, task_cancelpt.c, task_setcancelstate.c, task_setcanceltype.c. This may require another system call analagous to up_pthread_start().
I don't really understand what the analogous call would do for us there, could you expand?
----------------------------------------------------------------
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] antmerlino commented on pull request #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652611920
@gregory-nutt
I have some changes to what you have so far that address build issues. However, I can't open a PR against your branch on your repo.
Can we create a feature branch in the Apache repo to work on this together? That way we can make PRs against that branch.
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652639845
>
>
> @gregory-nutt
>
> I have some changes to what you have so far that address build issues. However, I can't open a PR against your branch on your repo.
>
> Can we create a feature branch in the Apache repo to work on this together? That way we can make PRs against that branch.
@antmerlino Yes, but you would have to do that. We have a policy here that no one can merge their own PRs, but you are also a committer and can to do the merge. You have registered your Github user (See https://whimsy.apache.org/roster/ppmc/nuttx). So I assume you have created created your 2 factor login: If not see https://cwiki.apache.org/confluence/display/NUTTX/Accessing+Apache+GitHub+as+a+Committer
If you have done those things, then here is what you would have to do to create the feature branch:
1. Create the feature branch in your incubator-nuttx clone, say "pthread-user"
2. Push the branch to the incubator-nuttx origin
3. Go to this PR and select "Edit" on the right near the top.
4. Change the destination
From:
patacongo wants to merge 2 commits into apache:master from patacongo:pthread
To:
patacongo wants to merge 2 commits into apache:pthread-user from patacongo:pthread
4. Then just do the rebase merge using the buttons at the bottom
That is all there is to it!
NOTE: Since this is a WIP, we do not expect the PR checks to pass at this time.
----------------------------------------------------------------
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 #1328: pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-706596832
@antmerlino I neither us has the interest in finishing this change. I propose that we both save the PR as a path and remove the branch for now. What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652639845
>
>
> @gregory-nutt
>
> I have some changes to what you have so far that address build issues. However, I can't open a PR against your branch on your repo.
>
> Can we create a feature branch in the Apache repo to work on this together? That way we can make PRs against that branch.
Yes, but you would have to do that. We have a policy here that no one can merge their own PRs, but you are also a committer and can to do the merge. You have registered your Github user (See https://whimsy.apache.org/roster/ppmc/nuttx). So I assume you have created created your 2 factor login: If not see https://cwiki.apache.org/confluence/display/NUTTX/Accessing+Apache+GitHub+as+a+Committer
If you have done those things, then here is what you would have to do to create the feature branch:
1. Create the feature branch in your incubator-nuttx clone, say "pthread-user"
2. Push the branch to the incubator-nuttx origin
3. Go to this PR and select "Edit" on the right near the top.
4. Change the distination
From:
patacongo wants to merge 2 commits into apache:master from patacongo:pthread
To:
patacongo wants to merge 2 commits into apache:pthread-user from patacongo:pthread
4. Then just do the rebase merge using the buttons at the bottom
That is all there is to 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 pull request #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652543985
@antmerlino How should we coordinate work on the branch? I just rebased the pthread branch on my fork. Then I release that this will really mess up what you are doing. rebasing changes the history of the changes branch and can result in spurious merges and occasionally worse.
Do you want to share the fork? Should I hold off on rebasing? I get concerned if about the state of the branch if we do not rebase frequently.
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652551262
> Why don't I create PRs onto your branch and you can pull them in or decline them as we go. This will give you a chance to review the changes as we go. You definitely understand the subtleties more than I.
Sounds good to me. The level of complexity and keeping the flow all in your head is tricky. I miss subtleties all of the time. And, as with most engineering topics, communicating the technical issues is always the most difficult part by far. I am terrible at trying to read and understand GIT differences.
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652525167
> But I feel like we should figure out the cancellation stuff before I do any of that.
>
> > There is a complexity... pthread_exit() is also called from within the OS from task_cancelpt.c, pthread_cancel.c, pthread_create.c, sig_default.c, task_cancelpt.c, task_setcancelstate.c, task_setcanceltype.c. This may require another system call analagous to up_pthread_start().
>
> I don't really understand what the analogous call would do for us there, could you expand?
Well, you are making me think on my feet, but I will do the best that I can. I might ramble a bit.
First, each of the C files referenced above should be treated as a special case. For example in pthread_create.c, pthread_exit() is only called as a error recovery measure. In that case, there can be no pthread_cleanup routines or pthread-specific destructors registered. So we can just replace the call to pthread_exit() with a call to nx_pthread_exit(). This might apply in other places too.
But in other cases, I am sure that it will be necessary to call the user-space pthread_exit() from the kernel code in order to perform those actions in user space. The basic problem is that in the PROTECTED build, the pthread_exit() symbol is not available to the kernel. That is because the user blob and the kernel blob are separately built and one blob knows nothing about the symbols internal to the other. The situation is worse in the KERNEL build because there will be multiple instances of pthread_exit() in each user block (aka, "process") and each will lie at a different virtual address -- unless of course we were to implement some kind of shared library support.
The simplest way that I can think of to do that is to extend the nx_pthread_create() interface again. On this branch it currently receives the address of the pthread_startup() function and the address of the pthread entry point. It could be extended to also receive the address of pthread_exit() function. This address is of no interest to the pthread_create() logic but it could be saved in pthread_tcb_s structure in the same way that the pthread_startup() function is, perhaps as 'pthread_exit'; Then, where ever pthread_exit() is called in the kernel, this could be replaced with tcb->pthread_exit(). That is not too bad.
[Hmm.. I suppose TLS would be a more appropriate place to keep all pthread data rather than the TCB. Although if you have the tcb in hand, you cannot be assured that the tcb corresponds to the current stack so you would have to use that stack base from the TCB.]
This approach would also require something like up_pthread_start(). That function calls the specified pthread startup function and switches to user mode. We need some mechanism to call the saved pthread_exit() also switching to user mode.
System calls are expensive and a lot of work. I wonder if we can combine the pthread_startup and the pthread_exit system calls into a single function that is agnostic about what it is calling as long as the function signature is the same. This would, then just require some renaming. Like SYS_pthread_startup -> SYS_pthread_action and up_pthread_start() -> up_pthread_action().
In the longer term, we need to get all of the pthread APIs out of the kernel, but this is certainly one positive step.
What do you think?
In KERNEL mode, there are other things to make sure we understand. In order to call a user space function in KERNEL mode, we have to have the correct address environment in place (up_select_addrenv()). I don't think that is will be a problem, because the main thread and all of the child pthreads share the same address environment. Therefore, I can't think of any situation were the cancellation could occur with the wrong address environment in place. But it is another complexity to keep in mind.
----------------------------------------------------------------
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] antmerlino commented on pull request #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652509617
> > There is a complexity... pthread_exit() is also called from within the OS from task_cancelpt.c, pthread_cancel.c, pthread_create.c, sig_default.c, task_cancelpt.c, task_setcancelstate.c, task_setcanceltype.c. This may require another system call analogous to up_pthread_start().
>
> The calls to pthread_exit() from the cancellation logic within the OS is a potential show-stopper. Or certainly a reason to stop and re-consider how thread cancellation should work. Thread cancellation should also be primarily a user-space implementation as well.
Greg,
I've now merged my branch in and am ready to put some time towards this. It sounds like the next big step is to unify the thread cancellation logic - we will definitely need to do some good testing on this!
All of these make sense. I could take care of those.
a. Rename pthread_exit() to nx_pthread_exit(). Remove logic that calls pthread_cleanup() functions.
b. Make nx_pthread_exit() a system call.
c. Create libc/pthread/pthread_exit() that contains only i) the logic that calls the pthread_cleanup functions, and ii) calls the nx_pthread_exit() system call.
d. Extend TLS and pthread-specific data function so that the destructor is retained in TLS
e. Extend pthread_exit() so that it also calls the pthread-specific data destructors from user-space.
But I feel like we should figure out the cancellation stuff before I do any of that.
> There is a complexity... pthread_exit() is also called from within the OS from task_cancelpt.c, pthread_cancel.c, pthread_create.c, sig_default.c, task_cancelpt.c, task_setcancelstate.c, task_setcanceltype.c. This may require another system call analagous to up_pthread_start().
I don't really understand what the analogous call would do for us there, could you expand?
----------------------------------------------------------------
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] ghn-certi commented on a change in pull request #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#discussion_r447699093
##########
File path: sched/pthread/pthread_create.c
##########
@@ -72,28 +72,29 @@ static const char g_pthreadname[] = "<pthread>";
****************************************************************************/
/****************************************************************************
- * Name: pthread_argsetup
+ * Name: pthread_tcb_setup
*
* Description:
* This functions sets up parameters in the Task Control Block (TCB) in
* preparation for starting a new thread.
*
- * pthread_argsetup() is called from nxtask_init() and nxtask_start() to
+ * pthread_tcb_setup() is called from nxtask_init() and nxtask_start() to
* create a new task (with arguments cloned via strdup) or pthread_create()
* which has one argument passed by value (distinguished by the pthread
* boolean argument).
*
* Input Parameters:
- * tcb - Address of the new task's TCB
- * arg - The argument to provide to the pthread on startup.
+ * tcb - Address of the new task's TCB
+ * startup - User-space pthread startup function
Review comment:
```suggestion
* startup - User space pthread startup function
```
##########
File path: arch/arm/src/armv6-m/arm_svcall.c
##########
@@ -313,22 +313,22 @@ int arm_svcall(int irq, FAR void *context, FAR void *arg)
* R2 = arg
*/
-#if defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_DISABLE_PTHREAD)
+#if !defined(CONFIG_BUILD_FLAT) && !defined(CONFIG_DISABLE_PTHREAD)
case SYS_pthread_start:
{
/* Set up to return to the user-space pthread start-up function in
* unprivileged mode.
*/
- regs[REG_PC] = (uint32_t)USERSPACE->pthread_startup;
+ regs[REG_PC] = (uint32_t)regs[REG_R1]; /* startup */
regs[REG_EXC_RETURN] = EXC_RETURN_UNPRIVTHR;
- /* Change the parameter ordering to match the expectation of struct
- * userpace_s pthread_startup:
+ /* Change the parameter ordering to match the expectation of the
+ * useri space pthread_startup:
Review comment:
```suggestion
* user space pthread_startup:
```
##########
File path: libs/libc/pthread/pthread_create.c
##########
@@ -0,0 +1,89 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_create.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <pthread.h>
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_startup
+ *
+ * Description:
+ * This function is the user-space, pthread startup function. Its purpose
+ * is to to catch the return from the pthread main function so that
Review comment:
```suggestion
* is to catch the return from the pthread main function so that
```
##########
File path: libs/libc/pthread/pthread_create.c
##########
@@ -0,0 +1,89 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_create.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <pthread.h>
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_startup
+ *
+ * Description:
+ * This function is the user-space, pthread startup function. Its purpose
Review comment:
```suggestion
* This function is the user space pthread startup function. Its purpose
```
##########
File path: sched/pthread/pthread_create.c
##########
@@ -72,28 +72,29 @@ static const char g_pthreadname[] = "<pthread>";
****************************************************************************/
/****************************************************************************
- * Name: pthread_argsetup
+ * Name: pthread_tcb_setup
*
* Description:
* This functions sets up parameters in the Task Control Block (TCB) in
Review comment:
```suggestion
* This function sets up parameters in the Task Control Block (TCB) in
```
##########
File path: sched/pthread/pthread_create.c
##########
@@ -205,13 +210,14 @@ static void pthread_start(void)
****************************************************************************/
/****************************************************************************
- * Name: pthread_create
+ * Name: nx_pthread_create
*
* Description:
* This function creates and activates a new thread with a specified
Review comment:
```suggestion
* This function creates and activates a new thread with specified
```
##########
File path: libs/libc/pthread/pthread_create.c
##########
@@ -0,0 +1,89 @@
+/****************************************************************************
+ * libs/libc/pthread/pthread_create.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <pthread.h>
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_startup
+ *
+ * Description:
+ * This function is the user-space, pthread startup function. Its purpose
+ * is to to catch the return from the pthread main function so that
+ * pthread_exit() can be called from user space
+ *
+ * Input Parameters:
+ * entry - The user-space address of the pthread entry point
+ * arg - Standard argument for the pthread entry point
+ *
+ * Returned Value:
+ * None. This function does not return.
+ *
+ ****************************************************************************/
+
+static void pthread_startup(pthread_startroutine_t entry,
+ pthread_addr_t arg)
+{
+ DEBUGASSERT(entry != NULL);
+
+ /* Pass control to the thread entry point. Handle any returned value. */
+
+ pthread_exit(entry(arg));
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_create
+ *
+ * Description:
+ * This function creates and activates a new thread with a specified
Review comment:
```suggestion
* This function creates and activates a new thread with specified
```
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652639845
>
>
> @gregory-nutt
>
> I have some changes to what you have so far that address build issues. However, I can't open a PR against your branch on your repo.
>
> Can we create a feature branch in the Apache repo to work on this together? That way we can make PRs against that branch.
@antmerlino Yes, but you would have to do that. We have a policy here that no one can merge their own PRs, but you are also a committer and can to do the merge. You have registered your Github user (See https://whimsy.apache.org/roster/ppmc/nuttx). So I assume you have created created your 2 factor login: If not see https://cwiki.apache.org/confluence/display/NUTTX/Accessing+Apache+GitHub+as+a+Committer
If you have done those things, then here is what you would have to do to create the feature branch:
1. Create the feature branch in your incubator-nuttx clone, say "pthread-user"
2. Push the branch to the incubator-nuttx origin
3. Go to this PR and select "Edit" on the right near the top.
4. Change the distination
From:
patacongo wants to merge 2 commits into apache:master from patacongo:pthread
To:
patacongo wants to merge 2 commits into apache:pthread-user from patacongo:pthread
4. Then just do the rebase merge using the buttons at the bottom
That is all there is to 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] Ouss4 merged pull request #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
Ouss4 merged pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652525167
> But I feel like we should figure out the cancellation stuff before I do any of that.
>
> > There is a complexity... pthread_exit() is also called from within the OS from task_cancelpt.c, pthread_cancel.c, pthread_create.c, sig_default.c, task_cancelpt.c, task_setcancelstate.c, task_setcanceltype.c. This may require another system call analagous to up_pthread_start().
>
> I don't really understand what the analogous call would do for us there, could you expand?
Well, you are making me think on my feet, but I will do the best that I can. I might ramble a bit.
First, each of the C files referenced above should be treated as a special case. For example in pthread_create.c, pthread_exit() is only called as a error recovery measure. In that case, there can be no pthread_cleanup routines or pthread-specific destructors registered. So we can just replace the call to pthread_exit() with a call to nx_pthread_exit(). This might apply in other places too.
But in other cases, I am sure that it will be necessary to call the user-space pthread_exit() from the kernel code in order to perform those actions in user space. The basic problem is that in the PROTECTED build, the pthread_exit() symbol is not available to the kernel. That is because the user blob and the kernel blob are separately built and one blob knows nothing about the symbols internal to the other. The situation is worse in the KERNEL build because there will be multiple instances of pthread_exit() in each user block (aka, "process") and each will lie at a different virtual address -- unless of course we were to implement some kind of shared library support.
The simplest way that I can think of to do that is to extend the nx_pthread_create() interface again. On this branch it currently receives the address of the pthread_startup() function and the address of the pthread entry point. It could be extended to also receive the address of pthread_exit() function. This address is of no interest to the pthread_create() logic but it could be saved in pthread_tcb_s structure in the same way that the pthread_startup() function is, perhaps as 'pthread_exit'; Then, where ever pthread_exit() is called in the kernel, this could be replaced with tcb->pthread_exit(). That is not too bad.
[Hmm.. I suppose TLS would be a more appropriate place to keep all pthread data rather than the TCB. Although if you have the tcb in hand, you cannot be assured that the tcb corresponds to the current stack so you would have to use that stack base from the TCB.]
In the longer term, we need to get all of the pthread APIs out of the kernel, but this is certainly one positive step.
What do you think?
In KERNEL mode, there are other things to make sure we understand. In order to call a user space function in KERNEL mode, we have to have the correct address environment in place (up_select_addrenv()). I don't think that is will be a problem, because the main thread and all of the child pthreads share the same address environment. Therefore, I can't think of any situation were the cancellation could occur with the wrong address environment in place. But it is another complexity to keep in mind.
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652639845
>
>
> @gregory-nutt
>
> I have some changes to what you have so far that address build issues. However, I can't open a PR against your branch on your repo.
>
> Can we create a feature branch in the Apache repo to work on this together? That way we can make PRs against that branch.
@antmerlino Yes, but you would have to do that. We have a policy here that no one can merge their own PRs, but you are also a committer and can to do the merge. You have registered your Github user (See https://whimsy.apache.org/roster/ppmc/nuttx). So I assume you have created created your 2 factor login: If not see https://cwiki.apache.org/confluence/display/NUTTX/Accessing+Apache+GitHub+as+a+Committer
If you have done those things, then here is what you would have to do to create the feature branch:
1. Create the feature branch in your incubator-nuttx clone, say "pthread-user"
2. Push the branch to the incubator-nuttx origin
3. Go to this PR and select "Edit" on the right near the top.
4. Change the distination
From:
patacongo wants to merge 2 commits into apache:master from patacongo:pthread
To:
patacongo wants to merge 2 commits into apache:pthread-user from patacongo:pthread
4. Then just do the rebase merge using the buttons at the bottom
That is all there is to it!
NOTE: Since this is a WIP, we do not expect the PR checks to pass as this time.
----------------------------------------------------------------
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 #1328: pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-653646516
@antmerlino The branch is ready for your use: https://github.com/apache/incubator-nuttx/tree/feature/pthread-user We need to thank @Ouss4 for doing that for us.
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652525167
> But I feel like we should figure out the cancellation stuff before I do any of that.
>
> > There is a complexity... pthread_exit() is also called from within the OS from task_cancelpt.c, pthread_cancel.c, pthread_create.c, sig_default.c, task_cancelpt.c, task_setcancelstate.c, task_setcanceltype.c. This may require another system call analagous to up_pthread_start().
>
> I don't really understand what the analogous call would do for us there, could you expand?
Well, you are making me think on my feet, but I will do the best that I can. I might ramble a bit.
First, each of the C files referenced above should be treated as a special case. For example in pthread_create.c, pthread_exit() is only called as a error recovery measure. In that case, there can be no pthread_cleanup routines or pthread-specific destructors registered. So we can just replace the call to pthread_exit() with a call to nx_pthread_exit(). This might apply in other places too.
But in other cases, I am sure that it will be necessary to call the user-space pthread_exit() from the kernel code in order to perform those actions in user space. The basic problem is that in the PROTECTED build, the pthread_exit() symbol is not available to the kernel. That is because the user blob and the kernel blob are separately built and one blob knows nothing about the symbols internal to the other. The situation is worse in the KERNEL build because there will be multiple instances of pthread_exit() in each user block (aka, "process") and each will lie at a different virtual address -- unless of course we were to implement some kind of shared library support.
The simplest way that I can think of to do that is to extend the nx_pthread_create() interface again. On this branch it currently receives the address of the pthread_startup() function and the address of the pthread entry point. It could be extended to also receive the address of pthread_exit() function. This address is of no interest to the pthread_create() logic but it could be saved in pthread_tcb_s structure in the same way that the pthread_startup() function is, perhaps as 'pthread_exit'; Then, where ever pthread_exit() is called in the kernel, this could be replaced with tcb->pthread_exit(). That is not too bad.
In the longer term, we need to get all of the pthread APIs out of the kernel, but this is certainly one positive step.
What do you think?
In KERNEL mode, there are other things to make sure we understand. In order to call a user space function in KERNEL mode, we have to have the correct address environment in place (up_select_addrenv()). I don't think that is will be a problem, because the main thread and all of the child pthreads share the same address environment. Therefore, I can't think of any situation were the cancellation could occur with the wrong address environment in place. But it is another complexity to keep in mind.
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652525167
> But I feel like we should figure out the cancellation stuff before I do any of that.
>
> > There is a complexity... pthread_exit() is also called from within the OS from task_cancelpt.c, pthread_cancel.c, pthread_create.c, sig_default.c, task_cancelpt.c, task_setcancelstate.c, task_setcanceltype.c. This may require another system call analagous to up_pthread_start().
>
> I don't really understand what the analogous call would do for us there, could you expand?
Well, you are making me think on my feet, but I will do the best that I can. I might ramble a bit.
First, each of the C files referenced above should be treated as a special case. For example in pthread_create.c, pthread_exit() is only called as a error recovery measure. In that case, there can be no pthread_cleanup routines or pthread-specific destructors registered. So we can just replace the call to pthread_exit() with a call to nx_pthread_exit(). This might apply in other places too.
But in other cases, I am sure that it will be necessary to call the user-space pthread_exit() from the kernel code in order to perform those actions in user space. The basic problem is that in the PROTECTED build, the pthread_exit() symbol is not available to the kernel. That is because the user blob and the kernel blob are separately built and one blob knows nothing about the symbols internal to the other. The situation is worse in the KERNEL build because there will be multiple instances of pthread_exit() in each user block (aka, "process") and each will lie at a different virtual address -- unless of course we were to implement some kind of shared library support.
The simplest way that I can think of to do that is to extend the nx_pthread_create() interface again. On this branch it currently receives the address of the pthread_startup() function and the address of the pthread entry point. It could be extended to also receive the address of pthread_exit() function. This address is of no interest to the pthread_create() logic but it could be saved in pthread_tcb_s structure in the same way that the pthread_startup() function is, perhaps as 'pthread_exit'; Then, where ever pthread_exit() is called in the kernel, this could be replaced with tcb->pthread_exit(). That is not too bad.
[Hmm.. I suppose TLS would be a more appropriate place to keep all pthread data rather than the TCB. Although if you have the tcb in hand, you cannot be assured that the tcb corresponds to the current stack so you would have to use that stack base from the TCB.]
This approach would also require something like up_pthread_start(). That function calls the specified pthread startup function and switches to user mode. We need some mechanism to call the saved pthread_exit() also switching to user mode.
In the longer term, we need to get all of the pthread APIs out of the kernel, but this is certainly one positive step.
What do you think?
In KERNEL mode, there are other things to make sure we understand. In order to call a user space function in KERNEL mode, we have to have the correct address environment in place (up_select_addrenv()). I don't think that is will be a problem, because the main thread and all of the child pthreads share the same address environment. Therefore, I can't think of any situation were the cancellation could occur with the wrong address environment in place. But it is another complexity to keep in mind.
----------------------------------------------------------------
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] antmerlino commented on pull request #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652546704
Ah okay, now I see. The piece I wasn't following is that you are going to provide a reference to the userspace exit hook inside the create/startup call, so that the kernel knows how to call.
I think what you laid out makes sense.
Why don't I create PRs onto your branch and you can pull them in or decline them as we go. This will give you a chance to review the changes as we go. You definitely understand the subtleties more than I.
----------------------------------------------------------------
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 #1328: [DRAFT] pthread_cleanup functions must be called from user space
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1328:
URL: https://github.com/apache/incubator-nuttx/pull/1328#issuecomment-652551262
> Why don't I create PRs onto your branch and you can pull them in or decline them as we go. This will give you a chance to review the changes as we go. You definitely understand the subtleties more than I.
Sounds good to me. The level of complexity and keeping the flow all in your head is tricky. I miss subtleties all of the time. And, as with most engineering topics, communicating the technical issues is always the most difficult part by far. I am terrible at trying to read and understand GIT differences.
I will try to update the discussion in this PR. I think that a good set of comments here could form a kind of design specification and could also be the basis for a document or WiKi page that documents how this works.
----------------------------------------------------------------
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