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

kernel: introduce k_smp_cpu_start() and k_smp_cpu_resume() #64755

Merged
merged 12 commits into from
Jan 17, 2024

Conversation

dcpleung
Copy link
Member

@dcpleung dcpleung commented Nov 2, 2023

  • This promotes z_smp_start_cpu() to public API as k_smp_cpu_start() so it can be used out of tree.
  • Also allow k_smp_cpu_start() to do additional initialization before handing off to scheduler to run threads if desired.
  • Add k_smp_cpu_resume() to address the need to power up a CPU without re-initialize per-CPU kernel structs.
  • This is a step towards eventual removal of z_smp_thread_init() and z_smp_thread_swap() exposed in include/zephyr/kernel/internal/smp.h specifically for SOF.

@dcpleung dcpleung force-pushed the kernel/custom_smp_start branch 3 times, most recently from 851de5e to 478101f Compare November 3, 2023 19:09
@zephyrbot
Copy link
Collaborator

zephyrbot commented Nov 3, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
sof zephyrproject-rtos/sof@e7cb489 zephyrproject-rtos/sof@37dd5e6 (zephyr) zephyrproject-rtos/sof@e7cb489d..37dd5e62

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@dcpleung
Copy link
Member Author

dcpleung commented Nov 3, 2023

@kv2019i zephyrproject-rtos/sof#32 is my crude attempt at changing the SOF secondary core power up code. I am not totally sure on the power management side. Please take a look and see if there are any issues.

cfriedt
cfriedt previously approved these changes Nov 5, 2023
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dcpleung and others added 12 commits January 17, 2024 07:36
For some reason, unrelated code change triggered compiler
warning about this function returns even though it is
marked nonreturn. So add CODE_UNREACHABLE to silence
the warning, possibly to catch any errors.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
qemu_x86_64 has default of 2 CPUs but the device tree only
has 1. For correctness, add another CPU node to the tree.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
This extends the wording so that not only architecture code can
start secondary CPUs at a later time. Also adds a missing 'to'.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
This makes the test a bit more generic so it can run on
other SMP enabled platforms. Though, we don't need CI
running this on every SMP platforms as this is not
testing the mechanism of bringing up a CPU. That is
being tested on the other SMP test. Therefore, only
enable qemu_x86_64 to be testing in CI so that this
feature will still be tested on CI.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
This renames z_smp_cpu_start() to k_smp_cpu_start().
This effectively promotes z_smp_cpu_start() into a public API
which allows out of tree usage. Since this is a new API,
we can afford to change it signature, where it allows
an additional initialization steps to be done before a newly
powered up CPU starts participating in scheduling threads
to run.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
Adds some comments to the SMP code to, hopefully, make it
a bit more clear to future readers.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
This provides a path to resume a previously suspended CPU.
This differs from k_smp_cpu_start() where per-CPU kernel
structs are not initialized such that execution context
can be saved during suspend and restored during resume.
Though the actual context saving and restoring are
platform specific.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
This extends the smp_boot_delay test to test the newly
introduced function k_smp_cpu_custom_start().

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
This updates the SOF to use the newly introduced
k_smp_cpu_custom_start() for secondary core power up.
This removes the need to mirror part of the SMP power up
code from Zephyr kernel into SOF tree. Also removes the need
to expose private kernel code to SOF.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
This removes z_smp_thread_init() and z_smp_thread_swap() as
SOF has been updated to use k_smp_cpu_custom_start() instead.
This removes the need for SOF to mirror part of the SMP
power up code, and thus these two functions are no longer
needed.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
z_init_cpu() is a private kernel API so move it under
kernel/include.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
When exiting power gated state, call the CPU start function
passed to arch_start_cpu().

Signed-off-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Daniel Leung <daniel.leung@intel.com>
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Jan 17, 2024
@dcpleung
Copy link
Member Author

SOF PR merged. Updated the manifest to use the commit SHA.

@nashif nashif merged commit 6a0b1da into zephyrproject-rtos:main Jan 17, 2024
26 of 27 checks passed
@dcpleung dcpleung deleted the kernel/custom_smp_start branch January 17, 2024 17:37
@RanderWang
Copy link
Contributor

RanderWang commented Jan 18, 2024

@kv2019i this PR was merged, so we need another PR to switch api in sof. I will submit it if the thesofproject/sof#8747 is merged

@kv2019i
Copy link
Collaborator

kv2019i commented Jan 19, 2024

@RanderWang please prepare a SOF PR that has your changes and updates the Zephyr version as well.

We have a SOF regression with Zephyr unfortunately now so thesofproject/sof#8747 might never be merged (in its current form) and we may need to jump to a newer Zephyr (if a fix is found on Zephyr side) and that will then require the k_smp changes, so better to prepare that PR now (even if some SOF CI fails are expected for it).

FYI @lyakh @marc-hb @abonislawski

z_dummy_thread_init(&dummy_thread);
#ifdef CONFIG_SYS_CLOCK_EXISTS
smp_timer_init();
#endif

/* Do additional initialization steps if needed. */
if ((csc != NULL) && (csc->fn != NULL)) {
csc->fn(csc->arg);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcpleung isn't this racy in general? On all CPUs csc points to cpu_start_fn, right? And the CPU gets here after signalling the initiator CPU, the one that started this one. So the initiator CPU can go ahead and start yet another CPU potentially with different parameters. So far that shouldn't be a problem for SOF, because there .fn is the same for all and .arg is always NULL, but in general this seems wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is potential for conflicts here. So the caller must take precautions to avoid that situation. Or we can implement a per-CPU data struct (but this is going to take up some space).

/* Secondary core is resumed by set_dx */
if (arch_proc_id()) {
mp_resume_entry();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been causing regressions and test failures (thesofproject/sof#8818). Tentative removal of this part in:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Audio area: Base OS Base OS Library (lib/os) area: Kernel area: X86 x86 Architecture (32-bit) manifest manifest-sof platform: Intel ADSP Intel Audio platforms platform: X86 x86 and x86-64
Projects
None yet
Development

Successfully merging this pull request may close these issues.