From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: uvm_fault_upper_lookup: delay pmap_extract()
To: Martin Pieuchot <mpi@grenadille.net>
Cc: tech@openbsd.org, tb@openbsd.org
Date: Thu, 28 Nov 2024 11:33:15 +0100

> Date: Thu, 28 Nov 2024 10:01:15 +0100
> From: Martin Pieuchot <mpi@grenadille.net>
> 
> uvm_fault_upper_lookup() enters existing neighbor pages found in the
> amap.
> 
> Diff below reorder the checks in the loop to avoid calling the expensive
> pmap_extract(9) when there is no corresponding anon or if the page is
> PG_RELEASED|PG_BUSY and won't be entered anyway.

Out of curiosity, do you know what part of pmap_extract() is
expensive?  Is it the locking?  Or is it the PTE (descriptor) lookup?

> While here call pmap_update() only if, at least, a page has been entered.
> 
> ok?
> 
> Index: uvm/uvm_fault.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> diff -u -p -r1.146 uvm_fault.c
> --- uvm/uvm_fault.c	27 Nov 2024 10:58:07 -0000	1.146
> +++ uvm/uvm_fault.c	28 Nov 2024 08:35:10 -0000
> @@ -841,10 +841,11 @@ uvm_fault_upper_lookup(struct uvm_faulti
>  {
>  	struct vm_amap *amap = ufi->entry->aref.ar_amap;
>  	struct vm_anon *anon;
> +	struct vm_page *pg;
>  	boolean_t shadowed;
>  	vaddr_t currva;
>  	paddr_t pa;
> -	int lcv;
> +	int lcv, entered = 0;
>  
>  	/* locked: maps(read), amap(if there) */
>  	KASSERT(amap == NULL ||
> @@ -859,16 +860,6 @@ uvm_fault_upper_lookup(struct uvm_faulti
>  	shadowed = FALSE;
>  	for (lcv = 0; lcv < flt->npages; lcv++, currva += PAGE_SIZE) {
>  		/*
> -		 * dont play with VAs that are already mapped
> -		 * except for center)
> -		 */
> -		if (lcv != flt->centeridx &&
> -		    pmap_extract(ufi->orig_map->pmap, currva, &pa)) {
> -			pages[lcv] = PGO_DONTCARE;
> -			continue;
> -		}
> -
> -		/*
>  		 * unmapped or center page.   check if any anon at this level.
>  		 */
>  		if (amap == NULL || anons[lcv] == NULL) {
> @@ -884,12 +875,20 @@ uvm_fault_upper_lookup(struct uvm_faulti
>  			shadowed = TRUE;
>  			continue;
>  		}
> +
>  		anon = anons[lcv];
> +		pg = anon->an_page;
> +
>  		KASSERT(anon->an_lock == amap->am_lock);
> -		if (anon->an_page &&
> -		    (anon->an_page->pg_flags & (PG_RELEASED|PG_BUSY)) == 0) {
> +
> +		/*
> +		 * ignore busy pages.
> +		 * don't play with VAs that are already mapped.
> +		 */
> +		if (pg && (pg->pg_flags & (PG_RELEASED|PG_BUSY)) == 0 &&
> +		    !pmap_extract(ufi->orig_map->pmap, currva, &pa)) {
>  			uvm_lock_pageq();
> -			uvm_pageactivate(anon->an_page);	/* reactivate */
> +			uvm_pageactivate(pg);	/* reactivate */
>  			uvm_unlock_pageq();
>  			counters_inc(uvmexp_counters, flt_namap);
>  
> @@ -902,13 +901,14 @@ uvm_fault_upper_lookup(struct uvm_faulti
>  			 * that we enter these right now.
>  			 */
>  			(void) pmap_enter(ufi->orig_map->pmap, currva,
> -			    VM_PAGE_TO_PHYS(anon->an_page) | flt->pa_flags,
> +			    VM_PAGE_TO_PHYS(pg) | flt->pa_flags,
>  			    (anon->an_ref > 1) ?
>  			    (flt->enter_prot & ~PROT_WRITE) : flt->enter_prot,
>  			    PMAP_CANFAIL);
> +			entered++;
>  		}
>  	}
> -	if (flt->npages > 1)
> +	if (entered > 0)
>  		pmap_update(ufi->orig_map->pmap);
>  
>  	return shadowed;
> 
> 
>