Remove the secondary_stacks array in arm64 and riscv kernels.
ClosedPublic
Actions

Authored by markj on Mar 22 2020, 9:06 PM.

Details

Reviewers

manu
kib

Group Reviewers

riscv
arm64

Commits

rS359280: Remove the secondary_stacks array in arm64 and riscv kernels.

Summary

Instead, dynamically allocate a page for the boot stack of each AP when
starting them up, like we do on x86. This shrinks the bss by
MAXCPU*KSTACK_PAGES pages, which corresponds to 4MB on arm64 and 256KB
on riscv. mpentry is slightly simplified as well.

Duplicate the logic used on x86 to free the bootstacks, by using a
sysinit to wait for APs to switch to a thread.

While here, mark some static MD variables as such.

Test Plan

Tested boot-up in QEMU for arm64 and riscv.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30051
Build 27862: arc lint + arc unit

Event Timeline

markj created this revision.Mar 22 2020, 9:06 PM

Herald added a reviewer: manu. · View Herald TranscriptMar 22 2020, 9:06 PM

Herald added subscribers: emaste, andrew. · View Herald Transcript

Harbormaster completed remote builds in B30050: Diff 69780.Mar 22 2020, 9:06 PM

markj added reviewers: kib, riscv, arm64.Mar 22 2020, 9:06 PM

markj edited the test plan for this revision. (Show Details)

kib accepted this revision.Mar 22 2020, 10:24 PM

kib added inline comments.

sys/arm64/arm64/mp_machdep.c
279	Don't you need to check for bootstacks[cpu] == NULL before waiting for curpcb change ?
479–480	I would add 'err' value to the printf.

This revision is now accepted and ready to land.Mar 22 2020, 10:24 PM

Address feedback:

Print the error from PSCI if we fail to start a CPU.
Check for a NULL bootstack pointer before freeing, on both riscv and arm64.

This revision now requires review to proceed.Mar 22 2020, 11:26 PM

Harbormaster completed remote builds in B30051: Diff 69784.Mar 22 2020, 11:26 PM

kib accepted this revision.Mar 22 2020, 11:31 PM

This revision is now accepted and ready to land.Mar 22 2020, 11:31 PM

andrew added inline comments.Mar 23 2020, 9:52 AM

sys/arm64/arm64/mp_machdep.c
485	Is there a reason to not use `wfe` here?

markj added inline comments.Mar 23 2020, 3:22 PM

sys/arm64/arm64/mp_machdep.c
485	I don't think so, I just assumed cpu_spinwait() would be fine since we should spend relatively little time here.

markj added inline comments.Mar 23 2020, 3:47 PM

sys/arm64/arm64/mp_machdep.c
485	... presumably there is some threshold where a short wfe interval is not any better from a power consumption perspective than a spinwait loop?

Closed by commit rS359280: Remove the secondary_stacks array in arm64 and riscv kernels. (authored by markj). · Explain WhyMar 24 2020, 6:43 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rS359280: Remove the secondary_stacks array in arm64 and riscv kernels..

Herald added a subscriber: imp. · View Herald TranscriptMar 24 2020, 6:43 PM

mmel added a subscriber: mmel.Jun 8 2022, 7:29 AM

mmel added inline comments.

head/sys/arm64/arm64/mp_machdep.c
281 ↗	(On Diff #69835)	Hi Mark, Unfortunately, this creates a race condition. In cpu_throw(), pc_curpcb[1] is set before switching to kernel stack[2] and moreover the stack is used between these two points (because pmap_switch() is implemented in C). This means that the bootstrap stack can be freed while the AP is still using it. I've encountered this problem on my own RK3399 boards, where the primary CPU cluster (i.e. bootstrap CPU) starts at a fast clock speed, but the secondary CPU cluster starts at a very low frequency (tens of MHz)... [1] https://cgit.freebsd.org/src/tree/sys/arm64/arm64/swtch.S#n84 [2] https://cgit.freebsd.org/src/tree/sys/arm64/arm64/swtch.S#n93