From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: uvm_fault_lower_lookup: add pmap_extract() check
To: Martin Pieuchot <mpi@grenadille.net>
Cc: tech@openbsd.org, tb@openbsd.org
Date: Thu, 28 Nov 2024 21:17:20 +0100

> Date: Thu, 28 Nov 2024 10:19:46 +0100
> From: Martin Pieuchot <mpi@grenadille.net>
> Cc: tb@openbsd.org
> Content-Type: text/plain; charset=utf-8
> Content-Disposition: inline
> 
> Similarly to uvm_fault_upper_lookup(), uvm_fault_lower_lookup() enters
> existing neighbor pages for lower layers faults (with an UVM object).
> This is where objects shared among many processes (binaries, libraries,
> etc) are contended.
> 
> To reduce the cost of entering neighbor pages ahead, use the same trick
> already present in uvm_fault_upper_lookup().  Call pmap_extract(9) to
> know if a page is already present in the memory space before entering it.
> 
> The 'goto next' is questionable.  I did it this way because it will
> disappear in the next step.  My current goal is to be able to call this
> function in parallel for a given UVM object.  That implies not setting the
> PG_BUSY flag on pages returned by PGO_LOCKED.  So the goto will soon be
> a continue.
> 
> While here add a cheap `wire_count' check before grabbing the heavily
> contended pageq mutex. 
> 
> Finally use the same `entered' technique to decide if pmap_update()
> should be called or not.
> 
> ok?

ok kettenis@

> 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 09:02:36 -0000
> @@ -1115,8 +1115,9 @@ uvm_fault_lower_lookup(
>  {
>  	struct uvm_object *uobj = ufi->entry->object.uvm_obj;
>  	struct vm_page *uobjpage = NULL;
> -	int lcv, gotpages;
> +	int lcv, gotpages, entered;
>  	vaddr_t currva;
> +	paddr_t pa;
>  
>  	rw_enter(uobj->vmobjlock, RW_WRITE);
>  
> @@ -1135,6 +1136,7 @@ uvm_fault_lower_lookup(
>  		return NULL;
>  	}
>  
> +	entered = 0;
>  	currva = flt->startva;
>  	for (lcv = 0; lcv < flt->npages; lcv++, currva += PAGE_SIZE) {
>  		if (pages[lcv] == NULL ||
> @@ -1155,18 +1157,14 @@ uvm_fault_lower_lookup(
>  			continue;
>  		}
>  
> -		/*
> -		 * note: calling pgo_get with locked data
> -		 * structures returns us pages which are
> -		 * neither busy nor released, so we don't
> -		 * need to check for this.   we can just
> -		 * directly enter the page (after moving it
> -		 * to the head of the active queue [useful?]).
> -		 */
> +		if (pmap_extract(ufi->orig_map->pmap, currva, &pa))
> +			goto next;
>  
> -		uvm_lock_pageq();
> -		uvm_pageactivate(pages[lcv]);	/* reactivate */
> -		uvm_unlock_pageq();
> +		if (pages[lcv]->wire_count == 0) {
> +			uvm_lock_pageq();
> +			uvm_pageactivate(pages[lcv]);
> +			uvm_unlock_pageq();
> +		}
>  		counters_inc(uvmexp_counters, flt_nomap);
>  
>  		/* No fault-ahead when wired. */
> @@ -1181,16 +1179,19 @@ uvm_fault_lower_lookup(
>  		(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);
> +		entered++;
>  
>  		/*
>  		 * NOTE: page can't be PG_WANTED because
>  		 * we've held the lock the whole time
>  		 * we've had the handle.
>  		 */
> +next:
>  		atomic_clearbits_int(&pages[lcv]->pg_flags, PG_BUSY);
>  		UVM_PAGE_OWN(pages[lcv], NULL);
>  	}
> -	pmap_update(ufi->orig_map->pmap);
> +	if (entered > 0)
> +		pmap_update(ufi->orig_map->pmap);
>  
>  	return uobjpage;
>  }
> 
> 
>