From: Theo Buehler <tb@openbsd.org>
Subject: Re: pgo_get() vs PG_BUSY
To: Mark Kettenis <mark.kettenis@xs4all.nl>, tech@openbsd.org
Date: Wed, 18 Dec 2024 13:35:21 +0100

On Sun, Dec 15, 2024 at 12:18:13PM +0100, Martin Pieuchot wrote:
> On 09/12/24(Mon) 10:49, Martin Pieuchot wrote:
> > On 07/12/24(Sat) 13:11, Mark Kettenis wrote:
> > > > Date: Tue, 3 Dec 2024 09:46:20 +0100
> > > > From: Martin Pieuchot <mpi@grenadille.net>
> > > > 
> > > > Diff below changes the behavior of the pgo_get() interface with PGO_LOCKED
> > > > and how the page currently fetched via uvm_fault_lower_io() is released.
> > > > 
> > > > The pgo_get() w/ PGO_LOCKED implementations are always called with
> > > > `uobj->vmobjlock' locked and the lock is never released while manipulating
> > > > the pages.  So there is not need to set the PG_BUSY flag on the pages.
> > > > This is what the uao_get() and uvn_get() chunks are about.
> > > > 
> > > > The chunks in uvm_fault_lower_lookup() are simplifications related to
> > > > the fact that none of the pgo_get() set the PG_BUSY flags.
> > > > 
> > > > The rest of the diff deals with `uobjpage', the physical page fetched
> > > > with pgo_get() w/ PGO_SYNCIO.  Instead of releasing the pages in
> > > > multiple places release it as soon as we've got the associated lock in
> > > > uvm_fault_lower_io().  The reasoning is the same as above.
> > > > 
> > > > This diff is the complicated part of the current parallelisation I'm
> > > > working on, so do not hesitate if you have any question. 
> > > > 
> > > > ok?
> > > 
> > > This seems to leave behind a comment that states that pgo_get() will
> > > set the PG_BUSY flag in uvm_fault_lower_lookup():
> > > 
> > >                 /*
> > >                  * if center page is resident and not
> > >                  * PG_BUSY, then pgo_get made it PG_BUSY
> > >                  * for us and gave us a handle to it.
> > >                  * remember this page as "uobjpage."
> > >                  * (for later use).
> > >                  */
> > > 
> > > That needs to be fixed.
> > 
> > Indeed, updated diff below with fixed comments.
> > 
> > > [...]
> > > > @@ -1324,21 +1323,6 @@ uvm_fault_lower(struct uvm_faultinfo *ufi, struct uvm_faultctx *flt,
> > > >  		 * out of memory resources?
> > > >  		 */
> > > >  		if (anon == NULL || pg == NULL) {
> > > > -			/*
> > > > -			 * arg!  must unbusy our page and fail or sleep.
> > > > -			 */
> > > > -			if (uobjpage != PGO_DONTCARE) {
> > > > -				uvm_lock_pageq();
> > > > -				uvm_pageactivate(uobjpage);
> > > > -				uvm_unlock_pageq();
> > > 
> > > Shouldn't we still activate the page in this case?
> > 
> > Yes we should.  We do it in uvm_fault_lower_io().
> 
> Any ok?

Mark's review questions helped remove some confusions I had. I've been
running an earlier version this on several machines (including my bulk
build box with the vfs syscalls unlocked) without any apparent issues.

ok tb