On Tue, Jun 28, 2022 at 02:13:04PM +0200, Martin Pieuchot wrote:
> I'd like to abstract the use of PG_WANTED to start unifying & cleaning
> the various cases where a code path is waiting for a busy page. Here's
> the first step.
>
> ok?
if it helps you, yes it is fine. ok semarie@
just one nit in uvm_pagewait() regarding one KASSERT() which could be avoided
(see below).
I also wonder about PNORELOCK usage: we replace only one `PVM | PNORELOCK`
call,
and all others needs a rw_enter() after calling uvm_pagewait(). but I don't
have
strong opinion about that.
> Index: uvm/uvm_amap.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
> retrieving revision 1.90
> diff -u -p -r1.90 uvm_amap.c
> --- uvm/uvm_amap.c 30 Aug 2021 16:59:17 -0000 1.90
> +++ uvm/uvm_amap.c 28 Jun 2022 11:53:08 -0000
> @@ -781,9 +781,7 @@ ReStart:
> * it and then restart.
> */
> if (pg->pg_flags & PG_BUSY) {
> - atomic_setbits_int(&pg->pg_flags, PG_WANTED);
> - rwsleep_nsec(pg, amap->am_lock, PVM | PNORELOCK,
> - "cownow", INFSLP);
> + uvm_pagewait(pg, amap->am_lock, "cownow");
> goto ReStart;
> }
>
> Index: uvm/uvm_aobj.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_aobj.c,v
> retrieving revision 1.103
> diff -u -p -r1.103 uvm_aobj.c
> --- uvm/uvm_aobj.c 29 Dec 2021 20:22:06 -0000 1.103
> +++ uvm/uvm_aobj.c 28 Jun 2022 11:53:08 -0000
> @@ -835,9 +835,8 @@ uao_detach(struct uvm_object *uobj)
> while ((pg = RBT_ROOT(uvm_objtree, &uobj->memt)) != NULL) {
> pmap_page_protect(pg, PROT_NONE);
> if (pg->pg_flags & PG_BUSY) {
> - atomic_setbits_int(&pg->pg_flags, PG_WANTED);
> - rwsleep_nsec(pg, uobj->vmobjlock, PVM, "uao_det",
> - INFSLP);
> + uvm_pagewait(pg, uobj->vmobjlock, "uao_det");
> + rw_enter(uobj->vmobjlock, RW_WRITE);
> continue;
> }
> uao_dropswap(&aobj->u_obj, pg->offset >> PAGE_SHIFT);
> @@ -909,9 +908,8 @@ uao_flush(struct uvm_object *uobj, voff_
>
> /* Make sure page is unbusy, else wait for it. */
> if (pg->pg_flags & PG_BUSY) {
> - atomic_setbits_int(&pg->pg_flags, PG_WANTED);
> - rwsleep_nsec(pg, uobj->vmobjlock, PVM, "uaoflsh",
> - INFSLP);
> + uvm_pagewait(pg, uobj->vmobjlock, "uaoflsh");
> + rw_enter(uobj->vmobjlock, RW_WRITE);
> curoff -= PAGE_SIZE;
> continue;
> }
> @@ -1147,9 +1145,8 @@ uao_get(struct uvm_object *uobj, voff_t
>
> /* page is there, see if we need to wait on it */
> if ((ptmp->pg_flags & PG_BUSY) != 0) {
> - atomic_setbits_int(&ptmp->pg_flags, PG_WANTED);
> - rwsleep_nsec(ptmp, uobj->vmobjlock, PVM,
> - "uao_get", INFSLP);
> + uvm_pagewait(ptmp, uobj->vmobjlock, "uao_get");
> + rw_enter(uobj->vmobjlock, RW_WRITE);
> continue; /* goto top of pps while loop */
> }
>
> Index: uvm/uvm_km.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_km.c,v
> retrieving revision 1.150
> diff -u -p -r1.150 uvm_km.c
> --- uvm/uvm_km.c 7 Jun 2022 12:07:45 -0000 1.150
> +++ uvm/uvm_km.c 28 Jun 2022 11:53:08 -0000
> @@ -255,9 +255,8 @@ uvm_km_pgremove(struct uvm_object *uobj,
> for (curoff = start ; curoff < end ; curoff += PAGE_SIZE) {
> pp = uvm_pagelookup(uobj, curoff);
> if (pp && pp->pg_flags & PG_BUSY) {
> - atomic_setbits_int(&pp->pg_flags, PG_WANTED);
> - rwsleep_nsec(pp, uobj->vmobjlock, PVM, "km_pgrm",
> - INFSLP);
> + uvm_pagewait(pp, uobj->vmobjlock, "km_pgrm");
> + rw_enter(uobj->vmobjlock, RW_WRITE);
> curoff -= PAGE_SIZE; /* loop back to us */
> continue;
> }
> Index: uvm/uvm_page.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_page.c,v
> retrieving revision 1.166
> diff -u -p -r1.166 uvm_page.c
> --- uvm/uvm_page.c 12 May 2022 12:48:36 -0000 1.166
> +++ uvm/uvm_page.c 28 Jun 2022 11:57:42 -0000
> @@ -1087,6 +1087,23 @@ uvm_page_unbusy(struct vm_page **pgs, in
> }
> }
>
> +/*
> + * uvm_pagewait: wait for a busy page
> + *
> + * => page must be known PG_BUSY
> + * => object must be locked
> + * => object will be unlocked on return
> + */
> +void
> +uvm_pagewait(struct vm_page *pg, struct rwlock *lock, const char *wmesg)
> +{
> + KASSERT(rw_lock_held(lock));
the KASSERT(rw_lock_held(lock)) shouldn't be required: rwsleep_nsec() will
already call rw_assert_anylock() (via rwsleep function).
rw_status() (used by both rw_lock_held and rw_assert_anylock) could only return
4 values:
- 0 (not locked)
- RW_READ
- RW_WRITE
- RW_WRITE_OTHER
using KASSERT(rw_lock_held(lock)) or rw_assert_anylock() permits only RW_READ
||
RW_WRITE (assert on 0 and on RW_WRITE_OTHER).
> + KASSERT((pg->pg_flags & PG_BUSY) != 0);
> +
> + atomic_setbits_int(&pg->pg_flags, PG_WANTED);
> + rwsleep_nsec(pg, lock, PVM | PNORELOCK, wmesg, INFSLP);
> +}
> +
> #if defined(UVM_PAGE_TRKOWN)
> /*
> * uvm_page_own: set or release page ownership
> Index: uvm/uvm_page.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_page.h,v
> retrieving revision 1.68
> diff -u -p -r1.68 uvm_page.h
> --- uvm/uvm_page.h 12 May 2022 12:48:36 -0000 1.68
> +++ uvm/uvm_page.h 28 Jun 2022 11:53:08 -0000
> @@ -233,7 +233,7 @@ void uvm_pagefree(struct vm_page *);
> void uvm_page_unbusy(struct vm_page **, int);
> struct vm_page *uvm_pagelookup(struct uvm_object *, voff_t);
> void uvm_pageunwire(struct vm_page *);
> -void uvm_pagewait(struct vm_page *, int);
> +void uvm_pagewait(struct vm_page *, struct rwlock *, const char *);
> void uvm_pagewake(struct vm_page *);
> void uvm_pagewire(struct vm_page *);
> void uvm_pagezero(struct vm_page *);
> Index: uvm/uvm_vnode.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v
> retrieving revision 1.124
> diff -u -p -r1.124 uvm_vnode.c
> --- uvm/uvm_vnode.c 3 May 2022 21:20:35 -0000 1.124
> +++ uvm/uvm_vnode.c 28 Jun 2022 11:53:08 -0000
> @@ -684,11 +684,10 @@ uvn_flush(struct uvm_object *uobj, voff_
> }
> } else if (flags & PGO_FREE) {
> if (pp->pg_flags & PG_BUSY) {
> - atomic_setbits_int(&pp->pg_flags,
> - PG_WANTED);
> uvm_unlock_pageq();
> - rwsleep_nsec(pp, uobj->vmobjlock, PVM,
> - "uvn_flsh", INFSLP);
> + uvm_pagewait(pp, uobj->vmobjlock,
> + "uvn_flsh");
> + rw_enter(uobj->vmobjlock, RW_WRITE);
> uvm_lock_pageq();
> curoff -= PAGE_SIZE;
> continue;
> @@ -1054,9 +1053,8 @@ uvn_get(struct uvm_object *uobj, voff_t
>
> /* page is there, see if we need to wait on it */
> if ((ptmp->pg_flags & PG_BUSY) != 0) {
> - atomic_setbits_int(&ptmp->pg_flags, PG_WANTED);
> - rwsleep_nsec(ptmp, uobj->vmobjlock, PVM,
> - "uvn_get", INFSLP);
> + uvm_pagewait(ptmp, uobj->vmobjlock, "uvn_get");
> + rw_enter(uobj->vmobjlock, RW_WRITE);
> continue; /* goto top of pps while loop */
> }
>
>
Thanks.
--
Sebastien Marie
