Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sched: nxtask_start should call entry point directly for kernel thread #1922

Merged
merged 1 commit into from Oct 8, 2020

Conversation

xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 commented Sep 30, 2020

Summary

since nxtask_startup will initialize c++ global variables which shouldn't be done inside the kernel thread.

Impact

C++ global variables may initialize twice when SMP is enabled because cxx_initialize don't have any sync code. But we don't need add mutex/spinlock code to cxx_initialize, because we should avoid the kernel thread call cxx_initialize in the first place. Once we ensure that cxx_initialize get called only inside the user task entry point, the sync isn't a problem anymore because NuttX kernel always create one and only one user task(init).

Testing

@jerpelea
Copy link
Contributor

jerpelea commented Sep 30, 2020

I don't think that this change is going into the right direction since we will see more and more SMP capable HW

sched/task/task_start.c Outdated Show resolved Hide resolved
since nxtask_startup will initialize c++ global variables which shouldn't
be done inside the kernel thread

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
@xiaoxiang781216
Copy link
Contributor Author

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.

@xiaoxiang781216
Copy link
Contributor Author

@jerpelea could you review again?

@masayuki2009
Copy link
Contributor

@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().

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Oct 8, 2020

@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: #1341 and apache/nuttx-apps#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.

@masayuki2009
Copy link
Contributor

@xiaoxiang781216
Thaks for your detailed explanation.

@jerpelea
I think it looks good to me but do you have any other comments.

@jerpelea
Copy link
Contributor

jerpelea commented Oct 8, 2020

LGTM

@jerpelea jerpelea merged commit c59fcd3 into apache:master Oct 8, 2020
@btashton btashton added this to To-Add in Release Notes - 10.1.0 Apr 12, 2021
@jerpelea jerpelea moved this from To-Add to core in Release Notes - 10.1.0 Apr 13, 2021
@jerpelea jerpelea moved this from core to Added in Release Notes - 10.1.0 Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants