svn commit: r253135 - in projects/bhyve_npt_pmap/sys/amd64: include vmm

[Konstantin Belousov]

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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-projects/attachments/20130710/886ff765/attachment.sig>