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
