Make the anonymous pager's fault handler and madvise reentrant.

2008-11-12  Neal H. Walfield  <neal@gnu.org>

	* anonymous.h (struct anonymous_pager): Improve documentation for
	the fill_lock field.
	* anonymous.c (fault): Drop ANON->LOCK before calling the user's
	fill function.
	(destroy): Before destroying ANON, take ANON->FILL_LOCK.

	* madvise.c (madvise): Refactor to not hold map lock when calling
	MAP->PAGER->ADVISE.

4 files changed, 82 insertions, 57 deletions

diff --git a/libhurd-mm/ChangeLog b/libhurd-mm/ChangeLog
index 032407e..070f680 100644
--- a/libhurd-mm/ChangeLog
+++ b/libhurd-mm/ChangeLog
@@ -1,3 +1,14 @@
+2008-11-12  Neal H. Walfield  <neal@gnu.org>
+
+	* anonymous.h (struct anonymous_pager): Improve documentation for
+	the fill_lock field.
+	* anonymous.c (fault): Drop ANON->LOCK before calling the user's
+	fill function.
+	(destroy): Before destroying ANON, take ANON->FILL_LOCK.
+
+	* madvise.c (madvise): Refactor to not hold map lock when calling
+	MAP->PAGER->ADVISE.
+
 2008-11-11  Neal H. Walfield  <neal@gnu.org>
 
 	* madvise.c (madvise): Add debugging output for bad arguments.
diff --git a/libhurd-mm/anonymous.c b/libhurd-mm/anonymous.c
index 2185613..4461360 100644
--- a/libhurd-mm/anonymous.c
+++ b/libhurd-mm/anonymous.c
@@ -292,6 +292,8 @@ fault (struct pager *pager, uintptr_t offset, int count, bool read_only,
 	}
     }
 
+  ss_mutex_unlock (&anon->lock);
+
   r = true;
 
   if (anon->fill)
@@ -328,8 +330,6 @@ fault (struct pager *pager, uintptr_t offset, int count, bool read_only,
 	}
     }
 
-  ss_mutex_unlock (&anon->lock);
-
   profile_region_end ();
 
   debug (5, "Fault at %x resolved", fault_addr);
@@ -436,6 +436,9 @@ destroy (struct pager *pager)
 
   struct anonymous_pager *anon = (struct anonymous_pager *) pager;
 
+  /* Wait any fill function returns.  */
+  ss_mutex_lock (&anon->fill_lock);
+
   if (anon->staging_area)
     /* Free the staging area.  */
     {
diff --git a/libhurd-mm/anonymous.h b/libhurd-mm/anonymous.h
index 19626d9..fd1bedb 100644
--- a/libhurd-mm/anonymous.h
+++ b/libhurd-mm/anonymous.h
@@ -123,7 +123,9 @@ struct anonymous_pager
 
   /* The thread in the fill function.  (If none, NULL.)  */
   l4_thread_id_t fill_thread;
-  /* Used to serialize the fill function.  */
+  /* Used to serialize the fill function.  Also protects FILL_THREAD.
+     If ANONYMOUS_THREAD_SAFE is set, then this lock protects the
+     staging area.  Must be taken while holding LOCK.  */
   ss_mutex_t fill_lock;
 };
 
diff --git a/libhurd-mm/madvise.c b/libhurd-mm/madvise.c
index d3985c2..b9494cd 100644
--- a/libhurd-mm/madvise.c
+++ b/libhurd-mm/madvise.c
@@ -31,6 +31,8 @@
 int
 madvise (void *addr, size_t length, int advice)
 {
+  debug (5, "(%p, %x, %d)", addr, length, advice);
+
   if (((uintptr_t) addr & (PAGESIZE - 1)) != 0)
     {
       debug (0, "Address %x not multiple of pagesize.", addr);
@@ -68,65 +70,72 @@ madvise (void *addr, size_t length, int advice)
 
   struct region region = { (uintptr_t) addr, length };
 
+  debug (5, "Region: %x-%x",
+	 region.start, region.start + region.length - 1);
+
   maps_lock_lock ();
 
-  /* Find any pager that overlaps within the designated region.  */
-  struct map *map = map_find (region);
-  if (! map)
-    /* There are none.  We're done.  */
+  /* Find the first map that overlaps with the designated region.  */
+  struct map *map;
+  while (region.length > 0
+	 && (map = hurd_btree_map_find_first (&maps, &region)))
     {
-      maps_lock_unlock ();
-      return 0;
+      uintptr_t map_start = map->region.start;
+      uintptr_t map_end = map_start + map->region.length - 1;
+
+      debug (5, "(%x-%x): considering %x-%x",
+	     start, end, map_start, map_end);
+
+      /*
+	map_start/map->offset                map_end
+	/   \                           /  \
+	v     v                         v    v
+	+------------------------------------+
+	|                                    | <- pager
+	+------------------------------------+
+
+	^     ^     ^                        ^     ^     ^
+	\    |    /                          \    |   /
+	start                                 end
+
+      */
+      uintptr_t s = map->offset;
+      uintptr_t l = map->region.length;
+      if (start > map_start)
+	{
+	  s += start - map_start;
+	  l -= start - map_start;
+	}
+      if (end < map_end)
+	l -= map_end - end;
+
+      /* Advance region to just after MAP.  */
+      if (map_end >= region.start + region.length)
+	region.length = 0;
+      else
+	{
+	  region.length -= map_end - region.start + 1;
+	  region.start += map_end - region.start + 1;
+	}
+
+      struct pager *pager = map->pager;
+      if (pager->advise)
+	{
+	  maps_lock_unlock ();
+	  pager->advise (map->pager, s, l, advice);
+
+	  /* If REGION.LENGTH is zero, don't bother to retake the
+	     lock: we are done.  */
+	  if (region.length > 0)
+	    maps_lock_lock ();
+	  else
+	    return 0;
+	}
+
+      debug (5, "Region: %x-%x",
+	     region.start, region.start + region.length - 1);
     }
 
-  /* There may be pagers that come lexically before as well as after
-     PAGER.  We start with PAGER and scan forward and then do the same
-     but scan backwards.  */
-  struct map *prev = hurd_btree_map_prev (map);
-
-  int dir;
-  for (dir = 0; dir < 2; dir ++, map = prev)
-    for (;
-	 map;
-	 map = (dir == 0 ? hurd_btree_map_next (map)
-		: hurd_btree_map_prev (map)))
-      {
-	uintptr_t map_start = map->region.start;
-	uintptr_t map_end = map_start + map->region.length - 1;
-
-	debug (5, "(%x-%x): considering %x-%x",
-	       start, end, map_start, map_end);
-
-	if (map_start > end || map_end < start)
-	  break;
-
-	/*
-	    map_start/map->offset                map_end
-                   /   \                           /  \
-                  v     v                         v    v
-	          +------------------------------------+
-	          |                                    | <- pager
-	          +------------------------------------+
-
-            ^     ^     ^                        ^     ^     ^
-	     \    |    /                          \    |   /
-                start                                 end
-
-	 */
-	uintptr_t s = map->offset;
-	uintptr_t l = map->region.length;
-	if (start > map_start)
-	  {
-	    s += start - map_start;
-	    l -= start - map_start;
-	  }
-	if (end < map_end)
-	  l -= map_end - end;
-
-	if (map->pager->advise)
-	  map->pager->advise (map->pager, s, l, advice);
-      }
-
   maps_lock_unlock ();
 
   return 0;