On Thu, Apr 28, 2022 at 12:28:45PM +0200, Martin Pieuchot wrote:
> rw_enter(9) can sleep. When the lock is finally acquired by the
> pagedaemon the previous check might no longer be true and the page
> could be busy. In this case we shouldn't touch it.
>
> Diff below recheck for PG_BUSY after acquiring the lock and also
> use a variable for the lock to reduce the differences with NetBSD.
>
> ok?
hep. ok semarie@
just cosmetic comment below, but if it makes the code diverges from NetBSD,
feel
free to ignore.
> Index: uvm/uvm_pdaemon.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_pdaemon.c,v
> retrieving revision 1.96
> diff -u -p -r1.96 uvm_pdaemon.c
> --- uvm/uvm_pdaemon.c 11 Apr 2022 16:43:49 -0000 1.96
> +++ uvm/uvm_pdaemon.c 28 Apr 2022 10:22:52 -0000
> @@ -879,6 +879,8 @@ uvmpd_scan(void)
> int free, inactive_shortage, swap_shortage, pages_freed;
> struct vm_page *p, *nextpg;
> struct uvm_object *uobj;
> + struct vm_anon *anon;
> + struct rwlock *slock;
> boolean_t got_it;
>
> MUTEX_ASSERT_LOCKED(&uvm.pageqlock);
> @@ -947,20 +949,34 @@ uvmpd_scan(void)
> p != NULL && (inactive_shortage > 0 || swap_shortage > 0);
> p = nextpg) {
> nextpg = TAILQ_NEXT(p, pageq);
> -
> - /* skip this page if it's busy. */
> - if (p->pg_flags & PG_BUSY)
> + if (p->pg_flags & PG_BUSY) {
> continue;
> + }
>
> - if (p->pg_flags & PQ_ANON) {
> - KASSERT(p->uanon != NULL);
> - if (rw_enter(p->uanon->an_lock, RW_WRITE|RW_NOSLEEP))
> + /*
> + * lock the page's owner.
> + */
> + if (p->uobject != NULL) {
> + uobj = p->uobject;
> + slock = uobj->vmobjlock;
> + if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) {
> continue;
> + }
> } else {
> - KASSERT(p->uobject != NULL);
> - if (rw_enter(p->uobject->vmobjlock,
> - RW_WRITE|RW_NOSLEEP))
> + anon = p->uanon;
> + KASSERT(p->uanon != NULL);
> + slock = anon->an_lock;
> + if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) {
> continue;
> + }
> + }
I would move the rw_enter() call after the if(p->pg_flags & PQ_ANON), for
better readability.
if (p->uobject != NULL) {
uobj = p->uobject;
slock = uobj->vmobjlock;
} else {
anon = p->uanon;
KASSERT(p->uanon != NULL);
slock = anon->an_lock;
}
if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) {
continue;
}
> +
> + /*
> + * skip this page if it's busy.
> + */
> + if ((p->pg_flags & PG_BUSY) != 0) {
> + rw_exit(slock);
> + continue;
> }
>
> /*
> @@ -997,10 +1013,11 @@ uvmpd_scan(void)
> uvmexp.pddeact++;
> inactive_shortage--;
> }
> - if (p->pg_flags & PQ_ANON)
> - rw_exit(p->uanon->an_lock);
> - else
> - rw_exit(p->uobject->vmobjlock);
> +
> + /*
> + * we're done with this page.
> + */
> + rw_exit(slock);
> }
> }
>
>
--
Sebastien Marie
