Move pcb out of the kstack for arm64 and riscv64 #23
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "jhb/pcb_kstack"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
@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.
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;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.
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 do not think the leak from user is real because flags are set. But I do not object against the rearrangement you propose there.
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.
@ -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;Perhaps move assignment to td_frame into set_top_of_stack_td()?
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()?
This would be fine as well. I only want to ensure that setting of td_frame was centralized.
@ -29,6 +29,7 @@#include <sys/param.h>Remove sys/param.h while you are there
@ -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; \ptoa(td->td_kstack_pages)
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.@ -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);ptoa
@kib wrote in #23 (comment):
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.
@jhb wrote in #23 (comment):
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.
0bc86490c42faa78c26b2faa78c26b2d5fdefe232d5fdefe23f2f3e6057a@ -441,8 +442,7 @@ init_proc0(void *kstack)#if defined(PERTHREAD_SSP)thread0.td_md.md_canary = boot_canary;#endifthread0.td_pcb = (struct pcb *)(thread0.td_kstack +I guess this was omitted in the prior commit's ptoa-ification as it was being replaced anyway?
Eh, no I think I just missed it.
@ -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) & ~STACKALIGNBYTESParens aren't doing much now
@ -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;Same comment as arm64 re ptoa
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.
f2f3e6057a217f2319fdarm64 changes look ok
217f2319fd4f876cec30