> Date: Wed, 18 Mar 2020 11:34:59 +0100
> From: Martin Pieuchot <m...@openbsd.org>
>
> On 17/03/20(Tue) 20:08, Mark Kettenis wrote:
> > While playing with dt(4)/btrace(4) flamegraphs on a 32-core arm64
> > machine, I noticed that the kernel was spending a lot of time (6.84%)
> > in uvm_map_inentry(). This is caused by kernel lock contention.
> > Pushing baack the kernel lock further into the slow path reduces the
> > time to 0.05%.
> >
> > Now mpi@ tried this before in sys/uvm/uvm_map.c rev 1.249. That was
> > backed out because it caused issues with golang. So I had a look
> > again and identified a potential issue. When we "fix" the cached
> > entry, we use the serial number that has been passed down to us. But
> > since that serial number was recorded before we grapped the vm_map
> > lock, it may be stale. Instead we should be using the current map
> > serial number.
> >
> > This makes me wonder whether having two serial numbers is a case of
> > premature optimization. But that is a different discussion.
> >
> > I'd like to see this tested, especially by people running/building
> > golang. Does anybody remember specific details of the issue? If it
> > manifests itself, it should be pretty obvious as it results in a
> > kernel printf and a killed process.
>
> Sadly this isn't the bug we're looking for. golang still segfaults
> with this diff.
How do you reproduce the golang segfault?
> That said it is still a bug, so please commit it without moving the
> KERNEL_LOCK(), ok mpi@.
>
> > Index: uvm/uvm_map.c
> > ===================================================================
> > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > retrieving revision 1.263
> > diff -u -p -r1.263 uvm_map.c
> > --- uvm/uvm_map.c 4 Mar 2020 21:15:38 -0000 1.263
> > +++ uvm/uvm_map.c 17 Mar 2020 18:57:19 -0000
> > @@ -161,7 +161,7 @@ void uvm_map_addr_augment(struct
> > vm_m
> > 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);
> > + vaddr_t, int (*)(vm_map_entry_t));
> > /*
> > * Tree management functions.
> > */
> > @@ -1848,10 +1848,11 @@ uvm_map_inentry_recheck(u_long serial, v
> > */
> > boolean_t
> > uvm_map_inentry_fix(struct proc *p, struct p_inentry *ie, vaddr_t addr,
> > - int (*fn)(vm_map_entry_t), u_long serial)
> > + int (*fn)(vm_map_entry_t))
> > {
> > vm_map_t map = &p->p_vmspace->vm_map;
> > vm_map_entry_t entry;
> > + u_long serial;
> > int ret;
> >
> > if (addr < map->min_offset || addr >= map->max_offset)
> > @@ -1866,6 +1867,11 @@ uvm_map_inentry_fix(struct proc *p, stru
> > return (FALSE);
> > }
> >
> > + if (ie == &p->p_spinentry)
> > + serial = map->sserial;
> > + else
> > + serial = map->wserial;
> > +
> > ret = (*fn)(entry);
> > if (ret == 0) {
> > vm_map_unlock_read(map);
> > @@ -1889,16 +1895,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);
> > + ok = uvm_map_inentry_fix(p, ie, addr, fn);
> > 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);
> > }
> >
>
