Hello Thomas, Thanks a lot for your great but report, see below for an explanation and a possible fix.
On 04/08/21(Wed) 12:18, Thomas L. wrote: > >Synopsis: page fault trap in rw_status > >Category: kernel > >Environment: > System : OpenBSD 6.9 > Details : OpenBSD 6.9 (GENERIC) #4: Mon Jun 7 08:20:14 MDT 2021 > > [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC > > Architecture: OpenBSD.amd64 > Machine : amd64 > >Description: > One of my VMs crashed with a page fault trap > ddb> show panic > kernel page fault > uvm_fault(0xfffffd801a844120, 0x0, 0, 1) -> e > rw_status(0) at rw_status+0x11 > end trace frame: 0xffff80000f9991e0, count: 0 > ddb> trace > rw_status(0) at rw_status+0x11 > amap_wipeout(fffffd8005893740) at amap_wipeout+0x2a > uvm_fault_check(ffff80000f999380,ffff80000f9993b8,ffff80000f9993e0) at > uvm_faul > t_check+0x272 > uvm_fault(fffffd801a844120,7f7ffffc0000,0,2) at uvm_fault+0xab > upageflttrap(ffff80000f9994e0,7f7ffffc04c8) at upageflttrap+0x59 The problem comes from an error path in amap_copy(). Your workload put some memory pressure which makes amap_chunk_get() fails when trying to handle a page fault. Then amap_wipeout() is called on a newly allocated amap which hasn't a lock. > [...] > ddb> show uvm > Current UVM status: > pagesize=4096 (0x1000), pagemask=0xfff, pageshift=12 > 121478 VM pages: 60248 active, 30019 inactive, 65 wired, 5 free (0 zero) > min 10% (25) anon, 10% (25) vnode, 5% (12) vtext > freemin=4049, free-target=5398, inactive-target=30036, wired-max=40492 > faults=9051552, traps=9231593, intrs=26202481, ctxswitch=6058861 fpuswitch=0 > softint=13077905, syscalls=25514901, kmapent=11 > fault counts: > noram=1191470, noanon=0, noamap=0, pgwait=7, pgrele=0 > ok relocks(total)=1652265(1652346), anget(retries)=4025885(1468035), > amapco > py=1540576 > neighbor anon/obj pg=1512054/4165883, gets(lock/unlock)=1652565/184269 > cases: anon=3835980, anoncow=189847, obj=1363156, prcopy=288413, > przero=337 > 7927 > daemon and swap counts: > woke=1165876, revs=1187026, scans=1832267541, obscans=21522202, > anscans=181 > 0745339 > busy=0, freed=746569, reactivate=0, deactivate=1694102 > pageouts=94994196, pending=91077, nswget=282429 > nswapdev=1 > swpages=262143, swpginuse=262142, swpgonly=240501 paging=0 > kernel pointers: > objs(kern)=0xffffffff82158318 The diff below fixes this by setting the "source" amap lock to the newly allocated one. This is not strictly necessary on OpenBSD since the amap is only inserted on the global list at the end of amap copy, but this satisfies the locking requirement of amap_wipeout() and is IMHO the simplest solution. This has also the advantage of reducing the differences with NetBSD. The diff below includes other styles issues that I might commit separately, but it is easier for me to include them since I checked NetBSD's sources anyway. Note that your workload, involving an application in ruby26 puts a lot of pressure on the memory subsystem. So I wont be surprise if you find other bugs. In any case I'd be delighted to look at your bug reports, thanks a lot! Index: uvm/uvm_amap.c =================================================================== RCS file: /cvs/src/sys/uvm/uvm_amap.c,v retrieving revision 1.89 diff -u -p -r1.89 uvm_amap.c --- uvm/uvm_amap.c 26 Mar 2021 13:40:05 -0000 1.89 +++ uvm/uvm_amap.c 5 Aug 2021 10:11:45 -0000 @@ -525,7 +525,6 @@ amap_wipeout(struct vm_amap *amap) /* * Finally, destroy the amap. */ - amap->am_ref = 0; /* ... was one */ amap->am_nused = 0; amap_unlock(amap); amap_free(amap); @@ -555,13 +554,17 @@ amap_copy(struct vm_map *map, struct vm_ int i, j, k, n, srcslot; struct vm_amap_chunk *chunk = NULL, *srcchunk = NULL; struct vm_anon *anon; + vsize_t len; KASSERT(map != kernel_map); /* we use sleeping locks */ + srcamap = entry->aref.ar_amap; + len = entry->end - entry->start; + /* * Is there an amap to copy? If not, create one. */ - if (entry->aref.ar_amap == NULL) { + if (srcamap == NULL) { /* * Check to see if we have a large amap that we can * chunk. We align startva/endva to chunk-sized @@ -571,7 +574,7 @@ amap_copy(struct vm_map *map, struct vm_ * that makes it grow or shrink dynamically with * the number of slots. */ - if (atop(entry->end - entry->start) >= UVM_AMAP_LARGE) { + if (atop(len) >= UVM_AMAP_LARGE) { if (canchunk) { /* convert slots to bytes */ chunksize = UVM_AMAP_CHUNK << PAGE_SHIFT; @@ -586,10 +589,10 @@ amap_copy(struct vm_map *map, struct vm_ } entry->aref.ar_pageoff = 0; - entry->aref.ar_amap = amap_alloc(entry->end - entry->start, - waitf, lazyalloc); - if (entry->aref.ar_amap != NULL) + entry->aref.ar_amap = amap_alloc(len, waitf, lazyalloc); + if (entry->aref.ar_amap != NULL) { entry->etype &= ~UVM_ET_NEEDSCOPY; + } return; } @@ -601,7 +604,7 @@ amap_copy(struct vm_map *map, struct vm_ * to the amap (via our locked map). If the value is greater than * one, then allocate amap and re-check the value. */ - if (entry->aref.ar_amap->am_ref == 1) { + if (srcamap->am_ref == 1) { entry->etype &= ~UVM_ET_NEEDSCOPY; return; } @@ -609,14 +612,19 @@ amap_copy(struct vm_map *map, struct vm_ /* * Allocate a new amap (note: not initialised, etc). */ - AMAP_B2SLOT(slots, entry->end - entry->start); - if (!UVM_AMAP_SMALL(entry->aref.ar_amap) && - entry->aref.ar_amap->am_hashshift != 0) + AMAP_B2SLOT(slots, len); + if (!UVM_AMAP_SMALL(srcamap) && srcamap->am_hashshift != 0) lazyalloc = 1; amap = amap_alloc1(slots, waitf, lazyalloc); if (amap == NULL) return; - srcamap = entry->aref.ar_amap; + + /* + * Make the new amap share the source amap's lock, and then lock + * both. + */ + amap->am_lock = srcamap->am_lock; + rw_obj_hold(amap->am_lock); amap_lock(srcamap); @@ -655,7 +663,7 @@ amap_copy(struct vm_map *map, struct vm_ chunk = amap_chunk_get(amap, lcv, 1, PR_NOWAIT); if (chunk == NULL) { - amap_unlock(srcamap); + /* amap_wipeout() releases the lock. */ amap->am_ref = 0; amap_wipeout(amap); return; @@ -682,12 +690,13 @@ amap_copy(struct vm_map *map, struct vm_ srcamap->am_ref--; KASSERT(srcamap->am_ref > 0); - if (srcamap->am_ref == 1 && (srcamap->am_flags & AMAP_SHARED) != 0) - srcamap->am_flags &= ~AMAP_SHARED; /* clear shared flag */ + if (srcamap->am_ref == 1 && (srcamap->am_flags & AMAP_SHARED) != 0) { + srcamap->am_flags &= ~AMAP_SHARED; + } #ifdef UVM_AMAP_PPREF if (srcamap->am_ppref && srcamap->am_ppref != PPREF_NONE) { amap_pp_adjref(srcamap, entry->aref.ar_pageoff, - (entry->end - entry->start) >> PAGE_SHIFT, -1); + len >> PAGE_SHIFT, -1); } #endif @@ -695,10 +704,10 @@ amap_copy(struct vm_map *map, struct vm_ * If we referenced any anons, then share the source amap's lock. * Otherwise, we have nothing in common, so allocate a new one. */ - KASSERT(amap->am_lock == NULL); - if (amap->am_nused != 0) { - amap->am_lock = srcamap->am_lock; - rw_obj_hold(amap->am_lock); + KASSERT(amap->am_lock == srcamap->am_lock); + if (amap->am_nused == 0) { + rw_obj_free(amap->am_lock); + amap->am_lock = NULL; } amap_unlock(srcamap);
