Download raw body.
Introduce uvm_promote()
> Date: Tue, 24 Dec 2024 09:50:09 +0100
> From: Martin Pieuchot <mpi@grenadille.net>
>
> On 23/12/24(Mon) 10:41, Mark Kettenis wrote:
> > > Date: Wed, 18 Dec 2024 18:06:56 +0100
> > > From: Martin Pieuchot <mpi@grenadille.net>
> > >
> > > 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
> > >
> > >
> > >
>
>
>
Introduce uvm_promote()