In uvm_map_inentry_fix(), two variables in the map are now being read without a per-map read lock, this was previously protected by the kernel lock
if (addr < map->min_offset || addr >= map->max_offset) return (FALSE); When I wrote this I was told to either use KERNEL_LOCK() or vm_map_lock_read(), which is vm_map_lock_read_ln() .. if KERNEL_LOCK() is no longer good should vm_map_lock_read be moved upwards, or are you confident those offsets are safe to read independently without any locking?? Martin Pieuchot <m...@openbsd.org> wrote: > When a userland program triggers a fault or does a syscall its SP and/or > PC are checked to be sure they are in expected regions. The result of > this check is cached in the "struct proc" such that a lookup isn't > always necessary. > > Currently when the cache is outdated the KERNEL_LOCK() is taken before > doing the lookup. This shouldn't be necessary, uvm_map_lookup_entry() > is protected by the `vm_map_lock' also taken before the lookup. This > function is already called w/o KERNEL_LOCK(), like in the futex code, > so it should be safe to push it down there. > > Removing the KERNEL_LOCK() from this code path reduces the overall time > spinning when executing syscalls from 30% to 25% when building the libc > on my 16 CPU sparc64. > > Even if it doesn't improve the performances significantly I'd like to > have more code exercising the `vm_map_lock', cause it's one of the > pieces to unlock uvm_fault(). > > While here document that `p_spinentry' and `p_pcinentry' are owned by > the current process and don't need any locking. > > Tests, comments and oks welcome :o) > > Index: uvm/uvm_map.c > =================================================================== > RCS file: /cvs/src/sys/uvm/uvm_map.c,v > retrieving revision 1.247 > diff -u -p -r1.247 uvm_map.c > --- uvm/uvm_map.c 9 Sep 2019 20:02:26 -0000 1.247 > +++ uvm/uvm_map.c 31 Oct 2019 21:45:51 -0000 > @@ -158,6 +158,10 @@ int uvm_map_findspace(struct > vm_map*, > vsize_t uvm_map_addr_augment_get(struct vm_map_entry*); > void uvm_map_addr_augment(struct vm_map_entry*); > > +int uvm_map_inentry_recheck(u_long, vaddr_t, > + struct p_inentry *); > +boolean_t uvm_map_inentry_fix(struct proc *, struct p_inentry *, > + vaddr_t, int (*)(vm_map_entry_t), u_long); > /* > * Tree management functions. > */ > @@ -1868,16 +1872,16 @@ uvm_map_inentry(struct proc *p, struct p > boolean_t ok = TRUE; > > if (uvm_map_inentry_recheck(serial, addr, ie)) { > - KERNEL_LOCK(); > ok = uvm_map_inentry_fix(p, ie, addr, fn, serial); > if (!ok) { > printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid, > addr, ie->ie_start, ie->ie_end); > + KERNEL_LOCK(); > p->p_p->ps_acflag |= AMAP; > sv.sival_ptr = (void *)PROC_PC(p); > trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv); > + KERNEL_UNLOCK(); > } > - KERNEL_UNLOCK(); > } > return (ok); > } > Index: uvm/uvm_map.h > =================================================================== > RCS file: /cvs/src/sys/uvm/uvm_map.h,v > retrieving revision 1.62 > diff -u -p -r1.62 uvm_map.h > --- uvm/uvm_map.h 14 Jun 2019 05:52:43 -0000 1.62 > +++ uvm/uvm_map.h 31 Oct 2019 21:46:00 -0000 > @@ -414,12 +414,8 @@ void uvm_unmap_remove(struct vm_map*, v > > struct p_inentry; > > -int uvm_map_inentry_recheck(u_long serial, vaddr_t, > - struct p_inentry *); > int uvm_map_inentry_sp(vm_map_entry_t); > int uvm_map_inentry_pc(vm_map_entry_t); > -boolean_t uvm_map_inentry_fix(struct proc *, struct p_inentry *, > - vaddr_t addr, int (*fn)(vm_map_entry_t), u_long serial); > boolean_t uvm_map_inentry(struct proc *, struct p_inentry *, vaddr_t addr, > const char *fmt, int (*fn)(vm_map_entry_t), u_long serial); > > Index: sys/proc.h > =================================================================== > RCS file: /cvs/src/sys/sys/proc.h,v > retrieving revision 1.275 > diff -u -p -r1.275 proc.h > --- sys/proc.h 22 Oct 2019 21:19:22 -0000 1.275 > +++ sys/proc.h 31 Oct 2019 21:27:43 -0000 > @@ -318,6 +318,7 @@ struct p_inentry { > * I immutable after creation > * s scheduler lock > * l read only reference, see lim_read_enter() > + * o owned (read/modified) by the current proc > */ > struct proc { > TAILQ_ENTRY(proc) p_runq; /* [s] current run/sleep queue */ > @@ -332,8 +333,8 @@ struct proc { > /* substructures: */ > struct filedesc *p_fd; /* copy of p_p->ps_fd */ > struct vmspace *p_vmspace; /* [I] copy of p_p->ps_vmspace */ > - struct p_inentry p_spinentry; > - struct p_inentry p_pcinentry; > + struct p_inentry p_spinentry; /* [o] */ > + struct p_inentry p_pcinentry; /* [o] */ > struct plimit *p_limit; /* [l] read ref. of p_p->ps_limit */ > > int p_flag; /* P_* flags. */ >