Date: Mon, 13 Sep 2010 22:12:47 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Alan Cox <alc@rice.edu> Cc: svn-src-head@freebsd.org, Ryan Stone <rstone@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org Subject: Re: svn commit: r212281 - head/sys/vm Message-ID: <20100913191247.GN2465@deviant.kiev.zoral.com.ua> In-Reply-To: <4C8E5FB5.9070009@rice.edu> References: <201009070023.o870Njtg072607@svn.freebsd.org> <20100907080446.GA2853@deviant.kiev.zoral.com.ua> <4C8E5FB5.9070009@rice.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
--vRCs+Tj6pwiFrQdW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: > > =20 > >>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= =20 > >> 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= =20 > >> 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. > >> =20 > >> Without this change I could cause a deadlock on a multicore 8.1-RELEA= SE > >> 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=20 > >> pmcstat > >> in system-wide sampling mode. > >> =20 > >> 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 > >>=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > >>--- 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 =3D (uintptr_t) NULL; > >> if (vm_map_lookup_entry(map, addr, &entry)) { > >> for (; > >> entry !=3D &map->header && entry->start < addr + size; > >>@@ -587,16 +588,23 @@ munmap(td, uap) > >> entry->end, VM_PROT_EXECUTE) =3D=3D TRUE) { > >> pkm.pm_address =3D (uintptr_t) addr; > >> pkm.pm_size =3D (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 !=3D (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); > >> } > >>=20 > >> =20 > >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. > > > > =20 >=20 > I'm afraid that this understates the effect. Over the weekend, when I=20 > updated one of my amd64 machines to include this change, I found that=20 > the delay in object and page deallocation is leading to severe=20 > fragmentation within the physical memory allocator. As a result, the=20 > time spent in the kernel during a "buildworld" increased by about 8%. =20 > Moreover, superpage promotion by applications effectively stopped. >=20 > For now, I think it would be best to back out r212281 and r212282. =20 > Ultimately, the fix may be to change the vm map synchronization=20 > primitives, and simply reinstate r212281 and r212282, but I'd like some= =20 > 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) =20 #ifdef HWPMC_HOOKS /* downgrade the lock to prevent a LOR with the pmc-sx lock */ - vm_map_lock_downgrade(map); - if (pkm.pm_address !=3D (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 !=3D (uintptr_t)NULL) { + vm_map_lock_downgrade(map); + if (pkm.pm_address !=3D (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); } --vRCs+Tj6pwiFrQdW Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (FreeBSD) iEYEARECAAYFAkyOd68ACgkQC3+MBN1Mb4h4YACg5VBzgUZtyKq6f5N0W1fl4pik YpoAoOCtlCJjs5Lq+dkjEHegFUvZwEXl =wtoz -----END PGP SIGNATURE----- --vRCs+Tj6pwiFrQdW--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100913191247.GN2465>