From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: uvmpd_iterator
To: tech@openbsd.org
Date: Tue, 4 Mar 2025 14:58:14 +0100

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

Apart from this OK claudio@
-- 
:wq Claudio