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/09/30 10:23:50 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1922: sched: nxtask_start should call entry point directly for kernel thread
xiaoxiang781216 opened a new pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922
## Summary
since nxtask_startup will initialize c++ global variables which shouldn't
be done inside the kernel thread
## 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] jerpelea merged pull request #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
jerpelea merged pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705377635
@xiaoxiang781216
Thaks for your detailed explanation.
@jerpelea
I think it looks good to me but do you have any other comments.
----------------------------------------------------------------
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 a change in pull request #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#discussion_r497481819
##########
File path: sched/task/task_start.c
##########
@@ -134,18 +134,18 @@ void nxtask_start(void)
* we have to switch to user-mode before calling the task.
*/
-#ifndef CONFIG_BUILD_FLAT
if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KERNEL)
{
+#ifndef CONFIG_BUILD_FLAT
Review comment:
@xiaoxiang781216 What about @jerpelea concerns?
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-701372155
> I don't think that this change is going into the right direction since we will see more and more SMP capable HW
@jerpelea could you give more info? This patch fix the problem we found in SMP configuration.
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705357685
> @xiaoxiang781216
>
> As long as I checked the latest upstream, I think the issue does not relate to SMP.
Yes, it isn't SMP specific. SMP just expose the problem:
Two CPUs idle thread call nxtask_startup->cxx_initialize at the same time which make all global constructors are executed twice.
But the real issue is that we shouldn't initialize C++ global variables inside kernel space at all, because all kernel threads(include idle thread) shouldn't use any C++ element at all).
The good side effect with this fix is that the synchronization issue also get resolved too, because kernel always create one and only one user task(init) and cxx_initialize get called before init's main has the chance to create more tasks concurrently.
> Actually the issue happens with lm3s6965-ek:discover configuration.
>
> Also, I noticed that this PR only resolves the issue for kernel thread as titled.
> However, for example, lm3s6965-ek:discover creates 'Telnet daemon' which
> will call cxx_initialize() in nxtask_startup().
Yes, it is the expected behaviour, please see PR: https://github.com/apache/incubator-nuttx/pull/1341 and https://github.com/apache/incubator-nuttx-apps/pull/316.
The motivation is centralize the c++ initialization to common place. Of course, all old code which call cxx_initialize manually should be removed to avoid the double initialization issue.
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705357685
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705357685
> @xiaoxiang781216
>
> As long as I checked the latest upstream, I think the issue does not relate to SMP.
Yes, it isn't SMP specific. SMP just expose the problem:
Two CPUs idle thread call nxtask_startup->cxx_initialize at the same time which make all global constructors are executed twice.
But the real issue is that we shouldn't initialize C++ global variables inside kernel space at all, because all kernel threads(include idle thread) shouldn't use any C++ element at all).
> Actually the issue happens with lm3s6965-ek:discover configuration.
>
> Also, I noticed that this PR only resolves the issue for kernel thread as titled.
> However, for example, lm3s6965-ek:discover creates 'Telnet daemon' which
> will call cxx_initialize() in nxtask_startup().
Yes, it is the expected behaviour, please see PR: https://github.com/apache/incubator-nuttx/pull/1341 and https://github.com/apache/incubator-nuttx-apps/pull/316. The motivation is centralize the c++ initialization to common place.
----------------------------------------------------------------
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 a change in pull request #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#discussion_r497407154
##########
File path: sched/task/task_start.c
##########
@@ -134,18 +134,18 @@ void nxtask_start(void)
* we have to switch to user-mode before calling the task.
*/
-#ifndef CONFIG_BUILD_FLAT
if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KERNEL)
{
+#ifndef CONFIG_BUILD_FLAT
Review comment:
How about making this positive logic?
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#discussion_r497479084
##########
File path: sched/task/task_start.c
##########
@@ -134,18 +134,18 @@ void nxtask_start(void)
* we have to switch to user-mode before calling the task.
*/
-#ifndef CONFIG_BUILD_FLAT
if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KERNEL)
{
+#ifndef CONFIG_BUILD_FLAT
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705259301
@xiaoxiang781216
As long as I checked the latest upstream, I think the issue does not relate to SMP.
Actually the issue happens with lm3s6965-ek:discover configuration.
Also, I noticed that this PR only resolves the issue for kernel thread as titled.
However, for example, lm3s6965-ek:discover creates 'Telnet daemon' which
will call cxx_initialize() in nxtask_startup().
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
jerpelea commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705379477
LGTM
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
jerpelea commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705379477
LGTM
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-703285408
@jerpelea could you review again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705377635
@xiaoxiang781216
Thaks for your detailed explanation.
@jerpelea
I think it looks good to me but do you have any other comments.
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705357685
> @xiaoxiang781216
>
> As long as I checked the latest upstream, I think the issue does not relate to SMP.
Yes, it isn't SMP specific. SMP just expose the problem:
Two CPUs idle thread call nxtask_startup->cxx_initialize at the same time which make all global constructors are executed twice.
But the real issue is that we shouldn't initialize C++ global variables inside kernel space at all, because all kernel threads(include idle thread) shouldn't use any C++ element at all).
The good side effect with this fix is that the synchronization issue also get resolved too, because kernel always create one and only one user task(init) and cxx_initialize get called before init's main has the chance to create more tasks concurrently.
> Actually the issue happens with lm3s6965-ek:discover configuration.
>
> Also, I noticed that this PR only resolves the issue for kernel thread as titled.
> However, for example, lm3s6965-ek:discover creates 'Telnet daemon' which
> will call cxx_initialize() in nxtask_startup().
Yes, it is the expected behaviour, please see PR: https://github.com/apache/incubator-nuttx/pull/1341 and https://github.com/apache/incubator-nuttx-apps/pull/316. The motivation is centralize the c++ initialization to common place.
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705357685
> @xiaoxiang781216
>
> As long as I checked the latest upstream, I think the issue does not relate to SMP.
Yes, it isn't SMP specific. SMP just expose the problem:
Two CPUs idle thread call nxtask_startup->cxx_initialize at the same time which make all global constructors are executed twice.
But the real issue is that we shouldn't initialize C++ global variables inside kernel space at all, because all kernel threads(include idle thread) shouldn't use any C++ element at all).
The good side effect with this fix is that the synchronization issue also get resolved too, because kernel always create one and only one user task(init) and cxx_initialize get called before init's main has the chance to create more tasks concurrently.
> Actually the issue happens with lm3s6965-ek:discover configuration.
>
> Also, I noticed that this PR only resolves the issue for kernel thread as titled.
> However, for example, lm3s6965-ek:discover creates 'Telnet daemon' which
> will call cxx_initialize() in nxtask_startup().
Yes, it is the expected behaviour, please see PR: https://github.com/apache/incubator-nuttx/pull/1341 and https://github.com/apache/incubator-nuttx-apps/pull/316. The motivation is centralize the c++ initialization to common place.
Of course, all old code which call cxx_initialize manually should be removed to avoid the double initialization issue.
----------------------------------------------------------------
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 merged pull request #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
jerpelea merged pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922
----------------------------------------------------------------
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 #1922: sched: nxtask_start should call entry point directly for kernel thread
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1922:
URL: https://github.com/apache/incubator-nuttx/pull/1922#issuecomment-705357685
> @xiaoxiang781216
>
> As long as I checked the latest upstream, I think the issue does not relate to SMP.
Yes, it isn't SMP specific. SMP just expose the problem:
Two CPUs idle thread call nxtask_startup->cxx_initialize at the same time which make all global constructors are executed twice.
But the real issue is that we shouldn't initialize C++ global variables inside kernel space at all, because all kernel threads(include idle thread) shouldn't use any C++ element at all).
> Actually the issue happens with lm3s6965-ek:discover configuration.
>
> Also, I noticed that this PR only resolves the issue for kernel thread as titled.
> However, for example, lm3s6965-ek:discover creates 'Telnet daemon' which
> will call cxx_initialize() in nxtask_startup().
Yes, it is the expected behaviour, please see PR: https://github.com/apache/incubator-nuttx/pull/1341 and https://github.com/apache/incubator-nuttx-apps/pull/316. The motivation is centralize the c++ initialization to common place.
----------------------------------------------------------------
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