On 24/09/20(Thu) 16:35, Mark Kettenis wrote:
> This avoids taking the kernel lock when ci_inatomic is set. This
> might speed up inteldrm(4) a bit. Since uvm_grow() still needs the
> kernel lock, some reorganization of the code is necessary.
>
> I'm not sure this actaully has an impact. If we end up here with
> ci_inatomic set we're going to return EFAULT and take a slow path
> anyway. So maybe it is better to leave this until we make uvm_grow()
> mpsafe?
I think it's valuable in itself. Some questions below:
> Index: arch/amd64/amd64/trap.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 trap.c
> --- arch/amd64/amd64/trap.c 14 Sep 2020 12:51:28 -0000 1.81
> +++ arch/amd64/amd64/trap.c 24 Sep 2020 14:21:24 -0000
> @@ -173,11 +173,10 @@ pageflttrap(struct trapframe *frame, uin
> pcb = &p->p_addr->u_pcb;
> va = trunc_page((vaddr_t)cr2);
>
> - KERNEL_LOCK();
> -
> if (!usermode) {
> /* This will only trigger if SMEP is enabled */
> if (cr2 <= VM_MAXUSER_ADDRESS && frame->tf_err & PGEX_I) {
> + KERNEL_LOCK();
This is necessary to prevent another thread from corrupting the global
array `faultbuf' if another fault occur, right?
That opens a question about how to protect `faultstr'. Maybe we could
even grab the KERNEL_LOCK() inside fault().
> fault("attempt to execute user address %p "
> "in supervisor mode", (void *)cr2);
> /* retain kernel lock */
> @@ -186,6 +185,7 @@ pageflttrap(struct trapframe *frame, uin
> /* This will only trigger if SMAP is enabled */
> if (pcb->pcb_onfault == NULL && cr2 <= VM_MAXUSER_ADDRESS &&
> frame->tf_err & PGEX_P) {
> + KERNEL_LOCK();
> fault("attempt to access user address %p "
> "in supervisor mode", (void *)cr2);
> /* retain kernel lock */
> @@ -216,28 +216,29 @@ pageflttrap(struct trapframe *frame, uin
> caddr_t onfault = pcb->pcb_onfault;
>
> pcb->pcb_onfault = NULL;
`pcb_onfault' is no longer modified under KERNEL_LOCK(). Is it safe
because this member is "owned" by the current thread? Maybe we should
start documenting this, the "struct proc" has some examples.
> + KERNEL_LOCK();
> error = uvm_fault(map, va, frame->tf_err & PGEX_P ?
> VM_FAULT_PROTECT : VM_FAULT_INVALID, ftype);
> + if (error == 0 && map != kernel_map)
> + uvm_grow(p, va);
Is there anything preventing us from having such idiom inside
uvm_fault()? Not now, but maybe in the future.
> + KERNEL_UNLOCK();
> pcb->pcb_onfault = onfault;
> } else
> error = EFAULT;
>
> - if (error == 0) {
> - if (map != kernel_map)
> - uvm_grow(p, va);
> - } else if (!usermode) {
> + if (error && !usermode) {
> if (pcb->pcb_onfault != 0) {
> - KERNEL_UNLOCK();
> frame->tf_rip = (u_int64_t)pcb->pcb_onfault;
> return 1;
> } else {
> /* bad memory access in the kernel */
> + KERNEL_LOCK();
> fault("uvm_fault(%p, 0x%llx, 0, %d) -> %x",
> map, cr2, ftype, error);
> /* retain kernel lock */
> return 0;
> }
> - } else {
> + } else if (error) {
> union sigval sv;
> int signal, sicode;
>
> @@ -260,8 +261,6 @@ pageflttrap(struct trapframe *frame, uin
> sv.sival_ptr = (void *)cr2;
> trapsignal(p, signal, T_PAGEFLT, sicode, sv);
> }
> -
> - KERNEL_UNLOCK();
>
> return 1;
> }
>
