Date: Thu, 11 Jul 2013 08:01:29 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Neel Natu <neelnatu@gmail.com> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r253135 - in projects/bhyve_npt_pmap/sys/amd64: include vmm Message-ID: <20130711050129.GJ91021@kib.kiev.ua> In-Reply-To: <CAFgRE9EQ4ibJ%2BzeK_9C5KOic=P5mMdNTPHp8WU4FLHfbW7hO4g@mail.gmail.com> References: <201307100712.r6A7CtsB031581@svn.freebsd.org> <20130710072036.GY91021@kib.kiev.ua> <CAFgRE9GLeE_8jVT4DBzZ-yAAaw6yPLVL43Lv1s=9ytZ-1cNWwA@mail.gmail.com> <20130710074839.GZ91021@kib.kiev.ua> <CAFgRE9EQ4ibJ%2BzeK_9C5KOic=P5mMdNTPHp8WU4FLHfbW7hO4g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--5JjwDxJV2lCoj2Nt Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 10, 2013 at 09:05:02PM -0700, Neel Natu wrote: > Hi Konstantin, >=20 > On Wed, Jul 10, 2013 at 12:48 AM, Konstantin Belousov > <kostikbel@gmail.com> wrote: > > On Wed, Jul 10, 2013 at 12:36:38AM -0700, Neel Natu wrote: > >> Hi Konstantin, > >> > >> On Wed, Jul 10, 2013 at 12:20 AM, Konstantin Belousov > >> <kostikbel@gmail.com> wrote: > >> > On Wed, Jul 10, 2013 at 07:12:55AM +0000, Neel Natu wrote: > >> >> Author: neel > >> >> Date: Wed Jul 10 07:12:55 2013 > >> >> New Revision: 253135 > >> >> URL: http://svnweb.freebsd.org/changeset/base/253135 > >> >> > >> >> Log: > >> >> Replace vm_gpa2hpa() with a pair of functions vm_gpa_hold()/vm_gp= a_release(). > >> >> > >> >> We guarantee that the vm_page backing the 'gpa' is not reclaimed = by > >> >> the page daemon until the caller indicates that they are done usi= ng it > >> >> by calling 'vm_gpa_release()'. > >> >> > >> >> Modified: > >> >> projects/bhyve_npt_pmap/sys/amd64/include/vmm.h > >> >> projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c > >> >> projects/bhyve_npt_pmap/sys/amd64/vmm/vmm_dev.c > >> >> projects/bhyve_npt_pmap/sys/amd64/vmm/vmm_instruction_emul.c > >> >> > >> >> Modified: projects/bhyve_npt_pmap/sys/amd64/include/vmm.h > >> >> =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 > >> >> --- projects/bhyve_npt_pmap/sys/amd64/include/vmm.h Wed Jul 10 06= :46:46 2013 (r253134) > >> >> +++ projects/bhyve_npt_pmap/sys/amd64/include/vmm.h Wed Jul 10 07= :12:55 2013 (r253135) > >> >> @@ -93,6 +93,9 @@ const char *vm_name(struct vm *vm); > >> >> int vm_malloc(struct vm *vm, vm_paddr_t gpa, size_t len); > >> >> int vm_map_mmio(struct vm *vm, vm_paddr_t gpa, size_t len, vm_padd= r_t hpa); > >> >> int vm_unmap_mmio(struct vm *vm, vm_paddr_t gpa, size_t len); > >> >> +void *vm_gpa_hold(struct vm *, vm_paddr_t gpa, size_t len, int pro= t, > >> >> + void **cookie); > >> >> +void vm_gpa_release(void *cookie); > >> >> vm_paddr_t vm_gpa2hpa(struct vm *vm, vm_paddr_t gpa, size_t size); > >> >> int vm_gpabase2memseg(struct vm *vm, vm_paddr_t gpabase, > >> >> struct vm_memory_segment *seg); > >> >> > >> >> Modified: projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.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 > >> >> --- projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c Wed Jul 10 06= :46:46 2013 (r253134) > >> >> +++ projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c Wed Jul 10 07= :12:55 2013 (r253135) > >> >> @@ -439,16 +439,48 @@ vm_malloc(struct vm *vm, vm_paddr_t gpa, > >> >> return (0); > >> >> } > >> >> > >> >> -vm_paddr_t > >> >> -vm_gpa2hpa(struct vm *vm, vm_paddr_t gpa, size_t len) > >> >> +void * > >> >> +vm_gpa_hold(struct vm *vm, vm_paddr_t gpa, size_t len, int reqprot, > >> >> + void **cookie) > >> >> { > >> >> - vm_paddr_t nextpage; > >> >> + int rv, pageoff; > >> >> + vm_page_t m; > >> >> + struct proc *p; > >> >> + > >> >> + pageoff =3D gpa & PAGE_MASK; > >> >> + if (len > PAGE_SIZE - pageoff) > >> >> + panic("vm_gpa_hold: invalid gpa/len: 0x%016lx/%lu", g= pa, len); > >> >> + > >> >> + p =3D curthread->td_proc; > >> >> + > >> >> + PROC_LOCK(p); > >> >> + p->p_lock++; > >> >> + PROC_UNLOCK(p); > >> > Why do you need to hold the process there ? > >> > >> I was following the idiom in trap_pfault() - the comment in trap.c > >> about swapout was enough to convince me :-) > >> > >> > I do not think that hold in the trap handler is really useful, proba= bly > >> > the reverse. > >> > >> I am happy to remove the lock if isn't needed. > >> > >> Could you explain a bit more on why it may be detrimental or point me > >> in the right direction so I can look for myself. > > The hold prevents the swap out of the process. This means that kernel > > stack is assured to be resident for all process threads, and the > > vm_pageout_map_deactivate_pages() is not called for the process. In > > other words, in the low memory condition, the VM choice of the pages > > to reuse is limited, which is probably esp. bad for the large address > > spaces like whole virtual machine. > > > > The swapped-out state of the process interacts correctly with the > > vm_fault_hold(), the synchronization of the access to map and objects > > would do the right thing. IMO the PHOLD() makes the page fault > > potentially faster to proceed by the cost of making the deadlock due to > > low memory condition more probable. >=20 > Thanks for the explanation. I submitted a change to get rid of the > 'p_lock'ing when resolving the guest fault. >=20 > Would the same reasoning lead us to get rid of the 'p_lock' increment > in 'trap_pfault()' as well? Yes, I think so. At least, it would remove two pairs of process lock' lock and unlock on the trap path. IMO, the only useful applications of the PHOLD() is when we need to ensure that other process does not exit while we manipulate it, or when the process kstack has to be accessed, e.g. in ptrace(2) to manipulate the trapframe or pcb. For vm_fault() calls, this means that it is enough to use PHOLD to interlock the call to vmspace_acquire_ref(). Other uses are probably vestiges of the time when struct user was swappable. Curproc cannot exit while thread is executing. --5JjwDxJV2lCoj2Nt Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (FreeBSD) iQIcBAEBAgAGBQJR3jwoAAoJEJDCuSvBvK1BkNYQAKmJgtjz35X+s4UrKw6f9XNb OziUOnxbjZsD/07aKsBa+b5ndsvrqtXDwNldrXLjsBWqFFHcLpKvITUTilvZfunm uAAp+2oECzu/7z4xKt1ZjieAluKnzD0cymiLvlQdeZCjMYD/BJCTZN8HwTZvG0TS owm9a3cdQiM28Bl01IzgMKTI8Amm6CecfnN7uw+lU/hTprtMTrN+KR+71OyF5US2 XnQTKnVmB/5+g0DU3pv06a4b9gBV661VKMGlnkjMCz3U/ni1l/atsCnTRbQaNUSp FpM8K5AruiGa/KyxQxx1YVmejTe1mgkj2jEQP2HbAIgzXZ938yXbRCMQA5tnDnvd opDhZzMuqwRyPXWC3bsch6GXb3hkmA/r+mkvGG3FXY/fRArf8cLt7uLtqWDKZZxr fxvmXOh1Z0tcBO9lYU+SLq/v/VI6mjI3ivQUJekLFk905ITwsPRPssqnYxgI5KYR /0nEXpOT4T0icWZ8sx3qFRThBmwc0X/iCQbJpFVpEYEXTGIu+/fl11n6yEyjWxeU 8sMCdgKqE736cc1FzwDJJGXsQlBbMMfgViQCxOHJz17VEJYmt+qFhjRSUU5pl2vT wCMUc4J0DaV6rgAMvJYiBV9ubbttt5fstp78NH7N0gCBEy0JAwCjDVsY6ymDGMW7 pMtJEit2JcY2Py0E9vH7 =exh0 -----END PGP SIGNATURE----- --5JjwDxJV2lCoj2Nt--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130711050129.GJ91021>