From owner-svn-src-projects@FreeBSD.ORG Thu Jul 11 04:05:03 2013 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 1F4F273; Thu, 11 Jul 2013 04:05:03 +0000 (UTC) (envelope-from neelnatu@gmail.com) Received: from mail-ie0-x22f.google.com (mail-ie0-x22f.google.com [IPv6:2607:f8b0:4001:c03::22f]) by mx1.freebsd.org (Postfix) with ESMTP id E13A61EC2; Thu, 11 Jul 2013 04:05:02 +0000 (UTC) Received: by mail-ie0-f175.google.com with SMTP id a11so9622664iee.6 for ; Wed, 10 Jul 2013 21:05:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=XkaduLBTqR9voYYFd3M+qD7C2l6PIrKUBWcBmYcR/sA=; b=p3Cf3LUZg4rYzDAL729mWaMiZG+Chsm50yGHqJiOurANXZbewj5Jzit7VXDjgBibhA AhqApyA1AQF8li5aMLS99Po6mw9NTatCgRqkc3wyFfo3W1VjjvchAjWw2sJ/s0U/PZPi xDtPufbvCAkVjmUecRiVh2w1zyNVf0CCV8BGDw6xVLMNymrYneR18mb6NR8s1m75OEFq ba1ohYeBhmtkpJNuDDeUsvnTBcKqJ/RzrJgqAIXfEY8elpILtDHOqzqGcGhZYEMUrTtq +EUE5Ut31IFOSxvVac6S/EK/FcuaNtJGoVmcWLWvqN0/ZpdV3v/yK0b+RvUJtaF+14H5 Xn3Q== MIME-Version: 1.0 X-Received: by 10.42.36.3 with SMTP id s3mr10931601icd.42.1373515502651; Wed, 10 Jul 2013 21:05:02 -0700 (PDT) Received: by 10.42.151.74 with HTTP; Wed, 10 Jul 2013 21:05:02 -0700 (PDT) In-Reply-To: <20130710074839.GZ91021@kib.kiev.ua> References: <201307100712.r6A7CtsB031581@svn.freebsd.org> <20130710072036.GY91021@kib.kiev.ua> <20130710074839.GZ91021@kib.kiev.ua> Date: Wed, 10 Jul 2013 21:05:02 -0700 Message-ID: Subject: Re: svn commit: r253135 - in projects/bhyve_npt_pmap/sys/amd64: include vmm From: Neel Natu To: Konstantin Belousov Content-Type: text/plain; charset=ISO-8859-1 Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jul 2013 04:05:03 -0000 Hi Konstantin, On Wed, Jul 10, 2013 at 12:48 AM, Konstantin Belousov 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 >> 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