Index | Thread | Search

From:
Martin Pieuchot <mpi@grenadille.net>
Subject:
Re: vm_upgrade
To:
tech@openbsd.org
Date:
Fri, 15 Aug 2025 09:45:22 +0200

Download raw body.

Thread
  • Martin Pieuchot:

    vm_upgrade

    • Martin Pieuchot:

      vm_upgrade

On 22/07/25(Tue) 16:54, Martin Pieuchot wrote:
> Instead of doing an unlock & relock dance before calling amap_copy() we
> can simply upgrade the lock.  This improves page fault performances from
> 1%.
> 
> As explained in the other thread we can't do better for the moment.
> 
> ok?

Anyone?

> Index: uvm/uvm_fault.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> diff -u -p -r1.169 uvm_fault.c
> --- uvm/uvm_fault.c	2 May 2025 10:19:09 -0000	1.169
> +++ uvm/uvm_fault.c	23 Jun 2025 18:48:03 -0000
> @@ -158,7 +158,6 @@ static struct uvm_advice uvmadvice[MADV_
>  /*
>   * private prototypes
>   */
> -static void uvmfault_amapcopy(struct uvm_faultinfo *);
>  static inline void uvmfault_anonflush(struct vm_anon **, int);
>  void	uvmfault_unlockmaps(struct uvm_faultinfo *, boolean_t);
>  void	uvmfault_update_stats(struct uvm_faultinfo *);
> @@ -219,49 +218,6 @@ uvmfault_init(void)
>  }
>  
>  /*
> - * uvmfault_amapcopy: clear "needs_copy" in a map.
> - *
> - * => called with VM data structures unlocked (usually, see below)
> - * => we get a write lock on the maps and clear needs_copy for a VA
> - * => if we are out of RAM we sleep (waiting for more)
> - */
> -static void
> -uvmfault_amapcopy(struct uvm_faultinfo *ufi)
> -{
> -	for (;;) {
> -		/*
> -		 * no mapping?  give up.
> -		 */
> -		if (uvmfault_lookup(ufi, TRUE) == FALSE)
> -			return;
> -
> -		/*
> -		 * copy if needed.
> -		 */
> -		if (UVM_ET_ISNEEDSCOPY(ufi->entry))
> -			amap_copy(ufi->map, ufi->entry, M_NOWAIT,
> -				UVM_ET_ISSTACK(ufi->entry) ? FALSE : TRUE,
> -				ufi->orig_rvaddr, ufi->orig_rvaddr + 1);
> -
> -		/*
> -		 * didn't work?  must be out of RAM.   unlock and sleep.
> -		 */
> -		if (UVM_ET_ISNEEDSCOPY(ufi->entry)) {
> -			uvmfault_unlockmaps(ufi, TRUE);
> -			uvm_wait("fltamapcopy");
> -			continue;
> -		}
> -
> -		/*
> -		 * got it!   unlock and return.
> -		 */
> -		uvmfault_unlockmaps(ufi, TRUE);
> -		return;
> -	}
> -	/*NOTREACHED*/
> -}
> -
> -/*
>   * uvmfault_anonget: get data in an anon into a non-busy, non-released
>   * page in that anon.
>   *
> @@ -734,14 +690,15 @@ uvm_fault_check(struct uvm_faultinfo *uf
>  	struct vm_amap *amap;
>  	struct uvm_object *uobj;
>  	int nback, nforw;
> +	boolean_t write_locked = FALSE;
>  
>  	/*
>  	 * lookup and lock the maps
>  	 */
> -	if (uvmfault_lookup(ufi, FALSE) == FALSE) {
> +lookup:
> +	if (uvmfault_lookup(ufi, write_locked) == FALSE) {
>  		return EFAULT;
>  	}
> -	/* locked: maps(read) */
>  
>  #ifdef DIAGNOSTIC
>  	if ((ufi->map->flags & VM_MAP_PAGEABLE) == 0)
> @@ -779,11 +736,27 @@ uvm_fault_check(struct uvm_faultinfo *uf
>  	if (UVM_ET_ISNEEDSCOPY(ufi->entry)) {
>  		if ((flt->access_type & PROT_WRITE) ||
>  		    (ufi->entry->object.uvm_obj == NULL)) {
> -			/* need to clear */
> -			uvmfault_unlockmaps(ufi, FALSE);
> -			uvmfault_amapcopy(ufi);
> +			/* modifing `ufi->entry' requires write lock */
> +			if (!write_locked) {
> +				write_locked = TRUE;
> +				if (!vm_map_upgrade(ufi->map)) {
> +					uvmfault_unlockmaps(ufi, FALSE);
> +					goto lookup;
> +				}
> +			}
> +
> +			amap_copy(ufi->map, ufi->entry, M_NOWAIT,
> +			    UVM_ET_ISSTACK(ufi->entry) ? FALSE : TRUE,
> +			    ufi->orig_rvaddr, ufi->orig_rvaddr + 1);
> +
> +			/*  didn't work?  must be out of RAM. */
> +			if (UVM_ET_ISNEEDSCOPY(ufi->entry)) {
> +				uvmfault_unlockmaps(ufi, write_locked);
> +				uvm_wait("fltamapcopy");
> +				return ERESTART;
> +			}
> +
>  			counters_inc(uvmexp_counters, flt_amcopy);
> -			return ERESTART;
>  		} else {
>  			/*
>  			 * ensure that we pmap_enter page R/O since
> @@ -791,6 +764,11 @@ uvm_fault_check(struct uvm_faultinfo *uf
>  			 */
>  			flt->enter_prot &= ~PROT_WRITE;
>  		}
> +	}
> +
> +	if (write_locked) {
> +		vm_map_downgrade(ufi->map);
> +		write_locked = FALSE;
>  	}
>  
>  	/*
> Index: uvm/uvm_map.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> diff -u -p -r1.345 uvm_map.c
> --- uvm/uvm_map.c	3 Jun 2025 08:38:17 -0000	1.345
> +++ uvm/uvm_map.c	23 Jun 2025 18:48:03 -0000
> @@ -5225,6 +5225,56 @@ vm_map_unlock_read_ln(struct vm_map *map
>  		mtx_leave(&map->mtx);
>  }
>  
> +boolean_t
> +vm_map_upgrade_ln(struct vm_map *map, char *file, int line)
> +{
> +	int rv;
> +
> +	if (map->flags & VM_MAP_INTRSAFE) {
> +		MUTEX_ASSERT_LOCKED(&map->mtx);
> +	} else {
> +		struct proc *busy;
> +
> +		mtx_enter(&map->flags_lock);
> +		busy = map->busy;
> +		mtx_leave(&map->flags_lock);
> +		if (busy != NULL && busy != curproc)
> +			return FALSE;
> +
> +		rv = rw_enter(&map->lock, RW_UPGRADE|RW_NOSLEEP);
> +		if (rv != 0)
> +			return FALSE;
> +	}
> +
> +	map->timestamp++;
> +	LPRINTF(("map   upgrade: %p (at %s %d)\n", map, file, line));
> +	uvm_tree_sanity(map, file, line);
> +	uvm_tree_size_chk(map, file, line);
> +
> +	return TRUE;
> +}
> +
> +boolean_t
> +vm_map_downgrade_ln(struct vm_map *map, char *file, int line)
> +{
> +	int rv;
> +
> +	if (map->flags & VM_MAP_INTRSAFE) {
> +		MUTEX_ASSERT_LOCKED(&map->mtx);
> +	} else {
> +		rv = rw_enter(&map->lock, RW_DOWNGRADE);
> +		if (rv != 0)
> +			return FALSE;
> +
> +	}
> +
> +	map->timestamp++;
> +	LPRINTF(("map   downgrade: %p (at %s %d)\n", map, file, line));
> +	uvm_tree_sanity(map, file, line);
> +	uvm_tree_size_chk(map, file, line);
> +
> +	return TRUE;
> +}
>  void
>  vm_map_busy_ln(struct vm_map *map, char *file, int line)
>  {
> Index: uvm/uvm_map.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.h,v
> diff -u -p -r1.94 uvm_map.h
> --- uvm/uvm_map.h	15 Nov 2024 02:59:23 -0000	1.94
> +++ uvm/uvm_map.h	23 Jun 2025 18:48:03 -0000
> @@ -402,6 +402,8 @@ void		vm_map_lock_ln(struct vm_map*, cha
>  void		vm_map_lock_read_ln(struct vm_map*, char*, int);
>  void		vm_map_unlock_ln(struct vm_map*, char*, int);
>  void		vm_map_unlock_read_ln(struct vm_map*, char*, int);
> +boolean_t	vm_map_upgrade_ln(struct vm_map*, char*, int);
> +boolean_t	vm_map_downgrade_ln(struct vm_map*, char*, int);
>  void		vm_map_busy_ln(struct vm_map*, char*, int);
>  void		vm_map_unbusy_ln(struct vm_map*, char*, int);
>  void		vm_map_assert_anylock_ln(struct vm_map*, char*, int);
> @@ -413,6 +415,8 @@ void		vm_map_assert_wrlock_ln(struct vm_
>  #define vm_map_lock_read(map)	vm_map_lock_read_ln(map, __FILE__, __LINE__)
>  #define vm_map_unlock(map)	vm_map_unlock_ln(map, __FILE__, __LINE__)
>  #define vm_map_unlock_read(map)	vm_map_unlock_read_ln(map, __FILE__, __LINE__)
> +#define vm_map_upgrade(map)	vm_map_upgrade_ln(map, __FILE__, __LINE__)
> +#define vm_map_downgrade(map)	vm_map_downgrade_ln(map, __FILE__, __LINE__)
>  #define vm_map_busy(map)	vm_map_busy_ln(map, __FILE__, __LINE__)
>  #define vm_map_unbusy(map)	vm_map_unbusy_ln(map, __FILE__, __LINE__)
>  #define vm_map_assert_anylock(map)	\
> 
>