Move pcb out of the kstack for arm64 and riscv64 #23

Manually merged
jhb merged 12 commits from jhb/pcb_kstack into main 2026-05-27 14:59:42 +00:00 AGit
Member

This moves the allocation of pcbs out of the kstack and into new
dedicated UMA zones. UMA zones were used instead of embedding the pcb
into struct mdthread due to the size of the pcb on these
architectures. Possibly these architectures should switch to
allocating the floating point/vector register sets separately from the
pcb as is done on amd64 in which case the pcb could be embedded in
mdthread.

This moves the allocation of pcbs out of the kstack and into new dedicated UMA zones. UMA zones were used instead of embedding the pcb into struct mdthread due to the size of the pcb on these architectures. Possibly these architectures should switch to allocating the floating point/vector register sets separately from the pcb as is done on amd64 in which case the pcb could be embedded in mdthread.
All other paths that return from the kernel to userspace pop the user
trapframe off of the kernel stack pointer before returning to
userspace in restore_registers.  fork_trampoline was missing this, so
all of the user faults after fork pushed another trapframe leaving a
trapframe's worth of wasted space on the kstack.

This would be fatal after a future change to remove duplicate
initialization of td_frame in cpu_fork() as without this fix each time
a thread was recycled it would "lose" another trapframe's worth of
space.
Sponsored by:	AFRL, DARPA
This used to be needed to initialize the pcb pointer when the pcb was
allocated on the kstack.
cpu_thread_alloc() already sets these fields anytime td_kstack changes.
Previously, the cpu_thread_alloc callback was invoked each time a
kernel stack was allocated for a thread.  This included thread
creation, but it was also invoked if a recycled thread had to allocate
a new kstack.  This means that cpu_thread_alloc could be called
multiple times for a single thread, but cpu_thread_free is only called
once.  Not only that, but the cpu_thread_alloc callback can't tell if
it is being invoked on a new thread object, or a recycled thread.
Calling *_alloc multiple times on an object is also atypical for
kernel APIs.

As a result of this confusion, amd64 was potentially leaking an XSAVE
buffer each time a new kstack was allocated for an existing thread,
since cpu_thread_alloc for amd64 always allocated a new XSAVE buffer.
In practice, this edge case is probably rare.  A process object needs
to be recycled where either the new or old process is a kernel process
with a non-default kernel stack size.

Nevertheless, to ease the confusion, redefine cpu_thread_alloc to only
be called once when a new thread is allocated.  The new callback,
cpu_thread_new_kstack is invoked each time a kstack is allocated for a
thread, including both at thread creation time and if a recycled
thread allocates a new kstack.  The new callback should set any fields
whose value is dependent on td_kstack (e.g. the user frame in
td_frame, or td_pcb if the PCB is allocated on the kstack).
This is similar to commit 5e921ff49e
which moved the pcb for amd64, but a bit different.  arm64's pcb is
much larger (over 1KB!) than amd64's since it still embeds FP
registers.  Moving the pcb out of the kstack frees up that much
additional kstack space.  Unlike amd64 however, embedding the pcb in
struct mdthread is not practical as the resulting struct thread would
grow such that UMA would now store 1 thread per 4k page instead of 2
threads per page.  By using a separate UMA zone for pcbs, 2 struct
threads can continue to fit in a single 4k page, and 3 pcbs can fit in
another 4k page.

Sponsored by:	AFRL, DARPA
riscv64: Move pcb out of kstack into a new UMA zone
Some checks failed
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Style Checker / Style Checker (pull_request) Blocked by required conditions
Checklist / commit (pull_request_target) Has been cancelled
0bc86490c4
Similar to arm64, riscv's pcb embeds a copy of the floating point
registers and is too large to store directly in struct mdthread as is
done on amd64.  Instead, use a separate UMA zone for pcbs.  riscv's
floating point state is not as large as arm64's, so its pcb is also
somewhat smaller and a single 4k page can hold 6 pcbs.
jhb requested reviews from kib, andrew, br, mhorne 2026-04-24 19:16:25 +00:00
Author
Member

@br, this might impact your open review to add vector support. In particular, if we want to move to allocating fp/vector support as a separate block from the PCB as is done on amd64.

@br, this might impact your open review to add vector support. In particular, if we want to move to allocating fp/vector support as a separate block from the PCB as is done on amd64.
kib approved these changes 2026-04-24 19:58:19 +00:00
Dismissed
Member

