svn commit: r212281 - head/sys/vm

[Kostik Belousov]

On Mon, Sep 13, 2010 at 12:30:29PM -0500, Alan Cox wrote:
> Kostik Belousov wrote:
> >On Tue, Sep 07, 2010 at 12:23:45AM +0000, Ryan Stone wrote:
> >  
> >>Author: rstone
> >>Date: Tue Sep  7 00:23:45 2010
> >>New Revision: 212281
> >>URL: http://svn.freebsd.org/changeset/base/212281
> >>
> >>Log:
> >>  In munmap() downgrade the vm_map_lock to a read lock before taking a 
> >>  read
> >>  lock on the pmc-sx lock.  This prevents a deadlock with
> >>  pmc_log_process_mappings, which has an exclusive lock on pmc-sx and 
> >>  tries
> >>  to get a read lock on a vm_map.  Downgrading the vm_map_lock in munmap
> >>  allows pmc_log_process_mappings to continue, preventing the deadlock.
> >>  
> >>  Without this change I could cause a deadlock on a multicore 8.1-RELEASE
> >>  system by having one thread constantly mmap'ing and then munmap'ing a
> >>  PROT_EXEC mapping in a loop while I repeatedly invoked and stopped 
> >>  pmcstat
> >>  in system-wide sampling mode.
> >>  
> >>  Reviewed by:	fabient
> >>  Approved by:	emaste (mentor)
> >>  MFC after:	2 weeks
> >>
> >>Modified:
> >>  head/sys/vm/vm_mmap.c
> >>
> >>Modified: head/sys/vm/vm_mmap.c
> >>==============================================================================
> >>--- head/sys/vm/vm_mmap.c	Mon Sep  6 23:52:04 2010	(r212280)
> >>+++ head/sys/vm/vm_mmap.c	Tue Sep  7 00:23:45 2010	(r212281)
> >>@@ -579,6 +579,7 @@ munmap(td, uap)
> >> 	 * Inform hwpmc if the address range being unmapped contains
> >> 	 * an executable region.
> >> 	 */
> >>+	pkm.pm_address = (uintptr_t) NULL;
> >> 	if (vm_map_lookup_entry(map, addr, &entry)) {
> >> 		for (;
> >> 		     entry != &map->header && entry->start < addr + size;
> >>@@ -587,16 +588,23 @@ munmap(td, uap)
> >> 				entry->end, VM_PROT_EXECUTE) == TRUE) {
> >> 				pkm.pm_address = (uintptr_t) addr;
> >> 				pkm.pm_size = (size_t) size;
> >>-				PMC_CALL_HOOK(td, PMC_FN_MUNMAP,
> >>-				    (void *) &pkm);
> >> 				break;
> >> 			}
> >> 		}
> >> 	}
> >> #endif
> >>-	/* returns nothing but KERN_SUCCESS anyway */
> >> 	vm_map_delete(map, addr, addr + size);
> >>+
> >>+#ifdef HWPMC_HOOKS
> >>+	/* downgrade the lock to prevent a LOR with the pmc-sx lock */
> >>+	vm_map_lock_downgrade(map);
> >>+	if (pkm.pm_address != (uintptr) NULL)
> >>+		PMC_CALL_HOOK(td, PMC_FN_MUNMAP, (void *) &pkm);
> >>+	vm_map_unlock_read(map);
> >>+#else
> >> 	vm_map_unlock(map);
> >>+#endif
> >>+	/* vm_map_delete returns nothing but KERN_SUCCESS anyway */
> >> 	return (0);
> >> }
> >> 
> >>    
> >Note that vm_map_unlock() is more then just dropping the lock on the map.
> >Due to ordering of the vnode lock before vm map lock, vm_map_unlock()
> >processes the deferred free entries after map lock is dropped. After your
> >change, the deferred list might keep entries for some time until next
> >unlock is performed.
> >
> >  
> 
> I'm afraid that this understates the effect.  Over the weekend, when I 
> updated one of my amd64 machines to include this change, I found that 
> the delay in object and page deallocation is leading to severe 
> fragmentation within the physical memory allocator.  As a result, the 
> time spent in the kernel during a "buildworld" increased by about 8%.  
> Moreover, superpage promotion by applications effectively stopped.
> 
> For now, I think it would be best to back out r212281 and r212282.  
> Ultimately, the fix may be to change the vm map synchronization 
> primitives, and simply reinstate r212281 and r212282, but I'd like some 
> time to consider the options.

Did you noted the thread on current@ about r212281 ? The submitter
claims that the rev. causes panics in unrelated code pathes when
vnode_create_vobject() locks vm object lock. I cannot understand
how this can happen, with or without the rev.

More, when I suggested the following change, that is intended to
minimize the window, the answer was that it makes the situation
worse.

diff --git a/sys/vm/vm_mmap.c b/sys/vm/vm_mmap.c
index 63dfb67..d13e488 100644
--- a/sys/vm/vm_mmap.c
+++ b/sys/vm/vm_mmap.c
@@ -597,13 +597,15 @@ munmap(td, uap)
 
 #ifdef HWPMC_HOOKS
 	/* downgrade the lock to prevent a LOR with the pmc-sx lock */
-	vm_map_lock_downgrade(map);
-	if (pkm.pm_address != (uintptr_t) NULL)
-		PMC_CALL_HOOK(td, PMC_FN_MUNMAP, (void *) &pkm);
-	vm_map_unlock_read(map);
-#else
-	vm_map_unlock(map);
+	if (pkm.pm_address != (uintptr_t)NULL) {
+		vm_map_lock_downgrade(map);
+		if (pkm.pm_address != (uintptr_t)NULL)
+			PMC_CALL_HOOK(td, PMC_FN_MUNMAP, (void *)&pkm);
+		vm_map_unlock_read(map);
+		vm_map_lock(map);
+	}
 #endif
+	vm_map_unlock(map);
 	/* vm_map_delete returns nothing but KERN_SUCCESS anyway */
 	return (0);
 }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20100913/141ddc58/attachment.pgp