From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: uvmpd_iterator
To: tech@openbsd.org
Date: Mon, 10 Mar 2025 15:18:40 +0100

On 04/03/25(Tue) 14:58, Claudio Jeker wrote:
> On Tue, Mar 04, 2025 at 02:26:11PM +0100, Martin Pieuchot wrote:
> > On 26/02/25(Wed) 15:09, Martin Pieuchot wrote:
> > > Currently the pdaemon does funny dances when iterating the inactive list
> > > in order to grab the next page on the list.  Diff below simplifies those
> > > dances by inserting a marker (iterator) on the list.  This is necessary
> > > to reduce the scope of the uvm_pageq_lock.
> > > 
> > > ok?
> > 
> > Anyone?
> 
> Two comments:
>  
> > +struct vm_page *
> > +uvmpd_iterator(struct pglist *pglst, struct vm_page *p, struct vm_page *iter)
> > +{
> > +	struct vm_page *nextpg = NULL;
> 
> There is no need to initalize this to NULL.
> 
> > +
> > +	MUTEX_ASSERT_LOCKED(&uvm.pageqlock);
> > +
> > +	/* p is null to signal final swap i/o. */
> > +	if (p == NULL)
> > +		return NULL;
> > +
> > +	do {
> > +		nextpg = TAILQ_NEXT(iter, pageq);
> > +	} while (nextpg && (nextpg->pg_flags & PQ_ITER));
> > +
> > +	if (nextpg) {
> > +		TAILQ_REMOVE(pglst, iter, pageq);
> > +		TAILQ_INSERT_AFTER(pglst, nextpg, iter, pageq);
> > +	}
> > +
> > +	return nextpg;
> > +}
> 
> Shouldn't uvmpd_drop() skip over PQ_ITER elements explicitly?

It doesn't need to because it doesn't run in parallel of the pagedaemon.

> In general if you use an iterator like this then all iteration of that
> list needs to be adjusted. At least I think it would be better to make
> this explicitly clear that iterator objects are skipped.

Sure, I'll convert all iterations to the iterator API in a next step.

> Apart from this OK claudio@

Thanks!