So this thing behaves like github, it approves the whole PR instead of individual commits. How unfortunate. I only mean to click on one of the changes.

So this thing behaves like github, it approves the whole PR instead of individual commits. How unfortunate. I only mean to click on one of the changes.
@ -376,0 +376,4 @@
if (use_xsave) {
xhdr = (struct xstate_hdr *)(pcb->pcb_save + 1);
bzero(xhdr, sizeof(*xhdr));
xhdr->xstate_bv = xsave_mask;
Member

And why do we need this. copy_thread() always initializes the save area, either by resetting flags for kthread, or copying from parent for user.

And why do we need this. copy_thread() always initializes the save area, either by resetting flags for kthread, or copying from parent for user.
Author
Member

I was just trying to match cpu_thread_alloc. It looks like we don't in fact initialize the save area for new kthreads/kprocs, but maybe we should fix copy_thread to explicitly clear the state for the kthread case and remove it from cpu_thread_alloc? Today the clearing in cpu_thread_alloc does seem redundant with the bcopy in copy_thread. I would just prefer we be consistent either way. (I guess today we might leak user FPU registers to a new kproc, but kprocs are rarely created?)

I was just trying to match cpu_thread_alloc. It looks like we don't in fact initialize the save area for new kthreads/kprocs, but maybe we should fix copy_thread to explicitly clear the state for the kthread case and remove it from cpu_thread_alloc? Today the clearing in cpu_thread_alloc does seem redundant with the bcopy in copy_thread. I would just prefer we be consistent either way. (I guess today we might leak user FPU registers to a new kproc, but kprocs are rarely created?)
Member

I do not think the leak from user is real because flags are set. But I do not object against the rearrangement you propose there.

I do not think the leak from user is real because flags are set. But I do not object against the rearrangement you propose there.
Author
Member

After reading the code, I agree with you that there is no leak and clearing the PCB flags is sufficient. Instead, I've changed this commit to remove the zero'ing from cpu_thread_alloc as it is always redundant.

After reading the code, I agree with you that there is no leak and clearing the PCB flags is sufficient. Instead, I've changed this commit to remove the zero'ing from cpu_thread_alloc as it is always redundant.
kib marked this conversation as resolved
@ -396,0 +400,4 @@
cpu_thread_new_kstack(struct thread *td)
{
set_top_of_stack_td(td);
td->td_frame = (struct trapframe *)td->td_md.md_stack_base - 1;
Member

Perhaps move assignment to td_frame into set_top_of_stack_td()?

Perhaps move assignment to td_frame into set_top_of_stack_td()?
Author
Member

Hmm, at the end of this series, there are only two calls to set_top_of_stack, one here in cpu_thread_new_kstack(), and one for thread0 in hammer_time(). Possibly what I should do instead is to inline set_top_of_stack_td() here and change hammer_time() to call cpu_thread_new_kstack()?

Hmm, at the end of this series, there are only two calls to set_top_of_stack, one here in cpu_thread_new_kstack(), and one for thread0 in hammer_time(). Possibly what I should do instead is to inline set_top_of_stack_td() here and change hammer_time() to call cpu_thread_new_kstack()?
Member

This would be fine as well. I only want to ensure that setting of td_frame was centralized.

This would be fine as well. I only want to ensure that setting of td_frame was centralized.
jhb marked this conversation as resolved
@ -29,6 +29,7 @@
#include <sys/param.h>
Member

Remove sys/param.h while you are there

Remove sys/param.h while you are there
jhb marked this conversation as resolved
@ -44,3 +42,3 @@
#define GET_STACK_USAGE(total, used) do { \
struct thread *td = curthread; \
(total) = td->td_kstack_pages * PAGE_SIZE - sizeof(struct pcb); \
(total) = td->td_kstack_pages * PAGE_SIZE; \
Member

ptoa(td->td_kstack_pages)

ptoa(td->td_kstack_pages)
Author
Member

I've done a general pass to use ptoa() with td_kstack_pages, but I also added a commit to add a new helper function td_kstack_top() that returns td->td_kstack + ptoa(td->td_kstack_pages) which I think is a useful cleanup.

I've done a general pass to use ptoa() with td_kstack_pages, but I also added a commit to add a new helper function td_kstack_top() that returns `td->td_kstack + ptoa(td->td_kstack_pages)` which I think is a useful cleanup.
kib marked this conversation as resolved
@ -53,3 +51,3 @@
return (va >= (vm_offset_t)td->td_kstack && va + len >= va &&
va + len <= (vm_offset_t)td->td_kstack + td->td_kstack_pages *
PAGE_SIZE - sizeof(struct pcb));
PAGE_SIZE);
Member

ptoa

ptoa
kib marked this conversation as resolved
Author
Member

@kib wrote in #23 (comment):

So this thing behaves like github, it approves the whole PR instead of individual commits. How unfortunate. I only mean to click on one of the changes.

That's fine, I won't merge anything until you are happy with the series. I do wonder about the general idea of if the FPU/vector state should move out of the pcb for these architectures more like amd64? Each of them already have a separate UMA zone with the FPU state for use by fpu_kern_* and I wonder if it is cleaner overall to do that instead of embedding in the PCB, but I'm not really sure.

@kib wrote in https://ron-dev.freebsd.org/FreeBSD/src/pulls/23#issuecomment-256: > So this thing behaves like github, it approves the whole PR instead of individual commits. How unfortunate. I only mean to click on one of the changes. That's fine, I won't merge anything until you are happy with the series. I do wonder about the general idea of if the FPU/vector state should move out of the pcb for these architectures more like amd64? Each of them already have a separate UMA zone with the FPU state for use by fpu_kern_* and I wonder if it is cleaner overall to do that instead of embedding in the PCB, but I'm not really sure.
Member

@jhb wrote in #23 (comment):

@kib wrote in #23 (comment):

So this thing behaves like github, it approves the whole PR instead of individual commits. How unfortunate. I only mean to click on one of the changes.

That's fine, I won't merge anything until you are happy with the series. I do wonder about the general idea of if the FPU/vector state should move out of the pcb for these architectures more like amd64? Each of them already have a separate UMA zone with the FPU state for use by fpu_kern_* and I wonder if it is cleaner overall to do that instead of embedding in the PCB, but I'm not really sure.

I moved both pcb and fpu area out of kstack because it felt like waste of the precious stack space. Most important, the XSAVE area size is not bound in time, in the sense that ISA additions would require more and more space there. I believe that the pcb on stack is the fossil from the PDP-11 times when the kernel stack was context-switched.
From this PoV, I consider it only cleaner to leave kstack use as the stack only.\

BTW, for amd64, there is one more thing that could be useful to do with pcb, I might do it after fred is landed. Namely, pcb_fsbase and pcb_gsbase really have nothing to do with pcb, and should be kept in ucontext.

@jhb wrote in https://ron-dev.freebsd.org/FreeBSD/src/pulls/23#issuecomment-267: > @kib wrote in #23 (comment): > > > So this thing behaves like github, it approves the whole PR instead of individual commits. How unfortunate. I only mean to click on one of the changes. > > That's fine, I won't merge anything until you are happy with the series. I do wonder about the general idea of if the FPU/vector state should move out of the pcb for these architectures more like amd64? Each of them already have a separate UMA zone with the FPU state for use by fpu_kern_* and I wonder if it is cleaner overall to do that instead of embedding in the PCB, but I'm not really sure. I moved both pcb and fpu area out of kstack because it felt like waste of the precious stack space. Most important, the XSAVE area size is not bound in time, in the sense that ISA additions would require more and more space there. I believe that the pcb on stack is the fossil from the PDP-11 times when the kernel stack was context-switched. From this PoV, I consider it only cleaner to leave kstack use as the stack only.\ BTW, for amd64, there is one more thing that could be useful to do with pcb, I might do it after fred is landed. Namely, pcb_fsbase and pcb_gsbase really have nothing to do with pcb, and should be kept in ucontext.
jhb force-pushed jhb/pcb_kstack from 0bc86490c4
Some checks failed
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Style Checker / Style Checker (pull_request) Blocked by required conditions
Checklist / commit (pull_request_target) Has been cancelled
to 2faa78c26b
Some checks failed
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Style Checker / Style Checker (pull_request) Blocked by required conditions
Checklist / commit (pull_request_target) Has been cancelled
2026-05-02 15:25:16 +00:00
Compare
jhb force-pushed jhb/pcb_kstack from 2faa78c26b
Some checks failed
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Style Checker / Style Checker (pull_request) Blocked by required conditions
Checklist / commit (pull_request_target) Has been cancelled
to 2d5fdefe23
Some checks are pending
Checklist / commit (pull_request_target) Waiting to run
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Style Checker / Style Checker (pull_request) Blocked by required conditions
2026-05-04 19:03:48 +00:00
Compare
jhb force-pushed jhb/pcb_kstack from 2d5fdefe23
Some checks are pending
Checklist / commit (pull_request_target) Waiting to run
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Style Checker / Style Checker (pull_request) Blocked by required conditions
to f2f3e6057a
Some checks failed
Checklist / commit (pull_request_target) Has been cancelled
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Has been cancelled
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Has been cancelled
Style Checker / Style Checker (pull_request) Has been cancelled
2026-05-05 15:07:40 +00:00
Compare
jrtc27 requested review from jrtc27 2026-05-06 21:06:01 +00:00
kib approved these changes 2026-05-06 21:32:13 +00:00
@ -441,8 +442,7 @@ init_proc0(void *kstack)
#if defined(PERTHREAD_SSP)
thread0.td_md.md_canary = boot_canary;
#endif
thread0.td_pcb = (struct pcb *)(thread0.td_kstack +
Member

I guess this was omitted in the prior commit's ptoa-ification as it was being replaced anyway?

I guess this was omitted in the prior commit's ptoa-ification as it was being replaced anyway?
Author
Member

Eh, no I think I just missed it.

Eh, no I think I just missed it.
jhb marked this conversation as resolved
@ -244,2 +244,2 @@
/* Allocate space for thread0 PCB and riscv_bootparams */
addi sp, sp, -(PCB_SIZE + RISCV_BOOTPARAMS_SIZE) & ~STACKALIGNBYTES
/* Allocate space for riscv_bootparams */
addi sp, sp, -(RISCV_BOOTPARAMS_SIZE) & ~STACKALIGNBYTES
Member

Parens aren't doing much now

Parens aren't doing much now
jhb marked this conversation as resolved
@ -297,4 +298,2 @@
thread0.td_kstack = kstack;
thread0.td_kstack_pages = KSTACK_PAGES;
thread0.td_pcb = (struct pcb *)(thread0.td_kstack +
thread0.td_kstack_pages * PAGE_SIZE) - 1;
Member

Same comment as arm64 re ptoa

Same comment as arm64 re ptoa
jhb marked this conversation as resolved
jrtc27 approved these changes 2026-05-07 22:37:21 +00:00
jrtc27 left a comment

A few minor comments. I don't claim to have checked all the details for some of the earlier commits, was more focusing on the arm64+riscv MD changes at the end.

A few minor comments. I don't claim to have checked all the details for some of the earlier commits, was more focusing on the arm64+riscv MD changes at the end.
jhb force-pushed jhb/pcb_kstack from f2f3e6057a
Some checks failed
Checklist / commit (pull_request_target) Has been cancelled
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Has been cancelled
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Has been cancelled
Style Checker / Style Checker (pull_request) Has been cancelled
to 217f2319fd
Some checks failed
Checklist / commit (pull_request_target) Has been cancelled
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Has been cancelled
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Has been cancelled
Style Checker / Style Checker (pull_request) Has been cancelled
2026-05-14 19:02:10 +00:00
Compare
andrew approved these changes 2026-05-27 11:00:36 +00:00
andrew left a comment

arm64 changes look ok

arm64 changes look ok
jhb force-pushed jhb/pcb_kstack from 217f2319fd
Some checks failed
Checklist / commit (pull_request_target) Has been cancelled
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Has been cancelled
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Has been cancelled
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Has been cancelled
Style Checker / Style Checker (pull_request) Has been cancelled
to 4f876cec30
Some checks failed
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Cross-build Kernel / aarch64 macos-latest (clang-18) (pull_request) Blocked by required conditions
Style Checker / Style Checker (pull_request) Blocked by required conditions
Cross-build Kernel / amd64 ubuntu-22.04 (clang-15) (push) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-22.04 (clang-15) (push) Has been cancelled
Cross-build Kernel / amd64 ubuntu-24.04 (clang-18) (push) Has been cancelled
Cross-build Kernel / aarch64 ubuntu-24.04 (clang-18) (push) Has been cancelled
Cross-build Kernel / amd64 macos-latest (clang-18) (push) Has been cancelled
Cross-build Kernel / aarch64 macos-latest (clang-18) (push) Has been cancelled
Checklist / commit (pull_request_target) Has been cancelled
2026-05-27 14:56:44 +00:00
Compare
jhb manually merged commit 4f876cec30 into main 2026-05-27 14:59:42 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
FreeBSD/src!23
No description provided.