Re: uvm_map_inentry diff for testing
On 18/03/20(Wed) 11:55, Mark Kettenis wrote: > > Date: Wed, 18 Mar 2020 11:34:59 +0100 > > From: Martin Pieuchot > > > > 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? $ cd /usr/ports/lang/go/ && make You'll see the build fail :)
Re: uvm_map_inentry diff for testing
> Date: Wed, 18 Mar 2020 11:34:59 +0100
> From: Martin Pieuchot
>
> 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 - 1.263
> > +++ uvm/uvm_map.c 17 Mar 2020 18:57:19 -
> > @@ -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_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_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);
> > }
> >
>
Re: uvm_map_inentry diff for testing
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.
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 - 1.263
> +++ uvm/uvm_map.c 17 Mar 2020 18:57:19 -
> @@ -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_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_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);
> }
>
uvm_map_inentry diff for testing
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.
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 - 1.263
+++ uvm/uvm_map.c 17 Mar 2020 18:57:19 -
@@ -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_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_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);
}
