On Tue, Nov 17, 2020 at 09:25:10AM -0300, Martin Pieuchot wrote:
> Here's another refactoring that moves the remaining logic of uvm_fault()
> handling lower faults, case 2, to its own function. This logic shouldn't
> be modified in the first step of unlocking amap & anon and will still be
> executed under KERNEL_LOCK(). Having a separate function will however
> help to turn the 'ReFault' goto into a more readable loop. This will be
> the next step.
>
> ok?
ok jmatthew@
>
> Index: uvm/uvm_fault.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> retrieving revision 1.107
> diff -u -p -r1.107 uvm_fault.c
> --- uvm/uvm_fault.c 16 Nov 2020 12:30:16 -0000 1.107
> +++ uvm/uvm_fault.c 16 Nov 2020 13:27:32 -0000
> @@ -484,6 +484,9 @@ struct uvm_faultctx {
> paddr_t pa_flags;
> };
>
> +int uvm_fault_lower(struct uvm_faultinfo *, struct uvm_faultctx *,
> + struct vm_page **, vm_fault_t, vm_prot_t);
> +
> /*
> * uvm_fault_check: check prot, handle needs-copy, etc.
> *
> @@ -901,19 +904,11 @@ uvm_fault(vm_map_t orig_map, vaddr_t vad
> {
> struct uvm_faultinfo ufi;
> struct uvm_faultctx flt;
> - boolean_t promote, locked, shadowed;
> - int result, lcv, gotpages;
> - vaddr_t currva;
> - voff_t uoff;
> - struct vm_amap *amap;
> - struct uvm_object *uobj;
> - struct vm_anon *anons_store[UVM_MAXRANGE], **anons, *anon;
> - struct vm_page *pages[UVM_MAXRANGE], *pg, *uobjpage;
> + boolean_t shadowed;
> + struct vm_anon *anons_store[UVM_MAXRANGE], **anons;
> + struct vm_page *pages[UVM_MAXRANGE];
> int error;
>
> - anon = NULL;
> - pg = NULL;
> -
> uvmexp.faults++; /* XXX: locking? */
> TRACEPOINT(uvm, fault, vaddr, fault_type, access_type, NULL);
>
> @@ -957,8 +952,28 @@ ReFault:
> }
> }
>
> - amap = ufi.entry->aref.ar_amap;
> - uobj = ufi.entry->object.uvm_obj;
> + /* handle case 2: faulting on backing object or zero fill */
> + error = uvm_fault_lower(&ufi, &flt, pages, fault_type, access_type);
> + switch (error) {
> + case ERESTART:
> + goto ReFault;
> + default:
> + return error;
> + }
> +}
> +
> +int
> +uvm_fault_lower(struct uvm_faultinfo *ufi, struct uvm_faultctx *flt,
> + struct vm_page **pages, vm_fault_t fault_type, vm_prot_t access_type)
> +{
> + struct vm_amap *amap = ufi->entry->aref.ar_amap;
> + struct uvm_object *uobj = ufi->entry->object.uvm_obj;
> + boolean_t promote, locked;
> + int result, lcv, gotpages;
> + struct vm_page *uobjpage, *pg = NULL;
> + struct vm_anon *anon = NULL;
> + vaddr_t currva;
> + voff_t uoff;
>
> /*
> * if the desired page is not shadowed by the amap and we have a
> @@ -967,15 +982,15 @@ ReFault:
> * with the usual pgo_get hook). the backing object signals this by
> * providing a pgo_fault routine.
> */
> - if (uobj && shadowed == FALSE && uobj->pgops->pgo_fault != NULL) {
> - result = uobj->pgops->pgo_fault(&ufi, flt.startva, pages,
> - flt.npages, flt.centeridx, fault_type, access_type,
> + if (uobj != NULL && uobj->pgops->pgo_fault != NULL) {
> + result = uobj->pgops->pgo_fault(ufi, flt->startva, pages,
> + flt->npages, flt->centeridx, fault_type, access_type,
> PGO_LOCKED);
>
> if (result == VM_PAGER_OK)
> return (0); /* pgo_fault did pmap enter */
> else if (result == VM_PAGER_REFAULT)
> - goto ReFault; /* try again! */
> + return ERESTART; /* try again! */
> else
> return (EACCES);
> }
> @@ -989,20 +1004,20 @@ ReFault:
> *
> * ("get" has the option of doing a pmap_enter for us)
> */
> - if (uobj && shadowed == FALSE) {
> + if (uobj != NULL) {
> uvmexp.fltlget++;
> - gotpages = flt.npages;
> - (void) uobj->pgops->pgo_get(uobj, ufi.entry->offset +
> - (flt.startva - ufi.entry->start),
> - pages, &gotpages, flt.centeridx,
> - access_type & MASK(ufi.entry),
> - ufi.entry->advice, PGO_LOCKED);
> + gotpages = flt->npages;
> + (void) uobj->pgops->pgo_get(uobj, ufi->entry->offset +
> + (flt->startva - ufi->entry->start),
> + pages, &gotpages, flt->centeridx,
> + access_type & MASK(ufi->entry),
> + ufi->entry->advice, PGO_LOCKED);
>
> /* check for pages to map, if we got any */
> uobjpage = NULL;
> if (gotpages) {
> - currva = flt.startva;
> - for (lcv = 0 ; lcv < flt.npages ;
> + currva = flt->startva;
> + for (lcv = 0 ; lcv < flt->npages ;
> lcv++, currva += PAGE_SIZE) {
> if (pages[lcv] == NULL ||
> pages[lcv] == PGO_DONTCARE)
> @@ -1017,7 +1032,7 @@ ReFault:
> * remember this page as "uobjpage."
> * (for later use).
> */
> - if (lcv == flt.centeridx) {
> + if (lcv == flt->centeridx) {
> uobjpage = pages[lcv];
> continue;
> }
> @@ -1042,11 +1057,11 @@ ReFault:
> * failures; it's not critical that we
> * enter these right now.
> */
> - (void) pmap_enter(ufi.orig_map->pmap, currva,
> - VM_PAGE_TO_PHYS(pages[lcv]) | flt.pa_flags,
> - flt.enter_prot & MASK(ufi.entry),
> + (void) pmap_enter(ufi->orig_map->pmap, currva,
> + VM_PAGE_TO_PHYS(pages[lcv]) | flt->pa_flags,
> + flt->enter_prot & MASK(ufi->entry),
> PMAP_CANFAIL |
> - (flt.wired ? PMAP_WIRED : 0));
> + (flt->wired ? PMAP_WIRED : 0));
>
> /*
> * NOTE: page can't be PG_WANTED because
> @@ -1057,7 +1072,7 @@ ReFault:
> PG_BUSY);
> UVM_PAGE_OWN(pages[lcv], NULL);
> } /* for "lcv" loop */
> - pmap_update(ufi.orig_map->pmap);
> + pmap_update(ufi->orig_map->pmap);
> } /* "gotpages" != 0 */
> /* note: object still _locked_ */
> } else {
> @@ -1073,7 +1088,6 @@ ReFault:
> * made it BUSY.
> */
>
> - /* handle case 2: faulting on backing object or zero fill */
> /*
> * note that uobjpage can not be PGO_DONTCARE at this point. we now
> * set uobjpage to PGO_DONTCARE if we are doing a zero fill. if we
> @@ -1086,7 +1100,7 @@ ReFault:
> } else {
> KASSERT(uobjpage != PGO_DONTCARE);
> promote = (access_type & PROT_WRITE) &&
> - UVM_ET_ISCOPYONWRITE(ufi.entry);
> + UVM_ET_ISCOPYONWRITE(ufi->entry);
> }
>
> /*
> @@ -1104,13 +1118,13 @@ ReFault:
> /* update rusage counters */
> curproc->p_ru.ru_majflt++;
>
> - uvmfault_unlockall(&ufi, amap, NULL);
> + uvmfault_unlockall(ufi, amap, NULL);
>
> uvmexp.fltget++;
> gotpages = 1;
> - uoff = (ufi.orig_rvaddr - ufi.entry->start) + ufi.entry->offset;
> + uoff = (ufi->orig_rvaddr - ufi->entry->start) +
> ufi->entry->offset;
> result = uobj->pgops->pgo_get(uobj, uoff, &uobjpage, &gotpages,
> - 0, access_type & MASK(ufi.entry), ufi.entry->advice,
> + 0, access_type & MASK(ufi->entry), ufi->entry->advice,
> PGO_SYNCIO);
>
> /* recover from I/O */
> @@ -1119,10 +1133,10 @@ ReFault:
>
> if (result == VM_PAGER_AGAIN) {
> tsleep_nsec(&lbolt, PVM, "fltagain2", INFSLP);
> - goto ReFault;
> + return ERESTART;
> }
>
> - if (!UVM_ET_ISNOFAULT(ufi.entry))
> + if (!UVM_ET_ISNOFAULT(ufi->entry))
> return (EIO);
>
> uobjpage = PGO_DONTCARE;
> @@ -1130,16 +1144,16 @@ ReFault:
> }
>
> /* re-verify the state of the world. */
> - locked = uvmfault_relock(&ufi);
> + locked = uvmfault_relock(ufi);
>
> /*
> * Re-verify that amap slot is still free. if there is
> * a problem, we clean up.
> */
> - if (locked && amap && amap_lookup(&ufi.entry->aref,
> - ufi.orig_rvaddr - ufi.entry->start)) {
> + if (locked && amap && amap_lookup(&ufi->entry->aref,
> + ufi->orig_rvaddr - ufi->entry->start)) {
> if (locked)
> - uvmfault_unlockall(&ufi, amap, NULL);
> + uvmfault_unlockall(ufi, amap, NULL);
> locked = FALSE;
> }
>
> @@ -1156,10 +1170,10 @@ ReFault:
> atomic_clearbits_int(&uobjpage->pg_flags,
> PG_BUSY|PG_WANTED);
> UVM_PAGE_OWN(uobjpage, NULL);
> - goto ReFault;
> + return ERESTART;
> }
> if (locked == FALSE)
> - goto ReFault;
> + return ERESTART;
>
> /*
> * we have the data in uobjpage which is PG_BUSY
> @@ -1181,8 +1195,8 @@ ReFault:
> * set "pg" to the page we want to map in (uobjpage, usually)
> */
> uvmexp.flt_obj++;
> - if (UVM_ET_ISCOPYONWRITE(ufi.entry))
> - flt.enter_prot &= ~PROT_WRITE;
> + if (UVM_ET_ISCOPYONWRITE(ufi->entry))
> + flt->enter_prot &= ~PROT_WRITE;
> pg = uobjpage; /* map in the actual object */
>
> /* assert(uobjpage != PGO_DONTCARE) */
> @@ -1230,7 +1244,7 @@ ReFault:
> }
>
> /* unlock and fail ... */
> - uvmfault_unlockall(&ufi, amap, uobj);
> + uvmfault_unlockall(ufi, amap, uobj);
> if (anon == NULL)
> uvmexp.fltnoanon++;
> else {
> @@ -1246,7 +1260,7 @@ ReFault:
> uvm_anwait();
> else
> uvm_wait("flt_noram5");
> - goto ReFault;
> + return ERESTART;
> }
>
> /* fill in the data */
> @@ -1281,18 +1295,18 @@ ReFault:
> */
> }
>
> - if (amap_add(&ufi.entry->aref,
> - ufi.orig_rvaddr - ufi.entry->start, anon, 0)) {
> - uvmfault_unlockall(&ufi, amap, NULL);
> + if (amap_add(&ufi->entry->aref,
> + ufi->orig_rvaddr - ufi->entry->start, anon, 0)) {
> + uvmfault_unlockall(ufi, amap, NULL);
> uvm_anfree(anon);
> uvmexp.fltnoamap++;
>
> if (uvm_swapisfull())
> return (ENOMEM);
>
> - amap_populate(&ufi.entry->aref,
> - ufi.orig_rvaddr - ufi.entry->start);
> - goto ReFault;
> + amap_populate(&ufi->entry->aref,
> + ufi->orig_rvaddr - ufi->entry->start);
> + return ERESTART;
> }
> }
>
> @@ -1301,9 +1315,9 @@ ReFault:
> * all resources are present. we can now map it in and free our
> * resources.
> */
> - if (pmap_enter(ufi.orig_map->pmap, ufi.orig_rvaddr,
> - VM_PAGE_TO_PHYS(pg) | flt.pa_flags, flt.enter_prot,
> - access_type | PMAP_CANFAIL | (flt.wired ? PMAP_WIRED : 0)) != 0) {
> + if (pmap_enter(ufi->orig_map->pmap, ufi->orig_rvaddr,
> + VM_PAGE_TO_PHYS(pg) | flt->pa_flags, flt->enter_prot,
> + access_type | PMAP_CANFAIL | (flt->wired ? PMAP_WIRED : 0)) != 0) {
> /*
> * No need to undo what we did; we can simply think of
> * this as the pmap throwing away the mapping information.
> @@ -1316,14 +1330,14 @@ ReFault:
>
> atomic_clearbits_int(&pg->pg_flags, PG_BUSY|PG_FAKE|PG_WANTED);
> UVM_PAGE_OWN(pg, NULL);
> - uvmfault_unlockall(&ufi, amap, uobj);
> + uvmfault_unlockall(ufi, amap, uobj);
> if (uvm_swapisfull()) {
> /* XXX instrumentation */
> return (ENOMEM);
> }
> /* XXX instrumentation */
> uvm_wait("flt_pmfail2");
> - goto ReFault;
> + return ERESTART;
> }
>
> uvm_lock_pageq();
> @@ -1351,8 +1365,8 @@ ReFault:
>
> atomic_clearbits_int(&pg->pg_flags, PG_BUSY|PG_FAKE|PG_WANTED);
> UVM_PAGE_OWN(pg, NULL);
> - uvmfault_unlockall(&ufi, amap, uobj);
> - pmap_update(ufi.orig_map->pmap);
> + uvmfault_unlockall(ufi, amap, uobj);
> + pmap_update(ufi->orig_map->pmap);
>
> return (0);
> }
>
