Index | Thread | Search

From:
Jeremie Courreges-Anglas <jca@wxcvbn.org>
Subject:
Re: Newest uvm_purge() to try
To:
Mark Kettenis <mark.kettenis@xs4all.nl>
Cc:
Martin Pieuchot <mpi@grenadille.net>, tech@openbsd.org
Date:
Thu, 22 May 2025 19:27:43 +0200

Download raw body.

Thread
On Thu, May 22, 2025 at 02:45:54PM +0200, Mark Kettenis wrote:
> > 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.

sys time doesn't increase as much as I would have expected on this mac
mini M2 pro. make -j10 in libc:

20250521
    1m10.05s real     1m50.54s user     5m54.05s system
    1m10.81s real     1m50.38s user     5m57.66s system
    1m10.44s real     1m49.87s user     5m55.47s system
    1m10.99s real     1m50.18s user     5m54.85s system
    1m10.74s real     1m48.01s user     5m57.09s system
    1m11.04s real     1m49.45s user     5m56.35s system
    1m11.14s real     1m48.92s user     5m58.13s system
    1m11.00s real     1m48.48s user     5m59.10s system
    1m11.37s real     1m49.11s user     5m56.68s system
    1m11.06s real     1m49.18s user     5m55.19s system
    1m11.06s real     1m49.67s user     5m52.18s system
    1m10.97s real     1m48.85s user     5m54.57s system
    1m11.44s real     1m49.09s user     5m58.65s system
    1m11.90s real     1m48.90s user     5m56.11s system

After your diff:
    1m05.25s real     1m50.20s user     5m58.32s system
    1m05.55s real     1m50.74s user     5m59.72s system
    1m05.50s real     1m51.12s user     5m59.94s system
    1m05.44s real     1m50.80s user     6m01.66s system
    1m05.65s real     1m50.54s user     6m00.15s system
    1m05.81s real     1m50.88s user     5m59.62s system
    1m06.00s real     1m50.12s user     6m02.71s system
    1m05.61s real     1m51.20s user     6m02.00s system
    1m05.52s real     1m51.79s user     5m59.73s system
    1m05.55s real     1m50.19s user     5m59.93s system
    1m05.46s real     1m51.53s user     6m00.99s system
    1m05.79s real     1m51.27s user     6m01.16s system
    1m06.04s real     1m51.83s user     6m02.29s system
    1m05.30s real     1m51.02s user     6m01.64s system
    1m05.63s real     1m51.30s user     6m02.40s system
    1m05.67s real     1m50.10s user     6m03.47s system
    1m05.56s real     1m52.64s user     5m59.89s system
    1m05.76s real     1m51.73s user     6m00.28s system
    1m05.51s real     1m53.57s user     5m57.90s system

Maybe the recent uvm and arm64 pmap optimizations are neutering the
increase?

> > Please test and report back.

Sure.

> Going to play with this!

Yep.  I'm happy you folks found a satisfactory way forward.  At first
I was dubious about this uvm_purge() proposal.

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

Please also find one typo fix inline.

> > 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?

FWIW in Bruges we learnt that a exit1() => single_thread_smth() =>
exit1() recursion was possible.  It feels like this is no longer the
case after one of claudio's commits, but maybe mpi has other paths in
mind?

> >  #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

=>		 * userland ones already tear down their 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);
> >  	}
> >  }
> > 
> > 
> > 
> 

-- 
jca