From 45b99937be5fe2f2fe485e8769f007086c2809b1 Mon Sep 17 00:00:00 2001
From: cinap_lenrek <cinap_lenrek@gmx.de>
Date: Tue, 16 Oct 2012 14:12:21 +0200
Subject: 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.
---
 sys/src/9/port/page.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

(limited to 'sys/src/9/port/page.c')

diff --git a/sys/src/9/port/page.c b/sys/src/9/port/page.c
index 17466ec7a..b004152e9 100644
--- a/sys/src/9/port/page.c
+++ b/sys/src/9/port/page.c
@@ -335,7 +335,10 @@ retry:
 * 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 @@ cachepage(Page *p, Image *i)
 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 @@ lookpage(Image *i, ulong daddr)
 {
 	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 @@ lookpage(Image *i, ulong daddr)
 			if(f->image != i || f->daddr != daddr) {
 				unlock(f);
 				unlock(&palloc);
-				return 0;
+				goto retry;
 			}
 			if(++f->ref == 1)
 				pageunchain(f);
-- 
cgit v1.2.3