ref: 68439f169de75de87aa7b2455196a82b9c6180c0
parent: 47d0c9f0792c7f573c26a461d4e75ac4f1c878a6
author: cinap_lenrek <cinap_lenrek@gmx.de>
date: Tue Oct 16 10:12:21 EDT 2012
kernel: cachedel() lock order, lookpage, cleanup the lock order of page.Lock -> palloc.hashlock was violated in cachedel() which is called from the pager. change the code to do it in the right oder to prevent deadlock. change lookpage to retry on false hit. i assume that a false hit means: a) we'r low on memory -> cached page got uncached/reused b) duppage() got called on the page, meaning theres another cached copy in the image now. paging in is expensive compared to the hashtable lookup, so i think retrying is better. cleanup fixfault, adding comments.
--- a/sys/src/9/port/fault.c
+++ b/sys/src/9/port/fault.c
@@ -140,15 +140,20 @@
lkp = *pg;
lock(lkp);
- if(lkp->ref < 1)
- panic("fault: lkp->ref %d < 1", lkp->ref);+ ref = lkp->ref;
+ if(ref == 0)
+ panic("fault %#p ref == 0", lkp);if(lkp->image == &swapimage)
- ref = lkp->ref + swapcount(lkp->daddr);
- else
- ref = lkp->ref;
- if(ref == 1 && lkp->image){- /* save a copy of the original for the image cache */
+ ref += swapcount(lkp->daddr);
+ if(ref == 1 && lkp->image) {+ /*
+ * save a copy of the original for the image cache
+ * and uncache the page. page might temporarily be
+ * unlocked while trying to acquire palloc lock so
+ * recheck ref in case it got grabbed.
+ */
duppage(lkp);
+
ref = lkp->ref;
}
unlock(lkp);
--- a/sys/src/9/port/page.c
+++ b/sys/src/9/port/page.c
@@ -335,7 +335,10 @@
* from the freelist, but still in the cache because of
* cachepage below. if someone else looks in the cache
* before they remove it, the page will have a nonzero ref
-* once they finally lock(np).
+* once they finally lock(np). This does not happen because
+* newpage, auxpage, duppage and lookpage all lock(&palloc)
+* so while they hold it nobody is going to grab anything
+* from the cache.
*/
lock(np);
if(np->ref != 0) /* should never happen */
@@ -412,23 +415,24 @@
void
cachedel(Image *i, ulong daddr)
{- Page *f, **l;
+ Page *f;
+retry:
lock(&palloc.hashlock);
- l = &pghash(daddr);
- for(f = *l; f; f = f->hash) {+ for(f = pghash(daddr); f; f = f->hash) { if(f->image == i && f->daddr == daddr) {+ unlock(&palloc.hashlock);
+
lock(f);
- if(f->image == i && f->daddr == daddr){- *l = f->hash;
- putimage(f->image);
- f->image = 0;
- f->daddr = 0;
+ if(f->image != i || f->daddr != daddr) {+ unlock(f);
+ goto retry;
}
+ uncachepage(f);
unlock(f);
- break;
+
+ return;
}
- l = &f->hash;
}
unlock(&palloc.hashlock);
}
@@ -438,6 +442,7 @@
{Page *f;
+retry:
lock(&palloc.hashlock);
for(f = pghash(daddr); f; f = f->hash) { if(f->image == i && f->daddr == daddr) {@@ -448,7 +453,7 @@
if(f->image != i || f->daddr != daddr) {unlock(f);
unlock(&palloc);
- return 0;
+ goto retry;
}
if(++f->ref == 1)
pageunchain(f);
--
⑨