Download raw body.
Kill UVM_LK_ENTER/EXIT
Martin Pieuchot <mpi@grenadille.net> writes:
> Diff below removes the `lockflags' argument from uvm_map_pageable(),
> grabs the exclusive lock explicitly where necessary and adds locking
> assertions in uvm_map_pageable_wire() and uvm_map_pageable().
>
> As a result uvm_map_pageable_wire() becomes much simpler, the locking
> requirement becomes obvious and I don't have to think to understand
> where locks are released.
>
> I'd like to get this in before fixing uvm_vslock_device() which suffers
> from the same race that was present in uvm_vslock().
>
> ok?
I like this and diff looks ok to me with one question about a removal of
a DIGNOSTIC section below.
>
> Index: dev/ic/psp.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/psp.c,v
> diff -u -p -r1.16 psp.c
> --- dev/ic/psp.c 25 Apr 2025 19:10:50 -0000 1.16
> +++ dev/ic/psp.c 14 May 2025 13:19:36 -0000
> @@ -504,7 +504,9 @@ psp_launch_update_data(struct psp_softc
> end = start + size;
>
> /* Wire mapping. */
> - error = uvm_map_pageable(&p->p_vmspace->vm_map, start, end, FALSE, 0);
> + vm_map_lock(&p->p_vmspace->vm_map);
> + error = uvm_map_pageable(&p->p_vmspace->vm_map, start, end, FALSE);
> + vm_map_unlock(&p->p_vmspace->vm_map);
> if (error)
> goto out;
>
> @@ -533,7 +535,9 @@ out:
> * Unwire again. Ignore new error. Error has either been set,
> * or PSP command has already succeeded.
> */
> - (void) uvm_map_pageable(&p->p_vmspace->vm_map, start, end, TRUE, 0);
> + vm_map_lock(&p->p_vmspace->vm_map);
> + (void) uvm_map_pageable(&p->p_vmspace->vm_map, start, end, TRUE);
> + vm_map_unlock(&p->p_vmspace->vm_map);
>
> return (error);
> }
> Index: uvm/uvm_extern.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_extern.h,v
> diff -u -p -r1.180 uvm_extern.h
> --- uvm/uvm_extern.h 19 Nov 2024 06:18:26 -0000 1.180
> +++ uvm/uvm_extern.h 14 May 2025 13:19:36 -0000
> @@ -153,12 +153,6 @@ typedef int vm_prot_t;
> #define UVM_PLA_USERESERVE 0x0040 /* can allocate from kernel reserve */
>
> /*
> - * lockflags that control the locking behavior of various functions.
> - */
> -#define UVM_LK_ENTER 0x00000001 /* map locked on entry */
> -#define UVM_LK_EXIT 0x00000002 /* leave map locked on exit */
> -
> -/*
> * flags to uvm_page_physload.
> */
> #define PHYSLOAD_DEVICE 0x01 /* don't add to the page queue */
> @@ -388,7 +383,7 @@ int uvm_map(vm_map_t, vaddr_t *, vsize
> struct uvm_object *, voff_t, vsize_t, unsigned int);
> int uvm_mapanon(vm_map_t, vaddr_t *, vsize_t, vsize_t, unsigned int);
> int uvm_map_pageable(vm_map_t, vaddr_t,
> - vaddr_t, boolean_t, int);
> + vaddr_t, boolean_t);
> int uvm_map_pageable_all(vm_map_t, int, vsize_t);
> boolean_t uvm_map_checkprot(vm_map_t, vaddr_t,
> vaddr_t, vm_prot_t);
> Index: uvm/uvm_glue.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_glue.c,v
> diff -u -p -r1.88 uvm_glue.c
> --- uvm/uvm_glue.c 21 Mar 2025 13:19:33 -0000 1.88
> +++ uvm/uvm_glue.c 14 May 2025 13:19:36 -0000
> @@ -108,13 +108,17 @@ uvm_vslock(struct proc *p, caddr_t addr,
> {
> struct vm_map *map = &p->p_vmspace->vm_map;
> vaddr_t start, end;
> + int error;
>
> start = trunc_page((vaddr_t)addr);
> end = round_page((vaddr_t)addr + len);
> if (end <= start)
> return (EINVAL);
>
> - return uvm_map_pageable(map, start, end, FALSE, 0);
> + vm_map_lock(map);
> + error = uvm_map_pageable(map, start, end, FALSE);
> + vm_map_unlock(map);
> + return error;
> }
>
> /*
> @@ -132,7 +136,9 @@ uvm_vsunlock(struct proc *p, caddr_t add
> end = round_page((vaddr_t)addr + len);
> KASSERT(end > start);
>
> - uvm_map_pageable(map, start, end, TRUE, 0);
> + vm_map_lock(map);
> + uvm_map_pageable(map, start, end, TRUE);
> + vm_map_unlock(map);
> }
>
> /*
> Index: uvm/uvm_map.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> diff -u -p -r1.341 uvm_map.c
> --- uvm/uvm_map.c 21 Apr 2025 14:46:18 -0000 1.341
> +++ uvm/uvm_map.c 14 May 2025 13:19:36 -0000
> @@ -135,7 +135,7 @@ void uvm_map_pageable_pgon(struct vm_
> vaddr_t, vaddr_t);
> int uvm_map_pageable_wire(struct vm_map*,
> struct vm_map_entry*, struct vm_map_entry*,
> - vaddr_t, vaddr_t, int);
> + vaddr_t, vaddr_t);
> void uvm_map_setup_entries(struct vm_map*);
> void uvm_map_setup_md(struct vm_map*);
> void uvm_map_teardown(struct vm_map*);
> @@ -2057,14 +2057,10 @@ uvm_map_pageable_pgon(struct vm_map *map
>
> /*
> * Mark all entries from first until end (exclusive) as wired.
> - *
> - * Lockflags determines the lock state on return from this function.
> - * Lock must be exclusive on entry.
> */
> int
> uvm_map_pageable_wire(struct vm_map *map, struct vm_map_entry *first,
> - struct vm_map_entry *end, vaddr_t start_addr, vaddr_t end_addr,
> - int lockflags)
> + struct vm_map_entry *end, vaddr_t start_addr, vaddr_t end_addr)
> {
> struct vm_map_entry *iter;
> #ifdef DIAGNOSTIC
> @@ -2072,6 +2068,8 @@ uvm_map_pageable_wire(struct vm_map *map
> #endif
> int error;
>
> + vm_map_assert_wrlock(map);
> +
> /*
> * Wire pages in two passes:
> *
> @@ -2129,12 +2127,12 @@ uvm_map_pageable_wire(struct vm_map *map
> vm_map_lock(map);
> vm_map_unbusy(map);
>
> - if (error) {
> #ifdef DIAGNOSTIC
> - if (timestamp_save != map->timestamp)
> - panic("uvm_map_pageable_wire: stale map");
> + if (timestamp_save != map->timestamp)
> + panic("stale map");
> #endif
>
> + if (error) {
> /*
> * first is no longer needed to restart loops.
> * Use it as iterator to unmap successful mappings.
> @@ -2162,37 +2160,20 @@ uvm_map_pageable_wire(struct vm_map *map
>
> iter->wired_count--;
> }
> -
> - if ((lockflags & UVM_LK_EXIT) == 0)
> - vm_map_unlock(map);
> - return error;
> }
>
> -
> - if ((lockflags & UVM_LK_EXIT) == 0) {
> - vm_map_unlock(map);
> - } else {
> -#ifdef DIAGNOSTIC
> - if (timestamp_save != map->timestamp)
> - panic("uvm_map_pageable_wire: stale map");
> -#endif
> - }
> - return 0;
> + return error;
Did you mean to remove this stale map check bef10;rgb:dcdc/dfdf/e4e4ore return? Or is this
because it's now checked unconditionally above?
> }
>
> /*
> * uvm_map_pageable: set pageability of a range in a map.
> *
> - * Flags:
> - * UVM_LK_ENTER: map is already locked by caller
> - * UVM_LK_EXIT: don't unlock map on exit
> - *
> * The full range must be in use (entries may not have fspace != 0).
> * UVM_ET_HOLE counts as unmapped.
> */
> int
> uvm_map_pageable(struct vm_map *map, vaddr_t start, vaddr_t end,
> - boolean_t new_pageable, int lockflags)
> + boolean_t new_pageable)
> {
> struct vm_map_entry *first, *last, *tmp;
> int error;
> @@ -2210,8 +2191,7 @@ uvm_map_pageable(struct vm_map *map, vad
> return EINVAL; /* why? see second XXX below */
>
> KASSERT(map->flags & VM_MAP_PAGEABLE);
> - if ((lockflags & UVM_LK_ENTER) == 0)
> - vm_map_lock(map);
> + vm_map_assert_wrlock(map);
>
> /*
> * Find first entry.
> @@ -2282,8 +2262,6 @@ uvm_map_pageable(struct vm_map *map, vad
> error = 0;
>
> out:
> - if ((lockflags & UVM_LK_EXIT) == 0)
> - vm_map_unlock(map);
> return error;
> } else {
> /*
> @@ -2303,8 +2281,7 @@ out:
> } else
> tmp = last;
>
> - return uvm_map_pageable_wire(map, first, tmp, start, end,
> - lockflags);
> + return uvm_map_pageable_wire(map, first, tmp, start, end);
> }
> }
>
> @@ -2370,7 +2347,7 @@ uvm_map_pageable_all(struct vm_map *map,
> * uvm_map_pageable_wire will release lock
> */
> return uvm_map_pageable_wire(map, RBT_MIN(uvm_map_addr, &map->addr),
> - NULL, map->min_offset, map->max_offset, 0);
> + NULL, map->min_offset, map->max_offset);
> }
>
> /*
> @@ -3196,7 +3175,7 @@ uvm_map_protect(struct vm_map *map, vadd
> old_prot == PROT_NONE &&
> new_prot != PROT_NONE) {
> if (uvm_map_pageable(map, iter->start, iter->end,
> - FALSE, UVM_LK_ENTER | UVM_LK_EXIT) != 0) {
> + FALSE) != 0) {
> /*
> * If locking the entry fails, remember the
> * error if it's the first one. Note we
> Index: uvm/uvm_mmap.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
> diff -u -p -r1.198 uvm_mmap.c
> --- uvm/uvm_mmap.c 6 Apr 2025 20:20:11 -0000 1.198
> +++ uvm/uvm_mmap.c 14 May 2025 13:19:36 -0000
> @@ -821,6 +821,7 @@ sys_mlock(struct proc *p, void *v, regis
> syscallarg(const void *) addr;
> syscallarg(size_t) len;
> } */ *uap = v;
> + vm_map_t map = &p->p_vmspace->vm_map;
> vaddr_t addr;
> vsize_t size, pageoff;
> int error;
> @@ -838,7 +839,7 @@ sys_mlock(struct proc *p, void *v, regis
> return EAGAIN;
>
> #ifdef pmap_wired_count
> - if (size + ptoa(pmap_wired_count(vm_map_pmap(&p->p_vmspace->vm_map))) >
> + if (size + ptoa(pmap_wired_count(vm_map_pmap(map))) >
> lim_cur(RLIMIT_MEMLOCK))
> return EAGAIN;
> #else
> @@ -846,8 +847,9 @@ sys_mlock(struct proc *p, void *v, regis
> return error;
> #endif
>
> - error = uvm_map_pageable(&p->p_vmspace->vm_map, addr, addr+size, FALSE,
> - 0);
> + vm_map_lock(map);
> + error = uvm_map_pageable(map, addr, addr+size, FALSE);
> + vm_map_unlock(map);
> return error == 0 ? 0 : ENOMEM;
> }
>
> @@ -862,6 +864,7 @@ sys_munlock(struct proc *p, void *v, reg
> syscallarg(const void *) addr;
> syscallarg(size_t) len;
> } */ *uap = v;
> + vm_map_t map = &p->p_vmspace->vm_map;
> vaddr_t addr;
> vsize_t size, pageoff;
> int error;
> @@ -880,8 +883,9 @@ sys_munlock(struct proc *p, void *v, reg
> return error;
> #endif
>
> - error = uvm_map_pageable(&p->p_vmspace->vm_map, addr, addr+size, TRUE,
> - 0);
> + vm_map_lock(map);
> + error = uvm_map_pageable(map, addr, addr+size, TRUE);
> + vm_map_unlock(map);
> return error == 0 ? 0 : ENOMEM;
> }
>
> @@ -962,12 +966,9 @@ uvm_mmaplock(vm_map_t map, vaddr_t *addr
> KERNEL_UNLOCK();
> return error;
> }
> - /*
> - * uvm_map_pageable() always returns the map
> - * unlocked.
> - */
> - error = uvm_map_pageable(map, *addr, *addr + size,
> - FALSE, UVM_LK_ENTER);
> +
> + error = uvm_map_pageable(map, *addr, *addr + size, FALSE);
> + vm_map_unlock(map);
> if (error != 0) {
> /* unmap the region! */
> uvm_unmap(map, *addr, *addr + size);
Kill UVM_LK_ENTER/EXIT