From: Martin Pieuchot Subject: Re: Introduce uvm_promote() To: Mark Kettenis Cc: tech@openbsd.org Date: Tue, 24 Dec 2024 09:50:09 +0100 On 23/12/24(Mon) 10:41, Mark Kettenis wrote: > > Date: Wed, 18 Dec 2024 18:06:56 +0100 > > From: Martin Pieuchot > > > > Diff below merges two code paths that allocate a new anon and copy the > > content of a page. > > > > ok? > > I think you should move the flt_prcopy counter increment into the new function as well. I'm not opposed. I believe fault counters could received some love. However the upper part of the handler use `flt_acow' instead of `flt_prcopy'. There's also amap_cow_now() which could re-use uvmfault_promote() if we move the counter out of it. Related to this there is also uvm_anon_pagein() which calls uvmfault_anonget() and increment fault counters outside the fault handler. I'm sharing all these concerns because I believe we can dramatically simplify this code and improve it. That said I'm for the moment focusing on bringing in all changes from NetBSD then we'll see what we do with all the knowledge we've built. > > Index: uvm/uvm_fault.c > > =================================================================== > > RCS file: /cvs/src/sys/uvm/uvm_fault.c,v > > diff -u -p -r1.154 uvm_fault.c > > --- uvm/uvm_fault.c 18 Dec 2024 16:41:27 -0000 1.154 > > +++ uvm/uvm_fault.c 18 Dec 2024 17:00:56 -0000 > > @@ -485,6 +485,68 @@ uvmfault_anonget(struct uvm_faultinfo *u > > } > > > > /* > > + * uvmfault_promote: promote data to a new anon. used for 1B and 2B. > > + * > > + * 1. allocate an anon and a page. > > + * 2. fill its contents. > > + * > > + * => if we fail (result != 0) we unlock everything. > > + * => on success, return a new locked anon via 'nanon'. > > + * => it's caller's responsibility to put the promoted nanon->an_page to the > > + * page queue. > > + */ > > +int > > +uvmfault_promote(struct uvm_faultinfo *ufi, > > + struct vm_page *uobjpage, > > + struct vm_anon **nanon, /* OUT: allocated anon */ > > + struct vm_page **npg) > > +{ > > + struct vm_amap *amap = ufi->entry->aref.ar_amap; > > + struct vm_anon *anon; > > + struct vm_page *pg = NULL; > > + > > + anon = uvm_analloc(); > > + if (anon) { > > + anon->an_lock = amap->am_lock; > > + pg = uvm_pagealloc(NULL, 0, anon, > > + (uobjpage == PGO_DONTCARE) ? UVM_PGA_ZERO : 0); > > + } > > + > > + /* check for out of RAM */ > > + if (anon == NULL || pg == NULL) { > > + uvmfault_unlockall(ufi, amap, NULL); > > + if (anon == NULL) > > + counters_inc(uvmexp_counters, flt_noanon); > > + else { > > + anon->an_lock = NULL; > > + anon->an_ref--; > > + uvm_anfree(anon); > > + counters_inc(uvmexp_counters, flt_noram); > > + } > > + > > + if (uvm_swapisfull()) > > + return ENOMEM; > > + > > + /* out of RAM, wait for more */ > > + if (anon == NULL) > > + uvm_anwait(); > > + else > > + uvm_wait("flt_noram3"); > > + return ERESTART; > > + } > > + > > + /* > > + * copy the page [pg now dirty] > > + */ > > + if (uobjpage != PGO_DONTCARE) > > + uvm_pagecopy(uobjpage, pg); > > + > > + *nanon = anon; > > + *npg = pg; > > + return 0; > > +} > > + > > +/* > > * Update statistics after fault resolution. > > * - maxrss > > */ > > @@ -972,37 +1034,11 @@ uvm_fault_upper(struct uvm_faultinfo *uf > > > > counters_inc(uvmexp_counters, flt_acow); > > oanon = anon; /* oanon = old */ > > - anon = uvm_analloc(); > > - if (anon) { > > - anon->an_lock = amap->am_lock; > > - pg = uvm_pagealloc(NULL, 0, anon, 0); > > - } > > - > > - /* check for out of RAM */ > > - if (anon == NULL || pg == NULL) { > > - uvmfault_unlockall(ufi, amap, NULL); > > - if (anon == NULL) > > - counters_inc(uvmexp_counters, flt_noanon); > > - else { > > - anon->an_lock = NULL; > > - anon->an_ref--; > > - uvm_anfree(anon); > > - counters_inc(uvmexp_counters, flt_noram); > > - } > > > > - if (uvm_swapisfull()) > > - return ENOMEM; > > - > > - /* out of RAM, wait for more */ > > - if (anon == NULL) > > - uvm_anwait(); > > - else > > - uvm_wait("flt_noram3"); > > - return ERESTART; > > - } > > + error = uvmfault_promote(ufi, oanon->an_page, &anon, &pg); > > + if (error) > > + return error; > > > > - /* got all resources, replace anon with nanon */ > > - uvm_pagecopy(oanon->an_page, pg); /* pg now !PG_CLEAN */ > > /* un-busy! new page */ > > atomic_clearbits_int(&pg->pg_flags, PG_BUSY|PG_FAKE); > > UVM_PAGE_OWN(pg, NULL); > > @@ -1314,53 +1350,15 @@ uvm_fault_lower(struct uvm_faultinfo *uf > > if (amap == NULL) > > panic("uvm_fault: want to promote data, but no anon"); > > #endif > > - > > - anon = uvm_analloc(); > > - if (anon) { > > - /* > > - * In `Fill in data...' below, if > > - * uobjpage == PGO_DONTCARE, we want > > - * a zero'd, dirty page, so have > > - * uvm_pagealloc() do that for us. > > - */ > > - anon->an_lock = amap->am_lock; > > - pg = uvm_pagealloc(NULL, 0, anon, > > - (uobjpage == PGO_DONTCARE) ? UVM_PGA_ZERO : 0); > > - } > > - > > - /* > > - * out of memory resources? > > - */ > > - if (anon == NULL || pg == NULL) { > > - /* unlock and fail ... */ > > - uvmfault_unlockall(ufi, amap, uobj); > > - if (anon == NULL) > > - counters_inc(uvmexp_counters, flt_noanon); > > - else { > > - anon->an_lock = NULL; > > - anon->an_ref--; > > - uvm_anfree(anon); > > - counters_inc(uvmexp_counters, flt_noram); > > - } > > - > > - if (uvm_swapisfull()) > > - return (ENOMEM); > > - > > - /* out of RAM, wait for more */ > > - if (anon == NULL) > > - uvm_anwait(); > > - else > > - uvm_wait("flt_noram5"); > > - return ERESTART; > > - } > > + error = uvmfault_promote(ufi, uobjpage, &anon, &pg); > > + if (error) > > + return error; > > > > /* > > * fill in the data > > */ > > if (uobjpage != PGO_DONTCARE) { > > counters_inc(uvmexp_counters, flt_prcopy); > > - /* copy page [pg now dirty] */ > > - uvm_pagecopy(uobjpage, pg); > > > > /* > > * promote to shared amap? make sure all sharing > > > > > >