Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: Newest uvm_purge() to try
To:
Martin Pieuchot <mpi@grenadille.net>
Cc:
tech@openbsd.org
Date:
Thu, 22 May 2025 14:45:54 +0200

Download raw body.

Thread
> Date: Thu, 22 May 2025 11:58:38 +0200
> From: Martin Pieuchot <mpi@grenadille.net>
> 
> Here's the latest version incorporating kettenis@'s pmap_purge() and
> claudio@'s feedbacks.  I observed a performance improvement of 10-15%
> on workloads using 24 to 48 CPUs.  %sys time obviously increases now
> that tearing down the VM space is accounted for.
> 
> Please test and report back.

Going to play with this!

I have a question I wanted to ask about the diff though...

> Index: kern/kern_exit.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_exit.c,v
> diff -u -p -r1.248 kern_exit.c
> --- kern/kern_exit.c	21 May 2025 09:42:59 -0000	1.248
> +++ kern/kern_exit.c	22 May 2025 09:30:43 -0000
> @@ -242,9 +242,14 @@ exit1(struct proc *p, int xexit, int xsi
>  		if (pr->ps_pptr->ps_sigacts->ps_sigflags & SAS_NOCLDWAIT)
>  			atomic_setbits_int(&pr->ps_flags, PS_NOZOMBIE);
>  
> -#ifdef __HAVE_PMAP_PURGE
> -		pmap_purge(p);
> +		/* Teardown the virtual address space. */
> +		if ((p->p_flag & P_SYSTEM) == 0) {
> +#ifdef MULTIPROCESSOR
> +			__mp_release_all(&kernel_lock);

Why do we need an __mp_release_all() here?  Is that because we have
multiple paths into exit1() that have different recurse counts for the
kernel lock?

>  #endif
> +			uvm_purge();
> +			KERNEL_LOCK();
> +		}
>  	}
>  
>  	p->p_fd = NULL;		/* zap the thread's copy */
> Index: uvm/uvm_extern.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_extern.h,v
> diff -u -p -r1.183 uvm_extern.h
> --- uvm/uvm_extern.h	21 May 2025 16:59:32 -0000	1.183
> +++ uvm/uvm_extern.h	22 May 2025 09:30:43 -0000
> @@ -268,6 +268,7 @@ int			uvm_fault(vm_map_t, vaddr_t, vm_fa
>  
>  vaddr_t			uvm_uarea_alloc(void);
>  void			uvm_uarea_free(struct proc *);
> +void			uvm_purge(void);
>  void			uvm_exit(struct process *);
>  void			uvm_init_limits(struct plimit *);
>  boolean_t		uvm_kernacc(caddr_t, size_t, int);
> @@ -401,6 +402,7 @@ void			uvmspace_init(struct vmspace *, s
>  void			uvmspace_exec(struct proc *, vaddr_t, vaddr_t);
>  struct vmspace		*uvmspace_fork(struct process *);
>  void			uvmspace_addref(struct vmspace *);
> +void			uvmspace_purge(struct vmspace *);
>  void			uvmspace_free(struct vmspace *);
>  struct vmspace		*uvmspace_share(struct process *);
>  int			uvm_sysctl(int *, u_int, void *, size_t *, 
> Index: uvm/uvm_glue.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_glue.c,v
> diff -u -p -r1.90 uvm_glue.c
> --- uvm/uvm_glue.c	20 May 2025 07:02:20 -0000	1.90
> +++ uvm/uvm_glue.c	22 May 2025 09:30:43 -0000
> @@ -286,6 +286,25 @@ uvm_uarea_free(struct proc *p)
>  }
>  
>  /*
> + * uvm_purge: teardown a virtual address space.
> + *
> + * If multi-threaded, must be called by the last thread of a process.
> + */
> +void
> +uvm_purge(void)
> +{
> +	struct proc *p = curproc;
> +	struct vmspace *vm = p->p_vmspace;
> +
> +	KERNEL_ASSERT_UNLOCKED();
> +
> +#ifdef __HAVE_PMAP_PURGE
> +	pmap_purge(p);
> +#endif
> +	uvmspace_purge(vm);
> +}
> +
> +/*
>   * uvm_exit: exit a virtual address space
>   */
>  void
> Index: uvm/uvm_map.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> diff -u -p -r1.344 uvm_map.c
> --- uvm/uvm_map.c	21 May 2025 16:59:32 -0000	1.344
> +++ uvm/uvm_map.c	22 May 2025 09:43:53 -0000
> @@ -2491,6 +2491,24 @@ uvm_map_teardown(struct vm_map *map)
>  		entry = TAILQ_NEXT(entry, dfree.deadq);
>  	}
>  
> +#ifdef VMMAP_DEBUG
> +	numt = numq = 0;
> +	RBT_FOREACH(entry, uvm_map_addr, &map->addr)
> +		numt++;
> +	TAILQ_FOREACH(entry, &dead_entries, dfree.deadq)
> +		numq++;
> +	KASSERT(numt == numq);
> +	/*
> +	 * Let VMMAP_DEBUG checks work on an empty map if uvm_map_teardown()
> +	 * is called twice.
> +	 */
> +	map->size = 0;
> +	map->min_offset = 0;
> +	map->max_offset = 0;
> +	map->flags &= ~VM_MAP_ISVMSPACE;
> +#endif
> +	/* reinit RB tree since the above removal leaves the tree corrupted. */
> +	RBT_INIT(uvm_map_addr, &map->addr);
>  	vm_map_unlock(map);
>  
>  	/* Remove address selectors. */
> @@ -2503,18 +2521,7 @@ uvm_map_teardown(struct vm_map *map)
>  	uvm_addr_destroy(map->uaddr_brk_stack);
>  	map->uaddr_brk_stack = NULL;
>  
> -#ifdef VMMAP_DEBUG
> -	numt = numq = 0;
> -	RBT_FOREACH(entry, uvm_map_addr, &map->addr)
> -		numt++;
> -	TAILQ_FOREACH(entry, &dead_entries, dfree.deadq)
> -		numq++;
> -	KASSERT(numt == numq);
> -#endif
>  	uvm_unmap_detach(&dead_entries, 0);
> -
> -	pmap_destroy(map->pmap);
> -	map->pmap = NULL;
>  }
>  
>  /*
> @@ -3395,6 +3402,24 @@ uvmspace_addref(struct vmspace *vm)
>  	atomic_inc_int(&vm->vm_refcnt);
>  }
>  
> +void
> +uvmspace_purge(struct vmspace *vm)
> +{
> +#ifdef SYSVSHM
> +	/* Get rid of any SYSV shared memory segments. */
> +	if (vm->vm_shm != NULL) {
> +		KERNEL_LOCK();
> +		shmexit(vm);
> +		KERNEL_UNLOCK();
> +	}
> +#endif
> +	/*
> +	 * Lock the map, to wait out all other references to it.  delete
> +	 * all of the mappings and pages they hold.
> +	 */
> +	uvm_map_teardown(&vm->vm_map);
> +}
> +
>  /*
>   * uvmspace_free: free a vmspace data structure
>   */
> @@ -3403,20 +3428,15 @@ uvmspace_free(struct vmspace *vm)
>  {
>  	if (atomic_dec_int_nv(&vm->vm_refcnt) == 0) {
>  		/*
> -		 * lock the map, to wait out all other references to it.  delete
> -		 * all of the mappings and pages they hold, then call the pmap
> -		 * module to reclaim anything left.
> +		 * Sanity check.  Kernel threads never end up here and
> +		 * userland ones allready teardown there VM space in
> +		 * exit1().
>  		 */
> -#ifdef SYSVSHM
> -		/* Get rid of any SYSV shared memory segments. */
> -		if (vm->vm_shm != NULL) {
> -			KERNEL_LOCK();
> -			shmexit(vm);
> -			KERNEL_UNLOCK();
> -		}
> -#endif
> +		uvmspace_purge(vm);
> +
> +		pmap_destroy(vm->vm_map.pmap);
> +		vm->vm_map.pmap = NULL;
>  
> -		uvm_map_teardown(&vm->vm_map);
>  		pool_put(&uvm_vmspace_pool, vm);
>  	}
>  }
> 
> 
>