svn commit: r212281 - head/sys/vm

[Kostik Belousov]

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 think the two changes could be done:
- only perform downgrade when hook is indeed going to be called;
- before vm_map_unlock_read, check for deferred_freelist being not
  NULL, and if so, do vm_map_lock()/vm_map_unlock() after unlock_read.
-------------- 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-head/attachments/20100907/a42e76b6/attachment.pgp