From: Mark Kettenis Subject: Re: Introduce uvm_promote() To: Martin Pieuchot Cc: tech@openbsd.org Date: Wed, 25 Dec 2024 13:06:24 +0100 > Date: Tue, 24 Dec 2024 09:50:09 +0100 > From: Martin Pieuchot > > 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'. Right. The increase of flt_prcopy would only happen if we do the actual copy, which I think doesn't happen in the flt_acow case. But I see your point... > 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. Keeping the counter handling somewhat consistently at a higher level is probably better. > 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. Good point. I was looking at an old NetBSD 1.5 tree. But now that I look at the right one I agree. Leave things as-is. > > > 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 > > > > > > > > > > > >