Date: Wed, 10 Jul 2013 21:05:02 -0700 From: Neel Natu <neelnatu@gmail.com> To: Konstantin Belousov <kostikbel@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: <CAFgRE9EQ4ibJ%2BzeK_9C5KOic=P5mMdNTPHp8WU4FLHfbW7hO4g@mail.gmail.com> In-Reply-To: <20130710074839.GZ91021@kib.kiev.ua> References: <201307100712.r6A7CtsB031581@svn.freebsd.org> <20130710072036.GY91021@kib.kiev.ua> <CAFgRE9GLeE_8jVT4DBzZ-yAAaw6yPLVL43Lv1s=9ytZ-1cNWwA@mail.gmail.com> <20130710074839.GZ91021@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Konstantin, 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_gpa_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 using 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 >> >> ============================================================================== >> >> --- 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_paddr_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 prot, >> >> + 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 >> >> ============================================================================== >> >> --- 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 = gpa & PAGE_MASK; >> >> + if (len > PAGE_SIZE - pageoff) >> >> + panic("vm_gpa_hold: invalid gpa/len: 0x%016lx/%lu", gpa, len); >> >> + >> >> + p = 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, probably >> > 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. Thanks for the explanation. I submitted a change to get rid of the 'p_lock'ing when resolving the guest fault. Would the same reasoning lead us to get rid of the 'p_lock' increment in 'trap_pfault()' as well? best Neel
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFgRE9EQ4ibJ%2BzeK_9C5KOic=P5mMdNTPHp8WU4FLHfbW7hO4g>