Re: pdaemon locking tweak
On Tue, Aug 30, 2022 at 10:40:23AM +0200, Martin Pieuchot wrote: > On 30/08/22(Tue) 15:28, Jonathan Gray wrote: > > On Mon, Aug 29, 2022 at 01:46:20PM +0200, Martin Pieuchot wrote: > > > Diff below refactors the pdaemon's locking by introducing a new *trylock() > > > function for a given page. This is shamelessly stolen from NetBSD. > > > > > > This is part of my ongoing effort to untangle the locks used by the page > > > daemon. > > > > > > ok? > > > > if (pmap_is_referenced(p)) { > > uvm_pageactivate(p); > > > > is no longer under held slock. Which I believe is intended, > > just not obvious looking at the diff. > > > > The page queue is already locked on entry to uvmpd_scan_inactive() > > Thanks for spotting this. Indeed the locking required for > uvm_pageactivate() is different in my local tree. For now > let's keep the existing order of operations. > > Updated diff below. ok jsg@ > > Index: uvm/uvm_pdaemon.c > === > RCS file: /cvs/src/sys/uvm/uvm_pdaemon.c,v > retrieving revision 1.103 > diff -u -p -r1.103 uvm_pdaemon.c > --- uvm/uvm_pdaemon.c 30 Aug 2022 08:30:58 - 1.103 > +++ uvm/uvm_pdaemon.c 30 Aug 2022 08:39:19 - > @@ -101,6 +101,7 @@ extern void drmbackoff(long); > * local prototypes > */ > > +struct rwlock*uvmpd_trylockowner(struct vm_page *); > void uvmpd_scan(struct uvm_pmalloc *); > void uvmpd_scan_inactive(struct uvm_pmalloc *, struct pglist *); > void uvmpd_tune(void); > @@ -367,6 +368,34 @@ uvm_aiodone_daemon(void *arg) > } > } > > +/* > + * uvmpd_trylockowner: trylock the page's owner. > + * > + * => return the locked rwlock on success. otherwise, return NULL. > + */ > +struct rwlock * > +uvmpd_trylockowner(struct vm_page *pg) > +{ > + > + struct uvm_object *uobj = pg->uobject; > + struct rwlock *slock; > + > + if (uobj != NULL) { > + slock = uobj->vmobjlock; > + } else { > + struct vm_anon *anon = pg->uanon; > + > + KASSERT(anon != NULL); > + slock = anon->an_lock; > + } > + > + if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { > + return NULL; > + } > + > + return slock; > +} > + > > /* > * uvmpd_dropswap: free any swap allocated to this page. > @@ -474,51 +503,43 @@ uvmpd_scan_inactive(struct uvm_pmalloc * > > anon = p->uanon; > uobj = p->uobject; > - if (p->pg_flags & PQ_ANON) { > + > + /* > + * first we attempt to lock the object that this page > + * belongs to. if our attempt fails we skip on to > + * the next page (no harm done). it is important to > + * "try" locking the object as we are locking in the > + * wrong order (pageq -> object) and we don't want to > + * deadlock. > + */ > + slock = uvmpd_trylockowner(p); > + if (slock == NULL) { > + continue; > + } > + > + /* > + * move referenced pages back to active queue > + * and skip to next page. > + */ > + if (pmap_is_referenced(p)) { > + uvm_pageactivate(p); > + rw_exit(slock); > + uvmexp.pdreact++; > + continue; > + } > + > + if (p->pg_flags & PG_BUSY) { > + rw_exit(slock); > + uvmexp.pdbusy++; > + continue; > + } > + > + /* does the page belong to an object? */ > + if (uobj != NULL) { > + uvmexp.pdobscan++; > + } else { > KASSERT(anon != NULL); > - slock = anon->an_lock; > - if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { > - /* lock failed, skip this page */ > - continue; > - } > - /* > - * move referenced pages back to active queue > - * and skip to next page. > - */ > - if (pmap_is_referenced(p)) { > - uvm_pageactivate(p); > - rw_exit(slock); > - uvmexp.pdreact++; > - continue; > - } > - if (p->pg_flags & PG_BUSY) { > -
Re: pdaemon locking tweak
On 30/08/22(Tue) 15:28, Jonathan Gray wrote: > On Mon, Aug 29, 2022 at 01:46:20PM +0200, Martin Pieuchot wrote: > > Diff below refactors the pdaemon's locking by introducing a new *trylock() > > function for a given page. This is shamelessly stolen from NetBSD. > > > > This is part of my ongoing effort to untangle the locks used by the page > > daemon. > > > > ok? > > if (pmap_is_referenced(p)) { > uvm_pageactivate(p); > > is no longer under held slock. Which I believe is intended, > just not obvious looking at the diff. > > The page queue is already locked on entry to uvmpd_scan_inactive() Thanks for spotting this. Indeed the locking required for uvm_pageactivate() is different in my local tree. For now let's keep the existing order of operations. Updated diff below. Index: uvm/uvm_pdaemon.c === RCS file: /cvs/src/sys/uvm/uvm_pdaemon.c,v retrieving revision 1.103 diff -u -p -r1.103 uvm_pdaemon.c --- uvm/uvm_pdaemon.c 30 Aug 2022 08:30:58 - 1.103 +++ uvm/uvm_pdaemon.c 30 Aug 2022 08:39:19 - @@ -101,6 +101,7 @@ extern void drmbackoff(long); * local prototypes */ +struct rwlock *uvmpd_trylockowner(struct vm_page *); void uvmpd_scan(struct uvm_pmalloc *); void uvmpd_scan_inactive(struct uvm_pmalloc *, struct pglist *); void uvmpd_tune(void); @@ -367,6 +368,34 @@ uvm_aiodone_daemon(void *arg) } } +/* + * uvmpd_trylockowner: trylock the page's owner. + * + * => return the locked rwlock on success. otherwise, return NULL. + */ +struct rwlock * +uvmpd_trylockowner(struct vm_page *pg) +{ + + struct uvm_object *uobj = pg->uobject; + struct rwlock *slock; + + if (uobj != NULL) { + slock = uobj->vmobjlock; + } else { + struct vm_anon *anon = pg->uanon; + + KASSERT(anon != NULL); + slock = anon->an_lock; + } + + if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { + return NULL; + } + + return slock; +} + /* * uvmpd_dropswap: free any swap allocated to this page. @@ -474,51 +503,43 @@ uvmpd_scan_inactive(struct uvm_pmalloc * anon = p->uanon; uobj = p->uobject; - if (p->pg_flags & PQ_ANON) { + + /* +* first we attempt to lock the object that this page +* belongs to. if our attempt fails we skip on to +* the next page (no harm done). it is important to +* "try" locking the object as we are locking in the +* wrong order (pageq -> object) and we don't want to +* deadlock. +*/ + slock = uvmpd_trylockowner(p); + if (slock == NULL) { + continue; + } + + /* +* move referenced pages back to active queue +* and skip to next page. +*/ + if (pmap_is_referenced(p)) { + uvm_pageactivate(p); + rw_exit(slock); + uvmexp.pdreact++; + continue; + } + + if (p->pg_flags & PG_BUSY) { + rw_exit(slock); + uvmexp.pdbusy++; + continue; + } + + /* does the page belong to an object? */ + if (uobj != NULL) { + uvmexp.pdobscan++; + } else { KASSERT(anon != NULL); - slock = anon->an_lock; - if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { - /* lock failed, skip this page */ - continue; - } - /* -* move referenced pages back to active queue -* and skip to next page. -*/ - if (pmap_is_referenced(p)) { - uvm_pageactivate(p); - rw_exit(slock); - uvmexp.pdreact++; - continue; - } - if (p->pg_flags & PG_BUSY) { - rw_exit(slock); - uvmexp.pdbusy++; - continue; - }
Re: pdaemon locking tweak
On Mon, Aug 29, 2022 at 01:46:20PM +0200, Martin Pieuchot wrote: > Diff below refactors the pdaemon's locking by introducing a new *trylock() > function for a given page. This is shamelessly stolen from NetBSD. > > This is part of my ongoing effort to untangle the locks used by the page > daemon. > > ok? if (pmap_is_referenced(p)) { uvm_pageactivate(p); is no longer under held slock. Which I believe is intended, just not obvious looking at the diff. The page queue is already locked on entry to uvmpd_scan_inactive() > > Index: uvm//uvm_pdaemon.c > === > RCS file: /cvs/src/sys/uvm/uvm_pdaemon.c,v > retrieving revision 1.102 > diff -u -p -r1.102 uvm_pdaemon.c > --- uvm//uvm_pdaemon.c22 Aug 2022 12:03:32 - 1.102 > +++ uvm//uvm_pdaemon.c29 Aug 2022 11:36:59 - > @@ -101,6 +101,7 @@ extern void drmbackoff(long); > * local prototypes > */ > > +struct rwlock*uvmpd_trylockowner(struct vm_page *); > void uvmpd_scan(struct uvm_pmalloc *); > void uvmpd_scan_inactive(struct uvm_pmalloc *, struct pglist *); > void uvmpd_tune(void); > @@ -367,6 +368,34 @@ uvm_aiodone_daemon(void *arg) > } > > > +/* > + * uvmpd_trylockowner: trylock the page's owner. > + * > + * => return the locked rwlock on success. otherwise, return NULL. > + */ > +struct rwlock * > +uvmpd_trylockowner(struct vm_page *pg) > +{ > + > + struct uvm_object *uobj = pg->uobject; > + struct rwlock *slock; > + > + if (uobj != NULL) { > + slock = uobj->vmobjlock; > + } else { > + struct vm_anon *anon = pg->uanon; > + > + KASSERT(anon != NULL); > + slock = anon->an_lock; > + } > + > + if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { > + return NULL; > + } > + > + return slock; > +} > + > > /* > * uvmpd_scan_inactive: scan an inactive list for pages to clean or free. > @@ -454,53 +483,44 @@ uvmpd_scan_inactive(struct uvm_pmalloc * > uvmexp.pdscans++; > nextpg = TAILQ_NEXT(p, pageq); > > + /* > + * move referenced pages back to active queue > + * and skip to next page. > + */ > + if (pmap_is_referenced(p)) { > + uvm_pageactivate(p); > + uvmexp.pdreact++; > + continue; > + } > + > anon = p->uanon; > uobj = p->uobject; > - if (p->pg_flags & PQ_ANON) { > + > + /* > + * first we attempt to lock the object that this page > + * belongs to. if our attempt fails we skip on to > + * the next page (no harm done). it is important to > + * "try" locking the object as we are locking in the > + * wrong order (pageq -> object) and we don't want to > + * deadlock. > + */ > + slock = uvmpd_trylockowner(p); > + if (slock == NULL) { > + continue; > + } > + > + if (p->pg_flags & PG_BUSY) { > + rw_exit(slock); > + uvmexp.pdbusy++; > + continue; > + } > + > + /* does the page belong to an object? */ > + if (uobj != NULL) { > + uvmexp.pdobscan++; > + } else { > KASSERT(anon != NULL); > - slock = anon->an_lock; > - if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { > - /* lock failed, skip this page */ > - continue; > - } > - /* > - * move referenced pages back to active queue > - * and skip to next page. > - */ > - if (pmap_is_referenced(p)) { > - uvm_pageactivate(p); > - rw_exit(slock); > - uvmexp.pdreact++; > - continue; > - } > - if (p->pg_flags & PG_BUSY) { > - rw_exit(slock); > - uvmexp.pdbusy++; > - continue; > - } > uvmexp.pdanscan++; > - } else { > -
pdaemon locking tweak
Diff below refactors the pdaemon's locking by introducing a new *trylock() function for a given page. This is shamelessly stolen from NetBSD. This is part of my ongoing effort to untangle the locks used by the page daemon. ok? Index: uvm//uvm_pdaemon.c === RCS file: /cvs/src/sys/uvm/uvm_pdaemon.c,v retrieving revision 1.102 diff -u -p -r1.102 uvm_pdaemon.c --- uvm//uvm_pdaemon.c 22 Aug 2022 12:03:32 - 1.102 +++ uvm//uvm_pdaemon.c 29 Aug 2022 11:36:59 - @@ -101,6 +101,7 @@ extern void drmbackoff(long); * local prototypes */ +struct rwlock *uvmpd_trylockowner(struct vm_page *); void uvmpd_scan(struct uvm_pmalloc *); void uvmpd_scan_inactive(struct uvm_pmalloc *, struct pglist *); void uvmpd_tune(void); @@ -367,6 +368,34 @@ uvm_aiodone_daemon(void *arg) } +/* + * uvmpd_trylockowner: trylock the page's owner. + * + * => return the locked rwlock on success. otherwise, return NULL. + */ +struct rwlock * +uvmpd_trylockowner(struct vm_page *pg) +{ + + struct uvm_object *uobj = pg->uobject; + struct rwlock *slock; + + if (uobj != NULL) { + slock = uobj->vmobjlock; + } else { + struct vm_anon *anon = pg->uanon; + + KASSERT(anon != NULL); + slock = anon->an_lock; + } + + if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { + return NULL; + } + + return slock; +} + /* * uvmpd_scan_inactive: scan an inactive list for pages to clean or free. @@ -454,53 +483,44 @@ uvmpd_scan_inactive(struct uvm_pmalloc * uvmexp.pdscans++; nextpg = TAILQ_NEXT(p, pageq); + /* +* move referenced pages back to active queue +* and skip to next page. +*/ + if (pmap_is_referenced(p)) { + uvm_pageactivate(p); + uvmexp.pdreact++; + continue; + } + anon = p->uanon; uobj = p->uobject; - if (p->pg_flags & PQ_ANON) { + + /* +* first we attempt to lock the object that this page +* belongs to. if our attempt fails we skip on to +* the next page (no harm done). it is important to +* "try" locking the object as we are locking in the +* wrong order (pageq -> object) and we don't want to +* deadlock. +*/ + slock = uvmpd_trylockowner(p); + if (slock == NULL) { + continue; + } + + if (p->pg_flags & PG_BUSY) { + rw_exit(slock); + uvmexp.pdbusy++; + continue; + } + + /* does the page belong to an object? */ + if (uobj != NULL) { + uvmexp.pdobscan++; + } else { KASSERT(anon != NULL); - slock = anon->an_lock; - if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { - /* lock failed, skip this page */ - continue; - } - /* -* move referenced pages back to active queue -* and skip to next page. -*/ - if (pmap_is_referenced(p)) { - uvm_pageactivate(p); - rw_exit(slock); - uvmexp.pdreact++; - continue; - } - if (p->pg_flags & PG_BUSY) { - rw_exit(slock); - uvmexp.pdbusy++; - continue; - } uvmexp.pdanscan++; - } else { - KASSERT(uobj != NULL); - slock = uobj->vmobjlock; - if (rw_enter(slock, RW_WRITE|RW_NOSLEEP)) { - continue; - } - /* -* move referenced pages back to active queue -* and skip to