Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 merged PR #11165: URL: https://github.com/apache/nuttx/pull/11165 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1393798174
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
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.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1393736227
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
Review Comment:
That would be an option yes. I think I will just drop this patch, we can
make the changes when pthread_ functions are moved to libc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1393601024
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
Review Comment:
Ok, why not move enter_cancellation_point/leave_cancellation_point close
nxsem_clockwait_uninterruptible ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1393167474
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
Review Comment:
nxsem_clockwait_uninterruptible should not be (and is not) a cancellation
point though.
Anything with the nx-prefix should not be a cancellation point. I think the
naming is like "nx-prefix means the function is a kernel API" and kernel does
not use cancel points.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1393167474
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
Review Comment:
nxsem_clockwait_uninterruptible should not (and is not) a cancellation point
though.
Anything with the nx-prefix should not be a cancellation point. I think the
naming is like "nx-prefix means the function is a kernel API" and kernel does
not use cancel points.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1392939538
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
Review Comment:
But since both pthread_cond_wait and nxsem_clockwait_uninterruptible are
cancellation point, it's safe and simple to remove
enter_cancellation_point/leave_cancellation_point from this function, if you
don't want the nest of cancellation point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1392930328
##
include/nuttx/tls.h:
##
@@ -309,9 +315,28 @@ uintptr_t task_tls_get_value(int tlsindex);
#elif defined(CONFIG_TLS_ALIGNED) && !defined(__KERNEL__)
Review Comment:
In this case, we can always use tls_get_info_pid, the macro is to boost the
speed by skipping the syscall.
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
Review Comment:
But since both pthread_cond_wait and nxsem_clockwait_uninterruptible are
cancellation point, it's safe to remove
enter_cancellation_point/leave_cancellation_point from this function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1392193610 ## include/nuttx/tls.h: ## @@ -309,9 +315,28 @@ uintptr_t task_tls_get_value(int tlsindex); #elif defined(CONFIG_TLS_ALIGNED) && !defined(__KERNEL__) Review Comment: The thing I'm not sure about is how to implement nxnotify_cancellation if some platform uses up_tls_info. nxnotify_cancellation is in the kernel (for now) and it needs to find the TLS area of another thread, I made tls_get_info_pid() for this. up_tls_info can only locate the TLS of the current thread. Another option would be to move nxnotify_cancellation to libc as well, and just sending a signal to the canceled thread instead. This will require moving pthread_cancel to libc though. It might also require adding a new non-maskable signal (SIGCANCELED?) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1392193610 ## include/nuttx/tls.h: ## @@ -309,9 +315,28 @@ uintptr_t task_tls_get_value(int tlsindex); #elif defined(CONFIG_TLS_ALIGNED) && !defined(__KERNEL__) Review Comment: The thing I'm not sure about is how to implement nxnotify_cancellation if some platform uses up_tls_info. nxnotify_cancellation is in the kernel (for now) and it needs to find the TLS area of another thread, I made tls_get_info_pid() for this. up_tls_info can only locate the TLS of the current thread. Another option would be to move nxnotify_cancellation to libc as well, and just sending a signal to the canceled thread instead. This will require moving pthread_cancel to libc though. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1391471383
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
Review Comment:
I suggest we leave this file as is, even though this is not a cancellation
point (as it is a GNU extension of POSIX). But GNU declares that this should
behave exactly like pthread_cond_timedwait , which is a cancellation point.
https://www.gnu.org/software/libc/manual/html_node/Waiting-with-Explicit-Clocks.html
At some point I hope to move this file (and the rest) to libc, and call
nx_pthread_cond_wait (et al) via call gate, and in this case the cancel point
logic must be in this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1391460655 ## sched/tls/tls_initinfo.c: ## @@ -22,9 +22,13 @@ * Included Files / +#include + #include #include +#include Review Comment: Not needed anymore, yes. It was because of the initializatin of tl_cpstate/count but I removed them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1391459966 ## sched/task/task_setcanceltype.c: ## @@ -52,77 +52,3 @@ * / Review Comment: Oops, yes it should be just removed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1391341383
##
include/nuttx/tls.h:
##
@@ -206,6 +206,12 @@ struct tls_info_s
struct pthread_cleanup_s tl_stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
#endif
+ uint8_t tl_cpstate; /* Cancellation state */
+
+#ifdef CONFIG_CANCELLATION_POINTS /* REVISIT: Remove this */
Review Comment:
remove `/* REVISIT: Remove this */`
##
sched/task/task_setcanceltype.c:
##
@@ -52,77 +52,3 @@
*
/
Review Comment:
why not remove this file directly
##
sched/pthread/pthread_condclockwait.c:
##
@@ -104,7 +105,7 @@ int pthread_cond_clockwait(FAR pthread_cond_t *cond,
else if (!abstime)
{
- ret = pthread_cond_wait(cond, mutex);
+ ret = nx_pthread_cond_wait(cond, mutex);
Review Comment:
why not remove enter_cancellation_point/leave_cancellation_point from this
file directly
##
sched/tls/tls_initinfo.c:
##
@@ -22,9 +22,13 @@
* Included Files
/
+#include
+
#include
#include
+#include
Review Comment:
why need change this file
##
include/nuttx/tls.h:
##
@@ -309,9 +315,28 @@ uintptr_t task_tls_get_value(int tlsindex);
#elif defined(CONFIG_TLS_ALIGNED) && !defined(__KERNEL__)
Review Comment:
no, as far as I know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1391172544 ## sched/tls/tls_initinfo.c: ## @@ -66,5 +70,17 @@ int tls_init_info(FAR struct tcb_s *tcb) /* Attach per-task info in group to TLS */ info->tl_task = tcb->group->tg_info; + + /* Initialize cancellation state */ + +#ifdef CONFIG_CANCELLATION_POINTS + /* Set the deferred cancellation type */ + + info->tl_cpstate |= CANCEL_FLAG_CANCEL_DEFERRED; + info->tl_cpcount = 0; /* REVISIT: Remove this */ Review Comment: I mean the assignment, since info is zerod by default. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on PR #11165: URL: https://github.com/apache/nuttx/pull/11165#issuecomment-1807967384 > @pussuw what are the pros and cons of moving the cancellation points to user space/libc ? @acassis My primary concern and motivation for doing these changes is to improve security via cleaner kernel/userspace separation. Improving security is a pro for me. It is also the most important improvement point in NuttX for me. The con is of course a possible regression. I presume many people are happy with how things are right now and would rather not see such architectural changes. **So why do it at all then ?** It is a prerequisite for moving `sem_` `pthread_` et all into libc/userspace as well (from the sched/ folder) and instead of exposing e.g. `sem_wait()` as system call, `nxsem_wait()` would be the gate into kernel, while the libc part handles errno and cancel points. **What is the problem I'm trying to fix?** I am trying to find a fix for the current user space semaphores, specifically for the kernel build. The problem is that the sem_t data goes into user memory (waitlist, flags, holderlist). The user should only need access to the counter, the rest is needed by the kernel. The problem with having that data in user memory is obviously that the user memory might not always be accessible via a user space pointer (wrong address environment can be active). This is a long story but some of it is in here https://github.com/apache/nuttx/issues/8917 and some is on the mailing list -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390941495 ## libs/libc/sched/CMakeLists.txt: ## @@ -26,11 +26,11 @@ set(SRCS clock_timespec_add.c clock_timespec_subtract.c clock_getcpuclockid.c -clock_getres.c) - -if(NOT CONFIG_CANCELLATION_POINTS) Review Comment: The way it worked previously: - There were two versions of task_setcanceltype.c and task_testcancel.c, one in sched/task and one in libc/sched - If cancelpoints=y then the sched/task versions are compiled, if cancelpoints=n then the libc versions are compiled - The libc versions were just dummies that do nothing, besides provide the task_setcanceltype() and task_testcancel() symbols for compilation - Now there is only 1 version, in libc, which means the libc version must always be compiled, otherwise the symbols are missing and the code will not compile -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390939391 ## include/nuttx/tls.h: ## @@ -205,6 +205,12 @@ struct tls_info_s struct pthread_cleanup_s stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE]; 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390938982 ## include/nuttx/tls.h: ## @@ -309,9 +315,28 @@ uintptr_t task_tls_get_value(int tlsindex); #elif defined(CONFIG_TLS_ALIGNED) && !defined(__KERNEL__) Review Comment: I mean I can not find a single reference in the upstream nuttx project, do you think such downstream implementations can exist ? ``` ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$ grep up_tls_info -nr . ./libs/libc/tls/tls_getinfo.c:32:#if !defined(up_tls_info) && (defined(__KERNEL__) || !defined(CONFIG_TLS_ALIGNED)) ./libs/libc/tls/tls_getinfo.c:74:#endif /* !defined(up_tls_info) && (defined(__KERNEL__) || !defined(CONFIG_TLS_ALIGNED)) */ ./include/nuttx/tls.h:313:#if defined(up_tls_info) ./include/nuttx/tls.h:314:# define tls_get_info() up_tls_info() ./include/nuttx/arch.h:1935: * Name: up_tls_info ./include/nuttx/arch.h:1963: * FAR struct tls_info_s *up_tls_info(void); ./Documentation/ReleaseNotes/NuttX-10.3.0:100:* [#5629](https://github.com/apache/nuttx/pull/5629) libc: fix up_tls_info define for BUILD_KERNEL ./Documentation/ReleaseNotes/NuttX-10.3.0:231:* [#4941](https://github.com/apache/nuttx/pull/4941) arch: Remove the duplicated up_tls_info implementation ``` ## include/nuttx/tls.h: ## @@ -309,9 +315,28 @@ uintptr_t task_tls_get_value(int tlsindex); #elif defined(CONFIG_TLS_ALIGNED) && !defined(__KERNEL__) Review Comment: I mean I can not find a single reference in the upstream nuttx project, do you think such downstream implementations can exist ? ``` ville@workpc:~/wpc_toolchain/workspace/nuttx-bare/nuttx$ grep up_tls_info -nr . ./libs/libc/tls/tls_getinfo.c:32:#if !defined(up_tls_info) && (defined(__KERNEL__) || !defined(CONFIG_TLS_ALIGNED)) ./libs/libc/tls/tls_getinfo.c:74:#endif /* !defined(up_tls_info) && (defined(__KERNEL__) || !defined(CONFIG_TLS_ALIGNED)) */ ./include/nuttx/tls.h:313:#if defined(up_tls_info) ./include/nuttx/tls.h:314:# define tls_get_info() up_tls_info() ./include/nuttx/arch.h:1935: * Name: up_tls_info ./include/nuttx/arch.h:1963: * FAR struct tls_info_s *up_tls_info(void); ./Documentation/ReleaseNotes/NuttX-10.3.0:100:* [#5629](https://github.com/apache/nuttx/pull/5629) libc: fix up_tls_info define for BUILD_KERNEL ./Documentation/ReleaseNotes/NuttX-10.3.0:231:* [#4941](https://github.com/apache/nuttx/pull/4941) arch: Remove the duplicated up_tls_info implementation ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390933827 ## sched/tls/tls_initinfo.c: ## @@ -66,5 +70,17 @@ int tls_init_info(FAR struct tcb_s *tcb) /* Attach per-task info in group to TLS */ info->tl_task = tcb->group->tg_info; + + /* Initialize cancellation state */ + +#ifdef CONFIG_CANCELLATION_POINTS + /* Set the deferred cancellation type */ + + info->tl_cpstate |= CANCEL_FLAG_CANCEL_DEFERRED; + info->tl_cpcount = 0; /* REVISIT: Remove this */ Review Comment: Can you please clarify what is not needed ? The comment ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390933549 ## include/nuttx/tls.h: ## @@ -205,6 +205,12 @@ struct tls_info_s struct pthread_cleanup_s stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE]; #endif + uint8_t tl_cpstate; /* Cancellation state */ Review Comment: No, thread cancellation (asynchronous cancel) must still work even if cancellation points are disabled. So this is needed regardless -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390933131
##
libs/libc/sched/task_cancelpt.c:
##
@@ -0,0 +1,289 @@
+/
+ * libs/libc/sched/task_cancelpt.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.
+ *
+ /
+
+/
+ * Cancellation Points.
+ *
+ * Cancellation points shall occur when a thread is executing the following
+ * functions:
+ *
+ * accept() mq_timedsend() putpmsg() sigtimedwait()
+ * aio_suspend() msgrcv() pwrite()sigwait()
+ * clock_nanosleep() msgsnd() read() sigwaitinfo()
+ * close() msync() readv() sleep()
+ * connect() nanosleep() recv() system()
+ * creat() open() recvfrom() tcdrain()
+ * fcntl() pause() recvmsg() usleep()
+ * fdatasync() poll() select()wait()
+ * fsync() pread() sem_timedwait() waitid()
+ * getmsg() pselect()sem_wait() waitpid()
+ * getpmsg() pthread_cond_timedwait() send() write()
+ * lockf() pthread_cond_wait() sendmsg() writev()
+ * mq_receive() pthread_join() sendto()
+ * mq_send() pthread_testcancel() sigpause()
+ * mq_timedreceive() putmsg() sigsuspend()
+ *
+ * Each of the above function must call enter_cancellation_point() on entry
+ * in order to establish the cancellation point and
+ * leave_cancellation_point() on exit. These functions are described below.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#ifdef CONFIG_CANCELLATION_POINTS
+
+/
+ * Public Functions
+ /
+
+/
+ * Name: enter_cancellation_point
+ *
+ * Description:
+ * Called at the beginning of the cancellation point to establish the
+ * cancellation point. This function does the following:
+ *
+ * 1. If deferred cancellation does not apply to this thread, nothing is
+ * done, otherwise, it
+ * 2. Sets state information in the caller's TCB and increments a nesting
+ * count.
+ * 3. If this is the outermost nesting level, it checks if there is a
+ * pending cancellation and, if so, calls either exit() or
+ * pthread_exit(), depending upon the type of the thread.
+ *
+ * Input Parameters:
+ * None
+ *
+ * Returned Value:
+ * true is returned if a cancellation is pending but cannot be performed
+ * now due to the nesting level.
+ *
+ /
+
+bool enter_cancellation_point(void)
+{
+ FAR struct tls_info_s *tls = tls_get_info();
+ bool ret = false;
+
+ /* Disabling pre-emption should provide sufficient protection. We only
+ * need the TCB to be stationary (no interrupt level modification is
+ * anticipated).
+ *
+ * REVISIT: is locking the scheduler sufficient in SMP mode?
+ */
+
+ sched_lock();
Review Comment:
Ok let me remove them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on PR #11165: URL: https://github.com/apache/nuttx/pull/11165#issuecomment-1807897534 > > @pkarashchenko @patacongo You might be interested in this. Also, do you mind if I remove the nested cancellation point logic now as well ? > > That is something I had planned to do and is a good idea if you are careful. My primary concern was "sneak" paths that could cause nesting. I don't know if the following is possible but consider this: > > 1. A task sets up a signal handler and calls a cancellation point function, > > 2. The function signals the task, since it is the same task, I think that the signal handler will run immediatedly (true?), > > 3. The signal handler runs and calls another cancellation pointer function, effectively resulting in nesting. > > > If such a think could happen, then that would likely result in a resource leak since the second cancellation point function would not be allowed to do its clean-up. > > One thing that you can do is: > > 1. Retain the nesting count (only) and only if CONFIG_DEBUG_ASSERTIONS is enabled > > 2. Then assert that the nesting count is zero on entry into the cancellation point function. Yes the situation you describe is entirely possible when system calls are disabled. When system calls are enabled the signal delivery gets deferred until the system call is finished. So maybe it is not a good idea for me to fiddle with that part after all.. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
patacongo commented on PR #11165: URL: https://github.com/apache/nuttx/pull/11165#issuecomment-1806995884 > @pkarashchenko @patacongo You might be interested in this. Also, do you mind if I remove the nested cancellation point logic now as well ? That is something I had planned to do and is a good idea if you are careful. My primary concern was "sneak" paths that could cause nesting. I don't know if the following is possible but consider this: 1. A task sets up a signal handler and calls a cancellation point function, 2. The function signals the task, since it is the same task, I think that the signal handler will run immediatedly (true?), 3. The signal handler runs and calls another cancellation pointer function, effectively resulting in nesting. If such a think could happen, then that would likely result in a resource leak since the second cancellation point function would not be allowed to do its clean-up. One thing that you can do is: 1. Retain the nesting count (only) and only if CONFIG_DEBUG_ASSERTIONS is enabled 2. Then assert that the nesting count is zero on entry into the cancellation point function. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390225032
##
include/nuttx/tls.h:
##
@@ -205,6 +205,12 @@ struct tls_info_s
struct pthread_cleanup_s stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE];
#endif
+ uint8_t tl_cpstate; /* Cancellation state */
Review Comment:
should we move into `#ifdef CONFIG_CANCELLATION_POINTS` too
##
libs/libc/sched/task_cancelpt.c:
##
@@ -0,0 +1,289 @@
+/
+ * libs/libc/sched/task_cancelpt.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.
+ *
+ /
+
+/
+ * Cancellation Points.
+ *
+ * Cancellation points shall occur when a thread is executing the following
+ * functions:
+ *
+ * accept() mq_timedsend() putpmsg() sigtimedwait()
+ * aio_suspend() msgrcv() pwrite()sigwait()
+ * clock_nanosleep() msgsnd() read() sigwaitinfo()
+ * close() msync() readv() sleep()
+ * connect() nanosleep() recv() system()
+ * creat() open() recvfrom() tcdrain()
+ * fcntl() pause() recvmsg() usleep()
+ * fdatasync() poll() select()wait()
+ * fsync() pread() sem_timedwait() waitid()
+ * getmsg() pselect()sem_wait() waitpid()
+ * getpmsg() pthread_cond_timedwait() send() write()
+ * lockf() pthread_cond_wait() sendmsg() writev()
+ * mq_receive() pthread_join() sendto()
+ * mq_send() pthread_testcancel() sigpause()
+ * mq_timedreceive() putmsg() sigsuspend()
+ *
+ * Each of the above function must call enter_cancellation_point() on entry
+ * in order to establish the cancellation point and
+ * leave_cancellation_point() on exit. These functions are described below.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#ifdef CONFIG_CANCELLATION_POINTS
+
+/
+ * Public Functions
+ /
+
+/
+ * Name: enter_cancellation_point
+ *
+ * Description:
+ * Called at the beginning of the cancellation point to establish the
+ * cancellation point. This function does the following:
+ *
+ * 1. If deferred cancellation does not apply to this thread, nothing is
+ * done, otherwise, it
+ * 2. Sets state information in the caller's TCB and increments a nesting
+ * count.
+ * 3. If this is the outermost nesting level, it checks if there is a
+ * pending cancellation and, if so, calls either exit() or
+ * pthread_exit(), depending upon the type of the thread.
+ *
+ * Input Parameters:
+ * None
+ *
+ * Returned Value:
+ * true is returned if a cancellation is pending but cannot be performed
+ * now due to the nesting level.
+ *
+ /
+
+bool enter_cancellation_point(void)
+{
+ FAR struct tls_info_s *tls = tls_get_info();
+ bool ret = false;
+
+ /* Disabling pre-emption should provide sufficient protection. We only
+ * need the TCB to be stationary (no interrupt level modification is
+ * anticipated).
+ *
+ * REVISIT: is locking the scheduler sufficient in SMP mode?
+ */
+
+ sched_lock();
+
+ /* If cancellation is disabled on this thread or if this thread is using
+ * asynchronous cancellation, then do nothing.
+
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390200245 ## libs/libc/sched/CMakeLists.txt: ## @@ -26,11 +26,11 @@ set(SRCS clock_timespec_add.c clock_timespec_subtract.c clock_getcpuclockid.c -clock_getres.c) - -if(NOT CONFIG_CANCELLATION_POINTS) Review Comment: why remove the check of CONFIG_CANCELLATION_POINTS ## sched/tls/tls_initinfo.c: ## @@ -66,5 +70,17 @@ int tls_init_info(FAR struct tcb_s *tcb) /* Attach per-task info in group to TLS */ info->tl_task = tcb->group->tg_info; + + /* Initialize cancellation state */ + +#ifdef CONFIG_CANCELLATION_POINTS + /* Set the deferred cancellation type */ + + info->tl_cpstate |= CANCEL_FLAG_CANCEL_DEFERRED; + info->tl_cpcount = 0; /* REVISIT: Remove this */ Review Comment: don't need ## include/nuttx/tls.h: ## @@ -205,6 +205,12 @@ struct tls_info_s struct pthread_cleanup_s stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE]; Review Comment: could you create a new patch adding tl_ prefix to tos and stack -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
xiaoxiang781216 commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1390199104 ## include/nuttx/tls.h: ## @@ -309,9 +315,28 @@ uintptr_t task_tls_get_value(int tlsindex); #elif defined(CONFIG_TLS_ALIGNED) && !defined(__KERNEL__) Review Comment: up_tls_info is for the grow up stack which doesn't exist on all supported arch. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
acassis commented on PR #11165: URL: https://github.com/apache/nuttx/pull/11165#issuecomment-1806267343 @pussuw what are the pros and cons of moving the cancellation points to user space/libc ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165:
URL: https://github.com/apache/nuttx/pull/11165#discussion_r1389263469
##
libs/libc/sched/task_setcanceltype.c:
##
@@ -46,14 +54,70 @@
int task_setcanceltype(int type, FAR int *oldtype)
{
+ FAR struct tls_info_s *tls = tls_get_info();
+ int ret = OK;
+
+ /* Suppress context changes for a bit so that the flags are stable. (the
+ * flags should not change in interrupt handling).
+ */
+
+ sched_lock();
Review Comment:
I'm not sure if this is needed any longer, guarding tcb->flags makes sense,
but now no one else but the thread itself writes to tl_cpstate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on code in PR #11165: URL: https://github.com/apache/nuttx/pull/11165#discussion_r1389261277 ## include/nuttx/tls.h: ## @@ -309,9 +315,28 @@ uintptr_t task_tls_get_value(int tlsindex); #elif defined(CONFIG_TLS_ALIGNED) && !defined(__KERNEL__) Review Comment: Does someone know what up_tls_info is about ? I can not find any references for it in the project ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw commented on PR #11165: URL: https://github.com/apache/nuttx/pull/11165#issuecomment-1805515910 @pkarashchenko @patacongo You might be interested in this. Also, do you mind if I remove the nested cancellation point logic now as 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] task/pthread_cancelpt: Move cancel point handling to libc, data to TLS [nuttx]
pussuw opened a new pull request, #11165: URL: https://github.com/apache/nuttx/pull/11165 ## Summary This moves task / thread cancel point logic from the NuttX kernel into libc, while the data needed by the cancel point logic is moved to TLS. Why do it ? The change is an enabler to move user-space APIs to libc as well, for a coherent user/kernel separation. The first one I will move is the semaphore API, and remove the _SEM_XX() macros that are currently needed to glue sem_ and nxsem_ APIs in libc. ## Impact Should be none functionally (if no regression), quite significant architecturally. ## Testing rv-virt:nsh64 (ostest), knsh64 (limited test), MPFS target with >100 processes threads that spawn / exit all the 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
