From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: uvm_swapout_threads() & freed pages
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: tech@openbsd.org
Date: Sun, 27 Oct 2024 19:08:40 +0100

On 23/10/24(Wed) 23:24, Mark Kettenis wrote:
> > Date: Wed, 23 Oct 2024 18:17:50 +0200
> > From: Martin Pieuchot <mpi@grenadille.net>
> > 
> > On 22/10/24(Tue) 21:14, Mark Kettenis wrote:
> > > > Date: Sun, 20 Oct 2024 11:25:41 +0200
> > > > From: Martin Pieuchot <mpi@grenadille.net>
> > > > 
> > > > Return the number of freed pages in uvm_swapout_threads().  This is
> > > > part of my ongoing work to reduce accesses to global variables inside 
> > > > the page deamon loop.
> > > > 
> > > > ok?
> > > 
> > > Hmm, but this still accesses uvmexp.free inside uvm_swapout_threads().
> > > So I'm not sure this is an improvement.  And there is no guarantee
> > > that the returned number of freed pages is accurate.
> > 
> > I agree this could be much better.  
> > 
> > The logic in the page daemon already access `uvmexp.free' multiple
> > times.  This isn't an improvement but isn't worst.  It is good enough
> > to get uvm_swapout_threads() out of my way for now because it is not
> > used on amd64.
> > 
> > > Wouldn't it be better to make pmap_collect() return the number of
> > > freed pages?  This is only actually implemented on i386 and it looks
> > > fairly simple to count the number of ptp pages freed in
> > > pmap_do_remove_86() and pmap_do_remove_pae() and have them return that
> > > number.
> > 
> > I'd appreciate any help with that.   However there are other archs apart 
> > from i386, what about sparc64, mips64, m88k and alpha?
> 
> Hmm, so I grepped for pmap_collect in arch/*/include*.  And I should
> have grepped for PMAP_COLLECT.  I guess we need to recruit miod@ for
> testing a diff.

Miod, if you're reading this email, I need you!

> > Can I get an ok for this diff so I can make progress with my work then
> > we get rid of the `uvmexp.free' access with the changes you suggest?
> 
> How important is it that the returned value is correct?  I mean, your
> implementation may return a non-zero value even if pmap_collect()
> wasn't called.

It is currently like that.  uvmexp.free is read before calling
uvm_swapout_threads() and after inside uvmpd_scan_inactive().  So the
current pagedaemon logic isn't suited for having multiple accesses to
the physical allocator.  So this diff doesn't make it worse.
 
> But if it doesn't matter, sure.  But please add an XXX comment.

Will do, sure.

> > > > Index: uvm/uvm_glue.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/uvm/uvm_glue.c,v
> > > > diff -u -p -r1.85 uvm_glue.c
> > > > --- uvm/uvm_glue.c	8 Oct 2024 02:29:10 -0000	1.85
> > > > +++ uvm/uvm_glue.c	20 Oct 2024 09:20:09 -0000
> > > > @@ -339,13 +339,13 @@ int	swapdebug = 0;
> > > >   *   are swapped... otherwise the longest-sleeping or stopped process
> > > >   *   is swapped, otherwise the longest resident process...
> > > >   */
> > > > -void
> > > > +int
> > > >  uvm_swapout_threads(void)
> > > >  {
> > > >  	struct process *pr;
> > > >  	struct proc *p, *slpp;
> > > >  	struct process *outpr;
> > > > -	int outpri;
> > > > +	int free, outpri;
> > > >  	int didswap = 0;
> > > >  	extern int maxslp; 
> > > >  	/* XXXCDC: should move off to uvmexp. or uvm., also in uvm_meter */
> > > > @@ -355,6 +355,8 @@ uvm_swapout_threads(void)
> > > >  		return;
> > > >  #endif
> > > >  
> > > > +	free = uvmexp.free;
> > > > +
> > > >  	/*
> > > >  	 * outpr/outpri  : stop/sleep process whose most active thread has
> > > >  	 *	the largest sleeptime < maxslp
> > > > @@ -403,8 +405,7 @@ next_process:	;
> > > >  	 * if we are real low on memory since we don't gain much by doing
> > > >  	 * it.
> > > >  	 */
> > > > -	if (didswap == 0 && uvmexp.free <= atop(round_page(USPACE)) &&
> > > > -	    outpr != NULL) {
> > > > +	if (didswap == 0 && free <= atop(round_page(USPACE)) && outpr != NULL) {
> > > >  #ifdef DEBUG
> > > >  		if (swapdebug & SDB_SWAPOUT)
> > > >  			printf("swapout_threads: no duds, try procpr %p\n",
> > > > @@ -412,6 +413,8 @@ next_process:	;
> > > >  #endif
> > > >  		pmap_collect(outpr->ps_vmspace->vm_map.pmap);
> > > >  	}
> > > > +
> > > > +	return (uvmexp.free - free);
> > > >  }
> > > >  
> > > >  #endif	/* __HAVE_PMAP_COLLECT */
> > > > Index: uvm/uvm_glue.h
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/uvm/uvm_glue.h,v
> > > > diff -u -p -r1.9 uvm_glue.h
> > > > --- uvm/uvm_glue.h	11 Jul 2014 16:35:40 -0000	1.9
> > > > +++ uvm/uvm_glue.h	20 Oct 2024 09:20:09 -0000
> > > > @@ -37,7 +37,7 @@
> > > >   * uvm_glue.h
> > > >   */
> > > >  
> > > > -void uvm_swapout_threads(void);
> > > > +int uvm_swapout_threads(void);
> > > >  
> > > >  struct vm_page	*uvm_atopg(vaddr_t);
> > > >  
> > > > 
> > > > 
> >